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 · Feb 1, 2024 · 1237538 · 1237538
1 parent f4224dc
commit 1237538
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: