From 8a42ca96c90c51ea97b76d8c25bcd6e110402f2e Mon Sep 17 00:00:00 2001
From: Boris Zbarsky <bzbarsky@apple.com>
Date: Wed, 18 Sep 2024 14:38:31 -0400
Subject: [PATCH 1/2] Fix behavior of MTRDevice readAttributePaths if it
happens mid-priming.
We might not know the values of AttributeList/ServerList/PartsList yet at that
point. Just go ahead and figure out which attribute values we have by walking
our own data structures, not the metadata list attribute values.
---
.../Framework/CHIP/MTRDevice_Concrete.mm | 72 +++++++------------
1 file changed, 25 insertions(+), 47 deletions(-)
diff --git a/src/darwin/Framework/CHIP/MTRDevice_Concrete.mm b/src/darwin/Framework/CHIP/MTRDevice_Concrete.mm
index d741d0124945fb..f7c888060aaf84 100644
--- a/src/darwin/Framework/CHIP/MTRDevice_Concrete.mm
+++ b/src/darwin/Framework/CHIP/MTRDevice_Concrete.mm
@@ -2983,11 +2983,31 @@ - (void)writeAttributeWithEndpointID:(NSNumber *)endpointID
// Determine the set of what the spec calls "existent paths" that correspond
// to the request paths. Building the whole set in-memory is OK, because
// we're going to need all those paths for our return value anyway.
+ //
+ // Note that we don't use the structural attributes (PartsList, ServerList,
+ // AttributeList) to determine this set, because we might be in the middle
+ // of priming right now and not have gotten those yet. Just use the set of
+ // attribute paths we actually have.
NSMutableSet<MTRAttributePath *> * existentPaths = [[NSMutableSet alloc] init];
{
std::lock_guard lock(_lock);
- for (MTRAttributeRequestPath * path in attributePaths) {
- [self _addExistentPathsFor:path to:existentPaths];
+ for (MTRAttributeRequestPath * requestPath in attributePaths) {
+ for (MTRClusterPath * clusterPath in [self _knownClusters]) {
+ if (requestPath.endpoint != nil && ![requestPath.endpoint isEqual:clusterPath.endpoint]) {
+ continue;
+ }
+ if (requestPath.cluster != nil && ![requestPath.cluster isEqual:clusterPath.cluster]) {
+ continue;
+ }
+ MTRDeviceClusterData * clusterData = [self _clusterDataForPath:clusterPath];
+ if (requestPath.attribute == nil) {
+ for (NSNumber * attributeID in clusterData.attributes) {
+ [existentPaths addObject:[MTRAttributePath attributePathWithEndpointID:clusterPath.endpoint clusterID:clusterPath.cluster attributeID:attributeID]];
+ }
+ } else if ([clusterData.attributes objectForKey:requestPath.attribute] != nil) {
+ [existentPaths addObject:[MTRAttributePath attributePathWithEndpointID:clusterPath.endpoint clusterID:clusterPath.cluster attributeID:requestPath.attribute]];
+ }
+ }
}
}
@@ -3006,51 +3026,6 @@ - (void)writeAttributeWithEndpointID:(NSNumber *)endpointID
return result;
}
-- (void)_addExistentPathsFor:(MTRAttributeRequestPath *)path to:(NSMutableSet<MTRAttributePath *> *)set
-{
- os_unfair_lock_assert_owner(&_lock);
-
- if (path.endpoint != nil) {
- [self _addExistentPathsForEndpoint:path.endpoint path:path to:set];
- return;
- }
-
- NSArray<NSNumber *> * endpointList = [self _endpointList];
- for (NSNumber * endpoint in endpointList) {
- [self _addExistentPathsForEndpoint:endpoint path:path to:set];
- }
-}
-
-- (void)_addExistentPathsForEndpoint:(NSNumber *)endpoint path:(MTRAttributeRequestPath *)path to:(NSMutableSet<MTRAttributePath *> *)set
-{
- os_unfair_lock_assert_owner(&_lock);
-
- if (path.cluster != nil) {
- [self _addExistentPathsForEndpoint:endpoint cluster:path.cluster attribute:path.attribute to:set];
- return;
- }
-
- auto * clusterList = [self _serverListForEndpointID:endpoint];
- for (NSNumber * cluster in clusterList) {
- [self _addExistentPathsForEndpoint:endpoint cluster:cluster attribute:path.attribute to:set];
- }
-}
-
-- (void)_addExistentPathsForEndpoint:(NSNumber *)endpoint cluster:(NSNumber *)cluster attribute:(NSNumber * _Nullable)attribute to:(NSMutableSet<MTRAttributePath *> *)set
-{
- os_unfair_lock_assert_owner(&_lock);
-
- if (attribute != nil) {
- [set addObject:[MTRAttributePath attributePathWithEndpointID:endpoint clusterID:cluster attributeID:attribute]];
- return;
- }
-
- auto * attributeList = [self _attributeListForEndpointID:endpoint clusterID:cluster];
- for (NSNumber * existentAttribute in attributeList) {
- [set addObject:[MTRAttributePath attributePathWithEndpointID:endpoint clusterID:cluster attributeID:existentAttribute]];
- }
-}
-
- (void)_invokeCommandWithEndpointID:(NSNumber *)endpointID
clusterID:(NSNumber *)clusterID
commandID:(NSNumber *)commandID
@@ -3652,6 +3627,9 @@ - (NSArray *)_getAttributesToReportWithReportedValues:(NSArray<NSDictionary<NSSt
return attributesToReport;
}
+// TODO: Figure out whether we can get rid of this in favor of readAttributePaths. This differs from
+// readAttributePaths in one respect: that function will do read-through for
+// C-quality attributes, but this one does not.
- (NSArray<NSDictionary<NSString *, id> *> *)getAllAttributesReport
{
std::lock_guard lock(_lock);
From 129bd9757970bcd6412c47feca0ba5b3d4b60039 Mon Sep 17 00:00:00 2001
From: Boris Zbarsky <bzbarsky@apple.com>
Date: Fri, 20 Sep 2024 14:23:06 -0400
Subject: [PATCH 2/2] Apply suggestion from code review.
Co-authored-by: Nivi Sarkar <55898241+nivi-apple@users.noreply.github.com>
---
src/darwin/Framework/CHIP/MTRDevice_Concrete.mm | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/darwin/Framework/CHIP/MTRDevice_Concrete.mm b/src/darwin/Framework/CHIP/MTRDevice_Concrete.mm
index f7c888060aaf84..bc26905d174834 100644
--- a/src/darwin/Framework/CHIP/MTRDevice_Concrete.mm
+++ b/src/darwin/Framework/CHIP/MTRDevice_Concrete.mm
@@ -2986,7 +2986,7 @@ - (void)writeAttributeWithEndpointID:(NSNumber *)endpointID
//
// Note that we don't use the structural attributes (PartsList, ServerList,
// AttributeList) to determine this set, because we might be in the middle
- // of priming right now and not have gotten those yet. Just use the set of
+ // of priming right now and have not gotten those yet. Just use the set of
// attribute paths we actually have.
NSMutableSet<MTRAttributePath *> * existentPaths = [[NSMutableSet alloc] init];
{