Skip to content

Commit

Permalink
Fix handling of path-specific errors in MTRBaseDevice subscribe code.
Browse files Browse the repository at this point in the history 
1) Improve documentation for subcribeTo* functions on MTRBaseDevice.
2) Fix the implementation to not call the subscriptionEstablished handler when
   the subscription actually errors out before the subscribe is complete.
3) Fix the error handling in the implementation to report path-specific errors
   using the same mechanism as MTRDevice.

Fixes #26076
  • Loading branch information
@bzbarsky-apple
bzbarsky-apple committed Apr 19, 2023
1 parent b8368a1 commit 01495be
Show file tree
Hide file tree
Showing 4 changed files with 187 additions and 52 deletions.
35 changes: 34 additions & 1 deletion src/darwin/Framework/CHIP/MTRBaseDevice.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ MTR_NEWLY_AVAILABLE
* eventReportHandler will be called any time an event is reported (with a
* non-nil "value")
*
* The array passed to eventReportHandler will contain CHIPEventReport
* The array passed to eventReportHandler will contain MTREventReport
* instances. Errors for specific paths, not the whole subscription, will be
* reported via those objects.
*
Expand Down Expand Up @@ -347,6 +347,17 @@ MTR_NEWLY_AVAILABLE
*
* A non-nil attributeID along with a nil clusterID will only succeed if the
* attribute ID is for a global attribute that applies to all clusters.
*
* The reportHandler will be called with an error if the subscription fails
* entirely.
*
* The reportHandler will be called with arrays of response-value dictionaries
* (which may be data or errors) as path-specific data is received.
*
* subscriptionEstablished will be called when the subscription is first
* successfully established (after the initial set of data reports has been
* delivered to reportHandler). If params allow automatic resubscription, it
* will be called any time resubscription succeeds.
*/
- (void)subscribeToAttributesWithEndpointID:(NSNumber * _Nullable)endpointID
clusterID:(NSNumber * _Nullable)clusterID
Expand All @@ -367,6 +378,17 @@ MTR_NEWLY_AVAILABLE
* The reportHandler will be called with an error if the inputs are invalid (e.g., both attributePaths and eventPaths are
* empty), or if the subscription fails entirely.
*
* The reportHandler will be called with arrays of response-value dictionaries
* (which may be data or errors) as path-specific data is received.
*
* subscriptionEstablished will be called when the subscription is first
* successfully established (after the initial set of data reports has been
* delivered to reportHandler). If params allow automatic resubscription, it
* will be called any time resubscription succeeds.
*
* resubscriptionScheduled will be called if subscription drop is detected and
* params allow automatic resubscription.
*
* If the sum of the lengths of attributePaths and eventPaths exceeds 3, the subscribe may fail due to the device not supporting
* that many paths for a subscription.
*/
Expand Down Expand Up @@ -465,6 +487,17 @@ MTR_NEWLY_AVAILABLE
*
* If all of endpointID, clusterID, eventID are nil, all events on the
* device will be subscribed to.
*
* The reportHandler will be called with an error if the subscription fails
* entirely.
*
* The reportHandler will be called with arrays of response-value dictionaries
* (which may be data or errors) as path-specific data is received.
*
* subscriptionEstablished will be called when the subscription is first
* successfully established (after the initial set of data reports has been
* delivered to reportHandler). If params allow automatic resubscription, it
* will be called any time resubscription succeeds.
*/
- (void)subscribeToEventsWithEndpointID:(NSNumber * _Nullable)endpointID
clusterID:(NSNumber * _Nullable)clusterID
Expand Down
41 changes: 24 additions & 17 deletions src/darwin/Framework/CHIP/MTRBaseDevice.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1356,29 +1356,36 @@ - (void)subscribeToAttributePaths:(NSArray<MTRAttributeRequestPath *> * _Nullabl
});
};

