Process endpoints in best-effort fashion even with invalid / missing …

…DeviceTypeList Also add some additional comments to parsing logic and a couple more tests.
project-chip · Dec 23, 2024 · b78a442 · b78a442
1 parent c0cce65
commit b78a442
Show file tree

Hide file tree

Showing 2 changed files with 62 additions and 10 deletions.
diff --git a/src/darwin/Framework/CHIP/MTREndpointInfo.mm b/src/darwin/Framework/CHIP/MTREndpointInfo.mm
@@ -52,6 +52,7 @@ - (instancetype)initWithEndpointID:(NSNumber *)endpointID
     _deviceTypes = [deviceTypes copy];
     _partsList = [partsList copy];
     _children = @[];
+    _mark = EndpointMark::NotVisited;
     return self;
 }
 
@@ -78,7 +79,7 @@ + (BOOL)populateChildrenForEndpoints:(NSDictionary<NSNumber *, MTREndpointInfo *
     }
 
     // Perform a depth-first search with an explicit stack and create a list of endpoint
-    // IDs in reverse topological order. The _parentID field is used mark traversal states.
+    // IDs in reverse topological order. Note that endpoints start with _mark == NotVisited.
     BOOL valid = YES;
     std::deque<EndpointId> deque; // stack followed by sorted list
     deque.emplace_front(root->_endpointID);
@@ -109,6 +110,11 @@ + (BOOL)populateChildrenForEndpoints:(NSDictionary<NSNumber *, MTREndpointInfo *
                 break; // visited the root, DFS traversal done
             }
         } else /* endpoint->_mark == EndpointMark::Visited */ {
+            // Nodes can be visited multiple times due to Full-Family
+            // ancestors like the root node, or in scenarios where a
+            // nodes is erroneously in the PartsList of two separate
+            // branches of the tree. There is no easy way to distinguish
+            // these cases here, so we are not setting valid = NO.
             deque.pop_front(); // nothing else to do
         }
     }
@@ -157,8 +163,7 @@ + (BOOL)populateChildrenForEndpoints:(NSDictionary<NSNumber *, MTREndpointInfo *
         CHIP_ERROR err = CHIP_NO_ERROR;
         NSArray<MTRDescriptorClusterDeviceTypeStruct *> * deviceTypeList = MTRDecodeAttributeValue(path, *cache, &err);
         if (!deviceTypeList) {
-            MTR_LOG_ERROR("Ignoring endpoint %u, failed to parse DeviceTypeList: %" CHIP_ERROR_FORMAT, path.mEndpointId, err.Format());
-            return CHIP_NO_ERROR;
+            MTR_LOG_ERROR("Ignoring invalid DeviceTypeList for endpoint %u: %" CHIP_ERROR_FORMAT, path.mEndpointId, err.Format());
         }
 
         NSMutableArray * deviceTypes = [[NSMutableArray alloc] initWithCapacity:deviceTypeList.count];
@@ -172,15 +177,11 @@ + (BOOL)populateChildrenForEndpoints:(NSDictionary<NSNumber *, MTREndpointInfo *
             }
             [deviceTypes addObject:type];
         }
-        if (!deviceTypes) {
-            MTR_LOG_ERROR("Ignoring endpoint %u, no device types", path.mEndpointId);
-            return CHIP_NO_ERROR;
-        }
 
         ConcreteAttributePath partsListPath(path.mEndpointId, path.mClusterId, PartsList::Id);
         NSArray<NSNumber *> * partsList = MTRDecodeAttributeValue(partsListPath, *cache, &err);
         if (!partsList) {
-            MTR_LOG_ERROR("Ignoring PartsList for endpoint %u: %" CHIP_ERROR_FORMAT, path.mEndpointId, err.Format());
+            MTR_LOG_ERROR("Ignoring invalid PartsList for endpoint %u: %" CHIP_ERROR_FORMAT, path.mEndpointId, err.Format());
             partsList = @[];
         }
 

diff --git a/src/darwin/Framework/CHIPTests/MTREndpointInfoTests.m b/src/darwin/Framework/CHIPTests/MTREndpointInfoTests.m
@@ -33,6 +33,13 @@ @implementation MTREndpointInfoTests
     return [[endpoint.children valueForKey:@"endpointID"] sortedArrayUsingSelector:@selector(compare:)];
 }
 
+static NSArray<NSNumber *> * Exclude(NSArray<NSNumber *> * numbers, NSNumber * numberToExclude)
+{
+    NSMutableArray * result = [numbers mutableCopy];
+    [result removeObject:numberToExclude];
+    return result;
+}
+
 - (NSDictionary<NSNumber *, MTREndpointInfo *> *)indexEndpoints:(NSArray<MTREndpointInfo *> *)endpoints
 {
     NSMutableDictionary * indexed = [[NSMutableDictionary alloc] init];
@@ -64,6 +71,28 @@ - (void)testPopulateChildren
     XCTAssertEqualObjects(ChildEndpointIDs(endpoints[@6]), (@[]));
 }
 
