🐛 multiple fixes

- return to default connection parameters from lib60870 (10s,15s,10s)
- increase default (connection) command timeout from 100ms to 10000ms to fit to connection parameters
- increase default (server) select timeout from 100ms to 10000ms to fit to connection parameters
- fix server responds only with last station to general interrogation
- fix all periodic tasks (periodic monitoring reports, selection timeout)
- Connection interrogation command to global common address blocks until all stations send ACT_TERM, not just first station

Author: m-unkel

c104/__init__.pyi

@@ -274,7 +274,7 @@ class Client:
     """
     This class represents a local client and provides access to meta information and connected remote servers
     """
-    def __init__(self, tick_rate_ms: int = 100, command_timeout_ms: int = 100, transport_security: TransportSecurity | None = None) -> None:
+    def __init__(self, tick_rate_ms: int = 100, command_timeout_ms: int = 10000, transport_security: TransportSecurity | None = None) -> None:
         """
         create a new 104er client
 
@@ -289,7 +289,7 @@ class Client:
 
         Example
         -------
-        >>> my_client = c104.Client(tick_rate_ms=100, command_timeout_ms=100)
+        >>> my_client = c104.Client(tick_rate_ms=100, command_timeout_ms=10000)
         """
     def add_connection(self, ip: str, port: int = 2404, init: Init | None = Init.ALL) -> Connection | None:
         """
@@ -2478,7 +2478,7 @@ class Server:
     """
     This class represents a local server and provides access to meta information and containing stations
     """
-    def __init__(self, ip: str = "0.0.0.0", port: int = 2404, tick_rate_ms: int = 100, select_timeout_ms = 100, max_connections: int = 0, transport_security: TransportSecurity | None = None) -> None:
+    def __init__(self, ip: str = "0.0.0.0", port: int = 2404, tick_rate_ms: int = 100, select_timeout_ms = 10000, max_connections: int = 0, transport_security: TransportSecurity | None = None) -> None:
         """
         create a new 104er server
 
@@ -2499,7 +2499,7 @@ class Server:
 
         Example
         -------
-        >>> my_server = c104.Server(ip="0.0.0.0", port=2404, tick_rate_ms=100, select_timeout_ms=100, max_connections=0)
+        >>> my_server = c104.Server(ip="0.0.0.0", port=2404, tick_rate_ms=100, select_timeout_ms=10000, max_connections=0)
         """
     def add_station(self, common_address: int) -> Station | None:
         """

docs/source/changelog.rst

@@ -12,6 +12,13 @@ Features
 Fixes
 ^^^^^^
 
+- return to default connection parameters from lib60870 (10s,15s,10s)
+- increase default (connection) command timeout from 100ms to 10000ms to fit to connection parameters
+- increase default (server) select timeout from 100ms to 10000ms to fit to connection parameters
+- fix server uses correct cause of transmission for interrogation response #57
+- fix server responds only with last station to general interrogation
+- fix all periodic tasks (periodic monitoring reports, selection timeout)
+- Connection interrogation command to global common address blocks until all stations send ACT_TERM, not just first station
 - fix docstring for c104.ProtocolParameters timings: unit is seconds not milliseconds
 - support try-except blocks in callbacks instead of catching all errors and removing the callback
 - prevent invalid pointer access from lib60870 callbacks

src/Client.cpp

