Skip to content

Commit

Permalink
Use safer System::Clock types in transport and messaging (#10913)
Browse files Browse the repository at this point in the history
#### Problem

Code uses plain integers to represent time values and relies on
users to get the unit scale correct.

Part of #10062 _Some operations on System::Clock types are not safe_

#### Change overview

Convert `src/transport` and `src/messaging` to use the safer `Clock` types.

#### Testing

CI; no change to functionality intended.
Conversion includes `src/messaging/tests/echo/echo_requester.cpp`
and several `src/transport/raw/tests/`.
  • Loading branch information
kpschoedel authored and pull[bot] committed Oct 28, 2021
1 parent 1864f92 commit 4ff81f5
Show file tree
Hide file tree
Showing 9 changed files with 43 additions and 41 deletions.
2 changes: 1 addition & 1 deletion src/messaging/ReliableMessageContext.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,7 @@ CHIP_ERROR ReliableMessageContext::HandleNeedsAckInner(uint32_t messageCounter,
SetPendingPeerAckMessageCounter(messageCounter);
mNextAckTimeTick = static_cast<uint16_t>(
CHIP_CONFIG_RMP_DEFAULT_ACK_TIMEOUT_TICK +
GetReliableMessageMgr()->GetTickCounterFromTimeDelta(System::SystemClock().GetMonotonicMilliseconds()));
GetReliableMessageMgr()->GetTickCounterFromTimeDelta(System::SystemClock().GetMonotonicTimestamp()));
return CHIP_NO_ERROR;
}
}
Expand Down
33 changes: 17 additions & 16 deletions src/messaging/ReliableMessageMgr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,8 @@ ReliableMessageMgr::~ReliableMessageMgr() {}
void ReliableMessageMgr::Init(chip::System::Layer * systemLayer, SessionManager * sessionManager)
{
mSystemLayer = systemLayer;
mTimeStampBase = System::SystemClock().GetMonotonicMilliseconds();
mCurrentTimerExpiry = 0;
mTimeStampBase = System::SystemClock().GetMonotonicTimestamp();
mCurrentTimerExpiry = System::Clock::Zero;
}

void ReliableMessageMgr::Shutdown()
Expand All @@ -75,12 +75,12 @@ void ReliableMessageMgr::Shutdown()
mSystemLayer = nullptr;
}

uint64_t ReliableMessageMgr::GetTickCounterFromTimePeriod(uint64_t period)
uint64_t ReliableMessageMgr::GetTickCounterFromTimePeriod(System::Clock::Milliseconds64 period)
{
return (period >> mTimerIntervalShift);
return (period.count() >> mTimerIntervalShift);
}

uint64_t ReliableMessageMgr::GetTickCounterFromTimeDelta(uint64_t newTime)
uint64_t ReliableMessageMgr::GetTickCounterFromTimeDelta(System::Clock::Timestamp newTime)
{
return GetTickCounterFromTimePeriod(newTime - mTimeStampBase);
}
Expand Down Expand Up @@ -197,7 +197,7 @@ static void TickProceed(uint16_t & time, uint64_t ticks)

void ReliableMessageMgr::ExpireTicks()
{
uint64_t now = System::SystemClock().GetMonotonicMilliseconds();
System::Clock::Timestamp now = System::SystemClock().GetMonotonicTimestamp();

// Number of full ticks elapsed since last timer processing. We always round down
// to the previous tick. If we are between tick boundaries, the extra time since the
Expand Down Expand Up @@ -231,10 +231,10 @@ void ReliableMessageMgr::ExpireTicks()
});

// Re-Adjust the base time stamp to the most recent tick boundary
mTimeStampBase += (deltaTicks << mTimerIntervalShift);
mTimeStampBase += System::Clock::Milliseconds32(deltaTicks << mTimerIntervalShift);

#if defined(RMP_TICKLESS_DEBUG)
ChipLogDetail(ExchangeManager, "ReliableMessageMgr::ExpireTicks mTimeStampBase to %" PRIu64, mTimeStampBase);
ChipLogDetail(ExchangeManager, "ReliableMessageMgr::ExpireTicks mTimeStampBase to %" PRIu64, mTimeStampBase.count());
#endif
}

