From 71a5b3f1d188d116cc4815e5bc08544204b1e6f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Damian=20Kr=C3=B3lik?= <66667989+Damian-Nordic@users.noreply.github.com> Date: Mon, 1 Aug 2022 15:00:00 +0200 Subject: [PATCH] [mrp] Align MRP backoff time calculation with spec (#21356) * [mrp] Align ReliableMessageMgr::GetBackoff() naming with spec Make variables and comments in the function more aligned with the current spec version so that it is easier to compare the code with the spec. Signed-off-by: Damian Krolik * [mrp] Implement MRP_BACKOFF_MARGIN Base interval (active or idle) should be multiplied by MRP_BACKOFF_MARGIN = 1.1 before putting it in the MRP backoff formula. Signed-off-by: Damian Krolik * [mrp] Increase MRP backoff time for SED sender When sender is sleepy, it may take more time until it receives an acknowledgment from a peer, so MRP backoff time should be extended by the sleepy active interval. Signed-off-by: Damian Krolik --- src/messaging/ReliableMessageMgr.cpp | 76 +++++++++++------ src/messaging/ReliableMessageMgr.h | 4 +- .../tests/TestReliableMessageProtocol.cpp | 83 ++++++++++++++----- .../secure_channel/tests/TestPASESession.cpp | 2 +- 4 files changed, 117 insertions(+), 48 deletions(-) diff --git a/src/messaging/ReliableMessageMgr.cpp b/src/messaging/ReliableMessageMgr.cpp index 2acb7827cff48e..96a8563b1a4464 100644 --- a/src/messaging/ReliableMessageMgr.cpp +++ b/src/messaging/ReliableMessageMgr.cpp @@ -34,6 +34,7 @@ #include #include #include +#include using namespace chip::System::Clock::Literals; @@ -202,40 +203,65 @@ CHIP_ERROR ReliableMessageMgr::AddToRetransTable(ReliableMessageContext * rc, Re return CHIP_NO_ERROR; } -System::Clock::Timestamp ReliableMessageMgr::GetBackoff(System::Clock::Timestamp backoffBase, uint8_t sendCount) +System::Clock::Timestamp ReliableMessageMgr::GetBackoff(System::Clock::Timestamp baseInterval, uint8_t sendCount) { - static constexpr uint32_t MRP_BACKOFF_JITTER_BASE = 1024; - static constexpr uint32_t MRP_BACKOFF_BASE_NUMERATOR = 16; - static constexpr uint32_t MRP_BACKOFF_BASE_DENOMENATOR = 10; - static constexpr uint32_t MRP_BACKOFF_THRESHOLD = 1; - - System::Clock::Timestamp backoff = backoffBase; - - // Implement `t = i⋅MRP_BACKOFF_BASE^max(0,n−MRP_BACKOFF_THRESHOLD)` from Section 4.11.2.1. Retransmissions - - // Generate fixed point equivalent of `retryCount = max(0,n−MRP_BACKOFF_THRESHOLD)` - int retryCount = sendCount - MRP_BACKOFF_THRESHOLD; - if (retryCount < 0) - retryCount = 0; // Enforce floor - if (retryCount > 4) - retryCount = 4; // Enforce reasonable maximum after 5 tries - - // Generate fixed point equivalent of `backoff = i⋅1.6^retryCount` + // See section "4.11.8. Parameters and Constants" for the parameters below: + // MRP_BACKOFF_JITTER = 0.25 + constexpr uint32_t MRP_BACKOFF_JITTER_BASE = 1024; + // MRP_BACKOFF_MARGIN = 1.1 + constexpr uint32_t MRP_BACKOFF_MARGIN_NUMERATOR = 1127; + constexpr uint32_t MRP_BACKOFF_MARGIN_DENOMINATOR = 1024; + // MRP_BACKOFF_BASE = 1.6 + constexpr uint32_t MRP_BACKOFF_BASE_NUMERATOR = 16; + 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: + // 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 + // n == sendCount + + // 1. Calculate exponent `max(0,n−MRP_BACKOFF_THRESHOLD)` + int exponent = sendCount - MRP_BACKOFF_THRESHOLD; + if (exponent < 0) + exponent = 0; // Enforce floor + if (exponent > 4) + exponent = 4; // Enforce reasonable maximum after 5 tries + + // 2. Calculate `mrpBackoffTime = i * MRP_BACKOFF_BASE^(max(0,n-MRP_BACKOFF_THRESHOLD))` uint32_t backoffNum = 1; uint32_t backoffDenom = 1; - for (int i = 0; i < retryCount; i++) + + for (int i = 0; i < exponent; i++) { backoffNum *= MRP_BACKOFF_BASE_NUMERATOR; - backoffDenom *= MRP_BACKOFF_BASE_DENOMENATOR; + backoffDenom *= MRP_BACKOFF_BASE_DENOMINATOR; } - backoff = backoff * backoffNum / backoffDenom; - // Implement jitter scaler: `t *= (1.0+random(0,1)⋅MRP_BACKOFF_JITTER)` - // where jitter is random multiplier from 1.000 to 1.250: + 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(); - backoff = backoff * jitter / MRP_BACKOFF_JITTER_BASE; + mrpBackoffTime = mrpBackoffTime * jitter / MRP_BACKOFF_JITTER_BASE; + +#if CHIP_DEVICE_CONFIG_ENABLE_SED + // Implement: + // "A sleepy sender SHOULD increase t to also account for its own sleepy interval + // required to receive the acknowledgment" + DeviceLayer::ConnectivityManager::SEDIntervalsConfig sedIntervals; + + if (DeviceLayer::ConnectivityMgr().GetSEDIntervalsConfig(sedIntervals) == CHIP_NO_ERROR) + { + mrpBackoffTime += System::Clock::Timestamp(sedIntervals.ActiveIntervalMS); + } +#endif - return backoff; + return mrpBackoffTime; } void ReliableMessageMgr::StartRetransmision(RetransTableEntry * entry) diff --git a/src/messaging/ReliableMessageMgr.h b/src/messaging/ReliableMessageMgr.h index ca3a9252e2f255..889d236746732c 100644 --- a/src/messaging/ReliableMessageMgr.h +++ b/src/messaging/ReliableMessageMgr.h @@ -101,7 +101,7 @@ class ReliableMessageMgr /** * Calculate the backoff timer for the retransmission. * - * @param[in] backoffBase The base interval to use for the backoff calculation, either the active or idle interval. + * @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, @@ -109,7 +109,7 @@ class ReliableMessageMgr * * @retval The backoff time value, including jitter. */ - static System::Clock::Timestamp GetBackoff(System::Clock::Timestamp backoffBase, uint8_t sendCount); + static System::Clock::Timestamp GetBackoff(System::Clock::Timestamp baseInterval, uint8_t sendCount); /** * Start retranmisttion of cached encryped packet for current entry. diff --git a/src/messaging/tests/TestReliableMessageProtocol.cpp b/src/messaging/tests/TestReliableMessageProtocol.cpp index 68c0ad717693f4..e4e9f609d2b275 100644 --- a/src/messaging/tests/TestReliableMessageProtocol.cpp +++ b/src/messaging/tests/TestReliableMessageProtocol.cpp @@ -183,44 +183,86 @@ struct BackoffComplianceTestVector theBackoffComplianceTestVector[] = { { .sendCount = 0, .backoffBase = System::Clock::Timeout(300), - .backoffMin = System::Clock::Timeout(300), - .backoffMax = System::Clock::Timeout(375), + .backoffMin = System::Clock::Timeout(330), + .backoffMax = System::Clock::Timeout(413), }, { .sendCount = 1, .backoffBase = System::Clock::Timeout(300), - .backoffMin = System::Clock::Timeout(300), - .backoffMax = System::Clock::Timeout(375), + .backoffMin = System::Clock::Timeout(330), + .backoffMax = System::Clock::Timeout(413), }, { .sendCount = 2, .backoffBase = System::Clock::Timeout(300), - .backoffMin = System::Clock::Timeout(480), - .backoffMax = System::Clock::Timeout(600), + .backoffMin = System::Clock::Timeout(528), + .backoffMax = System::Clock::Timeout(660), }, { .sendCount = 3, .backoffBase = System::Clock::Timeout(300), - .backoffMin = System::Clock::Timeout(768), - .backoffMax = System::Clock::Timeout(960), + .backoffMin = System::Clock::Timeout(844), + .backoffMax = System::Clock::Timeout(1057), }, { .sendCount = 4, .backoffBase = System::Clock::Timeout(300), - .backoffMin = System::Clock::Timeout(1228), - .backoffMax = System::Clock::Timeout(1536), + .backoffMin = System::Clock::Timeout(1351), + .backoffMax = System::Clock::Timeout(1690), }, { .sendCount = 5, .backoffBase = System::Clock::Timeout(300), - .backoffMin = System::Clock::Timeout(1966), - .backoffMax = System::Clock::Timeout(2458), + .backoffMin = System::Clock::Timeout(2162), + .backoffMax = System::Clock::Timeout(2704), }, { .sendCount = 6, .backoffBase = System::Clock::Timeout(300), - .backoffMin = System::Clock::Timeout(1966), - .backoffMax = System::Clock::Timeout(2458), + .backoffMin = System::Clock::Timeout(2162), + .backoffMax = System::Clock::Timeout(2704), + }, + { + .sendCount = 0, + .backoffBase = System::Clock::Timeout(4000), + .backoffMin = System::Clock::Timeout(4400), + .backoffMax = System::Clock::Timeout(5500), + }, + { + .sendCount = 1, + .backoffBase = System::Clock::Timeout(4000), + .backoffMin = System::Clock::Timeout(4400), + .backoffMax = System::Clock::Timeout(5500), + }, + { + .sendCount = 2, + .backoffBase = System::Clock::Timeout(4000), + .backoffMin = System::Clock::Timeout(7040), + .backoffMax = System::Clock::Timeout(8800), + }, + { + .sendCount = 3, + .backoffBase = System::Clock::Timeout(4000), + .backoffMin = System::Clock::Timeout(11264), + .backoffMax = System::Clock::Timeout(14081), + }, + { + .sendCount = 4, + .backoffBase = System::Clock::Timeout(4000), + .backoffMin = System::Clock::Timeout(18022), + .backoffMax = System::Clock::Timeout(22529), + }, + { + .sendCount = 5, + .backoffBase = System::Clock::Timeout(4000), + .backoffMin = System::Clock::Timeout(28835), + .backoffMax = System::Clock::Timeout(36045), + }, + { + .sendCount = 6, + .backoffBase = System::Clock::Timeout(4000), + .backoffMin = System::Clock::Timeout(28835), + .backoffMax = System::Clock::Timeout(36045), }, }; @@ -323,7 +365,7 @@ void CheckResendApplicationMessage(nlTestSuite * inSuite, void * inContext) NL_TEST_ASSERT(inSuite, loopback.mDroppedMessageCount == 1); NL_TEST_ASSERT(inSuite, rm->TestGetCountRetransTable() == 1); - // Wait for the initial message to fail (should take 300-375ms) + // Wait for the initial message to fail (should take 330-413ms) ctx.GetIOContext().DriveIOUntil(1000_ms32, [&] { return loopback.mSentMessageCount >= 2; }); now = System::SystemClock().GetMonotonicTimestamp(); timeoutTime = now - startTime; @@ -340,7 +382,7 @@ void CheckResendApplicationMessage(nlTestSuite * inSuite, void * inContext) NL_TEST_ASSERT(inSuite, loopback.mDroppedMessageCount == 2); NL_TEST_ASSERT(inSuite, rm->TestGetCountRetransTable() == 1); - // Wait for the 1st retry to fail (should take 300-375ms) + // Wait for the 1st retry to fail (should take 330-413ms) ctx.GetIOContext().DriveIOUntil(1000_ms32, [&] { return loopback.mSentMessageCount >= 3; }); now = System::SystemClock().GetMonotonicTimestamp(); timeoutTime = now - startTime; @@ -357,7 +399,7 @@ void CheckResendApplicationMessage(nlTestSuite * inSuite, void * inContext) NL_TEST_ASSERT(inSuite, loopback.mDroppedMessageCount == 3); NL_TEST_ASSERT(inSuite, rm->TestGetCountRetransTable() == 1); - // Wait for the 2nd retry to fail (should take 480-600msms) + // Wait for the 2nd retry to fail (should take 528-660ms) ctx.GetIOContext().DriveIOUntil(1000_ms32, [&] { return loopback.mSentMessageCount >= 4; }); now = System::SystemClock().GetMonotonicTimestamp(); timeoutTime = now - startTime; @@ -374,8 +416,8 @@ void CheckResendApplicationMessage(nlTestSuite * inSuite, void * inContext) NL_TEST_ASSERT(inSuite, loopback.mDroppedMessageCount == 4); NL_TEST_ASSERT(inSuite, rm->TestGetCountRetransTable() == 1); - // Wait for the 3rd retry to fail (should take 768-960ms) - ctx.GetIOContext().DriveIOUntil(1000_ms32, [&] { return loopback.mSentMessageCount >= 5; }); + // Wait for the 3rd retry to fail (should take 845-1056ms) + ctx.GetIOContext().DriveIOUntil(1500_ms32, [&] { return loopback.mSentMessageCount >= 5; }); now = System::SystemClock().GetMonotonicTimestamp(); timeoutTime = now - startTime; ChipLogProgress(Test, "Attempt #4 Timeout : %d ms", timeoutTime.count()); @@ -1613,7 +1655,8 @@ void CheckGetBackoff(nlTestSuite * inSuite, void * inContext) { struct BackoffComplianceTestVector * test = &theBackoffComplianceTestVector[i]; System::Clock::Timeout backoff = ReliableMessageMgr::GetBackoff(test->backoffBase, test->sendCount); - ChipLogProgress(Test, "Backoff # %d: %" PRIu32, test->sendCount, (uint32_t) backoff.count()); + ChipLogProgress(Test, "Backoff base %" PRIu32 " # %d: %" PRIu32, test->backoffBase.count(), test->sendCount, + backoff.count()); NL_TEST_ASSERT(inSuite, backoff >= test->backoffMin); NL_TEST_ASSERT(inSuite, backoff <= test->backoffMax); diff --git a/src/protocols/secure_channel/tests/TestPASESession.cpp b/src/protocols/secure_channel/tests/TestPASESession.cpp index 932d665a22b2c8..61830fa174b530 100644 --- a/src/protocols/secure_channel/tests/TestPASESession.cpp +++ b/src/protocols/secure_channel/tests/TestPASESession.cpp @@ -279,7 +279,7 @@ void SecurePairingHandshakeTestCommon(nlTestSuite * inSuite, void * inContext, S while (delegate.mMessageDropped) { - chip::test_utils::SleepMillis(85); + chip::test_utils::SleepMillis(100); delegate.mMessageDropped = false; ReliableMessageMgr::Timeout(&ctx.GetSystemLayer(), ctx.GetExchangeManager().GetReliableMessageMgr()); ctx.DrainAndServiceIO();