Fix support for readAttributePaths: for multiple paths in MTRDeviceOverXPC. (#29767)
Because MTRDevice will batch reads, we need to handle multiple paths here.
diff --git a/src/darwin/Framework/CHIP/MTRDevice.mm b/src/darwin/Framework/CHIP/MTRDevice.mm
index a37a7dd..6a80560 100644
--- a/src/darwin/Framework/CHIP/MTRDevice.mm
+++ b/src/darwin/Framework/CHIP/MTRDevice.mm
@@ -192,6 +192,8 @@
#ifdef DEBUG
@protocol MTRDeviceUnitTestDelegate <MTRDeviceDelegate>
- (void)unitTestReportEndForDevice:(MTRDevice *)device;
+- (BOOL)unitTestShouldSetUpSubscriptionForDevice:(MTRDevice *)device;
+- (BOOL)unitTestShouldSkipExpectedValuesForWrite:(MTRDevice *)device;
@end
#endif
@@ -235,11 +237,25 @@
- (void)setDelegate:(id<MTRDeviceDelegate>)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 @@
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 @@
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 @@
completion:^(NSArray<NSDictionary<NSString *, id> *> * _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 529962b..c37754e 100644
--- a/src/darwin/Framework/CHIP/MTRDeviceOverXPC.mm
+++ b/src/darwin/Framework/CHIP/MTRDeviceOverXPC.mm
@@ -139,22 +139,49 @@
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];
+ // 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<NSDictionary<NSString *, id> *> * seenValues = [[NSMutableArray alloc] init];
+ __block BOOL dispatched = NO;
- [self readAttributesWithEndpointID:path.endpoint
- clusterID:path.cluster
- attributeID:path.attribute
- params:params
- queue:queue
- completion:completion];
+ for (MTRAttributeRequestPath * path in attributePaths) {
+ __auto_type singleAttributeResponseHandler = ^(NSArray<NSDictionary<NSString *, id> *> * _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 35403d1..bebed9c 100644
--- a/src/darwin/Framework/CHIPTests/MTRXPCListenerSampleTests.m
+++ b/src/darwin/Framework/CHIPTests/MTRXPCListenerSampleTests.m
@@ -448,6 +448,39 @@
@end
+typedef void (^MTRDeviceTestDelegateDataHandler)(NSArray<NSDictionary<NSString *, id> *> *);
+
+@interface MTRXPCDeviceTestDelegate : NSObject <MTRDeviceDelegate>
+@property (nonatomic, nullable) MTRDeviceTestDelegateDataHandler onAttributeDataReceived;
+@end
+
+@implementation MTRXPCDeviceTestDelegate
+- (void)device:(MTRDevice *)device stateChanged:(MTRDeviceState)state
+{
+}
+
+- (void)device:(MTRDevice *)device receivedAttributeReport:(NSArray<NSDictionary<NSString *, id> *> *)attributeReport
+{
+ if (self.onAttributeDataReceived != nil) {
+ self.onAttributeDataReceived(attributeReport);
+ }
+}
+
+- (void)device:(MTRDevice *)device receivedEventReport:(NSArray<NSDictionary<NSString *, id> *> *)eventReport
+{
+}
+
+- (BOOL)unitTestShouldSetUpSubscriptionForDevice:(MTRDevice *)device
+{
+ return NO;
+}
+
+- (BOOL)unitTestShouldSkipExpectedValuesForWrite:(MTRDevice *)device
+{
+ return YES;
+}
+@end
+
@interface MTRXPCListenerSampleTests : XCTestCase
@end
@@ -465,7 +498,7 @@
// 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 @@
}];
[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 @@
__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<NSString *, id> * (^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<NSString *, id> * (^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<NSString *, id> * 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 @@
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;
};
- dispatch_async(queue, poller);
- [self waitForExpectations:@[ expectation ] timeout:kTimeoutInSeconds + 5];
+ delegate.onAttributeDataReceived = ^(NSArray<NSDictionary<NSString *, id> *> * responseValues) {
+ for (NSDictionary<NSString *, id> * responseValue in responseValues) {
+ if (checkValue(responseValue)) {
+ return;
+ }
+ }
+ };
+
+ __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 @@
}
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 @@
}
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 @@
}];
[self waitForExpectations:@[ onExpectation ] timeout:kTimeoutInSeconds];
- pollForValue(
+ waitForValue(
@(MTRAttributeIDTypeClusterOnOffAttributeOnOffID), ^{
return [onOffCluster readAttributeOnOffWithParams:nil];
}, @(YES));
@@ -1791,7 +1862,7 @@
}];
[self waitForExpectations:@[ offExpectation ] timeout:kTimeoutInSeconds];
- pollForValue(
+ waitForValue(
@(MTRAttributeIDTypeClusterOnOffAttributeOnOffID), ^{
return [onOffCluster readAttributeOnOffWithParams:nil];
}, @(NO));
