Skip to content

Commit

Permalink
Fix support for readAttributePaths: for multiple paths in MTRDeviceOv…
Browse files Browse the repository at this point in the history 
…erXPC. (project-chip#29767)

Because MTRDevice will batch reads, we need to handle multiple paths here.
  • Loading branch information
@bzbarsky-apple
bzbarsky-apple authored Oct 14, 2023
1 parent 78efca3 commit eac0c7c
Show file tree
Hide file tree
Showing 3 changed files with 191 additions and 61 deletions.
50 changes: 41 additions & 9 deletions src/darwin/Framework/CHIP/MTRDevice.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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);
}
}];
Expand All @@ -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
Expand All @@ -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);
}];
Expand Down
47 changes: 37 additions & 10 deletions src/darwin/Framework/CHIP/MTRDeviceOverXPC.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
155 changes: 113 additions & 42 deletions src/darwin/Framework/CHIPTests/MTRXPCListenerSampleTests.m
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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];
}
}

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand All @@ -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));
Expand All @@ -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));
Expand All @@ -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"];
Expand All @@ -1779,7 +1850,7 @@ - (void)test015_MTRDeviceInteraction
}];
[self waitForExpectations:@[ onExpectation ] timeout:kTimeoutInSeconds];

pollForValue(
waitForValue(
@(MTRAttributeIDTypeClusterOnOffAttributeOnOffID), ^{
return [onOffCluster readAttributeOnOffWithParams:nil];
}, @(YES));
Expand All @@ -1791,7 +1862,7 @@ - (void)test015_MTRDeviceInteraction
}];
[self waitForExpectations:@[ offExpectation ] timeout:kTimeoutInSeconds];

pollForValue(
waitForValue(
@(MTRAttributeIDTypeClusterOnOffAttributeOnOffID), ^{
return [onOffCluster readAttributeOnOffWithParams:nil];
}, @(NO));
Expand Down

0 comments on commit eac0c7c

Please sign in to comment.