Update GetAckTimeout to use the backoff algorithm (#23307)

* [MRP] Expose GetRetransmissionTimeout from ReliableMessageProtocolConfig such that an accurate maximum retransmission time could be calculated from the external world * [MRP] Update GetAckTimeout to take into account the PeerActive status and use GetRetransmissionTimeout to be more accurate * Update src/controller/python/test/test_scripts/network_commissioning.py ScanNetwork and ConnectNetwork tests with an explicit interactionTimeoutMs instead of relying on GetAckTimeout behavior
project-chip · Sep 29, 2023 · 3586135 · 3586135
1 parent 6ff6ea1
commit 3586135
Show file tree

Hide file tree

Showing 7 changed files with 57 additions and 15 deletions.
diff --git a/src/controller/python/test/test_scripts/network_commissioning.py b/src/controller/python/test/test_scripts/network_commissioning.py
@@ -143,7 +143,8 @@ async def test_wifi(self, endpointId):
         logger.info(f"Scan networks")
         req = Clusters.NetworkCommissioning.Commands.ScanNetworks(
             ssid=b'', breadcrumb=self.with_breadcrumb())
-        res = await self._devCtrl.SendCommand(nodeid=self._nodeid, endpoint=endpointId, payload=req)
+        interactionTimeoutMs = self._devCtrl.ComputeRoundTripTimeout(self._nodeid, upperLayerProcessingTimeoutMs=30000)
+        res = await self._devCtrl.SendCommand(nodeid=self._nodeid, endpoint=endpointId, payload=req, interactionTimeoutMs=interactionTimeoutMs)
         logger.info(f"Received response: {res}")
         if res.networkingStatus != Clusters.NetworkCommissioning.Enums.NetworkCommissioningStatus.kSuccess:
             raise AssertionError(f"Unexpected result: {res.networkingStatus}")
@@ -198,7 +199,8 @@ async def test_wifi(self, endpointId):
         logger.info(f"Connect to a network")
         req = Clusters.NetworkCommissioning.Commands.ConnectNetwork(
             networkID=TEST_WIFI_SSID.encode(), breadcrumb=self.with_breadcrumb())
-        res = await self._devCtrl.SendCommand(nodeid=self._nodeid, endpoint=endpointId, payload=req)
+        interactionTimeoutMs = self._devCtrl.ComputeRoundTripTimeout(self._nodeid, upperLayerProcessingTimeoutMs=30000)
+        res = await self._devCtrl.SendCommand(nodeid=self._nodeid, endpoint=endpointId, payload=req, interactionTimeoutMs=interactionTimeoutMs)
         logger.info(f"Got response: {res}")
         if res.networkingStatus != Clusters.NetworkCommissioning.Enums.NetworkCommissioningStatus.kSuccess:
             raise AssertionError(f"Unexpected result: {res.networkingStatus}")
@@ -277,7 +279,8 @@ async def test_thread(self, endpointId):
         logger.info(f"Scan networks")
         req = Clusters.NetworkCommissioning.Commands.ScanNetworks(
             ssid=b'', breadcrumb=self.with_breadcrumb())
-        res = await self._devCtrl.SendCommand(nodeid=self._nodeid, endpoint=endpointId, payload=req)
+        interactionTimeoutMs = self._devCtrl.ComputeRoundTripTimeout(self._nodeid, upperLayerProcessingTimeoutMs=30000)
+        res = await self._devCtrl.SendCommand(nodeid=self._nodeid, endpoint=endpointId, payload=req, interactionTimeoutMs=interactionTimeoutMs)
         logger.info(f"Received response: {res}")
         if res.networkingStatus != Clusters.NetworkCommissioning.Enums.NetworkCommissioningStatus.kSuccess:
             raise AssertionError(f"Unexpected result: {res.networkingStatus}")
@@ -332,7 +335,8 @@ async def test_thread(self, endpointId):
         logger.info(f"Connect to a network")
         req = Clusters.NetworkCommissioning.Commands.ConnectNetwork(
             networkID=TEST_THREAD_NETWORK_IDS[0], breadcrumb=self.with_breadcrumb())
-        res = await self._devCtrl.SendCommand(nodeid=self._nodeid, endpoint=endpointId, payload=req)
+        interactionTimeoutMs = self._devCtrl.ComputeRoundTripTimeout(self._nodeid, upperLayerProcessingTimeoutMs=30000)
+        res = await self._devCtrl.SendCommand(nodeid=self._nodeid, endpoint=endpointId, payload=req, interactionTimeoutMs=interactionTimeoutMs)
         logger.info(f"Got response: {res}")
         if res.networkingStatus != Clusters.NetworkCommissioning.Enums.NetworkCommissioningStatus.kSuccess:
             raise AssertionError(f"Unexpected result: {res.networkingStatus}")

diff --git a/src/messaging/ReliableMessageMgr.cpp b/src/messaging/ReliableMessageMgr.cpp
@@ -204,7 +204,8 @@ CHIP_ERROR ReliableMessageMgr::AddToRetransTable(ReliableMessageContext * rc, Re
     return CHIP_NO_ERROR;
 }
 
-System::Clock::Timestamp ReliableMessageMgr::GetBackoff(System::Clock::Timestamp baseInterval, uint8_t sendCount)
+System::Clock::Timestamp ReliableMessageMgr::GetBackoff(System::Clock::Timestamp baseInterval, uint8_t sendCount,
+                                                        bool computeMaxPossible)
 {
     // See section "4.11.8. Parameters and Constants" for the parameters below:
     // MRP_BACKOFF_JITTER = 0.25
@@ -247,7 +248,7 @@ System::Clock::Timestamp ReliableMessageMgr::GetBackoff(System::Clock::Timestamp
     System::Clock::Timestamp mrpBackoffTime = baseInterval * backoffNum / backoffDenom;
 
     // 3. Calculate `mrpBackoffTime *= (1.0 + random(0,1) * MRP_BACKOFF_JITTER)`
-    uint32_t jitter = MRP_BACKOFF_JITTER_BASE + Crypto::GetRandU8();
+    uint32_t jitter = MRP_BACKOFF_JITTER_BASE + (computeMaxPossible ? UINT8_MAX : Crypto::GetRandU8());
     mrpBackoffTime  = mrpBackoffTime * jitter / MRP_BACKOFF_JITTER_BASE;
 
 #if CHIP_DEVICE_CONFIG_ENABLE_SED

diff --git a/src/messaging/ReliableMessageMgr.h b/src/messaging/ReliableMessageMgr.h
@@ -102,15 +102,17 @@ class ReliableMessageMgr
     /**
      *  Calculate the backoff timer for the retransmission.
      *
-     *  @param[in]   baseInterval   The base interval to use for the backoff calculation, either the active or idle interval.
-     *  @param[in]   sendCount      Count of how many times this message
-     *                              has been retransmitted so far (0 if it has
-     *                              been sent only once with no retransmits,
-     *                              1 if it has been sent twice, etc).
+     *  @param[in]   baseInterval         The base interval to use for the backoff calculation, either the active or idle interval.
+     *  @param[in]   sendCount            Count of how many times this message
+     *                                    has been retransmitted so far (0 if it has
+     *                                    been sent only once with no retransmits,
+     *                                    1 if it has been sent twice, etc).
+     *  @param[in]   computeMaxPossible   Disable randomness such that the maximum value is used instead.
      *
      *  @retval  The backoff time value, including jitter.
      */
-    static System::Clock::Timestamp GetBackoff(System::Clock::Timestamp baseInterval, uint8_t sendCount);
+    static System::Clock::Timestamp GetBackoff(System::Clock::Timestamp baseInterval, uint8_t sendCount,
+                                               bool computeMaxPossible = false);
 
     /**
      *  Start retranmisttion of cached encryped packet for current entry.

diff --git a/src/messaging/ReliableMessageProtocolConfig.cpp b/src/messaging/ReliableMessageProtocolConfig.cpp
@@ -22,7 +22,7 @@
  *
  */
 
-#include <messaging/ReliableMessageProtocolConfig.h>
+#include <messaging/ReliableMessageMgr.h>
 
 #include <platform/CHIPDeviceLayer.h>
 #include <system/SystemClock.h>
@@ -88,4 +88,22 @@ Optional<ReliableMessageProtocolConfig> GetLocalMRPConfig()
                                              : Optional<ReliableMessageProtocolConfig>::Value(config);
 }
 
+System::Clock::Timestamp GetRetransmissionTimeout(System::Clock::Timestamp activeInterval, System::Clock::Timestamp idleInterval,
+                                                  System::Clock::Timestamp lastActivityTime,
+                                                  System::Clock::Timestamp activityThreshold)
+{
+    auto timeSinceLastActivity = (System::SystemClock().GetMonotonicTimestamp() - lastActivityTime);
+
+    // Calculate the retransmission timeout and take into account that an active/idle state change can happen
+    // in the middle.
+    System::Clock::Timestamp timeout(0);
+    for (uint8_t i = 0; i < CHIP_CONFIG_RMP_DEFAULT_MAX_RETRANS + 1; i++)
+    {
+        auto baseInterval = ((timeSinceLastActivity + timeout) < activityThreshold) ? activeInterval : idleInterval;
+        timeout += Messaging::ReliableMessageMgr::GetBackoff(baseInterval, i, /* computeMaxPossible */ true);
+    }
+
+    return timeout;
+}
+
 } // namespace chip
diff --git a/src/messaging/ReliableMessageProtocolConfig.h b/src/messaging/ReliableMessageProtocolConfig.h
@@ -156,6 +156,21 @@ ReliableMessageProtocolConfig GetDefaultMRPConfig();
  */
 Optional<ReliableMessageProtocolConfig> GetLocalMRPConfig();
 
+/**
+ * @brief
+ * Returns the maximum transmission time depending on the last activity time.
+ *
+ * @param[in] activeInterval    The active interval to use for the backoff calculation.
+ * @param[in] idleInterval      The idle interval to use for the backoff calculation.
+ * @param[in] lastActivityTime  The last time some activity has been recorded.
+ * @param[in] activityThreshold The activity threshold for a node to be considered active.
+ *
+ * @return The maximum transmission time
+ */
+System::Clock::Timestamp GetRetransmissionTimeout(System::Clock::Timestamp activeInterval, System::Clock::Timestamp idleInterval,
+                                                  System::Clock::Timestamp lastActivityTime,
+                                                  System::Clock::Timestamp activityThreshold);
+
 #if CONFIG_BUILD_FOR_HOST_UNIT_TEST
 
 /**

diff --git a/src/transport/SecureSession.h b/src/transport/SecureSession.h
@@ -162,7 +162,8 @@ class SecureSession : public Session, public ReferenceCounted<SecureSession, Sec
         switch (mPeerAddress.GetTransportType())
         {
         case Transport::Type::kUdp:
-            return GetRemoteMRPConfig().mIdleRetransTimeout * (CHIP_CONFIG_RMP_DEFAULT_MAX_RETRANS + 1);
+            return GetRetransmissionTimeout(mRemoteMRPConfig.mActiveRetransTimeout, mRemoteMRPConfig.mIdleRetransTimeout,
+                                            GetLastPeerActivityTime(), kMinActiveTime);
         case Transport::Type::kTcp:
             return System::Clock::Seconds16(30);
         case Transport::Type::kBle:

diff --git a/src/transport/UnauthenticatedSessionTable.h b/src/transport/UnauthenticatedSessionTable.h
@@ -87,7 +87,8 @@ class UnauthenticatedSession : public Session,
         switch (mPeerAddress.GetTransportType())
         {
         case Transport::Type::kUdp:
-            return GetRemoteMRPConfig().mIdleRetransTimeout * (CHIP_CONFIG_RMP_DEFAULT_MAX_RETRANS + 1);
+            return GetRetransmissionTimeout(mRemoteMRPConfig.mActiveRetransTimeout, mRemoteMRPConfig.mIdleRetransTimeout,
+                                            GetLastPeerActivityTime(), kMinActiveTime);
         case Transport::Type::kTcp:
             return System::Clock::Seconds16(30);
         default: