Skip to content

Commit ffaeaa1

Browse files
Damian-Nordicrestyled-commits
Damian-Nordic
and
restyled-commits
authored
[mrp] Make GetBackoff() use Timeout instead of Timestamp type (project-chip#33093)
* [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 <commits@restyled.io>
1 parent aec50d4 commit ffaeaa1

5 files changed

+111
-106
lines changed

‎src/messaging/ReliableMessageMgr.cpp

+14-12
Original file line numberDiff line numberDiff line change
@@ -213,8 +213,8 @@ CHIP_ERROR ReliableMessageMgr::AddToRetransTable(ReliableMessageContext * rc, Re
213213
return CHIP_NO_ERROR;
214214
}
215215

216-
System::Clock::Timestamp ReliableMessageMgr::GetBackoff(System::Clock::Timestamp baseInterval, uint8_t sendCount,
217-
bool computeMaxPossible)
216+
System::Clock::Timeout ReliableMessageMgr::GetBackoff(System::Clock::Timeout baseInterval, uint8_t sendCount,
217+
bool computeMaxPossible)
218218
{
219219
// See section "4.11.8. Parameters and Constants" for the parameters below:
220220
// MRP_BACKOFF_JITTER = 0.25
@@ -227,14 +227,16 @@ System::Clock::Timestamp ReliableMessageMgr::GetBackoff(System::Clock::Timestamp
227227
constexpr uint32_t MRP_BACKOFF_BASE_DENOMINATOR = 10;
228228
constexpr int MRP_BACKOFF_THRESHOLD = 1;
229229

230-
// Implement `i = MRP_BACKOFF_MARGIN * i` from section "4.11.2.1. Retransmissions", where:
231-
// i == baseInterval
232-
baseInterval = baseInterval * MRP_BACKOFF_MARGIN_NUMERATOR / MRP_BACKOFF_MARGIN_DENOMINATOR;
230+
// Implement `i = MRP_BACKOFF_MARGIN * i` from section "4.12.2.1. Retransmissions", where:
231+
// i == interval
232+
System::Clock::Milliseconds64 interval = baseInterval;
233+
interval *= MRP_BACKOFF_MARGIN_NUMERATOR;
234+
interval /= MRP_BACKOFF_MARGIN_DENOMINATOR;
233235

234236
// Implement:
235237
// mrpBackoffTime = i * MRP_BACKOFF_BASE^(max(0,n-MRP_BACKOFF_THRESHOLD)) * (1.0 + random(0,1) * MRP_BACKOFF_JITTER)
236-
// from section "4.11.2.1. Retransmissions", where:
237-
// i == baseInterval
238+
// from section "4.12.2.1. Retransmissions", where:
239+
// i == interval
238240
// n == sendCount
239241

240242
// 1. Calculate exponent `max(0,n−MRP_BACKOFF_THRESHOLD)`
@@ -254,7 +256,7 @@ System::Clock::Timestamp ReliableMessageMgr::GetBackoff(System::Clock::Timestamp
254256
backoffDenom *= MRP_BACKOFF_BASE_DENOMINATOR;
255257
}
256258

257-
System::Clock::Timestamp mrpBackoffTime = baseInterval * backoffNum / backoffDenom;
259+
System::Clock::Milliseconds64 mrpBackoffTime = interval * backoffNum / backoffDenom;
258260

259261
// 3. Calculate `mrpBackoffTime *= (1.0 + random(0,1) * MRP_BACKOFF_JITTER)`
260262
uint32_t jitter = MRP_BACKOFF_JITTER_BASE + (computeMaxPossible ? UINT8_MAX : Crypto::GetRandU8());
@@ -273,7 +275,7 @@ System::Clock::Timestamp ReliableMessageMgr::GetBackoff(System::Clock::Timestamp
273275
mrpBackoffTime += CHIP_CONFIG_MRP_RETRY_INTERVAL_SENDER_BOOST;
274276
#endif // CHIP_DEVICE_CONFIG_ENABLE_DYNAMIC_MRP_CONFIG
275277

276-
return mrpBackoffTime;
278+
return std::chrono::duration_cast<System::Clock::Timeout>(mrpBackoffTime);
277279
}
278280

279281
void ReliableMessageMgr::StartRetransmision(RetransTableEntry * entry)
@@ -463,7 +465,7 @@ CHIP_ERROR ReliableMessageMgr::MapSendError(CHIP_ERROR error, uint16_t exchangeI
463465

464466
void ReliableMessageMgr::CalculateNextRetransTime(RetransTableEntry & entry)
465467
{
466-
System::Clock::Timestamp baseTimeout = System::Clock::Milliseconds64(0);
468+
System::Clock::Timeout baseTimeout = System::Clock::Timeout(0);
467469

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

481-
System::Clock::Timestamp backoff = ReliableMessageMgr::GetBackoff(baseTimeout, entry.sendCount);
482-
entry.nextRetransTime = System::SystemClock().GetMonotonicTimestamp() + backoff;
483+
System::Clock::Timeout backoff = ReliableMessageMgr::GetBackoff(baseTimeout, entry.sendCount);
484+
entry.nextRetransTime = System::SystemClock().GetMonotonicTimestamp() + backoff;
483485
}
484486

485487
#if CHIP_CONFIG_TEST

‎src/messaging/ReliableMessageMgr.h

+2-2
Original file line numberDiff line numberDiff line change
@@ -112,8 +112,8 @@ class ReliableMessageMgr
112112
*
113113
* @retval The backoff time value, including jitter.
114114
*/
115-
static System::Clock::Timestamp GetBackoff(System::Clock::Timestamp baseInterval, uint8_t sendCount,
116-
bool computeMaxPossible = false);
115+
static System::Clock::Timeout GetBackoff(System::Clock::Timeout baseInterval, uint8_t sendCount,
116+
bool computeMaxPossible = false);
117117

118118
/**
119119
* Start retranmisttion of cached encryped packet for current entry.

‎src/messaging/ReliableMessageProtocolConfig.cpp

+2-3
Original file line numberDiff line numberDiff line change
@@ -125,9 +125,8 @@ Optional<ReliableMessageProtocolConfig> GetLocalMRPConfig()
125125
: Optional<ReliableMessageProtocolConfig>::Value(config);
126126
}
127127

128-
System::Clock::Timestamp GetRetransmissionTimeout(System::Clock::Timestamp activeInterval, System::Clock::Timestamp idleInterval,
129-
System::Clock::Timestamp lastActivityTime,
130-
System::Clock::Timestamp activityThreshold)
128+
System::Clock::Timeout GetRetransmissionTimeout(System::Clock::Timeout activeInterval, System::Clock::Timeout idleInterval,
129+
System::Clock::Timeout lastActivityTime, System::Clock::Timeout activityThreshold)
131130
{
132131
auto timeSinceLastActivity = (System::SystemClock().GetMonotonicTimestamp() - lastActivityTime);
133132

‎src/messaging/ReliableMessageProtocolConfig.h

+2-3
Original file line numberDiff line numberDiff line change
@@ -257,9 +257,8 @@ Optional<ReliableMessageProtocolConfig> GetLocalMRPConfig();
257257
*
258258
* @return The maximum transmission time
259259
*/
260-
System::Clock::Timestamp GetRetransmissionTimeout(System::Clock::Timestamp activeInterval, System::Clock::Timestamp idleInterval,
261-
System::Clock::Timestamp lastActivityTime,
262-
System::Clock::Timestamp activityThreshold);
260+
System::Clock::Timeout GetRetransmissionTimeout(System::Clock::Timeout activeInterval, System::Clock::Timeout idleInterval,
261+
System::Clock::Timeout lastActivityTime, System::Clock::Timeout activityThreshold);
263262

264263
#if CONFIG_BUILD_FOR_HOST_UNIT_TEST
265264

‎src/messaging/tests/TestReliableMessageProtocol.cpp

+91-86
Original file line numberDiff line numberDiff line change
@@ -232,92 +232,97 @@ struct BackoffComplianceTestVector
232232
System::Clock::Timeout backoffMax;
233233
};
234234

235-
struct BackoffComplianceTestVector theBackoffComplianceTestVector[] = {
236-
{
237-
.sendCount = 0,
238-
.backoffBase = System::Clock::Timeout(300),
239-
.backoffMin = System::Clock::Timeout(330),
240-
.backoffMax = System::Clock::Timeout(413),
241-
},
242-
{
243-
.sendCount = 1,
244-
.backoffBase = System::Clock::Timeout(300),
245-
.backoffMin = System::Clock::Timeout(330),
246-
.backoffMax = System::Clock::Timeout(413),
247-
},
248-
{
249-
.sendCount = 2,
250-
.backoffBase = System::Clock::Timeout(300),
251-
.backoffMin = System::Clock::Timeout(528),
252-
.backoffMax = System::Clock::Timeout(660),
253-
},
254-
{
255-
.sendCount = 3,
256-
.backoffBase = System::Clock::Timeout(300),
257-
.backoffMin = System::Clock::Timeout(844),
258-
.backoffMax = System::Clock::Timeout(1057),
259-
},
260-
{
261-
.sendCount = 4,
262-
.backoffBase = System::Clock::Timeout(300),
263-
.backoffMin = System::Clock::Timeout(1351),
264-
.backoffMax = System::Clock::Timeout(1690),
265-
},
266-
{
267-
.sendCount = 5,
268-
.backoffBase = System::Clock::Timeout(300),
269-
.backoffMin = System::Clock::Timeout(2162),
270-
.backoffMax = System::Clock::Timeout(2704),
271-
},
272-
{
273-
.sendCount = 6,
274-
.backoffBase = System::Clock::Timeout(300),
275-
.backoffMin = System::Clock::Timeout(2162),
276-
.backoffMax = System::Clock::Timeout(2704),
277-
},
278-
{
279-
.sendCount = 0,
280-
.backoffBase = System::Clock::Timeout(4000),
281-
.backoffMin = System::Clock::Timeout(4400),
282-
.backoffMax = System::Clock::Timeout(5500),
283-
},
284-
{
285-
.sendCount = 1,
286-
.backoffBase = System::Clock::Timeout(4000),
287-
.backoffMin = System::Clock::Timeout(4400),
288-
.backoffMax = System::Clock::Timeout(5500),
289-
},
290-
{
291-
.sendCount = 2,
292-
.backoffBase = System::Clock::Timeout(4000),
293-
.backoffMin = System::Clock::Timeout(7040),
294-
.backoffMax = System::Clock::Timeout(8800),
295-
},
296-
{
297-
.sendCount = 3,
298-
.backoffBase = System::Clock::Timeout(4000),
299-
.backoffMin = System::Clock::Timeout(11264),
300-
.backoffMax = System::Clock::Timeout(14081),
301-
},
302-
{
303-
.sendCount = 4,
304-
.backoffBase = System::Clock::Timeout(4000),
305-
.backoffMin = System::Clock::Timeout(18022),
306-
.backoffMax = System::Clock::Timeout(22529),
307-
},
308-
{
309-
.sendCount = 5,
310-
.backoffBase = System::Clock::Timeout(4000),
311-
.backoffMin = System::Clock::Timeout(28835),
312-
.backoffMax = System::Clock::Timeout(36045),
313-
},
314-
{
315-
.sendCount = 6,
316-
.backoffBase = System::Clock::Timeout(4000),
317-
.backoffMin = System::Clock::Timeout(28835),
318-
.backoffMax = System::Clock::Timeout(36045),
319-
},
320-
};
235+
struct BackoffComplianceTestVector theBackoffComplianceTestVector[] = { {
236+
.sendCount = 0,
237+
.backoffBase = System::Clock::Timeout(300),
238+
.backoffMin = System::Clock::Timeout(330),
239+
.backoffMax = System::Clock::Timeout(413),
240+
},
241+
{
242+
.sendCount = 1,
243+
.backoffBase = System::Clock::Timeout(300),
244+
.backoffMin = System::Clock::Timeout(330),
245+
.backoffMax = System::Clock::Timeout(413),
246+
},
247+
{
248+
.sendCount = 2,
249+
.backoffBase = System::Clock::Timeout(300),
250+
.backoffMin = System::Clock::Timeout(528),
251+
.backoffMax = System::Clock::Timeout(661),
252+
},
253+
{
254+
.sendCount = 3,
255+
.backoffBase = System::Clock::Timeout(300),
256+
.backoffMin = System::Clock::Timeout(844),
257+
.backoffMax = System::Clock::Timeout(1057),
258+
},
259+
{
260+
.sendCount = 4,
261+
.backoffBase = System::Clock::Timeout(300),
262+
.backoffMin = System::Clock::Timeout(1351),
263+
.backoffMax = System::Clock::Timeout(1691),
264+
},
265+
{
266+
.sendCount = 5,
267+
.backoffBase = System::Clock::Timeout(300),
268+
.backoffMin = System::Clock::Timeout(2162),
269+
.backoffMax = System::Clock::Timeout(2705),
270+
},
271+
{
272+
.sendCount = 6,
273+
.backoffBase = System::Clock::Timeout(300),
274+
.backoffMin = System::Clock::Timeout(2162),
275+
.backoffMax = System::Clock::Timeout(2705),
276+
},
277+
{
278+
.sendCount = 0,
279+
.backoffBase = System::Clock::Timeout(4000),
280+
.backoffMin = System::Clock::Timeout(4400),
281+
.backoffMax = System::Clock::Timeout(5503),
282+
},
283+
{
284+
.sendCount = 1,
285+
.backoffBase = System::Clock::Timeout(4000),
286+
.backoffMin = System::Clock::Timeout(4400),
287+
.backoffMax = System::Clock::Timeout(5503),
288+
},
289+
{
290+
.sendCount = 2,
291+
.backoffBase = System::Clock::Timeout(4000),
292+
.backoffMin = System::Clock::Timeout(7040),
293+
.backoffMax = System::Clock::Timeout(8805),
294+
},
295+
{
296+
.sendCount = 3,
297+
.backoffBase = System::Clock::Timeout(4000),
298+
.backoffMin = System::Clock::Timeout(11264),
299+
.backoffMax = System::Clock::Timeout(14088),
300+
},
301+
{
302+
.sendCount = 4,
303+
.backoffBase = System::Clock::Timeout(4000),
304+
.backoffMin = System::Clock::Timeout(18022),
305+
.backoffMax = System::Clock::Timeout(22541),
306+
},
307+
{
308+
.sendCount = 5,
309+
.backoffBase = System::Clock::Timeout(4000),
310+
.backoffMin = System::Clock::Timeout(28835),
311+
.backoffMax = System::Clock::Timeout(36065),
312+
},
313+
{
314+
.sendCount = 6,
315+
.backoffBase = System::Clock::Timeout(4000),
316+
.backoffMin = System::Clock::Timeout(28835),
317+
.backoffMax = System::Clock::Timeout(36065),
318+
},
319+
{
320+
// test theoretical worst-case 1-hour interval
321+
.sendCount = 4,
322+
.backoffBase = System::Clock::Timeout(3'600'000),
323+
.backoffMin = System::Clock::Timeout(16'220'160),
324+
.backoffMax = System::Clock::Timeout(20'286'001),
325+
} };
321326

322327
} // namespace
323328

0 commit comments

Comments
 (0)