From eac0c7cbe72aa3559a2d72b4a826b76e0dc73fdc Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Fri, 13 Oct 2023 22:54:08 -0400 Subject: [PATCH] Fix support for readAttributePaths: for multiple paths in MTRDeviceOverXPC. (#29767) Because MTRDevice will batch reads, we need to handle multiple paths here. --- src/darwin/Framework/CHIP/MTRDevice.mm | 50 +++++- src/darwin/Framework/CHIP/MTRDeviceOverXPC.mm | 47 ++++-- .../CHIPTests/MTRXPCListenerSampleTests.m | 155 +++++++++++++----- 3 files changed, 191 insertions(+), 61 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRDevice.mm b/src/darwin/Framework/CHIP/MTRDevice.mm index a37a7dd5fb0e75..6a805601aaff3c 100644 --- a/src/darwin/Framework/CHIP/MTRDevice.mm +++ b/src/darwin/Framework/CHIP/MTRDevice.mm @@ -192,6 +192,8 @@ @interface MTRDevice () #ifdef DEBUG @protocol MTRDeviceUnitTestDelegate - (void)unitTestReportEndForDevice:(MTRDevice *)device; +- (BOOL)unitTestShouldSetUpSubscriptionForDevice:(MTRDevice *)device; +- (BOOL)unitTestShouldSkipExpectedValuesForWrite:(MTRDevice *)device; @end #endif @@ -235,11 +237,25 @@ + (MTRDevice *)deviceWithNodeID:(NSNumber *)nodeID controller:(MTRDeviceControll - (void)setDelegate:(id)delegate queue:(dispatch_queue_t)queue { MTR_LOG_INFO("%@ setDelegate %@", self, delegate); + + BOOL setUpSubscription = YES; + + // For unit testing only +#ifdef DEBUG + id testDelegate = delegate; + if ([testDelegate respondsToSelector:@selector(unitTestShouldSetUpSubscriptionForDevice:)]) { + setUpSubscription = [testDelegate unitTestShouldSetUpSubscriptionForDevice:self]; + } +#endif + os_unfair_lock_lock(&self->_lock); _weakDelegate = [MTRWeakReference weakReferenceWithObject:delegate]; _delegateQueue = queue; - [self _setupSubscription]; + + if (setUpSubscription) { + [self _setupSubscription]; + } os_unfair_lock_unlock(&self->_lock); } @@ -974,7 +990,9 @@ static BOOL AttributeHasChangesOmittedQuality(MTRAttributePath * attributePath) MTR_LOG_ERROR("Read attribute work item [%llu] failed (will retry): %@", workItemID, error); completion(MTRAsyncWorkNeedsRetry); } else { - MTR_LOG_DEFAULT("Read attribute work item [%llu] failed (giving up): %@", workItemID, error); + if (error) { + MTR_LOG_DEFAULT("Read attribute work item [%llu] failed (giving up): %@", workItemID, error); + } completion(MTRAsyncWorkComplete); } }]; @@ -998,13 +1016,25 @@ - (void)writeAttributeWithEndpointID:(NSNumber *)endpointID expectedValueInterval = MTRClampedNumber(expectedValueInterval, @(1), @(UINT32_MAX)); MTRAttributePath * attributePath = [MTRAttributePath attributePathWithEndpointID:endpointID clusterID:clusterID + attributeID:attributeID]; - // Commit change into expected value cache - NSDictionary * newExpectedValueDictionary = @{ MTRAttributePathKey : attributePath, MTRDataKey : value }; - uint64_t expectedValueID; - [self setExpectedValues:@[ newExpectedValueDictionary ] - expectedValueInterval:expectedValueInterval - expectedValueID:&expectedValueID]; + + BOOL useValueAsExpectedValue = YES; +#ifdef DEBUG + id delegate = _weakDelegate.strongObject; + if ([delegate respondsToSelector:@selector(unitTestShouldSkipExpectedValuesForWrite:)]) { + useValueAsExpectedValue = ![delegate unitTestShouldSkipExpectedValuesForWrite:self]; + } +#endif + + uint64_t expectedValueID = 0; + if (useValueAsExpectedValue) { + // Commit change into expected value cache + NSDictionary * newExpectedValueDictionary = @{ MTRAttributePathKey : attributePath, MTRDataKey : value }; + [self setExpectedValues:@[ newExpectedValueDictionary ] + expectedValueInterval:expectedValueInterval + expectedValueID:&expectedValueID]; + } MTRAsyncWorkItem * workItem = [[MTRAsyncWorkItem alloc] initWithQueue:self.queue]; uint64_t workItemID = workItem.uniqueID; // capture only the ID, not the work item @@ -1026,7 +1056,9 @@ - (void)writeAttributeWithEndpointID:(NSNumber *)endpointID completion:^(NSArray *> * _Nullable values, NSError * _Nullable error) { if (error) { MTR_LOG_ERROR("Write attribute work item [%llu] failed: %@", workItemID, error); - [self removeExpectedValueForAttributePath:attributePath expectedValueID:expectedValueID]; + if (useValueAsExpectedValue) { + [self removeExpectedValueForAttributePath:attributePath expectedValueID:expectedValueID]; + } } completion(MTRAsyncWorkComplete); }]; diff --git a/src/darwin/Framework/CHIP/MTRDeviceOverXPC.mm b/src/darwin/Framework/CHIP/MTRDeviceOverXPC.mm index 529962bdb9f260..c37754e15a95e9 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceOverXPC.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceOverXPC.mm @@ -139,22 +139,49 @@ - (void)readAttributePaths:(NSArray * _Nullable)attri queue:(dispatch_queue_t)queue completion:(MTRDeviceResponseHandler)completion { - if (attributePaths == nil || attributePaths.count != 1 || eventPaths != nil) { - MTR_LOG_ERROR("MTRBaseDevice doesn't support reading more than a single attribute path at a time over XPC"); + if (attributePaths == nil || eventPaths != nil) { + MTR_LOG_ERROR("MTRBaseDevice doesn't support reading event paths over XPC"); dispatch_async(queue, ^{ completion(nil, [NSError errorWithDomain:MTRErrorDomain code:MTRErrorCodeInvalidState userInfo:nil]); }); return; } - auto * path = attributePaths[0]; - - [self readAttributesWithEndpointID:path.endpoint - clusterID:path.cluster - attributeID:path.attribute - params:params - queue:queue - completion:completion]; + // TODO: Have a better XPC setup for the multiple-paths case, instead of + // just converting it into a bunch of separate reads. + auto expectedResponses = attributePaths.count; + __block decltype(expectedResponses) responses = 0; + NSMutableArray *> * seenValues = [[NSMutableArray alloc] init]; + __block BOOL dispatched = NO; + + for (MTRAttributeRequestPath * path in attributePaths) { + __auto_type singleAttributeResponseHandler = ^(NSArray *> * _Nullable values, NSError * _Nullable error) { + if (dispatched) { + // We hit an error earlier or something. + return; + } + + if (error != nil) { + dispatched = YES; + completion(nil, error); + return; + } + + [seenValues addObjectsFromArray:values]; + ++responses; + if (responses == expectedResponses) { + dispatched = YES; + completion([seenValues copy], nil); + }; + }; + + [self readAttributesWithEndpointID:path.endpoint + clusterID:path.cluster + attributeID:path.attribute + params:params + queue:queue + completion:singleAttributeResponseHandler]; + } } - (void)writeAttributeWithEndpointID:(NSNumber *)endpointID diff --git a/src/darwin/Framework/CHIPTests/MTRXPCListenerSampleTests.m b/src/darwin/Framework/CHIPTests/MTRXPCListenerSampleTests.m index 35403d1290d483..bebed9c42fde84 100644 --- a/src/darwin/Framework/CHIPTests/MTRXPCListenerSampleTests.m +++ b/src/darwin/Framework/CHIPTests/MTRXPCListenerSampleTests.m @@ -448,6 +448,39 @@ - (void)controller:(MTRDeviceController *)controller commissioningComplete:(NSEr @end +typedef void (^MTRDeviceTestDelegateDataHandler)(NSArray *> *); + +@interface MTRXPCDeviceTestDelegate : NSObject +@property (nonatomic, nullable) MTRDeviceTestDelegateDataHandler onAttributeDataReceived; +@end + +@implementation MTRXPCDeviceTestDelegate +- (void)device:(MTRDevice *)device stateChanged:(MTRDeviceState)state +{ +} + +- (void)device:(MTRDevice *)device receivedAttributeReport:(NSArray *> *)attributeReport +{ + if (self.onAttributeDataReceived != nil) { + self.onAttributeDataReceived(attributeReport); + } +} + +- (void)device:(MTRDevice *)device receivedEventReport:(NSArray *> *)eventReport +{ +} + +- (BOOL)unitTestShouldSetUpSubscriptionForDevice:(MTRDevice *)device +{ + return NO; +} + +- (BOOL)unitTestShouldSkipExpectedValuesForWrite:(MTRDevice *)device +{ + return YES; +} +@end + @interface MTRXPCListenerSampleTests : XCTestCase @end @@ -465,7 +498,7 @@ + (void)tearDown // we're running only one of our test methods (using // -only-testing:MatterTests/MTROTAProviderTests/testMethodName), since // we did not run test999_TearDown. - [self shutdownStack]; + // [self shutdownStack]; } } @@ -1565,7 +1598,8 @@ - (void)test013_TimedWriteAttribute }]; [self waitForExpectationsWithTimeout:kTimeoutInSeconds handler:nil]; -#if 0 // The above attribute isn't for timed interaction. Hence, no verification till we have a capable attribute. +#if 0 + // The above attribute isn't for timed interaction. Hence, no verification till we have a capable attribute. // subscribe, which should get the new value at the timeout expectation = [self expectationWithDescription:@"Subscribed"]; __block void (^reportHandler)(id _Nullable values, NSError * _Nullable error); @@ -1682,28 +1716,50 @@ - (void)test015_MTRDeviceInteraction __auto_type * device = [MTRDevice deviceWithNodeID:@(kDeviceId) controller:mDeviceController]; dispatch_queue_t queue = dispatch_get_main_queue(); + __auto_type * delegate = [[MTRXPCDeviceTestDelegate alloc] init]; + [device setDelegate:delegate queue:queue]; + __auto_type * endpoint = @(1); __auto_type * onOffCluster = [[MTRClusterOnOff alloc] initWithDevice:device endpointID:endpoint queue:queue]; - // Since we have no subscription, reads don't really work right. We need to - // poll for values instead. - __auto_type pollForValue = ^(NSNumber * attributeID, NSDictionary * (^readBlock)(void), NSNumber * value) { - XCTestExpectation * expectation = [self expectationWithDescription:[NSString stringWithFormat:@"Polling for on/off=%@", value]]; + // The previous tests have left us in a not-so-great state where the device + // is (1) on and (2) in the middle of a level move. Reset to a known state + // where the device is off, the level is midway, so it's not doing either on + // or off due to the level move, and there is no level move going on. + XCTestExpectation * initialOffExpectation = [self expectationWithDescription:@"Turning off to reset to base state"]; + [onOffCluster offWithParams:nil expectedValues:nil expectedValueInterval:nil completion:^(NSError * error) { + XCTAssertNil(error); + [initialOffExpectation fulfill]; + }]; + [self waitForExpectations:@[ initialOffExpectation ] timeout:kTimeoutInSeconds]; + + __auto_type * levelCluster = [[MTRClusterLevelControl alloc] initWithDevice:device endpointID:endpoint queue:queue]; + XCTestExpectation * initialLevelExpectation = [self expectationWithDescription:@"Setting midpoint level"]; + __auto_type * params = [[MTRLevelControlClusterMoveToLevelParams alloc] init]; + params.level = @(128); + params.transitionTime = @(0); + params.optionsMask = @(MTRLevelControlOptionsExecuteIfOff); + params.optionsOverride = @(MTRLevelControlOptionsExecuteIfOff); + [levelCluster moveToLevelWithParams:params expectedValues:nil expectedValueInterval:nil completion:^(NSError * error) { + XCTAssertNil(error); + [initialLevelExpectation fulfill]; + }]; + [self waitForExpectations:@[ initialLevelExpectation ] timeout:kTimeoutInSeconds]; + + // Since we have no subscription, sync reads don't really work right. After doing + // them, if we get a value that does not match expectation we need to wait for our + // delegate to be notified about the attribute. + __auto_type waitForValue = ^(NSNumber * attributeID, NSDictionary * (^readBlock)(void), NSNumber * value) { + XCTestExpectation * expectation = [self expectationWithDescription:[NSString stringWithFormat:@"Waiting for attribute %@=%@", attributeID, value]]; __auto_type * path = [MTRAttributePath attributePathWithEndpointID:endpoint clusterID:@(MTRClusterIDTypeOnOffID) attributeID:attributeID]; - __block dispatch_block_t poller = ^{ - __auto_type * attrValue = readBlock(); - if (attrValue == nil) { - dispatch_async(queue, poller); - return; + __block __auto_type checkValue = ^(NSDictionary * responseValue) { + if (![path isEqual:responseValue[MTRAttributePathKey]]) { + // Not our attribute. + return NO; } - __auto_type * responseValue = @{ - MTRAttributePathKey : path, - MTRDataKey : attrValue, - }; - NSError * error; __auto_type * report = [[MTRAttributeReport alloc] initWithResponseValue:responseValue error:&error]; XCTAssertNil(error); @@ -1712,24 +1768,51 @@ - (void)test015_MTRDeviceInteraction XCTAssertNotNil(report.value); if ([report.value isEqual:value]) { - // Break cycle. - poller = nil; + delegate.onAttributeDataReceived = nil; [expectation fulfill]; - return; + return YES; } - dispatch_async(queue, poller); + // Keep waiting. + return NO; + }; + + delegate.onAttributeDataReceived = ^(NSArray *> * responseValues) { + for (NSDictionary * responseValue in responseValues) { + if (checkValue(responseValue)) { + return; + } + } }; - dispatch_async(queue, poller); - [self waitForExpectations:@[ expectation ] timeout:kTimeoutInSeconds + 5]; + __auto_type * attrValue = readBlock(); + if (attrValue != nil) { + __auto_type * responseValue = @{ + MTRAttributePathKey : path, + MTRDataKey : attrValue, + }; + + checkValue(responseValue); + } + + [self waitForExpectations:@[ expectation ] timeout:kTimeoutInSeconds]; }; - pollForValue( + // Wait until the OnOff value is read. But issue reads for multiple + // attributes, so that we test what happens if multiple reads are issued in + // a row. The attribute we care about should be last, so it gets batched + // with the others, and we do more than 2 reads so that even if the first + // one is dispatched immediately the others batch. Make sure none of the + // other reads involve attributes we care about later in this test. + waitForValue( @(MTRAttributeIDTypeClusterOnOffAttributeOnOffID), ^{ + [onOffCluster readAttributeOffWaitTimeWithParams:nil]; + [onOffCluster readAttributeGlobalSceneControlWithParams:nil]; + [onOffCluster readAttributeStartUpOnOffWithParams:nil]; return [onOffCluster readAttributeOnOffWithParams:nil]; }, @(NO)); - pollForValue( + + waitForValue( @(MTRAttributeIDTypeClusterOnOffAttributeOnTimeID), ^{ return [onOffCluster readAttributeOnTimeWithParams:nil]; }, @(0)); @@ -1741,14 +1824,8 @@ - (void)test015_MTRDeviceInteraction } expectedValueInterval:@(0)]; - // Wait until the expected value is removed. - pollForValue( - @(MTRAttributeIDTypeClusterOnOffAttributeOnTimeID), ^{ - return [onOffCluster readAttributeOnTimeWithParams:nil]; - }, @(0)); - - // Now wait until the new value is read. - pollForValue( + // Wait until the new value is read. + waitForValue( @(MTRAttributeIDTypeClusterOnOffAttributeOnTimeID), ^{ return [onOffCluster readAttributeOnTimeWithParams:nil]; }, @(100)); @@ -1759,17 +1836,11 @@ - (void)test015_MTRDeviceInteraction } expectedValueInterval:@(0)]; - // Wait until the expected value is removed. - pollForValue( - @(MTRAttributeIDTypeClusterOnOffAttributeOnTimeID), ^{ - return [onOffCluster readAttributeOnTimeWithParams:nil]; - }, @(100)); - // Now wait until the new value is read. - pollForValue( + waitForValue( @(MTRAttributeIDTypeClusterOnOffAttributeOnTimeID), ^{ return [onOffCluster readAttributeOnTimeWithParams:nil]; - }, @(00)); + }, @(0)); // Test that invokes work. XCTestExpectation * onExpectation = [self expectationWithDescription:@"Turning on"]; @@ -1779,7 +1850,7 @@ - (void)test015_MTRDeviceInteraction }]; [self waitForExpectations:@[ onExpectation ] timeout:kTimeoutInSeconds]; - pollForValue( + waitForValue( @(MTRAttributeIDTypeClusterOnOffAttributeOnOffID), ^{ return [onOffCluster readAttributeOnOffWithParams:nil]; }, @(YES)); @@ -1791,7 +1862,7 @@ - (void)test015_MTRDeviceInteraction }]; [self waitForExpectations:@[ offExpectation ] timeout:kTimeoutInSeconds]; - pollForValue( + waitForValue( @(MTRAttributeIDTypeClusterOnOffAttributeOnOffID), ^{ return [onOffCluster readAttributeOnOffWithParams:nil]; }, @(NO));