Fix support for readAttributePaths: for multiple paths in MTRDeviceOv…

…erXPC.

Because MTRDevice will batch reads, we need to handle multiple paths here.

src/darwin/Framework/CHIP/MTRDevice.mm

@@ -192,6 +192,8 @@ @interface MTRDevice ()
 #ifdef DEBUG
 @protocol MTRDeviceUnitTestDelegate <MTRDeviceDelegate>
 - (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<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 @@ 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<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);
  }];

src/darwin/Framework/CHIP/MTRDeviceOverXPC.mm

@@ -139,22 +139,49 @@ - (void)readAttributePaths:(NSArray<MTRAttributeRequestPath *> * _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<NSDictionary<NSString *, id> *> * seenValues = [[NSMutableArray alloc] init];
+ __block BOOL dispatched = NO;
+
+ 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

src/darwin/Framework/CHIPTests/MTRXPCListenerSampleTests.m

@@ -448,6 +448,39 @@ - (void)controller:(MTRDeviceController *)controller commissioningComplete:(NSEr
 
 @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 @@ + (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<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 @@ - (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<NSDictionary<NSString *, id> *> * responseValues) {
+ for (NSDictionary<NSString *, id> * 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));