From 1c030919fac82fc17c3182cbb5014bf6a817136b Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Tue, 16 Apr 2024 11:39:01 -0400 Subject: [PATCH 1/2] Stop assuming all unknown attributes have the C quality in MTRDevice. Assume things we don't know about don't have the C quality unless told to assume otherwise. Also fixes XPC detection check; non-XPC controllers also respond to the "sharedControllerWithID:xpcConnectBlock:" selector. --- src/darwin/Framework/CHIP/MTRCluster.h | 11 + src/darwin/Framework/CHIP/MTRCluster.mm | 3 + src/darwin/Framework/CHIP/MTRDevice.mm | 36 ++- .../CHIPTests/MTRPerControllerStorageTests.m | 215 +++++++++++------- .../TestHelpers/MTRDeviceTestDelegate.h | 1 + .../TestHelpers/MTRDeviceTestDelegate.m | 5 + 6 files changed, 186 insertions(+), 85 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRCluster.h b/src/darwin/Framework/CHIP/MTRCluster.h index f1863bbc05bc95..f1d876e493d73e 100644 --- a/src/darwin/Framework/CHIP/MTRCluster.h +++ b/src/darwin/Framework/CHIP/MTRCluster.h @@ -132,6 +132,17 @@ MTR_AVAILABLE(ios(16.1), macos(13.0), watchos(9.1), tvos(16.1)) */ @property (nonatomic, copy, nullable) NSNumber * minEventNumber MTR_AVAILABLE(ios(16.4), macos(13.3), watchos(9.4), tvos(16.4)); +/** + * Controls whether attributes without known schema (e.g. vendor-specific + * attributes) should be assumed to be reportable normally via subscriptions. + * The default is YES. + * + * This setting is only relevant to some consumers of MTRReadParams. One of + * those consumers is readAttributeWithEndpointID:clusterID:attributeID:params: + * on MTRDevice. + */ +@property (nonatomic, assign, getter=shouldAssumeUnknownAttributesReportable) BOOL assumeUnknownAttributesReportable MTR_NEWLY_AVAILABLE; + @end /** diff --git a/src/darwin/Framework/CHIP/MTRCluster.mm b/src/darwin/Framework/CHIP/MTRCluster.mm index 96d4c4bb462e34..9bcb38e4cbac99 100644 --- a/src/darwin/Framework/CHIP/MTRCluster.mm +++ b/src/darwin/Framework/CHIP/MTRCluster.mm @@ -84,6 +84,7 @@ - (instancetype)init { if (self = [super init]) { _filterByFabric = YES; + _assumeUnknownAttributesReportable = YES; } return self; } @@ -93,6 +94,7 @@ - (id)copyWithZone:(NSZone * _Nullable)zone auto other = [[MTRReadParams alloc] init]; other.filterByFabric = self.filterByFabric; other.minEventNumber = self.minEventNumber; + other.assumeUnknownAttributesReportable = self.assumeUnknownAttributesReportable; return other; } @@ -124,6 +126,7 @@ - (id)copyWithZone:(NSZone * _Nullable)zone auto other = [[MTRSubscribeParams alloc] initWithMinInterval:self.minInterval maxInterval:self.maxInterval]; other.filterByFabric = self.filterByFabric; other.minEventNumber = self.minEventNumber; + other.assumeUnknownAttributesReportable = self.assumeUnknownAttributesReportable; other.replaceExistingSubscriptions = self.replaceExistingSubscriptions; other.reportEventsUrgently = self.reportEventsUrgently; other.resubscribeAutomatically = self.resubscribeAutomatically; diff --git a/src/darwin/Framework/CHIP/MTRDevice.mm b/src/darwin/Framework/CHIP/MTRDevice.mm index aa63efe1033633..aae5be35df2381 100644 --- a/src/darwin/Framework/CHIP/MTRDevice.mm +++ b/src/darwin/Framework/CHIP/MTRDevice.mm @@ -336,6 +336,7 @@ - (void)unitTestReportEndForDevice:(MTRDevice *)device; - (BOOL)unitTestShouldSetUpSubscriptionForDevice:(MTRDevice *)device; - (BOOL)unitTestShouldSkipExpectedValuesForWrite:(MTRDevice *)device; - (NSNumber *)unitTestMaxIntervalOverrideForSubscription:(MTRDevice *)device; +- (BOOL)unitTestForceAttributeReportsIfMatchingCache:(MTRDevice *)device; @end #endif @@ -745,7 +746,7 @@ - (BOOL)_subscriptionAbleToReport // Unfortunately, we currently have no subscriptions over our hacked-up XPC // setup. Try to detect that situation. - if ([_deviceController.class respondsToSelector:@selector(sharedControllerWithID:xpcConnectBlock:)]) { + if ([_deviceController isKindOfClass:MTRDeviceControllerOverXPC.class]) { return NO; } @@ -1603,24 +1604,31 @@ static BOOL AttributeHasChangesOmittedQuality(MTRAttributePath * attributePath) - (NSDictionary * _Nullable)readAttributeWithEndpointID:(NSNumber *)endpointID clusterID:(NSNumber *)clusterID attributeID:(NSNumber *)attributeID - params:(MTRReadParams *)params + params:(MTRReadParams * _Nullable)params { MTRAttributePath * attributePath = [MTRAttributePath attributePathWithEndpointID:endpointID clusterID:clusterID attributeID:attributeID]; BOOL attributeIsSpecified = MTRAttributeIsSpecified(clusterID.unsignedIntValue, attributeID.unsignedIntValue); - BOOL hasChangesOmittedQuality = AttributeHasChangesOmittedQuality(attributePath); + BOOL hasChangesOmittedQuality; + if (attributeIsSpecified) { + hasChangesOmittedQuality = AttributeHasChangesOmittedQuality(attributePath); + } else { + if (params == nil) { + hasChangesOmittedQuality = NO; + } else { + hasChangesOmittedQuality = !params.assumeUnknownAttributesReportable; + } + } // Return current known / expected value right away NSDictionary * attributeValueToReturn = [self _attributeValueDictionaryForAttributePath:attributePath]; // Send read request to device if any of the following are true: - // 1. The attribute is not in the specification (so we don't know whether hasChangesOmittedQuality can be trusted). - // 2. Subscription not in a state we can expect reports - // 3. There is subscription but attribute has Changes Omitted quality - // TODO: add option for BaseSubscriptionCallback to report during priming, to reduce when case 4 is hit - if (!attributeIsSpecified || ![self _subscriptionAbleToReport] || hasChangesOmittedQuality) { + // 1. Subscription not in a state we can expect reports + // 2. The attribute has (or is assumed to have) the Changes Omitted quality, so we won't get reports for it. + if (![self _subscriptionAbleToReport] || hasChangesOmittedQuality) { // Read requests container will be a mutable array of items, each being an array containing: // [attribute request path, params] // Batching handler should only coalesce when params are equal. @@ -2344,6 +2352,18 @@ - (NSArray *)_getAttributesToReportWithReportedValues:(NSArray *> * _Nullable values, NSError * _Nullable error) { - checkSingleValue(values, error, unsignedIntValue1); - [readExpectation1 fulfill]; - }]; + [baseDevice readAttributePaths:@[ requestPath ] + eventPaths:nil + params:nil + queue:queue + completion:^(NSArray *> * _Nullable values, NSError * _Nullable error) { + checkSingleValue(values, error, unsignedIntValue1); + [readExpectation1 fulfill]; + }]; [self waitForExpectations:@[ readExpectation1 ] timeout:kTimeoutInSeconds]; // Now try a basic subscribe. @@ -1783,7 +1783,7 @@ - (void)testControllerServer XCTestExpectation * subscriptionEstablishedExpectation = [self expectationWithDescription:@"Basic subscription established"]; __auto_type * subscribeParams = [[MTRSubscribeParams alloc] initWithMinInterval:@(0) maxInterval:@(10)]; - [device subscribeToAttributesWithEndpointID:requestPath.endpoint clusterID:requestPath.cluster attributeID:requestPath.attribute + [baseDevice subscribeToAttributesWithEndpointID:requestPath.endpoint clusterID:requestPath.cluster attributeID:requestPath.attribute params:subscribeParams queue:queue reportHandler:^(NSArray *> * _Nullable values, NSError * _Nullable error) { @@ -1809,14 +1809,14 @@ - (void)testControllerServer requestPath = attribute2RequestPath; responsePath = attribute2ResponsePath; XCTestExpectation * readNoPermissionsExpectation1 = [self expectationWithDescription:@"Read 1 of attribute with no permissions complete"]; - [device readAttributePaths:@[ requestPath ] - eventPaths:nil - params:nil - queue:queue - completion:^(NSArray *> * _Nullable values, NSError * _Nullable error) { - checkSinglePathError(values, error, MTRInteractionErrorCodeUnsupportedAccess); - [readNoPermissionsExpectation1 fulfill]; - }]; + [baseDevice readAttributePaths:@[ requestPath ] + eventPaths:nil + params:nil + queue:queue + completion:^(NSArray *> * _Nullable values, NSError * _Nullable error) { + checkSinglePathError(values, error, MTRInteractionErrorCodeUnsupportedAccess); + [readNoPermissionsExpectation1 fulfill]; + }]; [self waitForExpectations:@[ readNoPermissionsExpectation1 ] timeout:kTimeoutInSeconds]; // Change the permissions to give Manage access on the cluster to some @@ -1826,14 +1826,14 @@ - (void)testControllerServer [cluster2 addAccessGrant:unrelatedGrant]; XCTestExpectation * readNoPermissionsExpectation2 = [self expectationWithDescription:@"Read 2 of attribute with no permissions complete"]; - [device readAttributePaths:@[ requestPath ] - eventPaths:nil - params:nil - queue:queue - completion:^(NSArray *> * _Nullable values, NSError * _Nullable error) { - checkSinglePathError(values, error, MTRInteractionErrorCodeUnsupportedAccess); - [readNoPermissionsExpectation2 fulfill]; - }]; + [baseDevice readAttributePaths:@[ requestPath ] + eventPaths:nil + params:nil + queue:queue + completion:^(NSArray *> * _Nullable values, NSError * _Nullable error) { + checkSinglePathError(values, error, MTRInteractionErrorCodeUnsupportedAccess); + [readNoPermissionsExpectation2 fulfill]; + }]; [self waitForExpectations:@[ readNoPermissionsExpectation2 ] timeout:kTimeoutInSeconds]; // Change the permissions to give Manage access on the cluster to our client @@ -1843,14 +1843,14 @@ - (void)testControllerServer [cluster2 addAccessGrant:clientManageGrant]; XCTestExpectation * readExpectation2 = [self expectationWithDescription:@"Read 2 of attribute complete"]; - [device readAttributePaths:@[ requestPath ] - eventPaths:nil - params:nil - queue:queue - completion:^(NSArray *> * _Nullable values, NSError * _Nullable error) { - checkSingleValue(values, error, listOfStructsValue1); - [readExpectation2 fulfill]; - }]; + [baseDevice readAttributePaths:@[ requestPath ] + eventPaths:nil + params:nil + queue:queue + completion:^(NSArray *> * _Nullable values, NSError * _Nullable error) { + checkSingleValue(values, error, listOfStructsValue1); + [readExpectation2 fulfill]; + }]; [self waitForExpectations:@[ readExpectation2 ] timeout:kTimeoutInSeconds]; // Adding Manage permissions to one cluster should not affect another one. @@ -1858,14 +1858,14 @@ - (void)testControllerServer responsePath = attribute3ResponsePath; XCTestExpectation * readNoPermissionsExpectation3 = [self expectationWithDescription:@"Read 3 of attribute with no permissions complete"]; - [device readAttributePaths:@[ requestPath ] - eventPaths:nil - params:nil - queue:queue - completion:^(NSArray *> * _Nullable values, NSError * _Nullable error) { - checkSinglePathError(values, error, MTRInteractionErrorCodeUnsupportedAccess); - [readNoPermissionsExpectation3 fulfill]; - }]; + [baseDevice readAttributePaths:@[ requestPath ] + eventPaths:nil + params:nil + queue:queue + completion:^(NSArray *> * _Nullable values, NSError * _Nullable error) { + checkSinglePathError(values, error, MTRInteractionErrorCodeUnsupportedAccess); + [readNoPermissionsExpectation3 fulfill]; + }]; [self waitForExpectations:@[ readNoPermissionsExpectation3 ] timeout:kTimeoutInSeconds]; // But adding Manage permissions on the endpoint should grant Operate on @@ -1873,28 +1873,28 @@ - (void)testControllerServer [endpoint1 addAccessGrant:clientManageGrant]; XCTestExpectation * readExpectation3 = [self expectationWithDescription:@"Read 3 of attribute complete"]; - [device readAttributePaths:@[ requestPath ] - eventPaths:nil - params:nil - queue:queue - completion:^(NSArray *> * _Nullable values, NSError * _Nullable error) { - checkSingleValue(values, error, unsignedIntValue1); - [readExpectation3 fulfill]; - }]; + [baseDevice readAttributePaths:@[ requestPath ] + eventPaths:nil + params:nil + queue:queue + completion:^(NSArray *> * _Nullable values, NSError * _Nullable error) { + checkSingleValue(values, error, unsignedIntValue1); + [readExpectation3 fulfill]; + }]; [self waitForExpectations:@[ readExpectation3 ] timeout:kTimeoutInSeconds]; // And removing that grant should remove the permissions again. [endpoint1 removeAccessGrant:clientManageGrant]; XCTestExpectation * readNoPermissionsExpectation4 = [self expectationWithDescription:@"Read 4 of attribute with no permissions complete"]; - [device readAttributePaths:@[ requestPath ] - eventPaths:nil - params:nil - queue:queue - completion:^(NSArray *> * _Nullable values, NSError * _Nullable error) { - checkSinglePathError(values, error, MTRInteractionErrorCodeUnsupportedAccess); - [readNoPermissionsExpectation4 fulfill]; - }]; + [baseDevice readAttributePaths:@[ requestPath ] + eventPaths:nil + params:nil + queue:queue + completion:^(NSArray *> * _Nullable values, NSError * _Nullable error) { + checkSinglePathError(values, error, MTRInteractionErrorCodeUnsupportedAccess); + [readNoPermissionsExpectation4 fulfill]; + }]; [self waitForExpectations:@[ readNoPermissionsExpectation4 ] timeout:kTimeoutInSeconds]; // Now do a wildcard read on the endpoint and check that this does the right @@ -1926,23 +1926,23 @@ - (void)testControllerServer }; #endif XCTestExpectation * wildcardReadExpectation = [self expectationWithDescription:@"Wildcard read of our endpoint"]; - [device readAttributePaths:@[ [MTRAttributeRequestPath requestPathWithEndpointID:endpointId1 clusterID:nil attributeID:nil] ] - eventPaths:nil - params:nil - queue:queue - completion:^(NSArray *> * _Nullable values, NSError * _Nullable error) { - XCTAssertNil(error); - XCTAssertNotNil(values); - - // TODO: Figure out how to test that values is correct that's not - // too fragile if things get returned in different valid order. - // For now just check that every path we got has a value, not an - // error. - for (NSDictionary * value in values) { - XCTAssertNotNil(value[MTRAttributePathKey]); - XCTAssertNil(value[MTRErrorKey]); - XCTAssertNotNil(value[MTRDataKey]); - } + [baseDevice readAttributePaths:@[ [MTRAttributeRequestPath requestPathWithEndpointID:endpointId1 clusterID:nil attributeID:nil] ] + eventPaths:nil + params:nil + queue:queue + completion:^(NSArray *> * _Nullable values, NSError * _Nullable error) { + XCTAssertNil(error); + XCTAssertNotNil(values); + + // TODO: Figure out how to test that values is correct that's not + // too fragile if things get returned in different valid order. + // For now just check that every path we got has a value, not an + // error. + for (NSDictionary * value in values) { + XCTAssertNotNil(value[MTRAttributePathKey]); + XCTAssertNil(value[MTRErrorKey]); + XCTAssertNotNil(value[MTRDataKey]); + } #if 0 XCTAssertEqualObjects(values, @[ // cluster1 @@ -1960,10 +1960,71 @@ - (void)testControllerServer ]); #endif - [wildcardReadExpectation fulfill]; - }]; + [wildcardReadExpectation fulfill]; + }]; [self waitForExpectations:@[ wildcardReadExpectation ] timeout:kTimeoutInSeconds]; + // Do some MTRDevice testing against this convenient server we have that has + // vendor-specific attributes. + __auto_type * device = [MTRDevice deviceWithNodeID:nodeIDServer controller:controllerClient]; + __auto_type * delegate = [[MTRDeviceTestDelegate alloc] init]; + delegate.forceAttributeReportsIfMatchingCache = YES; + + XCTestExpectation * gotReportsExpectation = [self expectationWithDescription:@"MTRDevice subscription established"]; + delegate.onReportEnd = ^() { + [gotReportsExpectation fulfill]; + }; + + [device setDelegate:delegate queue:queue]; + + [self waitForExpectations:@[ gotReportsExpectation ] timeout:kTimeoutInSeconds]; + + delegate.onReportEnd = nil; + + // Test read-through behavior of non-standard (as in, not present in Matter XML) attributes. + XCTestExpectation * nonStandardReadThroughExpectation = [self expectationWithDescription:@"Read-throughs of non-standard attributes complete"]; + + delegate.onAttributeDataReceived = ^(NSArray *> * attributeReports) { + XCTAssertNotNil(attributeReports); + + for (NSDictionary * report in attributeReports) { + XCTAssertNil(report[MTRErrorKey]); + + XCTAssertNotNil(report[MTRDataKey]); + XCTAssertNotNil(report[MTRAttributePathKey]); + + // We only expect to get a report for the read that opted in to be + // treated as "C" + XCTAssertEqualObjects(report[MTRAttributePathKey], attribute2ResponsePath); + + // Strip out the DataVersion before comparing values, since our + // local value does not have that. + __auto_type * reportValue = [NSMutableDictionary dictionaryWithDictionary:report[MTRDataKey]]; + reportValue[MTRDataVersionKey] = nil; + XCTAssertEqualObjects(reportValue, listOfStructsValue1); + + [nonStandardReadThroughExpectation fulfill]; + } + }; + + __auto_type * attrValue = [device readAttributeWithEndpointID:attribute1ResponsePath.endpoint + clusterID:attribute1ResponsePath.cluster + attributeID:attribute1ResponsePath.attribute + params:nil]; + XCTAssertNotNil(attrValue); + XCTAssertEqualObjects(attrValue, unsignedIntValue2); + + __auto_type * params = [[MTRReadParams alloc] init]; + params.assumeUnknownAttributesReportable = NO; + attrValue = [device readAttributeWithEndpointID:attribute2ResponsePath.endpoint + clusterID:attribute2ResponsePath.cluster + attributeID:attribute2ResponsePath.attribute + params:params]; + XCTAssertNotNil(attrValue); + XCTAssertEqualObjects(attrValue, listOfStructsValue1); + + [self waitForExpectations:@[ nonStandardReadThroughExpectation ] timeout:kTimeoutInSeconds]; + [controllerClient shutdown]; [controllerServer shutdown]; } diff --git a/src/darwin/Framework/CHIPTests/TestHelpers/MTRDeviceTestDelegate.h b/src/darwin/Framework/CHIPTests/TestHelpers/MTRDeviceTestDelegate.h index 27596bcafeae7f..5c42d2eb1022af 100644 --- a/src/darwin/Framework/CHIPTests/TestHelpers/MTRDeviceTestDelegate.h +++ b/src/darwin/Framework/CHIPTests/TestHelpers/MTRDeviceTestDelegate.h @@ -29,6 +29,7 @@ typedef void (^MTRDeviceTestDelegateDataHandler)(NSArray Date: Tue, 16 Apr 2024 13:54:14 -0400 Subject: [PATCH 2/2] Address review comments and fix tests to work in the new setup. --- src/darwin/Framework/CHIP/MTRDevice.mm | 4 +++- src/darwin/Framework/CHIPTests/MTRDeviceTests.m | 7 ++++++- src/darwin/Framework/CHIPTests/MTRSwiftDeviceTests.swift | 6 +++++- 3 files changed, 14 insertions(+), 3 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRDevice.mm b/src/darwin/Framework/CHIP/MTRDevice.mm index aae5be35df2381..5226759ebf4b51 100644 --- a/src/darwin/Framework/CHIP/MTRDevice.mm +++ b/src/darwin/Framework/CHIP/MTRDevice.mm @@ -1627,7 +1627,9 @@ static BOOL AttributeHasChangesOmittedQuality(MTRAttributePath * attributePath) // Send read request to device if any of the following are true: // 1. Subscription not in a state we can expect reports - // 2. The attribute has (or is assumed to have) the Changes Omitted quality, so we won't get reports for it. + // 2. The attribute has the Changes Omitted quality, so we won't get reports for it. + // 3. The attribute is not in the spec, and the read params asks to assume + // an unknown attribute has the Changes Omitted quality. if (![self _subscriptionAbleToReport] || hasChangesOmittedQuality) { // Read requests container will be a mutable array of items, each being an array containing: // [attribute request path, params] diff --git a/src/darwin/Framework/CHIPTests/MTRDeviceTests.m b/src/darwin/Framework/CHIPTests/MTRDeviceTests.m index 06d06404ffeaad..4f97026ed6644f 100644 --- a/src/darwin/Framework/CHIPTests/MTRDeviceTests.m +++ b/src/darwin/Framework/CHIPTests/MTRDeviceTests.m @@ -1518,6 +1518,10 @@ - (void)test017_TestMTRDeviceBasics [self waitForExpectations:@[ onTimeWriteSuccess, onTimePreviousValue ] timeout:10]; // Test if errors are properly received + // TODO: We might stop reporting these altogether from MTRDevice, and then + // this test will need updating. + __auto_type * readThroughForUnknownAttributesParams = [[MTRReadParams alloc] init]; + readThroughForUnknownAttributesParams.assumeUnknownAttributesReportable = NO; XCTestExpectation * attributeReportErrorExpectation = [self expectationWithDescription:@"Attribute read error"]; delegate.onAttributeDataReceived = ^(NSArray *> * data) { for (NSDictionary * attributeReponseValue in data) { @@ -1526,8 +1530,9 @@ - (void)test017_TestMTRDeviceBasics } } }; + // use the nonexistent attribute and expect read error - [device readAttributeWithEndpointID:testEndpointID clusterID:testClusterID attributeID:testAttributeID params:nil]; + [device readAttributeWithEndpointID:testEndpointID clusterID:testClusterID attributeID:testAttributeID params:readThroughForUnknownAttributesParams]; [self waitForExpectations:@[ attributeReportErrorExpectation ] timeout:10]; // Resubscription test setup diff --git a/src/darwin/Framework/CHIPTests/MTRSwiftDeviceTests.swift b/src/darwin/Framework/CHIPTests/MTRSwiftDeviceTests.swift index 1276868a14c5fa..21d61cb0360ac1 100644 --- a/src/darwin/Framework/CHIPTests/MTRSwiftDeviceTests.swift +++ b/src/darwin/Framework/CHIPTests/MTRSwiftDeviceTests.swift @@ -287,6 +287,10 @@ class MTRSwiftDeviceTests : XCTestCase { wait(for: [ expectedValueReportedExpectation, expectedValueRemovedExpectation ], timeout: 5, enforceOrder: true) // Test if errors are properly received + // TODO: We might stop reporting these altogether from MTRDevice, and then + // this test will need updating. + let readThroughForUnknownAttributesParams = MTRReadParams() + readThroughForUnknownAttributesParams.shouldAssumeUnknownAttributesReportable = false; let attributeReportErrorExpectation = expectation(description: "Attribute read error") delegate.onAttributeDataReceived = { (data: [[ String: Any ]]) -> Void in for attributeReponseValue in data { @@ -296,7 +300,7 @@ class MTRSwiftDeviceTests : XCTestCase { } } // use the nonexistent attribute and expect read error - device.readAttribute(withEndpointID: testEndpointID, clusterID: testClusterID, attributeID: testAttributeID, params: nil) + device.readAttribute(withEndpointID: testEndpointID, clusterID: testClusterID, attributeID: testAttributeID, params: readThroughForUnknownAttributesParams) wait(for: [ attributeReportErrorExpectation ], timeout: 10) // Resubscription test setup