auto establishedOrFailed = chip::Platform::MakeShared<BOOL>(NO);
auto onFailureCb = [establishedOrFailed, queue, subscriptionEstablished, reportHandler](
const app::ConcreteAttributePath * attributePath,
auto onFailureCb = [queue, reportHandler](const app::ConcreteAttributePath * attributePath,
const app::ConcreteEventPath * eventPath, CHIP_ERROR error) {
// TODO, Requires additional logic if attributePath or eventPath is not null
if (!(*establishedOrFailed)) {
*establishedOrFailed = YES;
if (subscriptionEstablished) {
dispatch_async(queue, subscriptionEstablished);
}
}
if (reportHandler) {
if (attributePath != nullptr) {
ConcreteAttributePath pathCopy(*attributePath);
dispatch_async(queue, ^{
reportHandler(@[ @ {
MTRAttributePathKey : [[MTRAttributePath alloc] initWithPath:pathCopy],
MTRErrorKey : [MTRError errorForCHIPErrorCode:error]
} ],
nil);
});
} else if (eventPath != nullptr) {
ConcreteEventPath pathCopy(*eventPath);
dispatch_async(queue, ^{
reportHandler(nil, [MTRError errorForCHIPErrorCode:error]);
reportHandler(@[ @ {
MTRAttributePathKey : [[MTREventPath alloc] initWithPath:pathCopy],
MTRErrorKey : [MTRError errorForCHIPErrorCode:error]
} ],
nil);
});
} else {
if (reportHandler) {
dispatch_async(queue, ^{
reportHandler(nil, [MTRError errorForCHIPErrorCode:error]);
});
}
}
};

auto onEstablishedCb = [establishedOrFailed, queue, subscriptionEstablished]() {
if (*establishedOrFailed) {
return;
}
*establishedOrFailed = YES;
auto onEstablishedCb = [queue, subscriptionEstablished]() {
if (subscriptionEstablished) {
dispatch_async(queue, subscriptionEstablished);
}
Expand Down
157 changes: 127 additions & 30 deletions src/darwin/Framework/CHIPTests/MTRDeviceTests.m
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1748,12 +1748,72 @@ - (void)test022_SubscribeMultipleAttributes
XCTestExpectation * expectation = [self expectationWithDescription:@"subscribe OnOff attribute"];
__auto_type * params = [[MTRSubscribeParams alloc] initWithMinInterval:@(1) maxInterval:@(10)];

NSArray<MTRAttributeRequestPath *> * attributePaths =
[NSArray arrayWithObjects:[MTRAttributeRequestPath requestPathWithEndpointID:@1 clusterID:@6 attributeID:@0],
[MTRAttributeRequestPath requestPathWithEndpointID:@0 clusterID:@40 attributeID:@5], nil];
NSNumber * failClusterId = @5678;
NSNumber * failEndpointId = @1000;

NSArray<MTRAttributeRequestPath *> * attributePaths = @[
[MTRAttributeRequestPath requestPathWithEndpointID:@1 clusterID:@6 attributeID:@0],
[MTRAttributeRequestPath requestPathWithEndpointID:@1 clusterID:failClusterId attributeID:@1],
];

NSArray<MTREventRequestPath *> * eventPaths = @[ [MTREventRequestPath requestPathWithEndpointID:failEndpointId
clusterID:@40
eventID:@0] ];

XCTestExpectation * onOffReportExpectation = [self expectationWithDescription:@"report OnOff attribute"];
XCTestExpectation * attributeErrorReportExpectation = [self expectationWithDescription:@"report nonexistent attribute"];
// TODO: Right now the server does not seem to actually produce an error
// when trying to subscribe to events on a non-existent endpoint.
// XCTestExpectation * eventErrorReportExpectation = [self expectationWithDescription:@"report nonexistent event"];
globalReportHandler = ^(id _Nullable values, NSError * _Nullable error) {
XCTAssertNil(error);
XCTAssertEqual([MTRErrorTestUtils errorToZCLErrorCode:error], 0);
XCTAssertTrue([values isKindOfClass:[NSArray class]]);

for (NSDictionary * result in values) {
if (result[@"attributePath"] != nil) {
MTRAttributePath * path = result[@"attributePath"];

if ([path.attribute isEqualToNumber:@0]) {
XCTAssertEqualObjects(path.endpoint, @1);
XCTAssertEqualObjects(path.cluster, @6);
XCTAssertTrue([result[@"data"] isKindOfClass:[NSDictionary class]]);
XCTAssertTrue([result[@"data"][@"type"] isEqualToString:@"Boolean"]);
XCTAssertEqualObjects(result[@"data"][@"value"], @NO);
[onOffReportExpectation fulfill];
} else if ([path.attribute isEqualToNumber:@1]) {
XCTAssertEqualObjects(path.endpoint, @1);
XCTAssertEqualObjects(path.cluster, failClusterId);
XCTAssertNil(result[@"data"]);
XCTAssertNotNil(result[@"error"]);
XCTAssertEqual(
[MTRErrorTestUtils errorToZCLErrorCode:result[@"error"]], MTRInteractionErrorCodeUnsupportedCluster);
[attributeErrorReportExpectation fulfill];
} else {
XCTFail("Unexpected attribute value");
}
} else if (result[@"eventPath"] != nil) {
MTREventPath * path = result[@"eventPath"];
XCTAssertEqualObjects(path.endpoint, @1);
XCTAssertEqualObjects(path.cluster, failClusterId);
XCTAssertEqualObjects(path.event, @0);
XCTAssertNil(result[@"data"]);
XCTAssertNotNil(result[@"error"]);
XCTAssertEqual(
[MTRErrorTestUtils errorToZCLErrorCode:result[@"error"]], MTRInteractionErrorCodeUnsupportedEndpoint);
// TODO: Right now the server does not seem to actually produce an error
// when trying to subscribe to events on a non-existent
// endpoint. Catch it if it starts doing that.
XCTFail("Need to re-enable the eventErrorReportExpectation bits");
// [eventErrorReportExpectation fulfill];
} else {
XCTFail("Unexpected result dictionary");
}
}
};

[device subscribeToAttributePaths:attributePaths
eventPaths:nil
eventPaths:eventPaths
params:params
queue:queue
reportHandler:^(id _Nullable values, NSError * _Nullable error) {
Expand All @@ -1771,31 +1831,30 @@ - (void)test022_SubscribeMultipleAttributes
resubscriptionScheduled:nil];

// Wait till establishment
[self waitForExpectations:[NSArray arrayWithObject:expectation] timeout:kTimeoutInSeconds];
[self waitForExpectations:@[
onOffReportExpectation, attributeErrorReportExpectation, /* eventErrorReportExpectation, */ expectation
]
timeout:kTimeoutInSeconds];

// Set up expectation for report
XCTestExpectation * reportExpectation = [self expectationWithDescription:@"report received"];
globalReportHandler = ^(id _Nullable values, NSError * _Nullable error) {
XCTAssertNil(error);
XCTAssertEqual([MTRErrorTestUtils errorToZCLErrorCode:error], 0);
XCTAssertTrue([values isKindOfClass:[NSArray class]]);
NSDictionary * result = values[0];
MTRAttributePath * path = result[@"attributePath"];
if (path.endpoint.unsignedShortValue == 1) {
XCTAssertEqual([path.cluster unsignedIntegerValue], 6);
XCTAssertEqual([path.attribute unsignedIntegerValue], 0);
XCTAssertTrue([result[@"data"] isKindOfClass:[NSDictionary class]]);
XCTAssertTrue([result[@"data"][@"type"] isEqualToString:@"Boolean"]);
if ([result[@"data"][@"value"] boolValue] == YES) {
[reportExpectation fulfill];
globalReportHandler = nil;
}
} else if (path.endpoint.unsignedShortValue == 0) {
XCTAssertEqual([path.cluster unsignedIntegerValue], 40);
XCTAssertEqual([path.attribute unsignedIntegerValue], 5);
XCTAssertTrue([result[@"data"] isKindOfClass:[NSDictionary class]]);
XCTAssertTrue([result[@"data"][@"type"] isEqualToString:@"UTF8String"]);
} else {
XCTAssertTrue(NO);

// We will only be getting incremental reports for the OnOff attribute.
XCTAssertEqualObjects(path.endpoint, @1);
XCTAssertEqualObjects(path.cluster, @6);
XCTAssertEqualObjects(path.attribute, @0);

XCTAssertTrue([result[@"data"] isKindOfClass:[NSDictionary class]]);
XCTAssertTrue([result[@"data"][@"type"] isEqualToString:@"Boolean"]);
if ([result[@"data"][@"value"] boolValue] == YES) {
[reportExpectation fulfill];
globalReportHandler = nil;
}
};

Expand All @@ -1819,9 +1878,9 @@ - (void)test022_SubscribeMultipleAttributes
NSArray * resultArray = values;
for (NSDictionary * result in resultArray) {
MTRCommandPath * path = result[@"commandPath"];
XCTAssertEqual([path.endpoint unsignedIntegerValue], 1);
XCTAssertEqual([path.cluster unsignedIntegerValue], 6);
XCTAssertEqual([path.command unsignedIntegerValue], 1);
XCTAssertEqualObjects(path.endpoint, @1);
XCTAssertEqualObjects(path.cluster, @6);
XCTAssertEqualObjects(path.command, @1);
XCTAssertNil(result[@"error"]);
}
XCTAssertEqual([resultArray count], 1);
Expand All @@ -1837,13 +1896,14 @@ - (void)test022_SubscribeMultipleAttributes
// Set up expectation for 2nd report
reportExpectation = [self expectationWithDescription:@"receive OnOff attribute report"];
globalReportHandler = ^(id _Nullable values, NSError * _Nullable error) {
XCTAssertNil(error);
XCTAssertEqual([MTRErrorTestUtils errorToZCLErrorCode:error], 0);
XCTAssertTrue([values isKindOfClass:[NSArray class]]);
NSDictionary * result = values[0];
MTRAttributePath * path = result[@"attributePath"];
XCTAssertEqual([path.endpoint unsignedIntegerValue], 1);
XCTAssertEqual([path.cluster unsignedIntegerValue], 6);
XCTAssertEqual([path.attribute unsignedIntegerValue], 0);
XCTAssertEqualObjects(path.endpoint, @1);
XCTAssertEqualObjects(path.cluster, @6);
XCTAssertEqualObjects(path.attribute, @0);
XCTAssertTrue([result[@"data"] isKindOfClass:[NSDictionary class]]);
XCTAssertTrue([result[@"data"][@"type"] isEqualToString:@"Boolean"]);
if ([result[@"data"][@"value"] boolValue] == NO) {
Expand Down Expand Up @@ -1871,9 +1931,9 @@ - (void)test022_SubscribeMultipleAttributes
NSArray * resultArray = values;
for (NSDictionary * result in resultArray) {
MTRCommandPath * path = result[@"commandPath"];
XCTAssertEqual([path.endpoint unsignedIntegerValue], 1);
XCTAssertEqual([path.cluster unsignedIntegerValue], 6);
XCTAssertEqual([path.command unsignedIntegerValue], 0);
XCTAssertEqualObjects(path.endpoint, @1);
XCTAssertEqualObjects(path.cluster, @6);
XCTAssertEqualObjects(path.command, @0);
XCTAssertNil(result[@"error"]);
}
XCTAssertEqual([resultArray count], 1);
Expand All @@ -1891,6 +1951,43 @@ - (void)test022_SubscribeMultipleAttributes
[self waitForExpectations:@[ expectation ] timeout:kTimeoutInSeconds];
}

- (void)test024_SubscribeMultipleAttributesAllErrors
{
MTRBaseDevice * device = GetConnectedDevice();
dispatch_queue_t queue = dispatch_get_main_queue();

// Subscribe
XCTestExpectation * errorExpectation = [self expectationWithDescription:@"subscribe failure"];
__auto_type * params = [[MTRSubscribeParams alloc] initWithMinInterval:@(1) maxInterval:@(10)];
params.resubscribeAutomatically = NO;

NSNumber * failClusterId = @5678;
NSNumber * failEndpointId = @1000;

// All the paths are invalid, so we will just get an INVALID_ACTION error.
NSArray<MTRAttributeRequestPath *> * attributePaths = @[
[MTRAttributeRequestPath requestPathWithEndpointID:failEndpointId clusterID:@6 attributeID:@0],
[MTRAttributeRequestPath requestPathWithEndpointID:@1 clusterID:failClusterId attributeID:@1],
];

[device subscribeToAttributePaths:attributePaths
eventPaths:nil
params:params
queue:queue
reportHandler:^(id _Nullable values, NSError * _Nullable error) {
XCTAssertNotNil(error);
XCTAssertEqual([MTRErrorTestUtils errorToZCLErrorCode:error], MTRInteractionErrorCodeInvalidAction);
XCTAssertNil(values);
[errorExpectation fulfill];
}
subscriptionEstablished:^{
XCTFail("This subscription should never be established");
}
resubscriptionScheduled:nil];

[self waitForExpectations:@[ errorExpectation ] timeout:kTimeoutInSeconds];
}

- (void)test900_SubscribeAllAttributes
{
MTRBaseDevice * device = GetConnectedDevice();
Expand Down
6 changes: 2 additions & 4 deletions src/darwin/Framework/CHIPTests/MTRXPCListenerSampleTests.m
View file
Original file line number Diff line number Diff line change
Expand Up @@ -883,8 +883,6 @@ - (void)test007_InvokeCommandFailure

- (void)test008_SubscribeFailure
{
XCTestExpectation * expectation = [self expectationWithDescription:@"subscribe OnOff attribute"];

// Set up expectation for report
XCTestExpectation * errorReportExpectation = [self expectationWithDescription:@"receive OnOff attribute report"];
globalReportHandler = ^(id _Nullable values, NSError * _Nullable error) {
Expand Down Expand Up @@ -916,11 +914,11 @@ - (void)test008_SubscribeFailure
}
subscriptionEstablished:^{
NSLog(@"subscribe attribute: OnOff established");
[expectation fulfill];
XCTFail("Should not have a subscriptionEstablished for an error case");
}];

// Wait till establishment and error report
[self waitForExpectations:[NSArray arrayWithObjects:expectation, errorReportExpectation, nil] timeout:kTimeoutInSeconds];
[self waitForExpectations:@[ errorReportExpectation ] timeout:kTimeoutInSeconds];
}

- (void)test009_ReadAttributeWithParams
Expand Down

0 comments on commit 01495be

Please sign in to comment.