@@ -361,38 +361,33 @@ void Client::scheduleDataPointTimer() {
 }
 
 void Client::schedulePeriodicTask(const std::function<void()> &task,
-                                  int interval) {
-  {
-    if (interval < 50) {
-      throw std::out_of_range(
-          "The interval for periodic tasks must be 1000ms at minimum.");
-    }
-    auto periodic = std::make_shared<std::function<void()>>(
-        []() {}); // Empty function initially
-    std::weak_ptr<std::function<void()>> weakPeriodic =
-        periodic; // Weak reference
-    std::weak_ptr<Client> weakSelf =
-        shared_from_this(); // Weak reference to `this`
-
-    *periodic = [weakSelf, task, interval, weakPeriodic]() {
-      auto self = weakSelf.lock();
-      if (!self)
-        return; // Prevent running if `this` was destroyed
-
-      // Schedule next execution
-      if (auto periodicLock = weakPeriodic.lock()) { // Try to lock weak_ptr
-        self->scheduleTask(*periodicLock, interval);
-      }
-      task();
-    };
-    // Schedule first execution
-    scheduleTask(*periodic, interval);
+                                  std::int_fast32_t interval) {
+  if (interval < 50) {
+    throw std::out_of_range(
+        "The interval for periodic tasks must be 50ms at minimum.");
   }
 
+  std::weak_ptr<Client> weakSelf =
+      shared_from_this(); // Weak reference to `this`
+
+  auto periodicCallback = [weakSelf, task, interval]() {
+    auto self = weakSelf.lock();
+    if (!self)
+      return; // Prevent running if `this` was destroyed
+
+    // Schedule next execution
+    self->schedulePeriodicTask(task, interval); // Reschedule itself
+
+    task();
+  };
+  // Schedule first execution
+  scheduleTask(periodicCallback, interval);
+
   runThread_wait.notify_one();
 }
 
-void Client::scheduleTask(const std::function<void()> &task, int delay) {
+void Client::scheduleTask(const std::function<void()> &task,
+                          std::int_fast32_t delay) {
   {
     std::lock_guard<std::mutex> lock(runThread_mutex);
     if (delay < 0) {

src/Client.h

@@ -57,14 +57,14 @@ class Client : public std::enable_shared_from_this<Client> {
    * @param tick_rate_ms Tick rate in milliseconds for the Client execution
    * loop. Defaults to 100.
    * @param timeout_ms Timeout in milliseconds for Client operations. Defaults
-   * to 100.
+   * to 10000.
    * @param transport_security Shared pointer to a TransportSecurity object for
    * remote communication. Defaults to nullptr.
    * @return A shared pointer to the newly created Client instance.
    */
   [[nodiscard]] static std::shared_ptr<Client> create(
       std::uint_fast16_t tick_rate_ms = 100,
-      std::uint_fast16_t timeout_ms = 100,
+      std::uint_fast16_t timeout_ms = 10000,
       std::shared_ptr<Remote::TransportSecurity> transport_security = nullptr) {
     // Not using std::make_shared because the constructor is private.
     return std::shared_ptr<Client>(
@@ -278,10 +278,11 @@ class Client : public std::enable_shared_from_this<Client> {
    * @param task A callable object representing the task to be executed
    * periodically.
    * @param interval The interval in milliseconds at which the task should be
-   * executed. Must be at least 1000ms. Throws std::out_of_range if the interval
+   * executed. Must be at least 50ms. Throws std::out_of_range if the interval
    * is less than 50ms.
    */
-  void schedulePeriodicTask(const std::function<void()> &task, int interval);
+  void schedulePeriodicTask(const std::function<void()> &task,
+                            std::int_fast32_t interval);
 
   /**
    * @brief Schedules a task to be executed after a specified delay (or
@@ -294,7 +295,8 @@ class Client : public std::enable_shared_from_this<Client> {
    * @param delay The delay in milliseconds before the task is executed. A
    * negative delay executes the task immediately.
    */
-  void scheduleTask(const std::function<void()> &task, int delay = 0);
+  void scheduleTask(const std::function<void()> &task,
+                    std::int_fast32_t delay = 0);
 
 private:
   /**
@@ -321,10 +323,10 @@ class Client : public std::enable_shared_from_this<Client> {
   void scheduleDataPointTimer();
 
   /// @brief minimum interval between two periodic tasks
-  const std::uint_fast16_t tickRate_ms{1000};
+  const std::uint_fast16_t tickRate_ms{100};
 
   /// @brief timeout in milliseconds before an inactive connection gets closed
-  const std::uint_fast16_t commandTimeout_ms{100};
+  const std::uint_fast16_t commandTimeout_ms{10000};
 
   /// @brief tls handler
   const std::shared_ptr<Remote::TransportSecurity> security{nullptr};

src/Server.cpp

@@ -100,11 +100,11 @@ Server::Server(const std::string &bind_ip, const std::uint_fast16_t tcp_port,
 
   appLayerParameters = CS104_Slave_getAppLayerParameters(slave);
 
-  // reduce lib60870-C connection timeout down to 2s
-  auto param = CS104_Slave_getConnectionParameters(slave);
-  param->t0 = 2; // socket connect timeout
-  param->t1 = 2; // acknowledgement timeout
-  param->t2 = 1; // acknowledgement interval
+  // use lib60870-C defaults for connection timeouts
+  // auto param = CS104_Slave_getConnectionParameters(slave);
+  // param->t0 = 10; // socket connect timeout
+  // param->t1 = 15; // acknowledgement timeout
+  // param->t2 = 10; // acknowledgement interval
 
   // Function pointers for custom handler functions
   void *key = static_cast<void *>(this);
@@ -243,38 +243,33 @@ void Server::scheduleDataPointTimer() {
 }
 
 void Server::schedulePeriodicTask(const std::function<void()> &task,
-                                  int interval) {
-  {
-    if (interval < 50) {
-      throw std::out_of_range(
-          "The interval for periodic tasks must be 1000ms at minimum.");
-    }
-    auto periodic = std::make_shared<std::function<void()>>(
-        []() {}); // Empty function initially
-    std::weak_ptr<std::function<void()>> weakPeriodic =
-        periodic; // Weak reference
-    std::weak_ptr<Server> weakSelf =
-        shared_from_this(); // Weak reference to `this`
+                                  std::int_fast32_t interval) {
+  if (interval < 50) {
+    throw std::out_of_range(
+        "The interval for periodic tasks must be 50ms at minimum.");
+  }
 
-    *periodic = [weakSelf, task, interval, weakPeriodic]() {
-      auto self = weakSelf.lock();
-      if (!self)
-        return; // Prevent running if `this` was destroyed
+  std::weak_ptr<Server> weakSelf =
+      shared_from_this(); // Weak reference to `this`
 
-      // Schedule next execution
-      if (auto periodicLock = weakPeriodic.lock()) { // Try to lock weak_ptr
-        self->scheduleTask(*periodicLock, interval);
-      }
-      task();
-    };
-    // Schedule first execution
-    scheduleTask(*periodic, interval);
-  }
+  auto periodicCallback = [weakSelf, task, interval]() {
+    auto self = weakSelf.lock();
+    if (!self)
+      return; // Prevent running if `this` was destroyed
+
+    // Schedule next execution
+    self->schedulePeriodicTask(task, interval); // Reschedule itself
+
+    task();
+  };
+  // Schedule first execution
+  scheduleTask(periodicCallback, interval);
 
   runThread_wait.notify_one();
 }
 
-void Server::scheduleTask(const std::function<void()> &task, int delay) {
+void Server::scheduleTask(const std::function<void()> &task,
+                          std::int_fast32_t delay) {
   {
     std::lock_guard<std::mutex> lock(runThread_mutex);
     if (delay < 0) {
@@ -741,44 +736,48 @@ void Server::onUnexpectedMessage(
     std::shared_ptr<Remote::Message::IncomingMessage> message,
     UnexpectedMessageCause cause) {
   CS101_ASDU asdu = message->getAsdu();
+
+  // manipulate and send copy instead of original ASDU
+  CS101_ASDU cp = CS101_ASDU_clone(asdu, nullptr);
   switch (cause) {
   case INVALID_TYPE_ID:
     DEBUG_PRINT(Debug::Server, "on_unexpected_message] Invalid type id");
-    CS101_ASDU_setCOT(asdu, CS101_COT_UNKNOWN_TYPE_ID);
-    IMasterConnection_sendASDU(connection, asdu);
+    CS101_ASDU_setCOT(cp, CS101_COT_UNKNOWN_TYPE_ID);
+    IMasterConnection_sendASDU(connection, cp);
     break;
   case MISMATCHED_TYPE_ID:
     DEBUG_PRINT(Debug::Server, "on_unexpected_message] Mismatching type id");
-    CS101_ASDU_setCOT(asdu, CS101_COT_UNKNOWN_TYPE_ID);
-    IMasterConnection_sendASDU(connection, asdu);
+    CS101_ASDU_setCOT(cp, CS101_COT_UNKNOWN_TYPE_ID);
+    IMasterConnection_sendASDU(connection, cp);
     break;
   case UNKNOWN_TYPE_ID:
     DEBUG_PRINT(Debug::Server, "on_unexpected_message] Unknown type id");
-    CS101_ASDU_setCOT(asdu, CS101_COT_UNKNOWN_TYPE_ID);
-    IMasterConnection_sendASDU(connection, asdu);
+    CS101_ASDU_setCOT(cp, CS101_COT_UNKNOWN_TYPE_ID);
+    IMasterConnection_sendASDU(connection, cp);
     break;
   case INVALID_COT:
   case UNKNOWN_COT:
     DEBUG_PRINT(Debug::Server, "on_unexpected_message] Invalid COT");
-    CS101_ASDU_setCOT(asdu, CS101_COT_UNKNOWN_COT);
-    IMasterConnection_sendASDU(connection, asdu);
+    CS101_ASDU_setCOT(cp, CS101_COT_UNKNOWN_COT);
+    IMasterConnection_sendASDU(connection, cp);
     break;
   case UNKNOWN_CA:
     DEBUG_PRINT(Debug::Server, "on_unexpected_message] Unknown CA");
-    CS101_ASDU_setCOT(asdu, CS101_COT_UNKNOWN_CA);
-    IMasterConnection_sendASDU(connection, asdu);
+    CS101_ASDU_setCOT(cp, CS101_COT_UNKNOWN_CA);
+    IMasterConnection_sendASDU(connection, cp);
     break;
   case UNKNOWN_IOA:
     DEBUG_PRINT(Debug::Server, "on_unexpected_message] Unknown IOA");
-    CS101_ASDU_setCOT(asdu, CS101_COT_UNKNOWN_IOA);
-    IMasterConnection_sendASDU(connection, asdu);
+    CS101_ASDU_setCOT(cp, CS101_COT_UNKNOWN_IOA);
+    IMasterConnection_sendASDU(connection, cp);
     break;
   case UNIMPLEMENTED_GROUP:
     DEBUG_PRINT(Debug::Server, "on_unexpected_message] Unimplemented group");
     break;
   default: {
   }
   }
+  CS101_ASDU_destroy(cp);
 
   if (py_onUnexpectedMessage.is_set()) {
     std::weak_ptr<Server> weakSelf =
@@ -1102,40 +1101,46 @@ void Server::sendActivationConfirmation(IMasterConnection connection,
   if (!isExistingConnection(connection))
     return;
 
-  CS101_ASDU_setCOT(asdu, CS101_COT_ACTIVATION_CON);
-  CS101_ASDU_setNegative(asdu, negative);
+  // manipulate copy instead of original asdu
+  CS101_ASDU cp = CS101_ASDU_clone(asdu, nullptr);
+  CS101_ASDU_setCOT(cp, CS101_COT_ACTIVATION_CON);
+  CS101_ASDU_setNegative(cp, negative);
 
   if (isGlobalCommonAddress(CS101_ASDU_getCA(asdu))) {
     DEBUG_PRINT(Debug::Server, "send_activation_confirmation] to all MTUs");
     std::lock_guard<Module::GilAwareMutex> const st_lock(station_mutex);
     for (auto &s : stations) {
-      CS101_ASDU_setCA(asdu, s->getCommonAddress());
-      IMasterConnection_sendASDU(connection, asdu);
+      CS101_ASDU_setCA(cp, s->getCommonAddress());
+      IMasterConnection_sendASDU(connection, cp);
     }
   } else {
     DEBUG_PRINT(Debug::Server,
                 "send_activation_confirmation] to requesting MTU");
-    IMasterConnection_sendASDU(connection, asdu);
+    IMasterConnection_sendASDU(connection, cp);
   }
+  CS101_ASDU_destroy(cp);
 }
 
 void Server::sendActivationTermination(IMasterConnection connection,
                                        CS101_ASDU asdu) {
   if (!isExistingConnection(connection))
     return;
 
-  CS101_ASDU_setCOT(asdu, CS101_COT_ACTIVATION_TERMINATION);
+  // manipulate copy instead of original asdu
+  CS101_ASDU cp = CS101_ASDU_clone(asdu, nullptr);
+  CS101_ASDU_setCOT(cp, CS101_COT_ACTIVATION_TERMINATION);
 
   if (isGlobalCommonAddress(CS101_ASDU_getCA(asdu))) {
 
     std::lock_guard<Module::GilAwareMutex> const st_lock(station_mutex);
     for (auto &s : stations) {
-      CS101_ASDU_setCA(asdu, s->getCommonAddress());
-      IMasterConnection_sendASDU(connection, asdu);
+      CS101_ASDU_setCA(cp, s->getCommonAddress());
+      IMasterConnection_sendASDU(connection, cp);
     }
   } else {
-    IMasterConnection_sendASDU(connection, asdu);
+    IMasterConnection_sendASDU(connection, cp);
   }
+  CS101_ASDU_destroy(cp);
 }
 
 void Server::sendEndOfInitialization(const std::uint_fast16_t commonAddress,
@@ -1332,7 +1337,7 @@ bool Server::interrogationHandler(void *parameter, IMasterConnection connection,
               }
             } catch (const std::exception &e) {
               DEBUG_PRINT(Debug::Server,
-                          "Invalid point message for counter interrogation: " +
+                          "Invalid point message for interrogation: " +
                               std::string(e.what()));
             }
           }