Skip to content

Commit

Permalink
[mrp] Make GetBackoff() use Timeout instead of Timestamp type (#33093)
Browse files Browse the repository at this point in the history 
* [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.

* Restyled by clang-format

* Code review

---------

Co-authored-by: Restyled.io <[email protected]>
  • Loading branch information
@Damian-Nordic @restyled-commits
Damian-Nordic and restyled-commits authored Apr 30, 2024
1 parent aec50d4 commit ffaeaa1
Show file tree
Hide file tree
Showing 5 changed files with 111 additions and 106 deletions.
26 changes: 14 additions & 12 deletions src/messaging/ReliableMessageMgr.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)`
Expand All @@ -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());
Expand All @@ -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)
Expand Down Expand Up @@ -463,7 +465,7 @@ CHIP_ERROR ReliableMessageMgr::MapSendError(CHIP_ERROR error, uint16_t exchangeI

void ReliableMessageMgr::CalculateNextRetransTime(RetransTableEntry & entry)
{
System::Clock::Timestamp baseTimeout = System::Clock::Milliseconds64(0);
System::Clock::Timeout baseTimeout = System::Clock::Timeout(0);

// Check if we have received at least one application-level message
if (entry.ec->HasReceivedAtLeastOneMessage())
Expand All @@ -478,8 +480,8 @@ void ReliableMessageMgr::CalculateNextRetransTime(RetransTableEntry & entry)
baseTimeout = entry.ec->GetSessionHandle()->GetMRPBaseTimeout();
}

System::Clock::Timestamp backoff = ReliableMessageMgr::GetBackoff(baseTimeout, entry.sendCount);
entry.nextRetransTime = System::SystemClock().GetMonotonicTimestamp() + backoff;
System::Clock::Timeout backoff = ReliableMessageMgr::GetBackoff(baseTimeout, entry.sendCount);
entry.nextRetransTime = System::SystemClock().GetMonotonicTimestamp() + backoff;
}

#if CHIP_CONFIG_TEST
Expand Down
4 changes: 2 additions & 2 deletions src/messaging/ReliableMessageMgr.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
5 changes: 2 additions & 3 deletions src/messaging/ReliableMessageProtocolConfig.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
5 changes: 2 additions & 3 deletions src/messaging/ReliableMessageProtocolConfig.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
177 changes: 91 additions & 86 deletions src/messaging/tests/TestReliableMessageProtocol.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -232,92 +232,97 @@ struct BackoffComplianceTestVector
System::Clock::Timeout backoffMax;
};

struct BackoffComplianceTestVector theBackoffComplianceTestVector[] = {
{
.sendCount = 0,
.backoffBase = System::Clock::Timeout(300),
.backoffMin = System::Clock::Timeout(330),
.backoffMax = System::Clock::Timeout(413),
},
{
.sendCount = 1,
.backoffBase = System::Clock::Timeout(300),
.backoffMin = System::Clock::Timeout(330),
.backoffMax = System::Clock::Timeout(413),
},
{
.sendCount = 2,
.backoffBase = System::Clock::Timeout(300),
.backoffMin = System::Clock::Timeout(528),
.backoffMax = System::Clock::Timeout(660),
},
{
.sendCount = 3,
.backoffBase = System::Clock::Timeout(300),
.backoffMin = System::Clock::Timeout(844),
.backoffMax = System::Clock::Timeout(1057),
},
{
.sendCount = 4,
.backoffBase = System::Clock::Timeout(300),
.backoffMin = System::Clock::Timeout(1351),
.backoffMax = System::Clock::Timeout(1690),
},
{
.sendCount = 5,
.backoffBase = System::Clock::Timeout(300),
.backoffMin = System::Clock::Timeout(2162),
.backoffMax = System::Clock::Timeout(2704),
},
{
.sendCount = 6,
.backoffBase = System::Clock::Timeout(300),
.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),
},
};
struct BackoffComplianceTestVector theBackoffComplianceTestVector[] = { {
.sendCount = 0,
.backoffBase = System::Clock::Timeout(300),
.backoffMin = System::Clock::Timeout(330),
.backoffMax = System::Clock::Timeout(413),
},
{
.sendCount = 1,
.backoffBase = System::Clock::Timeout(300),
.backoffMin = System::Clock::Timeout(330),
.backoffMax = System::Clock::Timeout(413),
},
{
.sendCount = 2,
.backoffBase = System::Clock::Timeout(300),
.backoffMin = System::Clock::Timeout(528),
.backoffMax = System::Clock::Timeout(661),
},
{
.sendCount = 3,
.backoffBase = System::Clock::Timeout(300),
.backoffMin = System::Clock::Timeout(844),
.backoffMax = System::Clock::Timeout(1057),
},
{
.sendCount = 4,
.backoffBase = System::Clock::Timeout(300),
.backoffMin = System::Clock::Timeout(1351),
.backoffMax = System::Clock::Timeout(1691),
},
{
.sendCount = 5,
.backoffBase = System::Clock::Timeout(300),
.backoffMin = System::Clock::Timeout(2162),
.backoffMax = System::Clock::Timeout(2705),
},
{
.sendCount = 6,
.backoffBase = System::Clock::Timeout(300),
.backoffMin = System::Clock::Timeout(2162),
.backoffMax = System::Clock::Timeout(2705),
},
{
.sendCount = 0,
.backoffBase = System::Clock::Timeout(4000),
.backoffMin = System::Clock::Timeout(4400),
.backoffMax = System::Clock::Timeout(5503),
},
{
.sendCount = 1,
.backoffBase = System::Clock::Timeout(4000),
.backoffMin = System::Clock::Timeout(4400),
.backoffMax = System::Clock::Timeout(5503),
},
{
.sendCount = 2,
.backoffBase = System::Clock::Timeout(4000),
.backoffMin = System::Clock::Timeout(7040),
.backoffMax = System::Clock::Timeout(8805),
},
{
.sendCount = 3,
.backoffBase = System::Clock::Timeout(4000),
.backoffMin = System::Clock::Timeout(11264),
.backoffMax = System::Clock::Timeout(14088),
},
{
.sendCount = 4,
.backoffBase = System::Clock::Timeout(4000),
.backoffMin = System::Clock::Timeout(18022),
.backoffMax = System::Clock::Timeout(22541),
},
{
.sendCount = 5,
.backoffBase = System::Clock::Timeout(4000),
.backoffMin = System::Clock::Timeout(28835),
.backoffMax = System::Clock::Timeout(36065),
},
{
.sendCount = 6,
.backoffBase = System::Clock::Timeout(4000),
.backoffMin = System::Clock::Timeout(28835),
.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

Expand Down

0 comments on commit ffaeaa1

Please sign in to comment.