From 5539882e5fc2acb9e68b0c054d937e83ab4a7563 Mon Sep 17 00:00:00 2001 From: C Freeman Date: Tue, 26 Sep 2023 15:02:44 -0400 Subject: [PATCH] Time sync: Remove ClusterStateCache (#29396) * 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. * pack all the things into a class. * Fiddle with the build file * Comments on the delegate. * remove one more app prefix * Apply suggestions from code review Co-authored-by: Boris Zbarsky --------- Co-authored-by: Boris Zbarsky --- src/app/chip_data_model.gni | 8 ++- .../time-synchronization-delegate.h | 6 +- .../time-synchronization-server.cpp | 72 +++++++++++-------- .../time-synchronization-server.h | 21 ++++-- 4 files changed, 68 insertions(+), 39 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", 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..a80b3207aff992 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 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 * granularity set to kNoTimeGranularity. 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 83f249fde66c12..39e00dd98dbe14 100644 --- a/src/app/clusters/time-synchronization-server/time-synchronization-server.cpp +++ b/src/app/clusters/time-synchronization-server/time-synchronization-server.cpp @@ -300,41 +300,30 @@ 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) + 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; } - auto readClient = chip::Platform::MakeUnique(engine, &exchangeMgr, attributeCache->GetBufferedCallback(), - app::ReadClient::InteractionType::Read); - if (readClient == 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; } - mAttributeCache = std::move(attributeCache); - mReadClient = std::move(readClient); + mTimeReadInfo = std::move(readInfo); } void TimeSynchronizationServer::OnDeviceConnectionFailureFn() @@ -343,20 +332,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, mTimeReadInfo->utcTime) != CHIP_NO_ERROR) + { + mTimeReadInfo->utcTime.SetNull(); + } + break; + case Attributes::Granularity::Id: + if (DataModel::Decode(*apData, mTimeReadInfo->granularity) != CHIP_NO_ERROR) + { + mTimeReadInfo->granularity = 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 (!mTimeReadInfo->utcTime.IsNull() && mTimeReadInfo->granularity != GranularityEnum::kNoTimeGranularity) { GranularityEnum ourGranularity; // Being conservative with granularity - nothing smaller than seconds because of network delay - switch (granularity) + switch (mTimeReadInfo->granularity) { case GranularityEnum::kMinutesGranularity: case GranularityEnum::kSecondsGranularity: @@ -367,7 +375,8 @@ void TimeSynchronizationServer::OnDone(ReadClient * apReadClient) break; } - err = SetUTCTime(kRootEndpointId, time.Value(), ourGranularity, TimeSourceEnum::kNodeTimeCluster); + CHIP_ERROR err = + SetUTCTime(kRootEndpointId, mTimeReadInfo->utcTime.Value(), ourGranularity, TimeSourceEnum::kNodeTimeCluster); if (err == CHIP_NO_ERROR) { return; @@ -377,6 +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(); + 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 b17fd0f880e1e7..200714d3cf7def 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,21 @@ 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; + 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; + ReadClient readClient; + }; + Platform::UniquePtr mTimeReadInfo; #endif chip::Callback::Callback mOnTimeSyncCompletion; chip::Callback::Callback mOnFallbackNTPCompletion;