Skip to content

Commit

Permalink
Clear data from MTRDevice's ClusterStateCache when elements are remov…
Browse files Browse the repository at this point in the history
…ed. (project-chip#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.
  • Loading branch information
bzbarsky-apple authored and j-ororke committed Jul 15, 2024
1 parent b9a9f66 commit 8037cb3
Show file tree
Hide file tree
Showing 3 changed files with 61 additions and 0 deletions.
7 changes: 7 additions & 0 deletions src/darwin/Framework/CHIP/MTRBaseSubscriptionCallback.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include <app/BufferedReadCallback.h>
#include <app/ClusterStateCache.h>
#include <app/ConcreteAttributePath.h>
#include <app/ConcreteClusterPath.h>
#include <app/EventHeader.h>
#include <app/MessageDef/StatusIB.h>
#include <app/ReadClient.h>
Expand Down Expand Up @@ -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<chip::app::ReadClient> aReadClient) { mReadClient = std::move(aReadClient); }
void AdoptClusterStateCache(std::unique_ptr<chip::app::ClusterStateCache> aClusterStateCache)
Expand Down
24 changes: 24 additions & 0 deletions src/darwin/Framework/CHIP/MTRBaseSubscriptionCallback.mm
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
30 changes: 30 additions & 0 deletions src/darwin/Framework/CHIP/MTRDevice.mm
Original file line number Diff line number Diff line change
Expand Up @@ -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<EndpointId>(endpoint.unsignedLongLongValue));
}
} errorHandler:nil];
}
}

Expand All @@ -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>(endpointID.unsignedLongLongValue),
static_cast<ClusterId>(cluster.unsignedLongLongValue));
self->_currentSubscriptionCallback->ClearCachedAttributeState(clusterPath);
}
}
} errorHandler:nil];
}

- (void)_pruneAttributesIn:(MTRDeviceDataValueDictionary)previousAttributeListValue
Expand All @@ -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<EndpointId>(clusterPath.endpoint.unsignedLongLongValue),
static_cast<ClusterId>(clusterPath.cluster.unsignedLongLongValue),
static_cast<AttributeId>(attribute.unsignedLongLongValue));
self->_currentSubscriptionCallback->ClearCachedAttributeState(attributePath);
}
}
} errorHandler:nil];
}

- (void)_pruneStoredDataForPath:(MTRAttributePath *)attributePath
Expand Down

0 comments on commit 8037cb3

Please sign in to comment.