From b5568077120b54c5eb2cbd011e27ac2ff2cebf6a Mon Sep 17 00:00:00 2001 From: Cecille Freeman Date: Wed, 20 Sep 2023 21:23:57 -0400 Subject: [PATCH 1/6] Time sync: Remove ClusterStateCache It's easy to use, but it sure is big. Since we aren't asking for lists or anything, it's also overkill. --- .../time-synchronization-server.cpp | 69 +++++++++++-------- .../time-synchronization-server.h | 11 +-- 2 files changed, 46 insertions(+), 34 deletions(-) diff --git a/src/app/clusters/time-synchronization-server/time-synchronization-server.cpp b/src/app/clusters/time-synchronization-server/time-synchronization-server.cpp index c7c79f3d1fc499..870cd9593bd510 100644 --- a/src/app/clusters/time-synchronization-server/time-synchronization-server.cpp +++ b/src/app/clusters/time-synchronization-server/time-synchronization-server.cpp @@ -300,26 +300,18 @@ void TimeSynchronizationServer::AttemptToGetFallbackNTPTimeFromDelegate() void TimeSynchronizationServer::OnDeviceConnectedFn(Messaging::ExchangeManager & exchangeMgr, const SessionHandle & sessionHandle) { // Connected to our trusted time source, let's read the time. - app::AttributePathParams readPaths[2]; - readPaths[0] = app::AttributePathParams(kRootEndpointId, app::Clusters::TimeSynchronization::Id, - app::Clusters::TimeSynchronization::Attributes::UTCTime::Id); - readPaths[1] = app::AttributePathParams(kRootEndpointId, app::Clusters::TimeSynchronization::Id, - app::Clusters::TimeSynchronization::Attributes::Granularity::Id); - - app::InteractionModelEngine * engine = app::InteractionModelEngine::GetInstance(); - app::ReadPrepareParams readParams(sessionHandle); + AttributePathParams readPaths[2]; + readPaths[0] = AttributePathParams(kRootEndpointId, Id, Attributes::UTCTime::Id); + readPaths[1] = AttributePathParams(kRootEndpointId, Id, Attributes::Granularity::Id); + + InteractionModelEngine * engine = InteractionModelEngine::GetInstance(); + ReadPrepareParams readParams(sessionHandle); readParams.mpAttributePathParamsList = readPaths; readParams.mAttributePathParamsListSize = 2; - auto attributeCache = Platform::MakeUnique(*this); - if (attributeCache == nullptr) - { - // This is unlikely to work if we don't have memory, but let's try - OnDeviceConnectionFailureFn(); - return; - } - auto readClient = chip::Platform::MakeUnique(engine, &exchangeMgr, attributeCache->GetBufferedCallback(), - app::ReadClient::InteractionType::Read); + mTrustedNodeUtcTime.SetNull(); + mTrustedNodeGranularity = GranularityEnum::kNoTimeGranularity; + auto readClient = Platform::MakeUnique(engine, &exchangeMgr, *this, ReadClient::InteractionType::Read); if (readClient == nullptr) { // This is unlikely to work if we don't have memory, but let's try @@ -333,8 +325,7 @@ void TimeSynchronizationServer::OnDeviceConnectedFn(Messaging::ExchangeManager & OnDeviceConnectionFailureFn(); return; } - mAttributeCache = std::move(attributeCache); - mReadClient = std::move(readClient); + mReadClient = std::move(readClient); } void TimeSynchronizationServer::OnDeviceConnectionFailureFn() @@ -343,20 +334,39 @@ void TimeSynchronizationServer::OnDeviceConnectionFailureFn() AttemptToGetFallbackNTPTimeFromDelegate(); } -void TimeSynchronizationServer::OnDone(ReadClient * apReadClient) +void TimeSynchronizationServer::OnAttributeData(const ConcreteDataAttributePath & aPath, TLV::TLVReader * apData, + const StatusIB & aStatus) { - using namespace chip::app::Clusters::TimeSynchronization::Attributes; - - Granularity::TypeInfo::Type granularity = GranularityEnum::kNoTimeGranularity; - mAttributeCache->Get(kRootEndpointId, granularity); + if (aPath.mClusterId != Id || aStatus.IsFailure()) + { + return; + } + switch (aPath.mAttributeId) + { + case Attributes::UTCTime::Id: + if (DataModel::Decode(*apData, mTrustedNodeUtcTime) != CHIP_NO_ERROR) + { + mTrustedNodeUtcTime.SetNull(); + } + break; + case Attributes::Granularity::Id: + if (DataModel::Decode(*apData, mTrustedNodeGranularity) != CHIP_NO_ERROR) + { + mTrustedNodeGranularity = GranularityEnum::kNoTimeGranularity; + } + break; + default: + break; + } +} - UTCTime::TypeInfo::Type time; - CHIP_ERROR err = mAttributeCache->Get(kRootEndpointId, time); - if (err == CHIP_NO_ERROR && !time.IsNull() && granularity != GranularityEnum::kNoTimeGranularity) +void TimeSynchronizationServer::OnDone(ReadClient * apReadClient) +{ + if (!mTrustedNodeUtcTime.IsNull() && mTrustedNodeGranularity != GranularityEnum::kNoTimeGranularity) { GranularityEnum ourGranularity; // Being conservative with granularity - nothing smaller than seconds because of network delay - switch (granularity) + switch (mTrustedNodeGranularity) { case GranularityEnum::kMinutesGranularity: case GranularityEnum::kSecondsGranularity: @@ -367,7 +377,7 @@ void TimeSynchronizationServer::OnDone(ReadClient * apReadClient) break; } - err = SetUTCTime(kRootEndpointId, time.Value(), ourGranularity, TimeSourceEnum::kNodeTimeCluster); + CHIP_ERROR err = SetUTCTime(kRootEndpointId, mTrustedNodeUtcTime.Value(), ourGranularity, TimeSourceEnum::kNodeTimeCluster); if (err == CHIP_NO_ERROR) { return; @@ -377,6 +387,7 @@ void TimeSynchronizationServer::OnDone(ReadClient * apReadClient) // If we failed to set the UTC time, it doesn't hurt to try the backup - NTP system might have different permissions on the // system clock AttemptToGetFallbackNTPTimeFromDelegate(); + mReadClient = nullptr; } #endif diff --git a/src/app/clusters/time-synchronization-server/time-synchronization-server.h b/src/app/clusters/time-synchronization-server/time-synchronization-server.h index b17fd0f880e1e7..bd319cf4f1e467 100644 --- a/src/app/clusters/time-synchronization-server/time-synchronization-server.h +++ b/src/app/clusters/time-synchronization-server/time-synchronization-server.h @@ -72,7 +72,7 @@ enum class TimeSyncEventFlag : uint8_t class TimeSynchronizationServer : public FabricTable::Delegate #if TIME_SYNC_ENABLE_TSC_FEATURE , - public ClusterStateCache::Callback + public ReadClient::Callback #endif { public: @@ -117,8 +117,8 @@ class TimeSynchronizationServer : public FabricTable::Delegate void OnDeviceConnectedFn(Messaging::ExchangeManager & exchangeMgr, const SessionHandle & sessionHandle); void OnDeviceConnectionFailureFn(); - // AttributeCache::Callback functions - void OnAttributeChanged(ClusterStateCache * cache, const ConcreteAttributePath & path) override {} + // ReadClient::Callback functions + void OnAttributeData(const ConcreteDataAttributePath & aPath, TLV::TLVReader * apData, const StatusIB & aStatus) override; void OnDone(ReadClient * apReadClient) override; #endif @@ -142,10 +142,11 @@ class TimeSynchronizationServer : public FabricTable::Delegate static TimeSynchronizationServer sTimeSyncInstance; TimeSyncEventFlag mEventFlag = TimeSyncEventFlag::kNone; #if TIME_SYNC_ENABLE_TSC_FEATURE - Platform::UniquePtr mAttributeCache; - Platform::UniquePtr mReadClient; chip::Callback::Callback mOnDeviceConnectedCallback; chip::Callback::Callback mOnDeviceConnectionFailureCallback; + Attributes::UTCTime::TypeInfo::DecodableType mTrustedNodeUtcTime; + Attributes::Granularity::TypeInfo::DecodableType mTrustedNodeGranularity; + Platform::UniquePtr mReadClient; #endif chip::Callback::Callback mOnTimeSyncCompletion; chip::Callback::Callback mOnFallbackNTPCompletion; From ab6f831588f30df5acc317b3af2ac20f0c4afd9b Mon Sep 17 00:00:00 2001 From: Cecille Freeman Date: Thu, 21 Sep 2023 19:03:06 -0400 Subject: [PATCH 2/6] pack all the things into a class. --- .../time-synchronization-server.cpp | 27 +++++++++---------- .../time-synchronization-server.h | 16 ++++++++--- 2 files changed, 26 insertions(+), 17 deletions(-) diff --git a/src/app/clusters/time-synchronization-server/time-synchronization-server.cpp b/src/app/clusters/time-synchronization-server/time-synchronization-server.cpp index 870cd9593bd510..fed7671097b41a 100644 --- a/src/app/clusters/time-synchronization-server/time-synchronization-server.cpp +++ b/src/app/clusters/time-synchronization-server/time-synchronization-server.cpp @@ -309,23 +309,21 @@ void TimeSynchronizationServer::OnDeviceConnectedFn(Messaging::ExchangeManager & readParams.mpAttributePathParamsList = readPaths; readParams.mAttributePathParamsListSize = 2; - mTrustedNodeUtcTime.SetNull(); - mTrustedNodeGranularity = GranularityEnum::kNoTimeGranularity; - auto readClient = Platform::MakeUnique(engine, &exchangeMgr, *this, ReadClient::InteractionType::Read); - if (readClient == nullptr) + auto readInfo = Platform::MakeUnique(engine, &exchangeMgr, *this, ReadClient::InteractionType::Read); + if (readInfo == nullptr) { // This is unlikely to work if we don't have memory, but let's try OnDeviceConnectionFailureFn(); return; } - CHIP_ERROR err = readClient->SendRequest(readParams); + CHIP_ERROR err = readInfo->readClient.SendRequest(readParams); if (err != CHIP_NO_ERROR) { ChipLogError(Zcl, "Failed to read UTC time from trusted source"); OnDeviceConnectionFailureFn(); return; } - mReadClient = std::move(readClient); + mTimeReadInfo = std::move(readInfo); } void TimeSynchronizationServer::OnDeviceConnectionFailureFn() @@ -344,15 +342,15 @@ void TimeSynchronizationServer::OnAttributeData(const ConcreteDataAttributePath switch (aPath.mAttributeId) { case Attributes::UTCTime::Id: - if (DataModel::Decode(*apData, mTrustedNodeUtcTime) != CHIP_NO_ERROR) + if (DataModel::Decode(*apData, mTimeReadInfo->utcTime) != CHIP_NO_ERROR) { - mTrustedNodeUtcTime.SetNull(); + mTimeReadInfo->utcTime.SetNull(); } break; case Attributes::Granularity::Id: - if (DataModel::Decode(*apData, mTrustedNodeGranularity) != CHIP_NO_ERROR) + if (DataModel::Decode(*apData, mTimeReadInfo->granularity) != CHIP_NO_ERROR) { - mTrustedNodeGranularity = GranularityEnum::kNoTimeGranularity; + mTimeReadInfo->granularity = GranularityEnum::kNoTimeGranularity; } break; default: @@ -362,11 +360,11 @@ void TimeSynchronizationServer::OnAttributeData(const ConcreteDataAttributePath void TimeSynchronizationServer::OnDone(ReadClient * apReadClient) { - if (!mTrustedNodeUtcTime.IsNull() && mTrustedNodeGranularity != GranularityEnum::kNoTimeGranularity) + if (!mTimeReadInfo->utcTime.IsNull() && mTimeReadInfo->granularity != GranularityEnum::kNoTimeGranularity) { GranularityEnum ourGranularity; // Being conservative with granularity - nothing smaller than seconds because of network delay - switch (mTrustedNodeGranularity) + switch (mTimeReadInfo->granularity) { case GranularityEnum::kMinutesGranularity: case GranularityEnum::kSecondsGranularity: @@ -377,7 +375,8 @@ void TimeSynchronizationServer::OnDone(ReadClient * apReadClient) break; } - CHIP_ERROR err = SetUTCTime(kRootEndpointId, mTrustedNodeUtcTime.Value(), ourGranularity, TimeSourceEnum::kNodeTimeCluster); + CHIP_ERROR err = + SetUTCTime(kRootEndpointId, mTimeReadInfo->utcTime.Value(), ourGranularity, TimeSourceEnum::kNodeTimeCluster); if (err == CHIP_NO_ERROR) { return; @@ -387,7 +386,7 @@ void TimeSynchronizationServer::OnDone(ReadClient * apReadClient) // If we failed to set the UTC time, it doesn't hurt to try the backup - NTP system might have different permissions on the // system clock AttemptToGetFallbackNTPTimeFromDelegate(); - mReadClient = nullptr; + mTimeReadInfo = nullptr; } #endif diff --git a/src/app/clusters/time-synchronization-server/time-synchronization-server.h b/src/app/clusters/time-synchronization-server/time-synchronization-server.h index bd319cf4f1e467..4959cb1a7365d5 100644 --- a/src/app/clusters/time-synchronization-server/time-synchronization-server.h +++ b/src/app/clusters/time-synchronization-server/time-synchronization-server.h @@ -144,9 +144,19 @@ class TimeSynchronizationServer : public FabricTable::Delegate #if TIME_SYNC_ENABLE_TSC_FEATURE chip::Callback::Callback mOnDeviceConnectedCallback; chip::Callback::Callback mOnDeviceConnectionFailureCallback; - Attributes::UTCTime::TypeInfo::DecodableType mTrustedNodeUtcTime; - Attributes::Granularity::TypeInfo::DecodableType mTrustedNodeGranularity; - Platform::UniquePtr mReadClient; + struct TimeReadInfo + { + TimeReadInfo(InteractionModelEngine * apImEngine, Messaging::ExchangeManager * apExchangeMgr, + ReadClient::Callback & apCallback, ReadClient::InteractionType aInteractionType) : + readClient(apImEngine, apExchangeMgr, apCallback, aInteractionType) + { + utcTime.SetNull(); + } + Attributes::UTCTime::TypeInfo::DecodableType utcTime; + Attributes::Granularity::TypeInfo::DecodableType granularity = GranularityEnum::kNoTimeGranularity; + app::ReadClient readClient; + }; + Platform::UniquePtr mTimeReadInfo; #endif chip::Callback::Callback mOnTimeSyncCompletion; chip::Callback::Callback mOnFallbackNTPCompletion; From e1a1150cc1d549ce18629eaed5bb4e1a2820be62 Mon Sep 17 00:00:00 2001 From: Cecille Freeman Date: Thu, 21 Sep 2023 19:15:12 -0400 Subject: [PATCH 3/6] Fiddle with the build file --- src/app/chip_data_model.gni | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/app/chip_data_model.gni b/src/app/chip_data_model.gni index fc127022d9aa78..54abef97926e04 100644 --- a/src/app/chip_data_model.gni +++ b/src/app/chip_data_model.gni @@ -203,6 +203,10 @@ template("chip_data_model") { cflags = [] } + if (!defined(defines)) { + defines = [] + } + foreach(cluster, _cluster_sources) { if (cluster == "door-lock-server") { sources += [ @@ -262,8 +266,8 @@ template("chip_data_model") { "${_app_root}/clusters/${cluster}/DefaultTimeSyncDelegate.cpp", "${_app_root}/clusters/${cluster}/TimeSyncDataProvider.cpp", ] - cflags += - [ "-DTIME_SYNC_ENABLE_TSC_FEATURE=${time_sync_enable_tsc_feature}" ] + defines += + [ "TIME_SYNC_ENABLE_TSC_FEATURE=${time_sync_enable_tsc_feature}" ] } else if (cluster == "scenes-server") { sources += [ "${_app_root}/clusters/${cluster}/${cluster}.cpp", From e6fe4e79f2aa0ef6b2e744d0b23d4a46ac2e5d72 Mon Sep 17 00:00:00 2001 From: Cecille Freeman Date: Fri, 22 Sep 2023 09:59:17 -0400 Subject: [PATCH 4/6] Comments on the delegate. --- .../time-synchronization-delegate.h | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/app/clusters/time-synchronization-server/time-synchronization-delegate.h b/src/app/clusters/time-synchronization-server/time-synchronization-delegate.h index d04f3018a01f02..e2470c02e86205 100644 --- a/src/app/clusters/time-synchronization-server/time-synchronization-delegate.h +++ b/src/app/clusters/time-synchronization-server/time-synchronization-delegate.h @@ -84,7 +84,11 @@ class Delegate * local network defined NTP (DHCPv6 -> DHCP -> DNS-SD sources) * If the delegate is unable to support any source, it may return an error immediately. If the delegate is going * to attempt to obtain time from any source, it returns CHIP_NO_ERROR and calls the callback on completion. - * If the delegate successfully obtains the time, it sets the time using the platform time API (SetClock_RealTime) + * If the delegate has a time available at the time of this call, it may call the callback synchronously from within + * this function. + * If the delegate need to reach out asynchronously to obtain a time, it saves this callback to call asynchronously. + * The delegate should track these callbacks in a list to ensure they can be properly cancelled. + * If the delegate is successful in obtaining the time, it sets the time using the platform time API (SetClock_RealTime) * and calls the callback with the time source and granularity set as appropriate. * If the delegate is unsuccessful in obtaining the time, it calls the callback with timeSource set to kNone and * granularity set to kNoTimeGranularity. From a281e31c3ac7fd3132ac50f6ce1d3f89eeb95f60 Mon Sep 17 00:00:00 2001 From: Cecille Freeman Date: Fri, 22 Sep 2023 10:06:16 -0400 Subject: [PATCH 5/6] remove one more app prefix --- .../time-synchronization-server/time-synchronization-server.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/clusters/time-synchronization-server/time-synchronization-server.h b/src/app/clusters/time-synchronization-server/time-synchronization-server.h index 4959cb1a7365d5..200714d3cf7def 100644 --- a/src/app/clusters/time-synchronization-server/time-synchronization-server.h +++ b/src/app/clusters/time-synchronization-server/time-synchronization-server.h @@ -154,7 +154,7 @@ class TimeSynchronizationServer : public FabricTable::Delegate } Attributes::UTCTime::TypeInfo::DecodableType utcTime; Attributes::Granularity::TypeInfo::DecodableType granularity = GranularityEnum::kNoTimeGranularity; - app::ReadClient readClient; + ReadClient readClient; }; Platform::UniquePtr mTimeReadInfo; #endif From df313db45d874a626d2f7ace2a9e370e23ccbafe Mon Sep 17 00:00:00 2001 From: C Freeman Date: Tue, 26 Sep 2023 12:53:06 -0400 Subject: [PATCH 6/6] Apply suggestions from code review Co-authored-by: Boris Zbarsky --- .../time-synchronization-delegate.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/app/clusters/time-synchronization-server/time-synchronization-delegate.h b/src/app/clusters/time-synchronization-server/time-synchronization-delegate.h index e2470c02e86205..a80b3207aff992 100644 --- a/src/app/clusters/time-synchronization-server/time-synchronization-delegate.h +++ b/src/app/clusters/time-synchronization-server/time-synchronization-delegate.h @@ -86,8 +86,8 @@ class Delegate * to attempt to obtain time from any source, it returns CHIP_NO_ERROR and calls the callback on completion. * If the delegate has a time available at the time of this call, it may call the callback synchronously from within * this function. - * If the delegate need to reach out asynchronously to obtain a time, it saves this callback to call asynchronously. - * The delegate should track these callbacks in a list to ensure they can be properly cancelled. + * If the delegate needs to reach out asynchronously to obtain a time, it saves this callback to call asynchronously. + * The delegate should track these callbacks in a CallbackDeque to ensure they can be properly cancelled. * If the delegate is successful in obtaining the time, it sets the time using the platform time API (SetClock_RealTime) * and calls the callback with the time source and granularity set as appropriate. * If the delegate is unsuccessful in obtaining the time, it calls the callback with timeSource set to kNone and