[mrp] Make GetBackoff() use Timeout instead of Timestamp type

All users of ReliableMessageMgr::GetBackoff() seem to assume it takes and returns 32-bit Timeout while it actually takes and returns 64-bit Timestamp. Hence all the users do implicit casts. Replace Timestamp with Timeout in the function's signature and only use 64-bit type for internal calculations to prevent overflowing the underlying integer type.
Damian-Nordic · Apr 22, 2024 · 0960312 · 0960312
1 parent a860932
commit 0960312
Show file tree

Hide file tree

Showing 5 changed files with 35 additions and 28 deletions.
diff --git a/src/messaging/ReliableMessageMgr.cpp b/src/messaging/ReliableMessageMgr.cpp
@@ -213,8 +213,8 @@ CHIP_ERROR ReliableMessageMgr::AddToRetransTable(ReliableMessageContext * rc, Re
     return CHIP_NO_ERROR;
 }
 
-System::Clock::Timestamp ReliableMessageMgr::GetBackoff(System::Clock::Timestamp baseInterval, uint8_t sendCount,
-                                                        bool computeMaxPossible)
+System::Clock::Timeout ReliableMessageMgr::GetBackoff(System::Clock::Timeout baseInterval, uint8_t sendCount,
+                                                      bool computeMaxPossible)
 {
     // See section "4.11.8. Parameters and Constants" for the parameters below:
     // MRP_BACKOFF_JITTER = 0.25
@@ -227,14 +227,16 @@ System::Clock::Timestamp ReliableMessageMgr::GetBackoff(System::Clock::Timestamp
     constexpr uint32_t MRP_BACKOFF_BASE_DENOMINATOR = 10;
     constexpr int MRP_BACKOFF_THRESHOLD             = 1;
 
-    // Implement `i = MRP_BACKOFF_MARGIN * i` from section "4.11.2.1. Retransmissions", where:
-    //   i == baseInterval
-    baseInterval = baseInterval * MRP_BACKOFF_MARGIN_NUMERATOR / MRP_BACKOFF_MARGIN_DENOMINATOR;
+    // Implement `i = MRP_BACKOFF_MARGIN * i` from section "4.12.2.1. Retransmissions", where:
+    //   i == interval
+    System::Clock::Milliseconds64 interval = baseInterval;
+    interval *= MRP_BACKOFF_MARGIN_NUMERATOR;
+    interval /= MRP_BACKOFF_MARGIN_DENOMINATOR;
 
     // Implement:
     //   mrpBackoffTime = i * MRP_BACKOFF_BASE^(max(0,n-MRP_BACKOFF_THRESHOLD)) * (1.0 + random(0,1) * MRP_BACKOFF_JITTER)
-    // from section "4.11.2.1. Retransmissions", where:
-    //   i == baseInterval
+    // from section "4.12.2.1. Retransmissions", where:
+    //   i == interval
     //   n == sendCount
 
     // 1. Calculate exponent `max(0,n−MRP_BACKOFF_THRESHOLD)`
@@ -254,7 +256,7 @@ System::Clock::Timestamp ReliableMessageMgr::GetBackoff(System::Clock::Timestamp
         backoffDenom *= MRP_BACKOFF_BASE_DENOMINATOR;
     }
 
-    System::Clock::Timestamp mrpBackoffTime = baseInterval * backoffNum / backoffDenom;
+    System::Clock::Milliseconds64 mrpBackoffTime = interval * backoffNum / backoffDenom;
 
     // 3. Calculate `mrpBackoffTime *= (1.0 + random(0,1) * MRP_BACKOFF_JITTER)`
     uint32_t jitter = MRP_BACKOFF_JITTER_BASE + (computeMaxPossible ? UINT8_MAX : Crypto::GetRandU8());
@@ -273,7 +275,7 @@ System::Clock::Timestamp ReliableMessageMgr::GetBackoff(System::Clock::Timestamp
     mrpBackoffTime += CHIP_CONFIG_MRP_RETRY_INTERVAL_SENDER_BOOST;
 #endif // CHIP_DEVICE_CONFIG_ENABLE_DYNAMIC_MRP_CONFIG
 
-    return mrpBackoffTime;
+    return std::chrono::duration_cast<System::Clock::Timeout>(mrpBackoffTime);
 }
 
 void ReliableMessageMgr::StartRetransmision(RetransTableEntry * entry)

diff --git a/src/messaging/ReliableMessageMgr.h b/src/messaging/ReliableMessageMgr.h
@@ -112,8 +112,8 @@ class ReliableMessageMgr
      *
      *  @retval  The backoff time value, including jitter.
      */
-    static System::Clock::Timestamp GetBackoff(System::Clock::Timestamp baseInterval, uint8_t sendCount,
-                                               bool computeMaxPossible = false);
+    static System::Clock::Timeout GetBackoff(System::Clock::Timeout 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
@@ -125,9 +125,8 @@ 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)
+System::Clock::Timeout GetRetransmissionTimeout(System::Clock::Timeout activeInterval, System::Clock::Timeout idleInterval,
+                                                System::Clock::Timeout lastActivityTime, System::Clock::Timeout activityThreshold)
 {
     auto timeSinceLastActivity = (System::SystemClock().GetMonotonicTimestamp() - lastActivityTime);
 

diff --git a/src/messaging/ReliableMessageProtocolConfig.h b/src/messaging/ReliableMessageProtocolConfig.h
@@ -257,9 +257,8 @@ Optional<ReliableMessageProtocolConfig> GetLocalMRPConfig();
  *
  * @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);
+System::Clock::Timeout GetRetransmissionTimeout(System::Clock::Timeout activeInterval, System::Clock::Timeout idleInterval,
+                                                System::Clock::Timeout lastActivityTime, System::Clock::Timeout activityThreshold);
 
 #if CONFIG_BUILD_FOR_HOST_UNIT_TEST
 

diff --git a/src/messaging/tests/TestReliableMessageProtocol.cpp b/src/messaging/tests/TestReliableMessageProtocol.cpp
@@ -242,7 +242,7 @@ struct BackoffComplianceTestVector theBackoffComplianceTestVector[] = {
         .sendCount   = 2,
         .backoffBase = System::Clock::Timeout(300),
         .backoffMin  = System::Clock::Timeout(528),
-        .backoffMax  = System::Clock::Timeout(660),
+        .backoffMax  = System::Clock::Timeout(661),
     },
     {
         .sendCount   = 3,
@@ -254,62 +254,69 @@ struct BackoffComplianceTestVector theBackoffComplianceTestVector[] = {
         .sendCount   = 4,
         .backoffBase = System::Clock::Timeout(300),
         .backoffMin  = System::Clock::Timeout(1351),
-        .backoffMax  = System::Clock::Timeout(1690),
+        .backoffMax  = System::Clock::Timeout(1691),
     },
     {
         .sendCount   = 5,
         .backoffBase = System::Clock::Timeout(300),
         .backoffMin  = System::Clock::Timeout(2162),
-        .backoffMax  = System::Clock::Timeout(2704),
+        .backoffMax  = System::Clock::Timeout(2705),
     },
     {
         .sendCount   = 6,
         .backoffBase = System::Clock::Timeout(300),
         .backoffMin  = System::Clock::Timeout(2162),
-        .backoffMax  = System::Clock::Timeout(2704),
+        .backoffMax  = System::Clock::Timeout(2705),
     },
     {
         .sendCount   = 0,
         .backoffBase = System::Clock::Timeout(4000),
         .backoffMin  = System::Clock::Timeout(4400),
-        .backoffMax  = System::Clock::Timeout(5500),
+        .backoffMax  = System::Clock::Timeout(5503),
     },
     {
         .sendCount   = 1,
         .backoffBase = System::Clock::Timeout(4000),
         .backoffMin  = System::Clock::Timeout(4400),
-        .backoffMax  = System::Clock::Timeout(5500),
+        .backoffMax  = System::Clock::Timeout(5503),
     },
     {
         .sendCount   = 2,
         .backoffBase = System::Clock::Timeout(4000),
         .backoffMin  = System::Clock::Timeout(7040),
-        .backoffMax  = System::Clock::Timeout(8800),
+        .backoffMax  = System::Clock::Timeout(8805),
     },
     {
         .sendCount   = 3,
         .backoffBase = System::Clock::Timeout(4000),
         .backoffMin  = System::Clock::Timeout(11264),
-        .backoffMax  = System::Clock::Timeout(14081),
+        .backoffMax  = System::Clock::Timeout(14088),
     },
     {
         .sendCount   = 4,
         .backoffBase = System::Clock::Timeout(4000),
         .backoffMin  = System::Clock::Timeout(18022),
-        .backoffMax  = System::Clock::Timeout(22529),
+        .backoffMax  = System::Clock::Timeout(22541),
     },
     {
         .sendCount   = 5,
         .backoffBase = System::Clock::Timeout(4000),
         .backoffMin  = System::Clock::Timeout(28835),
-        .backoffMax  = System::Clock::Timeout(36045),
+        .backoffMax  = System::Clock::Timeout(36065),
     },
     {
         .sendCount   = 6,
         .backoffBase = System::Clock::Timeout(4000),
         .backoffMin  = System::Clock::Timeout(28835),
-        .backoffMax  = System::Clock::Timeout(36045),
+        .backoffMax  = System::Clock::Timeout(36065),
     },
+    {
+        // test theoretical worst-case 1-hour interval
+        .sendCount   = 4,
+        .backoffBase = System::Clock::Timeout(3'600'000),
+        .backoffMin  = System::Clock::Timeout(16'220'160),
+        .backoffMax  = System::Clock::Timeout(20'286'001),
+    }
 };
 
 } // namespace