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
#26181 fails CI.  That CI
also acts as a test for this functionality, once
#26181 happens.

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<uint8_t> backingBuffer;
@@ -57,18 +82,23 @@ 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<uint8_t> backingBuffer;
  backingBuffer.Calloc(elementSize);
  VerifyOrReturnError(backingBuffer.Get() != nullptr, CHIP_ERROR_NO_MEMORY);
  TLV::ScopedBufferTLVWriter writer(std::move(backingBuffer), elementSize);
  ReturnErrorOnFailure(writer.CopyElement(TLV::AnonymousTag(), *apData));
  ReturnErrorOnFailure(writer.Finalize(backingBuffer));
 
- state.Set<Platform::ScopedMemoryBufferWithSize<uint8_t>>(std::move(backingBuffer));
+ state.Set<AttributeData>(std::move(backingBuffer));
+ }
+ else
+ {
+ state.Set<size_t>(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<StatusIB>(aStatus);
  }
+ else
+ {
+ state.Set<size_t>(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<Platform::ScopedMemoryBufferWithSize<uint8_t>>().Get(),
- attributeState->Get<Platform::ScopedMemoryBufferWithSize<uint8_t>>().AllocatedSize());
+ if (!attributeState->Is<AttributeData>())
+ {
+ return CHIP_ERROR_KEY_NOT_FOUND;
+ }
+
+ reader.Init(attributeState->Get<AttributeData>().Get(), attributeState->Get<AttributeData>().AllocatedSize());
  return reader.Next();
 }
 
@@ -419,27 +458,25 @@ void ClusterStateCache::GetSortedFilters(std::vector<std::pair<DataVersionFilter
  continue;
  }
  DataVersion dataVersion = clusterIter.second.mCommittedDataVersion.Value();
- uint32_t clusterSize = 0;
+ size_t clusterSize  = 0;
  ClusterId clusterId = clusterIter.first;
 
  for (auto const & attributeIter : clusterIter.second.mAttributes)
  {
  if (attributeIter.second.Is<StatusIB>())
  {
- 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<StatusIB>().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<StatusIB>());
+ }
+ else if (attributeIter.second.Is<size_t>())
+ {
+ clusterSize += attributeIter.second.Get<size_t>();
  }
  else
  {
+ VerifyOrDie(attributeIter.second.Is<AttributeData>());
  TLV::TLVReader bufReader;
- bufReader.Init(attributeIter.second.Get<Platform::ScopedMemoryBufferWithSize<uint8_t>>().Get(),
- attributeIter.second.Get<Platform::ScopedMemoryBufferWithSize<uint8_t>>().AllocatedSize());
+ bufReader.Init(attributeIter.second.Get<AttributeData>().Get(),
+ attributeIter.second.Get<AttributeData>().AllocatedSize());
  ReturnOnFailure(bufReader.Next());
  // Skip to the end of the element.
  ReturnOnFailure(bufReader.Skip());
@@ -448,8 +485,11 @@ void ClusterStateCache::GetSortedFilters(std::vector<std::pair<DataVersionFilter
  clusterSize += bufReader.GetLengthRead();
  }
  }
+
  if (clusterSize == 0)
  {
+ // No data in this cluster, so no point in sending a dataVersion
+ // along at all.
  continue;
  }
 
@@ -458,6 +498,7 @@ void ClusterStateCache::GetSortedFilters(std::vector<std::pair<DataVersionFilter
  aVector.push_back(std::make_pair(filter, clusterSize));
  }
  }
+
  std::sort(aVector.begin(), aVector.end(),
  [](const std::pair<DataVersionFilter, size_t> & x, const std::pair<DataVersionFilter, size_t> & y) {
  return x.second > y.second;

src/app/ClusterStateCache.h

@@ -500,7 +500,16 @@ class ClusterStateCache : protected ReadClient::Callback
  CHIP_ERROR GetLastReportDataPath(ConcreteClusterPath & aPath);
 
 private:
- using AttributeState = Variant<Platform::ScopedMemoryBufferWithSize<uint8_t>, 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<uint8_t>;
+ using AttributeState = Variant<StatusIB, AttributeData, size_t>;
  // 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<ConcreteEventPath, StatusIB> mEventStatusCache;
  BufferedReadCallback mBufferedReader;
  ConcreteClusterPath mLastReportDataPath = ConcreteClusterPath(kInvalidEndpointId, kInvalidClusterId);
- bool mCacheData  = true;
+ const bool mCacheData = true;
 };
 
 }; // namespace app