Skip to content

Commit 4699857

Browse files
bzbarsky-applepull[bot]
authored andcommitted
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 #26076 * Address review comment.
1 parent 4948a5a commit 4699857

File tree

4 files changed

+192
-64
lines changed

4 files changed

+192
-64
lines changed

src/darwin/Framework/CHIP/MTRBaseDevice.h

+34-1
Original file line numberDiff line numberDiff line change
@@ -200,7 +200,7 @@ MTR_NEWLY_AVAILABLE
200200
* eventReportHandler will be called any time an event is reported (with a
201201
* non-nil "value")
202202
*
203-
* The array passed to eventReportHandler will contain CHIPEventReport
203+
* The array passed to eventReportHandler will contain MTREventReport
204204
* instances. Errors for specific paths, not the whole subscription, will be
205205
* reported via those objects.
206206
*
@@ -347,6 +347,17 @@ MTR_NEWLY_AVAILABLE
347347
*
348348
* A non-nil attributeID along with a nil clusterID will only succeed if the
349349
* attribute ID is for a global attribute that applies to all clusters.
350+
*
351+
* The reportHandler will be called with an error if the subscription fails
352+
* entirely.
353+
*
354+
* The reportHandler will be called with arrays of response-value dictionaries
355+
* (which may be data or errors) as path-specific data is received.
356+
*
357+
* subscriptionEstablished will be called when the subscription is first
358+
* successfully established (after the initial set of data reports has been
359+
* delivered to reportHandler). If params allow automatic resubscription, it
360+
* will be called any time resubscription succeeds.
350361
*/
351362
- (void)subscribeToAttributesWithEndpointID:(NSNumber * _Nullable)endpointID
352363
clusterID:(NSNumber * _Nullable)clusterID
@@ -367,6 +378,17 @@ MTR_NEWLY_AVAILABLE
367378
* The reportHandler will be called with an error if the inputs are invalid (e.g., both attributePaths and eventPaths are
368379
* empty), or if the subscription fails entirely.
369380
*
381+
* The reportHandler will be called with arrays of response-value dictionaries
382+
* (which may be data or errors) as path-specific data is received.
383+
*
384+
* subscriptionEstablished will be called when the subscription is first
385+
* successfully established (after the initial set of data reports has been
386+
* delivered to reportHandler). If params allow automatic resubscription, it
387+
* will be called any time resubscription succeeds.
388+
*
389+
* resubscriptionScheduled will be called if subscription drop is detected and
390+
* params allow automatic resubscription.
391+
*
370392
* If the sum of the lengths of attributePaths and eventPaths exceeds 3, the subscribe may fail due to the device not supporting
371393
* that many paths for a subscription.
372394
*/
@@ -465,6 +487,17 @@ MTR_NEWLY_AVAILABLE
465487
*
466488
* If all of endpointID, clusterID, eventID are nil, all events on the
467489
* device will be subscribed to.
490+
*
491+
* The reportHandler will be called with an error if the subscription fails
492+
* entirely.
493+
*
494+
* The reportHandler will be called with arrays of response-value dictionaries
495+
* (which may be data or errors) as path-specific data is received.
496+
*
497+
* subscriptionEstablished will be called when the subscription is first
498+
* successfully established (after the initial set of data reports has been
499+
* delivered to reportHandler). If params allow automatic resubscription, it
500+
* will be called any time resubscription succeeds.
468501
*/
469502
- (void)subscribeToEventsWithEndpointID:(NSNumber * _Nullable)endpointID
470503
clusterID:(NSNumber * _Nullable)clusterID

src/darwin/Framework/CHIP/MTRBaseDevice.mm

+27-26
Original file line numberDiff line numberDiff line change
@@ -1324,11 +1324,9 @@ - (void)subscribeToAttributePaths:(NSArray<MTRAttributeRequestPath *> * _Nullabl
13241324
completion:^(ExchangeManager * _Nullable exchangeManager, const Optional<SessionHandle> & session,
13251325
NSError * _Nullable error) {
13261326
if (error != nil) {
1327-
if (reportHandler) {
1328-
dispatch_async(queue, ^{
1329-
reportHandler(nil, error);
1330-
});
1331-
}
1327+
dispatch_async(queue, ^{
1328+
reportHandler(nil, error);
1329+
});
13321330
return;
13331331
}
13341332

@@ -1356,29 +1354,34 @@ - (void)subscribeToAttributePaths:(NSArray<MTRAttributeRequestPath *> * _Nullabl
13561354
});
13571355
};
13581356

1359-
auto establishedOrFailed = chip::Platform::MakeShared<BOOL>(NO);
1360-
auto onFailureCb = [establishedOrFailed, queue, subscriptionEstablished, reportHandler](
1361-
const app::ConcreteAttributePath * attributePath,
1357+
auto onFailureCb = [queue, reportHandler](const app::ConcreteAttributePath * attributePath,
13621358
const app::ConcreteEventPath * eventPath, CHIP_ERROR error) {
1363-
// TODO, Requires additional logic if attributePath or eventPath is not null
1364-
if (!(*establishedOrFailed)) {
1365-
*establishedOrFailed = YES;
1366-
if (subscriptionEstablished) {
1367-
dispatch_async(queue, subscriptionEstablished);
1368-
}
1369-
}
1370-
if (reportHandler) {
1359+
if (attributePath != nullptr) {
1360+
ConcreteAttributePath pathCopy(*attributePath);
1361+
dispatch_async(queue, ^{
1362+
reportHandler(@[ @ {
1363+
MTRAttributePathKey : [[MTRAttributePath alloc] initWithPath:pathCopy],
1364+
MTRErrorKey : [MTRError errorForCHIPErrorCode:error]
1365+
} ],
1366+
nil);
1367+
});
1368+
} else if (eventPath != nullptr) {
1369+
ConcreteEventPath pathCopy(*eventPath);
1370+
dispatch_async(queue, ^{
1371+
reportHandler(@[ @ {
1372+
MTRAttributePathKey : [[MTREventPath alloc] initWithPath:pathCopy],
1373+
MTRErrorKey : [MTRError errorForCHIPErrorCode:error]
1374+
} ],
1375+
nil);
1376+
});
1377+
} else {
13711378
dispatch_async(queue, ^{
13721379
reportHandler(nil, [MTRError errorForCHIPErrorCode:error]);
13731380
});
13741381
}
13751382
};
13761383

1377-
auto onEstablishedCb = [establishedOrFailed, queue, subscriptionEstablished]() {
1378-
if (*establishedOrFailed) {
1379-
return;
1380-
}
1381-
*establishedOrFailed = YES;
1384+
auto onEstablishedCb = [queue, subscriptionEstablished]() {
13821385
if (subscriptionEstablished) {
13831386
dispatch_async(queue, subscriptionEstablished);
13841387
}
@@ -1456,11 +1459,9 @@ - (void)subscribeToAttributePaths:(NSArray<MTRAttributeRequestPath *> * _Nullabl
14561459
}
14571460

14581461
if (err != CHIP_NO_ERROR) {
1459-
if (reportHandler) {
1460-
dispatch_async(queue, ^{
1461-
reportHandler(nil, [MTRError errorForCHIPErrorCode:err]);
1462-
});
1463-
}
1462+
dispatch_async(queue, ^{
1463+
reportHandler(nil, [MTRError errorForCHIPErrorCode:err]);
1464+
});
14641465
Platform::Delete(readClient);
14651466
if (container.pathParams != nullptr) {
14661467
Platform::MemoryFree(container.pathParams);

0 commit comments

Comments
 (0)