Expand Down Expand Up @@ -278,9 +278,8 @@ CHIP_ERROR ReliableMessageMgr::AddToRetransTable(ReliableMessageContext * rc, Re

void ReliableMessageMgr::StartRetransmision(RetransTableEntry * entry)
{
entry->nextRetransTimeTick =
static_cast<uint16_t>(entry->ec->GetInitialRetransmitTimeoutTick() +
GetTickCounterFromTimeDelta(System::SystemClock().GetMonotonicMilliseconds()));
entry->nextRetransTimeTick = static_cast<uint16_t>(entry->ec->GetInitialRetransmitTimeoutTick() +
GetTickCounterFromTimeDelta(System::SystemClock().GetMonotonicTimestamp()));

// Check if the timer needs to be started and start it.
StartTimer();
Expand Down Expand Up @@ -437,7 +436,8 @@ void ReliableMessageMgr::StartTimer()
if (foundWake)
{
// Set timer for next tick boundary - subtract the elapsed time from the current tick
System::Clock::MonotonicMilliseconds timerExpiry = (nextWakeTimeTick << mTimerIntervalShift) + mTimeStampBase;
System::Clock::Timestamp timerExpiry =
System::Clock::Milliseconds32(nextWakeTimeTick << mTimerIntervalShift) + mTimeStampBase;

#if defined(RMP_TICKLESS_DEBUG)
ChipLogDetail(ExchangeManager, "ReliableMessageMgr::StartTimer wake at %" PRIu64 " ms (%" PRIu64 " %" PRIu64 ")",
Expand All @@ -447,14 +447,15 @@ void ReliableMessageMgr::StartTimer()
{
// If the tick boundary has expired in the past (delayed processing of event due to other system activity),
// expire the timer immediately
uint64_t now = System::SystemClock().GetMonotonicMilliseconds();
uint64_t timerArmValue = (timerExpiry > now) ? timerExpiry - now : 0;
System::Clock::Timestamp now = System::SystemClock().GetMonotonicTimestamp();
System::Clock::Timeout timerArmValue = (timerExpiry > now) ? timerExpiry - now : System::Clock::Zero;

#if defined(RMP_TICKLESS_DEBUG)
ChipLogDetail(ExchangeManager, "ReliableMessageMgr::StartTimer set timer for %" PRIu64, timerArmValue);
ChipLogDetail(ExchangeManager, "ReliableMessageMgr::StartTimer set timer for %" PRIu32 " ms",
System::Clock::Milliseconds32(timerArmValue).count());
#endif
StopTimer();
res = mSystemLayer->StartTimer(System::Clock::Milliseconds32(timerArmValue), Timeout, this);
res = mSystemLayer->StartTimer(timerArmValue, Timeout, this);

VerifyOrDieWithMsg(res == CHIP_NO_ERROR, ExchangeManager,
"Cannot start ReliableMessageMgr::Timeout %" CHIP_ERROR_FORMAT, res.Format());
Expand Down
10 changes: 5 additions & 5 deletions src/messaging/ReliableMessageMgr.h
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ class ReliableMessageMgr
*
* @return Tick count for the time period.
*/
uint64_t GetTickCounterFromTimePeriod(uint64_t period);
uint64_t GetTickCounterFromTimePeriod(System::Clock::Milliseconds64 period);

/**
* Return a tick counter value between the given time and the stored time.
Expand All @@ -92,7 +92,7 @@ class ReliableMessageMgr
*
* @return Tick count of the difference between the given time and the stored time.
*/
uint64_t GetTickCounterFromTimeDelta(uint64_t newTime);
uint64_t GetTickCounterFromTimeDelta(System::Clock::Timestamp newTime);

/**
* Iterate through active exchange contexts and retrans table entries. If an
Expand Down Expand Up @@ -230,9 +230,9 @@ class ReliableMessageMgr
private:
BitMapObjectPool<ExchangeContext, CHIP_CONFIG_MAX_EXCHANGE_CONTEXTS> & mContextPool;
chip::System::Layer * mSystemLayer;
uint64_t mTimeStampBase; // ReliableMessageProtocol timer base value to add offsets to evaluate timeouts
System::Clock::MonotonicMilliseconds mCurrentTimerExpiry; // Tracks when the ReliableMessageProtocol timer will next expire
uint16_t mTimerIntervalShift; // ReliableMessageProtocol Timer tick period shift
System::Clock::Timestamp mTimeStampBase; // ReliableMessageProtocol timer base value to add offsets to evaluate timeouts
System::Clock::Timestamp mCurrentTimerExpiry; // Tracks when the ReliableMessageProtocol timer will next expire
uint16_t mTimerIntervalShift; // ReliableMessageProtocol Timer tick period shift

/* Placeholder function to run a function for all exchanges */
template <typename Function>
Expand Down
21 changes: 11 additions & 10 deletions src/messaging/tests/echo/echo_requester.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,8 @@ namespace {
// Max value for the number of EchoRequests sent.
constexpr size_t kMaxEchoCount = 3;

// The CHIP Echo interval time in milliseconds.
constexpr int32_t gEchoInterval = 1000;
// The CHIP Echo interval time.
constexpr chip::System::Clock::Timeout gEchoInterval = chip::System::Clock::Seconds16(1);

constexpr chip::FabricIndex gFabricIndex = 0;

Expand All @@ -62,7 +62,7 @@ chip::TransportMgr<chip::Transport::TCP<kMaxTcpActiveConnectionCount, kMaxTcpPen
chip::Inet::IPAddress gDestAddr;

// The last time a CHIP Echo was attempted to be sent.
uint64_t gLastEchoTime = 0;
chip::System::Clock::Timestamp gLastEchoTime{ 0 };

// Count of the number of EchoRequests sent.
uint64_t gEchoCount = 0;
Expand Down Expand Up @@ -121,9 +121,9 @@ CHIP_ERROR SendEchoRequest()
return CHIP_ERROR_NO_MEMORY;
}

gLastEchoTime = chip::System::SystemClock().GetMonotonicMilliseconds();
gLastEchoTime = chip::System::SystemClock().GetMonotonicTimestamp();

err = chip::DeviceLayer::SystemLayer().StartTimer(chip::System::Clock::Milliseconds32(gEchoInterval), EchoTimerHandler, NULL);
err = chip::DeviceLayer::SystemLayer().StartTimer(gEchoInterval, EchoTimerHandler, NULL);
if (err != CHIP_NO_ERROR)
{
printf("Unable to schedule timer\n");
Expand Down Expand Up @@ -173,7 +173,7 @@ CHIP_ERROR EstablishSecureSession()
if (err != CHIP_NO_ERROR)
{
printf("Establish secure session failed, err: %s\n", chip::ErrorStr(err));
gLastEchoTime = chip::System::SystemClock().GetMonotonicMilliseconds();
gLastEchoTime = chip::System::SystemClock().GetMonotonicTimestamp();
}
else
{
Expand All @@ -185,13 +185,14 @@ CHIP_ERROR EstablishSecureSession()

void HandleEchoResponseReceived(chip::Messaging::ExchangeContext * ec, chip::System::PacketBufferHandle && payload)
{
uint32_t respTime = chip::System::SystemClock().GetMonotonicMilliseconds();
uint32_t transitTime = respTime - gLastEchoTime;
chip::System::Clock::Timestamp respTime = chip::System::SystemClock().GetMonotonicTimestamp();
chip::System::Clock::Timeout transitTime = respTime - gLastEchoTime;

gEchoRespCount++;

printf("Echo Response: %" PRIu64 "/%" PRIu64 "(%.2f%%) len=%u time=%.3fms\n", gEchoRespCount, gEchoCount,
static_cast<double>(gEchoRespCount) * 100 / gEchoCount, payload->DataLength(), static_cast<double>(transitTime) / 1000);
printf("Echo Response: %" PRIu64 "/%" PRIu64 "(%.2f%%) len=%u time=%.3fs\n", gEchoRespCount, gEchoCount,
static_cast<double>(gEchoRespCount) * 100 / gEchoCount, payload->DataLength(),
static_cast<double>(chip::System::Clock::Milliseconds32(transitTime).count()) / 1000);
}

} // namespace
Expand Down
4 changes: 2 additions & 2 deletions src/transport/SessionManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ CHIP_ERROR SessionManager::SendPreparedMessage(SessionHandle session, const Encr
"Sending %s msg %p with MessageCounter:" ChipLogFormatMessageCounter " to 0x" ChipLogFormatX64
" at monotonic time: %" PRId64 " msec",
"encrypted", &preparedMessage, preparedMessage.GetMessageCounter(), ChipLogValueX64(state->GetPeerNodeId()),
System::SystemClock().GetMonotonicMilliseconds());
System::SystemClock().GetMonotonicMilliseconds64().count());
}
else
{
Expand All @@ -196,7 +196,7 @@ CHIP_ERROR SessionManager::SendPreparedMessage(SessionHandle session, const Encr
"Sending %s msg %p with MessageCounter:" ChipLogFormatMessageCounter " to 0x" ChipLogFormatX64
" at monotonic time: %" PRId64 " msec",
"plaintext", &preparedMessage, preparedMessage.GetMessageCounter(), ChipLogValueX64(kUndefinedNodeId),
System::SystemClock().GetMonotonicMilliseconds());
System::SystemClock().GetMonotonicMilliseconds64().count());
}

PacketBufferHandle msgBuf = preparedMessage.CastToWritable();
Expand Down
6 changes: 3 additions & 3 deletions src/transport/raw/tests/NetworkTestHelpers.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -60,15 +60,15 @@ void IOContext::DriveIO()
ServiceEvents(kSleepTimeMilliseconds);
}

void IOContext::DriveIOUntil(unsigned maxWaitMs, std::function<bool(void)> completionFunction)
void IOContext::DriveIOUntil(System::Clock::Timeout maxWait, std::function<bool(void)> completionFunction)
{
uint64_t mStartTime = System::SystemClock().GetMonotonicMilliseconds();
System::Clock::Timestamp startTime = System::SystemClock().GetMonotonicTimestamp();

while (true)
{
DriveIO(); // at least one IO loop is guaranteed

if (completionFunction() || ((System::SystemClock().GetMonotonicMilliseconds() - mStartTime) >= maxWaitMs))
if (completionFunction() || ((System::SystemClock().GetMonotonicTimestamp() - startTime) >= maxWait))
{
break;
}
Expand Down
2 changes: 1 addition & 1 deletion src/transport/raw/tests/NetworkTestHelpers.h
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ class IOContext

/// DriveIO until the specified number of milliseconds has passed or until
/// completionFunction returns true
void DriveIOUntil(unsigned maxWaitMs, std::function<bool(void)> completionFunction);
void DriveIOUntil(System::Clock::Timeout maxWait, std::function<bool(void)> completionFunction);

nlTestSuite * GetTestSuite() { return mSuite; }
System::Layer & GetSystemLayer() { return *mSystemLayer; }
Expand Down
4 changes: 2 additions & 2 deletions src/transport/raw/tests/TestTCP.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ class MockTransportMgrDelegate : public chip::TransportMgrDelegate

NL_TEST_ASSERT(mSuite, err == CHIP_NO_ERROR);

mContext.DriveIOUntil(5000 /* ms */, [this]() { return mReceiveHandlerCallCount != 0; });
mContext.DriveIOUntil(chip::System::Clock::Seconds16(5), [this]() { return mReceiveHandlerCallCount != 0; });
NL_TEST_ASSERT(mSuite, mReceiveHandlerCallCount == 1);

SetCallback(nullptr);
Expand All @@ -152,7 +152,7 @@ class MockTransportMgrDelegate : public chip::TransportMgrDelegate
{
// Disconnect and wait for seeing peer close
tcp.Disconnect(Transport::PeerAddress::TCP(addr));
mContext.DriveIOUntil(5000 /* ms */, [&tcp]() { return !tcp.HasActiveConnections(); });
mContext.DriveIOUntil(chip::System::Clock::Seconds16(5), [&tcp]() { return !tcp.HasActiveConnections(); });
}

int mReceiveHandlerCallCount = 0;
Expand Down
2 changes: 1 addition & 1 deletion src/transport/raw/tests/TestUDP.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ void CheckMessageTest(nlTestSuite * inSuite, void * inContext, const IPAddress &

NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);

ctx.DriveIOUntil(1000 /* ms */, []() { return ReceiveHandlerCallCount != 0; });
ctx.DriveIOUntil(chip::System::Clock::Seconds16(1), []() { return ReceiveHandlerCallCount != 0; });

NL_TEST_ASSERT(inSuite, ReceiveHandlerCallCount == 1);
}
Expand Down

0 comments on commit 4ff81f5

Please sign in to comment.