From 0960312889c6435beb61e074c36ab49a9c6c6d25 Mon Sep 17 00:00:00 2001 From: Damian Krolik Date: Mon, 22 Apr 2024 17:45:05 +0200 Subject: [PATCH] [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. --- src/messaging/ReliableMessageMgr.cpp | 20 +++++++------ src/messaging/ReliableMessageMgr.h | 4 +-- .../ReliableMessageProtocolConfig.cpp | 5 ++-- src/messaging/ReliableMessageProtocolConfig.h | 5 ++-- .../tests/TestReliableMessageProtocol.cpp | 29 ++++++++++++------- 5 files changed, 35 insertions(+), 28 deletions(-) diff --git a/src/messaging/ReliableMessageMgr.cpp b/src/messaging/ReliableMessageMgr.cpp index 53e292c9b32b00..ac029fb1aae7a0 100644 --- 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(mrpBackoffTime); } void ReliableMessageMgr::StartRetransmision(RetransTableEntry * entry) diff --git a/src/messaging/ReliableMessageMgr.h b/src/messaging/ReliableMessageMgr.h index ae953cbddd5ad5..953de1db0aea40 100644 --- 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 index e635e91940e0ef..352e89cf4576c1 100644 --- a/src/messaging/ReliableMessageProtocolConfig.cpp +++ b/src/messaging/ReliableMessageProtocolConfig.cpp @@ -125,9 +125,8 @@ Optional GetLocalMRPConfig() : Optional::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 index 2ab1c139657fc6..e3255a780b7b70 100644 --- a/src/messaging/ReliableMessageProtocolConfig.h +++ b/src/messaging/ReliableMessageProtocolConfig.h @@ -257,9 +257,8 @@ Optional 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 index 04b8a1aadb149f..671808eef9070a 100644 --- 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