+- (void)testPopulateChildren2
+{
+    // Same as testPopulateChildren, but with reversed PartsLists
+    NSDictionary<NSNumber *, MTREndpointInfo *> * endpoints = [self indexEndpoints:@[
+        MakeEndpoint(@0, @[ @6, @5, @4, @3, @2, @1 ]), // full-family pattern
+        MakeEndpoint(@1, @[ @3, @2 ]),
+        MakeEndpoint(@2, @[]),
+        MakeEndpoint(@3, @[]),
+        MakeEndpoint(@4, @[ @6, @5 ]), // full-family pattern
+        MakeEndpoint(@5, @[ @6 ]),
+        MakeEndpoint(@6, @[]),
+    ]];
+    XCTAssertTrue([MTREndpointInfo populateChildrenForEndpoints:endpoints]);
+    XCTAssertEqualObjects(ChildEndpointIDs(endpoints[@0]), (@[ @1, @4 ]));
+    XCTAssertEqualObjects(ChildEndpointIDs(endpoints[@1]), (@[ @2, @3 ]));
+    XCTAssertEqualObjects(ChildEndpointIDs(endpoints[@2]), (@[]));
+    XCTAssertEqualObjects(ChildEndpointIDs(endpoints[@3]), (@[]));
+    XCTAssertEqualObjects(ChildEndpointIDs(endpoints[@4]), (@[ @5 ]));
+    XCTAssertEqualObjects(ChildEndpointIDs(endpoints[@5]), (@[ @6 ]));
+    XCTAssertEqualObjects(ChildEndpointIDs(endpoints[@6]), (@[]));
+}
+
 - (void)testPopulateChildrenRootOnly
 {
     NSDictionary<NSNumber *, MTREndpointInfo *> * endpoints = [self indexEndpoints:@[
@@ -73,7 +102,7 @@ - (void)testPopulateChildrenRootOnly
     XCTAssertEqualObjects(ChildEndpointIDs(endpoints[@0]), (@[]));
 }
 
-- (void)testPopulateChildrenWithCompositionCycle
+- (void)testPopulateChildrenInvalidCompositionCycle
 {
     NSDictionary<NSNumber *, MTREndpointInfo *> * endpoints = [self indexEndpoints:@[
         MakeEndpoint(@0, @[ @1, @2, @3, @4, @5, @6 ]), // full-family pattern
@@ -82,7 +111,7 @@ - (void)testPopulateChildrenWithCompositionCycle
         MakeEndpoint(@3, @[]),
         MakeEndpoint(@4, @[ @5, @6 ]), // full-family pattern
         MakeEndpoint(@5, @[ @6 ]),
-        MakeEndpoint(@6, @[ @4 ]), // cycle 4 -> 5 -> 6 -> 4
+        MakeEndpoint(@6, @[ @4 ]), // not valid per spec: cycle 4 -> 5 -> 6 -> 4
     ]];
     XCTAssertFalse([MTREndpointInfo populateChildrenForEndpoints:endpoints]);
     XCTAssertEqualObjects(ChildEndpointIDs(endpoints[@0]), (@[ @1, @4 ]));
@@ -92,4 +121,26 @@ - (void)testPopulateChildrenWithCompositionCycle
     // We make no promises about child lists for endpoints involved in a cycle
 }
 
+- (void)testPopulateChildrenInvalidNonTree
+{
+    NSDictionary<NSNumber *, MTREndpointInfo *> * endpoints = [self indexEndpoints:@[
+        MakeEndpoint(@0, @[ @1, @2, @3, @4, @5, @6 ]), // full-family pattern
+        MakeEndpoint(@1, @[ @2, @3, @6 ]),
+        MakeEndpoint(@2, @[]),
+        MakeEndpoint(@3, @[]),
+        MakeEndpoint(@4, @[ @5, @6 ]), // full-family pattern
+        MakeEndpoint(@5, @[ @6 ]),
+        MakeEndpoint(@6, @[]), // not valid per spec: 6 is a child of both 1 and 5
+    ]];
+    // Note: Not asserting a false return value here, this scenario is currently not detected.
+    [MTREndpointInfo populateChildrenForEndpoints:endpoints];
+    XCTAssertEqualObjects(ChildEndpointIDs(endpoints[@0]), (@[ @1, @4 ]));
+    XCTAssertEqualObjects(ChildEndpointIDs(endpoints[@2]), (@[]));
+    XCTAssertEqualObjects(ChildEndpointIDs(endpoints[@3]), (@[]));
+    XCTAssertEqualObjects(ChildEndpointIDs(endpoints[@4]), (@[ @5 ]));
+    // Endpoint 6 has multiple parents, so we make no guarantees where (or if) it shows up
+    XCTAssertEqualObjects(Exclude(ChildEndpointIDs(endpoints[@1]), @6), (@[ @2, @3 ]));
+    XCTAssertEqualObjects(Exclude(ChildEndpointIDs(endpoints[@5]), @6), (@[]));
+}
+
 @end