From a858f413d004842271567b322ab9bce20efbc317 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Mon, 24 Apr 2023 13:37:18 -0400 Subject: [PATCH] Fix the "not storing data" mode of ClusterStateCache to actually work. (#26187) Since we were storing no data, we were computing clusterSize to 0 for all clusters in GetSortedFilters, and then we were not adding any DataVersion filters into our requests at all. The fix is that in "don't store data" mode we still store how big the data would have been, so we can correctly prioritize our DataVersion filters. Without this fix, the change in https://github.com/project-chip/connectedhomeip/pull/26181 fails CI. That CI also acts as a test for this functionality, once https://github.com/project-chip/connectedhomeip/pull/26181 happens. --- src/app/ClusterStateCache.cpp | 75 +++++++++++++++++++++++++++-------- src/app/ClusterStateCache.h | 13 +++++- 2 files changed, 69 insertions(+), 19 deletions(-) diff --git a/src/app/ClusterStateCache.cpp b/src/app/ClusterStateCache.cpp index 302f70e44eea00..2644c1a89bbbe8 100644 --- a/src/app/ClusterStateCache.cpp +++ b/src/app/ClusterStateCache.cpp @@ -24,6 +24,31 @@ namespace chip { namespace app { +namespace { + +// Determine how much space a StatusIB takes up on the wire. +size_t SizeOfStatusIB(const StatusIB & aStatus) +{ + // 1 byte: anonymous tag control byte for struct. + // 1 byte: control byte for uint8 value. + // 1 byte: context-specific tag for uint8 value. + // 1 byte: the uint8 value. + // 1 byte: end of container. + size_t size = 5; + + if (aStatus.mClusterStatus.HasValue()) + { + // 1 byte: control byte for uint8 value. + // 1 byte: context-specific tag for uint8 value. + // 1 byte: the uint8 value. + size += 3; + } + + return size; +} + +} // anonymous namespace + CHIP_ERROR ClusterStateCache::GetElementTLVSize(TLV::TLVReader * apData, size_t & aSize) { Platform::ScopedMemoryBufferWithSize backingBuffer; @@ -57,10 +82,11 @@ CHIP_ERROR ClusterStateCache::UpdateCache(const ConcreteDataAttributePath & aPat if (apData) { + size_t elementSize = 0; + ReturnErrorOnFailure(GetElementTLVSize(apData, elementSize)); + if (mCacheData) { - size_t elementSize = 0; - ReturnErrorOnFailure(GetElementTLVSize(apData, elementSize)); Platform::ScopedMemoryBufferWithSize backingBuffer; backingBuffer.Calloc(elementSize); VerifyOrReturnError(backingBuffer.Get() != nullptr, CHIP_ERROR_NO_MEMORY); @@ -68,7 +94,11 @@ CHIP_ERROR ClusterStateCache::UpdateCache(const ConcreteDataAttributePath & aPat ReturnErrorOnFailure(writer.CopyElement(TLV::AnonymousTag(), *apData)); ReturnErrorOnFailure(writer.Finalize(backingBuffer)); - state.Set>(std::move(backingBuffer)); + state.Set(std::move(backingBuffer)); + } + else + { + state.Set(elementSize); } // // Clear out the committed data version and only set it again once we have received all data for this cluster. @@ -106,6 +136,10 @@ CHIP_ERROR ClusterStateCache::UpdateCache(const ConcreteDataAttributePath & aPat { state.Set(aStatus); } + else + { + state.Set(SizeOfStatusIB(aStatus)); + } } // @@ -117,9 +151,10 @@ CHIP_ERROR ClusterStateCache::UpdateCache(const ConcreteDataAttributePath & aPat mAddedEndpoints.push_back(aPath.mEndpointId); } + mCache[aPath.mEndpointId][aPath.mClusterId].mAttributes[aPath.mAttributeId] = std::move(state); + if (mCacheData) { - mCache[aPath.mEndpointId][aPath.mClusterId].mAttributes[aPath.mAttributeId] = std::move(state); mChangedAttributeSet.insert(aPath); } @@ -235,8 +270,12 @@ CHIP_ERROR ClusterStateCache::Get(const ConcreteAttributePath & path, TLV::TLVRe return CHIP_ERROR_IM_STATUS_CODE_RECEIVED; } - reader.Init(attributeState->Get>().Get(), - attributeState->Get>().AllocatedSize()); + if (!attributeState->Is()) + { + return CHIP_ERROR_KEY_NOT_FOUND; + } + + reader.Init(attributeState->Get().Get(), attributeState->Get().AllocatedSize()); return reader.Next(); } @@ -419,27 +458,25 @@ void ClusterStateCache::GetSortedFilters(std::vector()) { - clusterSize += - 5; // 1 byte: anonymous tag control byte for struct. 1 byte: control byte for uint8 value. 1 byte: - // context-specific tag for uint8 value.1 byte: the uint8 value. 1 byte: end of container. - if (attributeIter.second.Get().mClusterStatus.HasValue()) - { - clusterSize += 3; // 1 byte: control byte for uint8 value. 1 byte: context-specific tag for uint8 value. 1 - // byte: the uint8 value. - } + clusterSize += SizeOfStatusIB(attributeIter.second.Get()); + } + else if (attributeIter.second.Is()) + { + clusterSize += attributeIter.second.Get(); } else { + VerifyOrDie(attributeIter.second.Is()); TLV::TLVReader bufReader; - bufReader.Init(attributeIter.second.Get>().Get(), - attributeIter.second.Get>().AllocatedSize()); + bufReader.Init(attributeIter.second.Get().Get(), + attributeIter.second.Get().AllocatedSize()); ReturnOnFailure(bufReader.Next()); // Skip to the end of the element. ReturnOnFailure(bufReader.Skip()); @@ -448,8 +485,11 @@ void ClusterStateCache::GetSortedFilters(std::vector & x, const std::pair & y) { return x.second > y.second; diff --git a/src/app/ClusterStateCache.h b/src/app/ClusterStateCache.h index b0a2d2b540345f..b6fb2029c85b49 100644 --- a/src/app/ClusterStateCache.h +++ b/src/app/ClusterStateCache.h @@ -500,7 +500,16 @@ class ClusterStateCache : protected ReadClient::Callback CHIP_ERROR GetLastReportDataPath(ConcreteClusterPath & aPath); private: - using AttributeState = Variant, StatusIB>; + // An attribute state can be one of three things: + // * If we got a path-specific error for the attribute, the corresponding + // status. + // * If we got data for the attribute and we are storing data ourselves, the + // data. + // * If we got data for the attribute and we are not storing data + // oureselves, the size of the data, so we can still prioritize sending + // DataVersions correctly. + using AttributeData = Platform::ScopedMemoryBufferWithSize; + using AttributeState = Variant; // mPendingDataVersion represents a tentative data version for a cluster that we have gotten some reports for. // // mCurrentDataVersion represents a known data version for a cluster. In order for this to have a @@ -632,7 +641,7 @@ class ClusterStateCache : protected ReadClient::Callback std::map mEventStatusCache; BufferedReadCallback mBufferedReader; ConcreteClusterPath mLastReportDataPath = ConcreteClusterPath(kInvalidEndpointId, kInvalidClusterId); - bool mCacheData = true; + const bool mCacheData = true; }; }; // namespace app