Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use safer System::Clock types in transport and messaging #10913

Merged
merged 1 commit into from
Oct 27, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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