From 25927935817bdbea7319a11ebc34185d15102940 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Thu, 20 Apr 2023 03:55:20 -0400 Subject: [PATCH] Fix handling of path-specific errors in MTRBaseDevice subscribe code. (#26168) * Fix handling of path-specific errors in MTRBaseDevice subscribe code. 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 https://github.com/project-chip/connectedhomeip/issues/26076 * Address review comment. --- src/darwin/Framework/CHIP/MTRBaseDevice.h | 35 +++- src/darwin/Framework/CHIP/MTRBaseDevice.mm | 53 +++--- .../Framework/CHIPTests/MTRDeviceTests.m | 162 ++++++++++++++---- .../CHIPTests/MTRXPCListenerSampleTests.m | 6 +- 4 files changed, 192 insertions(+), 64 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRBaseDevice.h b/src/darwin/Framework/CHIP/MTRBaseDevice.h index fecf8757befdc3..65050131d2fa12 100644 --- a/src/darwin/Framework/CHIP/MTRBaseDevice.h +++ b/src/darwin/Framework/CHIP/MTRBaseDevice.h @@ -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. * @@ -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 @@ -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. */ @@ -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 diff --git a/src/darwin/Framework/CHIP/MTRBaseDevice.mm b/src/darwin/Framework/CHIP/MTRBaseDevice.mm index a8e585d6bf33a9..a966a34dde8cf1 100644 --- a/src/darwin/Framework/CHIP/MTRBaseDevice.mm +++ b/src/darwin/Framework/CHIP/MTRBaseDevice.mm @@ -1324,11 +1324,9 @@ - (void)subscribeToAttributePaths:(NSArray * _Nullabl completion:^(ExchangeManager * _Nullable exchangeManager, const Optional & session, NSError * _Nullable error) { if (error != nil) { - if (reportHandler) { - dispatch_async(queue, ^{ - reportHandler(nil, error); - }); - } + dispatch_async(queue, ^{ + reportHandler(nil, error); + }); return; } @@ -1356,29 +1354,34 @@ - (void)subscribeToAttributePaths:(NSArray * _Nullabl }); }; - auto establishedOrFailed = chip::Platform::MakeShared(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(@[ @ { + MTRAttributePathKey : [[MTREventPath alloc] initWithPath:pathCopy], + MTRErrorKey : [MTRError errorForCHIPErrorCode:error] + } ], + nil); + }); + } else { 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); } @@ -1456,11 +1459,9 @@ - (void)subscribeToAttributePaths:(NSArray * _Nullabl } if (err != CHIP_NO_ERROR) { - if (reportHandler) { - dispatch_async(queue, ^{ - reportHandler(nil, [MTRError errorForCHIPErrorCode:err]); - }); - } + dispatch_async(queue, ^{ + reportHandler(nil, [MTRError errorForCHIPErrorCode:err]); + }); Platform::Delete(readClient); if (container.pathParams != nullptr) { Platform::MemoryFree(container.pathParams); diff --git a/src/darwin/Framework/CHIPTests/MTRDeviceTests.m b/src/darwin/Framework/CHIPTests/MTRDeviceTests.m index cf2757097c1ca4..a71f3878d7515a 100644 --- a/src/darwin/Framework/CHIPTests/MTRDeviceTests.m +++ b/src/darwin/Framework/CHIPTests/MTRDeviceTests.m @@ -672,7 +672,6 @@ - (void)test008_InvokeCommandFailure - (void)test009_SubscribeFailure { - XCTestExpectation * expectation = [self expectationWithDescription:@"subscribe OnOff attribute"]; __block void (^reportHandler)(id _Nullable values, NSError * _Nullable error) = nil; // Set up expectation for report @@ -715,11 +714,11 @@ - (void)test009_SubscribeFailure } subscriptionEstablished:^{ NSLog(@"subscribe attribute: OnOff established"); - [expectation fulfill]; + XCTFail("Should not get subscriptionEstablished in the error case"); }]; // Wait till establishment and error report - [self waitForExpectations:[NSArray arrayWithObjects:expectation, errorReportExpectation, nil] timeout:kTimeoutInSeconds]; + [self waitForExpectations:@[ errorReportExpectation ] timeout:kTimeoutInSeconds]; } - (void)test010_ReadAllAttribute @@ -1840,12 +1839,72 @@ - (void)test023_SubscribeMultipleAttributes XCTestExpectation * expectation = [self expectationWithDescription:@"subscribe OnOff attribute"]; __auto_type * params = [[MTRSubscribeParams alloc] initWithMinInterval:@(1) maxInterval:@(10)]; - NSArray * 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 * attributePaths = @[ + [MTRAttributeRequestPath requestPathWithEndpointID:@1 clusterID:@6 attributeID:@0], + [MTRAttributeRequestPath requestPathWithEndpointID:@1 clusterID:failClusterId attributeID:@1], + ]; + + NSArray * 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) { @@ -1863,31 +1922,30 @@ - (void)test023_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; } }; @@ -1911,9 +1969,9 @@ - (void)test023_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); @@ -1929,13 +1987,14 @@ - (void)test023_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) { @@ -1963,9 +2022,9 @@ - (void)test023_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); @@ -1983,6 +2042,43 @@ - (void)test023_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 * 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(); diff --git a/src/darwin/Framework/CHIPTests/MTRXPCListenerSampleTests.m b/src/darwin/Framework/CHIPTests/MTRXPCListenerSampleTests.m index c44b6baf0f32e4..c22bc7a72125ea 100644 --- a/src/darwin/Framework/CHIPTests/MTRXPCListenerSampleTests.m +++ b/src/darwin/Framework/CHIPTests/MTRXPCListenerSampleTests.m @@ -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) { @@ -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