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
…#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 #26076

* Address review comment.
  • Loading branch information
bzbarsky-apple authored and pull[bot] committed Nov 30, 2023
1 parent d20f378 commit 90c2010
Show file tree
Hide file tree
Showing 4 changed files with 192 additions and 64 deletions.
35 changes: 34 additions & 1 deletion src/darwin/Framework/CHIP/MTRBaseDevice.h
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
53 changes: 27 additions & 26 deletions src/darwin/Framework/CHIP/MTRBaseDevice.mm
Original file line number Diff line number Diff line change
Expand Up @@ -1324,11 +1324,9 @@ - (void)subscribeToAttributePaths:(NSArray<MTRAttributeRequestPath *> * _Nullabl
completion:^(ExchangeManager * _Nullable exchangeManager, const Optional<SessionHandle> & session,
NSError * _Nullable error) {
if (error != nil) {
if (reportHandler) {
dispatch_async(queue, ^{
reportHandler(nil, error);
});
}
dispatch_async(queue, ^{
reportHandler(nil, error);
});
return;
}

Expand Down Expand Up @@ -1356,29 +1354,34 @@ - (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(@[ @ {
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);
}
Expand Down Expand Up @@ -1456,11 +1459,9 @@ - (void)subscribeToAttributePaths:(NSArray<MTRAttributeRequestPath *> * _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);
Expand Down
Loading

0 comments on commit 90c2010

Please sign in to comment.