Skip to content

Commit

Permalink
Convert TimeSource to safer Clock types (#11449)
Browse files Browse the repository at this point in the history
#### Problem

`System::TimeSource` still uses raw integer times rather than
the safer `Syste::Clock` types based on `std::chrono::duration`.

Followup to #10062 _Some operations on System::Clock types are not safe_

#### Change overview

Convert `TimeSource` and its uses.

#### Testing

CI; no changes to functionality.
Converted `TestTimeSource`.
  • Loading branch information
kpschoedel authored and pull[bot] committed Dec 14, 2022
1 parent 0562e22 commit 3381241
Show file tree
Hide file tree
Showing 14 changed files with 107 additions and 99 deletions.
21 changes: 11 additions & 10 deletions src/app/server/Dnssd.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,8 @@ void OnPlatformEventWrapper(const DeviceLayer::ChipDeviceEvent * event, intptr_t

} // namespace

constexpr System::Clock::Timestamp DnssdServer::kTimeoutCleared;

#if CHIP_DEVICE_CONFIG_ENABLE_EXTENDED_DISCOVERY

constexpr const char kExtendedDiscoveryTimeoutKeypairStorage[] = "ExtDiscKey";
Expand Down Expand Up @@ -110,15 +112,15 @@ void HandleExtendedDiscoveryExpiration(System::Layer * aSystemLayer, void * aApp

void DnssdServer::OnExtendedDiscoveryExpiration(System::Layer * aSystemLayer, void * aAppState)
{
if (!DnssdServer::OnExpiration(mExtendedDiscoveryExpirationMs))
if (!DnssdServer::OnExpiration(mExtendedDiscoveryExpiration))
{
ChipLogDetail(Discovery, "OnExtendedDiscoveryExpiration callback for cleared session");
return;
}

ChipLogDetail(Discovery, "OnExtendedDiscoveryExpiration callback for valid session");

mExtendedDiscoveryExpirationMs = TIMEOUT_CLEARED;
mExtendedDiscoveryExpiration = kTimeoutCleared;
}
#endif // CHIP_DEVICE_CONFIG_ENABLE_EXTENDED_DISCOVERY

Expand All @@ -128,14 +130,14 @@ void HandleDiscoveryExpiration(System::Layer * aSystemLayer, void * aAppState)
DnssdServer::Instance().OnDiscoveryExpiration(aSystemLayer, aAppState);
}

bool DnssdServer::OnExpiration(uint64_t expirationMs)
bool DnssdServer::OnExpiration(System::Clock::Timestamp expirationMs)
{
if (expirationMs == TIMEOUT_CLEARED)
if (expirationMs == kTimeoutCleared)
{
ChipLogDetail(Discovery, "OnExpiration callback for cleared session");
return false;
}
uint64_t now = mTimeSource.GetCurrentMonotonicTimeMs();
System::Clock::Timestamp now = mTimeSource.GetMonotonicTimestamp();
if (expirationMs > now)
{
ChipLogDetail(Discovery, "OnExpiration callback for reset session");
Expand Down Expand Up @@ -183,7 +185,7 @@ bool DnssdServer::OnExpiration(uint64_t expirationMs)

void DnssdServer::OnDiscoveryExpiration(System::Layer * aSystemLayer, void * aAppState)
{
if (!DnssdServer::OnExpiration(mDiscoveryExpirationMs))
if (!DnssdServer::OnExpiration(mDiscoveryExpiration))
{
ChipLogDetail(Discovery, "OnDiscoveryExpiration callback for cleared session");
return;
Expand All @@ -205,7 +207,7 @@ void DnssdServer::OnDiscoveryExpiration(System::Layer * aSystemLayer, void * aAp
}
#endif // CHIP_DEVICE_CONFIG_ENABLE_EXTENDED_DISCOVERY

mDiscoveryExpirationMs = TIMEOUT_CLEARED;
mDiscoveryExpiration = kTimeoutCleared;
}

CHIP_ERROR DnssdServer::ScheduleDiscoveryExpiration()
Expand All @@ -216,7 +218,7 @@ CHIP_ERROR DnssdServer::ScheduleDiscoveryExpiration()
}
ChipLogDetail(Discovery, "Scheduling Discovery timeout in secs=%d", mDiscoveryTimeoutSecs);

mDiscoveryExpirationMs = mTimeSource.GetCurrentMonotonicTimeMs() + static_cast<uint64_t>(mDiscoveryTimeoutSecs) * 1000;
mDiscoveryExpiration = mTimeSource.GetMonotonicTimestamp() + System::Clock::Seconds16(mDiscoveryTimeoutSecs);

return DeviceLayer::SystemLayer().StartTimer(System::Clock::Seconds16(mDiscoveryTimeoutSecs), HandleDiscoveryExpiration,
nullptr);
Expand All @@ -232,8 +234,7 @@ CHIP_ERROR DnssdServer::ScheduleExtendedDiscoveryExpiration()
}
ChipLogDetail(Discovery, "Scheduling Extended Discovery timeout in secs=%d", extendedDiscoveryTimeoutSecs);

mExtendedDiscoveryExpirationMs =
mTimeSource.GetCurrentMonotonicTimeMs() + static_cast<uint64_t>(extendedDiscoveryTimeoutSecs) * 1000;
mExtendedDiscoveryExpiration = mTimeSource.GetMonotonicTimestamp() + System::Clock::Seconds16(extendedDiscoveryTimeoutSecs);

return DeviceLayer::SystemLayer().StartTimer(System::Clock::Seconds16(extendedDiscoveryTimeoutSecs),
HandleExtendedDiscoveryExpiration, nullptr);
Expand Down
15 changes: 8 additions & 7 deletions src/app/server/Dnssd.h
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,11 @@
namespace chip {
namespace app {

#define TIMEOUT_CLEARED 0
class DLL_EXPORT DnssdServer
{
public:
static constexpr System::Clock::Timestamp kTimeoutCleared = System::Clock::kZero;

/// Provides the system-wide implementation of the service advertiser
static DnssdServer & Instance()
{
Expand Down Expand Up @@ -104,9 +105,9 @@ class DLL_EXPORT DnssdServer

void ClearTimeouts()
{
mDiscoveryExpirationMs = TIMEOUT_CLEARED;
mDiscoveryExpiration = kTimeoutCleared;
#if CHIP_DEVICE_CONFIG_ENABLE_EXTENDED_DISCOVERY
mExtendedDiscoveryExpirationMs = TIMEOUT_CLEARED;
mExtendedDiscoveryExpiration = kTimeoutCleared;
#endif // CHIP_DEVICE_CONFIG_ENABLE_EXTENDED_DISCOVERY
}

Expand All @@ -115,11 +116,11 @@ class DLL_EXPORT DnssdServer

/// schedule next discovery expiration
CHIP_ERROR ScheduleDiscoveryExpiration();
int16_t mDiscoveryTimeoutSecs = CHIP_DEVICE_CONFIG_DISCOVERY_TIMEOUT_SECS;
uint64_t mDiscoveryExpirationMs = TIMEOUT_CLEARED;
int16_t mDiscoveryTimeoutSecs = CHIP_DEVICE_CONFIG_DISCOVERY_TIMEOUT_SECS;
System::Clock::Timestamp mDiscoveryExpiration = kTimeoutCleared;

/// return true if expirationMs is valid (not cleared and not in the future)
bool OnExpiration(uint64_t expirationMs);
bool OnExpiration(System::Clock::Timestamp expiration);

#if CHIP_DEVICE_CONFIG_ENABLE_EXTENDED_DISCOVERY
/// get the current extended discovery timeout (from persistent storage)
Expand All @@ -128,7 +129,7 @@ class DLL_EXPORT DnssdServer
/// schedule next extended discovery expiration
CHIP_ERROR ScheduleExtendedDiscoveryExpiration();

uint64_t mExtendedDiscoveryExpirationMs = TIMEOUT_CLEARED;
System::Clock::Timestamp mExtendedDiscoveryExpiration = kTimeoutCleared;
#endif // CHIP_DEVICE_CONFIG_ENABLE_EXTENDED_DISCOVERY
};

Expand Down
3 changes: 2 additions & 1 deletion src/lib/dnssd/Discovery_ImplPlatform.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -589,7 +589,8 @@ void DiscoveryImplPlatform::HandleNodeIdResolve(void * context, DnssdService * r
#if CHIP_CONFIG_MDNS_CACHE_SIZE > 0
// TODO -- define appropriate TTL, for now use 2000 msec (rfc default)
// figure out way to use TTL value from mDNS packet in future update
error = mgr->sDnssdCache.Insert(nodeData.mPeerId, result->mAddress.Value(), result->mPort, result->mInterface, 2 * 1000);
error = mgr->sDnssdCache.Insert(nodeData.mPeerId, result->mAddress.Value(), result->mPort, result->mInterface,
System::Clock::Seconds16(2));

if (CHIP_NO_ERROR != error)
{
Expand Down
25 changes: 13 additions & 12 deletions src/lib/dnssd/DnssdCache.h
Original file line number Diff line number Diff line change
Expand Up @@ -55,18 +55,19 @@ class DnssdCache
// return error if cache is full
// TODO: have an eviction policy so if the cache is full, an entry may be deleted.
// One policy may be Least-time-to-live
CHIP_ERROR Insert(PeerId peerId, const Inet::IPAddress & addr, uint16_t port, Inet::InterfaceId iface, uint32_t TTLms)
CHIP_ERROR Insert(PeerId peerId, const Inet::IPAddress & addr, uint16_t port, Inet::InterfaceId iface,
System::Clock::Timestamp TTL)
{
const uint64_t currentTime = mTimeSource.GetCurrentMonotonicTimeMs();
const System::Clock::Timestamp currentTime = mTimeSource.GetMonotonicTimestamp();

DnssdCacheEntry * entry;

entry = FindPeerId(peerId, currentTime);
if (entry)
{
// update timeout if found entry
entry->expiryTime = currentTime + TTLms;
entry->TTL = TTLms; // in case it changes */
entry->expiryTime = currentTime + TTL;
entry->TTL = TTL; // in case it changes */
return CHIP_NO_ERROR;
}

Expand All @@ -77,8 +78,8 @@ class DnssdCache
entry->ipAddr = addr;
entry->port = port;
entry->ifaceId = iface;
entry->TTL = TTLms;
entry->expiryTime = currentTime + TTLms;
entry->TTL = TTL;
entry->expiryTime = currentTime + TTL;
elementsUsed++;

return CHIP_NO_ERROR;
Expand All @@ -87,7 +88,7 @@ class DnssdCache
CHIP_ERROR Delete(PeerId peerId)
{
DnssdCacheEntry * pentry;
const uint64_t currentTime = mTimeSource.GetCurrentMonotonicTimeMs();
const System::Clock::Timestamp currentTime = mTimeSource.GetMonotonicTimestamp();

VerifyOrReturnError(pentry = FindPeerId(peerId, currentTime), CHIP_ERROR_KEY_NOT_FOUND);

Expand All @@ -99,7 +100,7 @@ class DnssdCache
CHIP_ERROR Lookup(PeerId peerId, Inet::IPAddress & addr, uint16_t & port, Inet::InterfaceId & iface)
{
DnssdCacheEntry * pentry;
const uint64_t currentTime = mTimeSource.GetCurrentMonotonicTimeMs();
const System::Clock::Timestamp currentTime = mTimeSource.GetMonotonicTimestamp();

VerifyOrReturnError(pentry = FindPeerId(peerId, currentTime), CHIP_ERROR_KEY_NOT_FOUND);

Expand Down Expand Up @@ -141,16 +142,16 @@ class DnssdCache
Inet::IPAddress ipAddr;
uint16_t port;
Inet::InterfaceId ifaceId;
uint64_t TTL; // from mdns record -- units?
uint64_t expiryTime; // units?
System::Clock::Timestamp TTL;
System::Clock::Timestamp expiryTime;
};
PeerId nullPeerId; // indicates a cache entry is unused
int elementsUsed; // running count of how many entries are used -- for a sanity check

DnssdCacheEntry mLookupTable[CACHE_SIZE];
Time::TimeSource<Time::Source::kSystem> mTimeSource;

DnssdCacheEntry * findSlot(uint64_t currentTime)
DnssdCacheEntry * findSlot(System::Clock::Timestamp currentTime)
{
for (DnssdCacheEntry & entry : mLookupTable)
{
Expand All @@ -166,7 +167,7 @@ class DnssdCache
return nullptr;
}

DnssdCacheEntry * FindPeerId(PeerId peerId, uint64_t current_time)
DnssdCacheEntry * FindPeerId(PeerId peerId, System::Clock::Timestamp current_time)
{
for (DnssdCacheEntry & entry : mLookupTable)
{
Expand Down
10 changes: 5 additions & 5 deletions src/lib/dnssd/tests/TestDnssdCache.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ void TestInsert(nlTestSuite * inSuite, void * inContext)

Inet::IPAddress addr;
Inet::IPAddress addrV6;
const int ttl = 2; /* seconds */
const System::Clock::Timestamp ttl = System::Clock::Seconds16(2);
Inet::InterfaceId iface_out;
Inet::IPAddress addr_out;
uint16_t port_out;
Expand All @@ -71,7 +71,7 @@ void TestInsert(nlTestSuite * inSuite, void * inContext)

// ml -- why doesn't adding 2 uint16_t give a uint16_t?
peerId.SetNodeId((NodeId) id + i);
result = tDnssdCache.Insert(peerId, addr, (uint16_t)(port + i), iface, 1000 * ttl);
result = tDnssdCache.Insert(peerId, addr, (uint16_t)(port + i), iface, ttl);
if (i < sizeOfCache)
{
NL_TEST_ASSERT(inSuite, result == CHIP_NO_ERROR);
Expand All @@ -83,15 +83,15 @@ void TestInsert(nlTestSuite * inSuite, void * inContext)
}

tDnssdCache.DumpCache();
usleep(static_cast<useconds_t>((ttl + 1) * 1000 * 1000));
usleep(static_cast<useconds_t>(std::chrono::microseconds(ttl + System::Clock::Seconds16(1)).count()));
id = 0x200;
port = 3000;
for (uint16_t i = 0; i < sizeOfCache; i++)
{
CHIP_ERROR result;

peerId.SetNodeId((NodeId) id + i);
result = tDnssdCache.Insert(peerId, addr, (uint16_t)(port + i), iface, 1000 * ttl);
result = tDnssdCache.Insert(peerId, addr, (uint16_t)(port + i), iface, ttl);
NL_TEST_ASSERT(inSuite, result == CHIP_NO_ERROR);
}
tDnssdCache.DumpCache();
Expand All @@ -115,7 +115,7 @@ void TestInsert(nlTestSuite * inSuite, void * inContext)
CHIP_ERROR result;

peerId.SetNodeId((NodeId) id + i);
result = tDnssdCache.Insert(peerId, addrV6, (uint16_t)(port + i), iface, 1000 * ttl);
result = tDnssdCache.Insert(peerId, addrV6, (uint16_t)(port + i), iface, ttl);
NL_TEST_ASSERT(inSuite, result == CHIP_NO_ERROR);
}

Expand Down
12 changes: 6 additions & 6 deletions src/protocols/user_directed_commissioning/UDCClientState.h
Original file line number Diff line number Diff line change
Expand Up @@ -78,13 +78,13 @@ class UDCClientState
UDCClientProcessingState GetUDCClientProcessingState() const { return mUDCClientProcessingState; }
void SetUDCClientProcessingState(UDCClientProcessingState state) { mUDCClientProcessingState = state; }

uint64_t GetExpirationTimeMs() const { return mExpirationTimeMs; }
void SetExpirationTimeMs(uint64_t value) { mExpirationTimeMs = value; }
System::Clock::Timestamp GetExpirationTime() const { return mExpirationTime; }
void SetExpirationTime(System::Clock::Timestamp value) { mExpirationTime = value; }

bool IsInitialized(uint64_t currentTime)
bool IsInitialized(System::Clock::Timestamp currentTime)
{
// if state is not the "not-initialized" and it has not expired
return (mUDCClientProcessingState != UDCClientProcessingState::kNotInitialized && mExpirationTimeMs > currentTime);
return (mUDCClientProcessingState != UDCClientProcessingState::kNotInitialized && mExpirationTime > currentTime);
}

/**
Expand All @@ -93,7 +93,7 @@ class UDCClientState
void Reset()
{
mPeerAddress = PeerAddress::Uninitialized();
mExpirationTimeMs = 0;
mExpirationTime = System::Clock::kZero;
mUDCClientProcessingState = UDCClientProcessingState::kNotInitialized;
}

Expand All @@ -103,7 +103,7 @@ class UDCClientState
char mDeviceName[Dnssd::kMaxDeviceNameLen + 1];
uint16_t mLongDiscriminator = 0;
UDCClientProcessingState mUDCClientProcessingState;
uint64_t mExpirationTimeMs = 0;
System::Clock::Timestamp mExpirationTime = System::Clock::kZero;
};

} // namespace UserDirectedCommissioning
Expand Down
16 changes: 8 additions & 8 deletions src/protocols/user_directed_commissioning/UDCClients.h
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ namespace Protocols {
namespace UserDirectedCommissioning {

// UDC client state times out after 1 hour. This may need to be tweaked.
constexpr const uint64_t kUDCClientTimeoutMs = 60 * 60 * 1000;
constexpr const System::Clock::Timestamp kUDCClientTimeout = System::Clock::Milliseconds64(60 * 60 * 1000);

/**
* Handles a set of UDC Client Processing States.
Expand Down Expand Up @@ -54,7 +54,7 @@ class UDCClients
CHECK_RETURN_VALUE
CHIP_ERROR CreateNewUDCClientState(const char * instanceName, UDCClientState ** state)
{
const uint64_t currentTime = mTimeSource.GetCurrentMonotonicTimeMs();
const System::Clock::Timestamp currentTime = mTimeSource.GetMonotonicTimestamp();

CHIP_ERROR err = CHIP_ERROR_NO_MEMORY;

Expand All @@ -68,7 +68,7 @@ class UDCClients
if (!stateiter.IsInitialized(currentTime))
{
stateiter.SetInstanceName(instanceName);
stateiter.SetExpirationTimeMs(currentTime + kUDCClientTimeoutMs);
stateiter.SetExpirationTime(currentTime + kUDCClientTimeout);
stateiter.SetUDCClientProcessingState(UDCClientProcessingState::kDiscoveringNode);

if (state)
Expand Down Expand Up @@ -99,8 +99,8 @@ class UDCClients
return nullptr;
}

const uint64_t currentTime = mTimeSource.GetCurrentMonotonicTimeMs();
UDCClientState state = mStates[index];
const System::Clock::Timestamp currentTime = mTimeSource.GetMonotonicTimestamp();
UDCClientState state = mStates[index];
if (!state.IsInitialized(currentTime))
{
return nullptr;
Expand All @@ -118,7 +118,7 @@ class UDCClients
CHECK_RETURN_VALUE
UDCClientState * FindUDCClientState(const PeerAddress & address)
{
const uint64_t currentTime = mTimeSource.GetCurrentMonotonicTimeMs();
const System::Clock::Timestamp currentTime = mTimeSource.GetMonotonicTimestamp();

UDCClientState * state = nullptr;

Expand Down Expand Up @@ -147,7 +147,7 @@ class UDCClients
CHECK_RETURN_VALUE
UDCClientState * FindUDCClientState(const char * instanceName)
{
const uint64_t currentTime = mTimeSource.GetCurrentMonotonicTimeMs();
const System::Clock::Timestamp currentTime = mTimeSource.GetMonotonicTimestamp();

UDCClientState * state = nullptr;

Expand Down Expand Up @@ -180,7 +180,7 @@ class UDCClients
/// Convenience method to mark a UDC Client state as active (non-expired)
void MarkUDCClientActive(UDCClientState * state)
{
state->SetExpirationTimeMs(mTimeSource.GetCurrentMonotonicTimeMs() + kUDCClientTimeoutMs);
state->SetExpirationTime(mTimeSource.GetMonotonicTimestamp() + kUDCClientTimeout);
}

private:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -256,11 +256,11 @@ void TestUDCClients(nlTestSuite * inSuite, void * inContext)

// test re-activation
NL_TEST_ASSERT(inSuite, CHIP_NO_ERROR == mUdcClients.CreateNewUDCClientState(instanceName4, &state));
uint64_t expirationTime = state->GetExpirationTimeMs();
state->SetExpirationTimeMs(expirationTime - 1);
NL_TEST_ASSERT(inSuite, (expirationTime - 1) == state->GetExpirationTimeMs());
System::Clock::Timestamp expirationTime = state->GetExpirationTime();
state->SetExpirationTime(expirationTime - System::Clock::Milliseconds64(1));
NL_TEST_ASSERT(inSuite, (expirationTime - System::Clock::Milliseconds64(1)) == state->GetExpirationTime());
mUdcClients.MarkUDCClientActive(state);
NL_TEST_ASSERT(inSuite, (expirationTime - 1) < state->GetExpirationTimeMs());
NL_TEST_ASSERT(inSuite, (expirationTime - System::Clock::Milliseconds64(1)) < state->GetExpirationTime());
}

// Test Suite
Expand Down
Loading

0 comments on commit 3381241

Please sign in to comment.