From bc30268c35e49178721ab3839805c6a156c16255 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Wed, 19 Apr 2023 20:20:04 -0400 Subject: [PATCH] Minor cleanup around read/subscribe(To)AttributePaths. 1) Use initWithArray:copyItems:YES when copying arrays, instead of hand-rolling the copy. 2) For read, empty paths should just lead to an empty result, not an error, just like they would if the read actually happened. Fixes https://github.com/project-chip/connectedhomeip/issues/26169 --- src/darwin/Framework/CHIP/MTRBaseDevice.h | 11 +++--- src/darwin/Framework/CHIP/MTRBaseDevice.mm | 39 +++++++++------------- 2 files changed, 21 insertions(+), 29 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRBaseDevice.h b/src/darwin/Framework/CHIP/MTRBaseDevice.h index e9caa66561f484..0d4fbdd1f498f2 100644 --- a/src/darwin/Framework/CHIP/MTRBaseDevice.h +++ b/src/darwin/Framework/CHIP/MTRBaseDevice.h @@ -287,9 +287,10 @@ MTR_NEWLY_AVAILABLE * * Lists of attribute and event paths to read can be provided via attributePaths and eventPaths. * - * The completion will be called with an error if the input parameters are invalid (e.g., both attributePaths and eventPaths are - * empty.) or the entire read interaction fails. Otherwise it will be called with values, which may be empty (e.g. if no paths - * matched the wildcard paths passed in) or may include per-path errors if particular paths failed. + * The completion will be called with an error if the entire read interaction fails. Otherwise it + * will be called with an array of values. This array may be empty (e.g. if no paths matched the + * wildcard paths passed in, or if empty lists of paths were passed in) or may include per-path + * errors if particular paths failed. * * If the sum of the lengths of attributePaths and eventPaths exceeds 9, the read may fail due to the device not supporting that * many read paths. @@ -394,8 +395,8 @@ MTR_NEWLY_AVAILABLE * * Lists of attribute and event paths to subscribe to can be provided via attributePaths and eventPaths. * - * 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 an error if the subscription fails + * entirely (including when both attributePaths and eventPaths are empty). * * The reportHandler will be called with arrays of response-value dictionaries * (which may be data or errors) as path-specific data is received. diff --git a/src/darwin/Framework/CHIP/MTRBaseDevice.mm b/src/darwin/Framework/CHIP/MTRBaseDevice.mm index 4522c63d7989c1..f3251dbbc4db8f 100644 --- a/src/darwin/Framework/CHIP/MTRBaseDevice.mm +++ b/src/darwin/Framework/CHIP/MTRBaseDevice.mm @@ -917,26 +917,21 @@ - (void)readAttributePaths:(NSArray * _Nullable)attri completion:(MTRDeviceResponseHandler)completion { if ((attributePaths == nil || [attributePaths count] == 0) && (eventPaths == nil || [eventPaths count] == 0)) { + // No paths, just return an empty array. dispatch_async(queue, ^{ - completion(nil, [MTRError errorForCHIPErrorCode:CHIP_ERROR_INVALID_ARGUMENT]); + completion(@[], nil); }); return; } - NSMutableArray * attributes = nil; + NSArray * attributes = nil; if (attributePaths != nil) { - attributes = [[NSMutableArray alloc] init]; - for (MTRAttributeRequestPath * attributePath in attributePaths) { - [attributes addObject:[attributePath copy]]; - } + attributes = [[NSArray alloc] initWithArray:attributePaths copyItems:YES]; } - NSMutableArray * events = nil; + NSArray * events = nil; if (eventPaths != nil) { - events = [[NSMutableArray alloc] init]; - for (MTRAttributeRequestPath * eventPath in eventPaths) { - [events addObject:[eventPath copy]]; - } + events = [[NSArray alloc] initWithArray:eventPaths copyItems:YES]; } params = (params == nil) ? nil : [params copy]; auto * bridge = new MTRDataValueDictionaryCallbackBridge(queue, completion, @@ -1006,9 +1001,9 @@ - (void)readAttributePaths:(NSArray * _Nullable)attri chip::app::ReadPrepareParams readParams(session); [params toReadPrepareParams:readParams]; readParams.mpAttributePathParamsList = attributePathParamsList.Get(); - readParams.mAttributePathParamsListSize = [attributePaths count]; + readParams.mAttributePathParamsListSize = [attributes count]; readParams.mpEventPathParamsList = eventPathParamsList.Get(); - readParams.mEventPathParamsListSize = [eventPaths count]; + readParams.mEventPathParamsListSize = [events count]; AttributePathParams * attributePathParamsListToFree = attributePathParamsList.Get(); EventPathParams * eventPathParamsListToFree = eventPathParamsList.Get(); @@ -1288,8 +1283,10 @@ - (void)subscribeToAttributePaths:(NSArray * _Nullabl resubscriptionScheduled:(MTRDeviceResubscriptionScheduledHandler _Nullable)resubscriptionScheduled { if ((attributePaths == nil || [attributePaths count] == 0) && (eventPaths == nil || [eventPaths count] == 0)) { + // Per spec a server would respond InvalidAction to this, so just go + // ahead and do that. dispatch_async(queue, ^{ - reportHandler(nil, [MTRError errorForCHIPErrorCode:CHIP_ERROR_INVALID_ARGUMENT]); + reportHandler(nil, [MTRError errorForIMStatus:StatusIB(Status::InvalidAction)]); }); return; } @@ -1303,20 +1300,14 @@ - (void)subscribeToAttributePaths:(NSArray * _Nullabl } // Copy params before going async. - NSMutableArray * attributes = nil; + NSArray * attributes = nil; if (attributePaths != nil) { - attributes = [[NSMutableArray alloc] init]; - for (MTRAttributeRequestPath * attributePath in attributePaths) { - [attributes addObject:[attributePath copy]]; - } + attributes = [[NSArray alloc] initWithArray:attributePaths copyItems:YES]; } - NSMutableArray * events = nil; + NSArray * events = nil; if (eventPaths != nil) { - events = [[NSMutableArray alloc] init]; - for (MTRAttributeRequestPath * eventPath in eventPaths) { - [events addObject:[eventPath copy]]; - } + events = [[NSArray alloc] initWithArray:eventPaths copyItems:YES]; } params = (params == nil) ? nil : [params copy];