From 4710f91705111c1acdd3960c632728c3fd5be740 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Fri, 12 Jul 2024 13:41:17 -0400 Subject: [PATCH] Clear data from MTRDevice's ClusterStateCache when elements are removed. (#34212) * Clear data from MTRDevice's ClusterStateCache when elements are removed. The idea is for that cache to store DataVersions and cluster data sizes, so on resubscribe we send the right data along. But ClusterStateCache itself never removes anything, so if we detect that an endpoint or cluster or attribute has been removed, we need to explicitly remove the relevant bits from the cache. Not doing that can lead to incorrect (so less efficient) DataVersion sorting for the case when an attribute was removed, of to unnecessary DataVersions being included in a subscribe (and potentially preventing useful ones from being included) when a cluster or endpoint was removed. * Address review comments. --- .../CHIP/MTRBaseSubscriptionCallback.h | 7 +++++ .../CHIP/MTRBaseSubscriptionCallback.mm | 24 +++++++++++++++ src/darwin/Framework/CHIP/MTRDevice.mm | 30 +++++++++++++++++++ 3 files changed, 61 insertions(+) diff --git a/src/darwin/Framework/CHIP/MTRBaseSubscriptionCallback.h b/src/darwin/Framework/CHIP/MTRBaseSubscriptionCallback.h index ebcf3aae949433..9edd153fd2410c 100644 --- a/src/darwin/Framework/CHIP/MTRBaseSubscriptionCallback.h +++ b/src/darwin/Framework/CHIP/MTRBaseSubscriptionCallback.h @@ -22,6 +22,7 @@ #include #include #include +#include #include #include #include @@ -93,6 +94,12 @@ class MTRBaseSubscriptionCallback : public chip::app::ClusterStateCache::Callbac chip::app::BufferedReadCallback & GetBufferedCallback() { return mBufferedReadAdapter; } + // Methods to clear state from our cluster state cache. Must be called on + // the Matter queue. + void ClearCachedAttributeState(chip::EndpointId aEndpoint); + void ClearCachedAttributeState(const chip::app::ConcreteClusterPath & aCluster); + void ClearCachedAttributeState(const chip::app::ConcreteAttributePath & aAttribute); + // We need to exist to get a ReadClient, so can't take this as a constructor argument. void AdoptReadClient(std::unique_ptr aReadClient) { mReadClient = std::move(aReadClient); } void AdoptClusterStateCache(std::unique_ptr aClusterStateCache) diff --git a/src/darwin/Framework/CHIP/MTRBaseSubscriptionCallback.mm b/src/darwin/Framework/CHIP/MTRBaseSubscriptionCallback.mm index ca91aed5016126..4d91c76a385a9f 100644 --- a/src/darwin/Framework/CHIP/MTRBaseSubscriptionCallback.mm +++ b/src/darwin/Framework/CHIP/MTRBaseSubscriptionCallback.mm @@ -201,3 +201,27 @@ }); } } + +void MTRBaseSubscriptionCallback::ClearCachedAttributeState(EndpointId aEndpoint) +{ + assertChipStackLockedByCurrentThread(); + if (mClusterStateCache) { + mClusterStateCache->ClearAttributes(aEndpoint); + } +} + +void MTRBaseSubscriptionCallback::ClearCachedAttributeState(const ConcreteClusterPath & aCluster) +{ + assertChipStackLockedByCurrentThread(); + if (mClusterStateCache) { + mClusterStateCache->ClearAttributes(aCluster); + } +} + +void MTRBaseSubscriptionCallback::ClearCachedAttributeState(const ConcreteAttributePath & aAttribute) +{ + assertChipStackLockedByCurrentThread(); + if (mClusterStateCache) { + mClusterStateCache->ClearAttribute(aAttribute); + } +} diff --git a/src/darwin/Framework/CHIP/MTRDevice.mm b/src/darwin/Framework/CHIP/MTRDevice.mm index 298b31a5406bce..fff962dcc94188 100644 --- a/src/darwin/Framework/CHIP/MTRDevice.mm +++ b/src/darwin/Framework/CHIP/MTRDevice.mm @@ -3441,6 +3441,13 @@ - (void)_pruneEndpointsIn:(MTRDeviceDataValueDictionary)previousPartsListValue } [self _removeClusters:clusterPathsToRemove doRemoveFromDataStore:NO]; [self.deviceController.controllerDataStore clearStoredClusterDataForNodeID:self.nodeID endpointID:endpoint]; + + [_deviceController asyncDispatchToMatterQueue:^{ + std::lock_guard lock(self->_lock); + if (self->_currentSubscriptionCallback) { + self->_currentSubscriptionCallback->ClearCachedAttributeState(static_cast(endpoint.unsignedLongLongValue)); + } + } errorHandler:nil]; } } @@ -3461,6 +3468,17 @@ - (void)_pruneClustersIn:(MTRDeviceDataValueDictionary)previousServerListValue } } [self _removeClusters:clusterPathsToRemove doRemoveFromDataStore:YES]; + + [_deviceController asyncDispatchToMatterQueue:^{ + std::lock_guard lock(self->_lock); + if (self->_currentSubscriptionCallback) { + for (NSNumber * cluster in toBeRemovedClusters) { + ConcreteClusterPath clusterPath(static_cast(endpointID.unsignedLongLongValue), + static_cast(cluster.unsignedLongLongValue)); + self->_currentSubscriptionCallback->ClearCachedAttributeState(clusterPath); + } + } + } errorHandler:nil]; } - (void)_pruneAttributesIn:(MTRDeviceDataValueDictionary)previousAttributeListValue @@ -3474,6 +3492,18 @@ - (void)_pruneAttributesIn:(MTRDeviceDataValueDictionary)previousAttributeListVa [toBeRemovedAttributes minusSet:attributesStillInCluster]; [self _removeAttributes:toBeRemovedAttributes fromCluster:clusterPath]; + + [_deviceController asyncDispatchToMatterQueue:^{ + std::lock_guard lock(self->_lock); + if (self->_currentSubscriptionCallback) { + for (NSNumber * attribute in toBeRemovedAttributes) { + ConcreteAttributePath attributePath(static_cast(clusterPath.endpoint.unsignedLongLongValue), + static_cast(clusterPath.cluster.unsignedLongLongValue), + static_cast(attribute.unsignedLongLongValue)); + self->_currentSubscriptionCallback->ClearCachedAttributeState(attributePath); + } + } + } errorHandler:nil]; } - (void)_pruneStoredDataForPath:(MTRAttributePath *)attributePath