Make the error handling in readAttributesWithEndpointID more reasonab…

…le. (#25203) * Make the error handling in readAttributesWithEndpointID more reasonable. * Apply suggestions from code review Co-authored-by: Karsten Sperling <113487422+ksperling-apple@users.noreply.github.com> * Address review comments. * Addressing more review comments. --------- Co-authored-by: Karsten Sperling <113487422+ksperling-apple@users.noreply.github.com>
project-chip · Oct 26, 2023 · 4683747 · 4683747
1 parent 2595ff2
commit 4683747
Show file tree

Hide file tree

Showing 9 changed files with 177 additions and 93 deletions.
diff --git a/src/darwin/Framework/CHIP/MTRBaseDevice.h b/src/darwin/Framework/CHIP/MTRBaseDevice.h
@@ -215,6 +215,11 @@ typedef NS_ENUM(uint8_t, MTRTransportType) {
  *
  * 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 completion will be called with an error if the entire read interaction fails.
+ * Otherwise it will be called with values, which may be empty (e.g. if no paths
+ * matched the wildcard) or may include per-path errors if particular paths
+ * failed.
  */
 - (void)readAttributesWithEndpointID:(NSNumber * _Nullable)endpointID
  clusterID:(NSNumber * _Nullable)clusterID
@@ -344,6 +349,11 @@ typedef NS_ENUM(uint8_t, MTRTransportType) {
  *
  * If all of endpointID, clusterID, eventID are nil, all events on the
  * device will be read.
+ *
+ * The completion will be called with an error if the entire read interaction fails.
+ * Otherwise it will be called with values, which may be empty (e.g. if no paths
+ * matched the wildcard) or may include per-path errors if particular paths
+ * failed.
  */
 
 - (void)readEventsWithEndpointID:(NSNumber * _Nullable)endpointID

diff --git a/src/darwin/Framework/CHIP/MTRBaseDevice.mm b/src/darwin/Framework/CHIP/MTRBaseDevice.mm
@@ -847,31 +847,34 @@ - (void)readAttributesWithEndpointID:(NSNumber * _Nullable)endpointID
  auto * bridge = new MTRDataValueDictionaryCallbackBridge(queue, completion,
  ^(ExchangeManager & exchangeManager, const SessionHandle & session, MTRDataValueDictionaryCallback successCb,
  MTRErrorCallback failureCb, MTRCallbackBridgeBase * bridge) {
+ // interactionStatus tracks whether the whole read interaction has failed.
+ //
+ // Make sure interactionStatus survives even if this block scope is destroyed.
+ auto interactionStatus = std::make_shared<CHIP_ERROR>(CHIP_NO_ERROR);
+
  auto resultArray = [[NSMutableArray alloc] init];
- auto resultSuccess = [[NSMutableArray alloc] init];
- auto resultFailure = [[NSMutableArray alloc] init];
- auto onSuccessCb = [resultArray, resultSuccess](const app::ConcreteClusterPath & clusterPath, const uint32_t aValueId,
+ auto onSuccessCb = [resultArray](const app::ConcreteClusterPath & clusterPath, const uint32_t aValueId,
  const MTRDataValueDictionaryDecodableType & aData) {
  app::ConcreteAttributePath attribPath(clusterPath.mEndpointId, clusterPath.mClusterId, aValueId);
  [resultArray addObject:@ {
  MTRAttributePathKey : [[MTRAttributePath alloc] initWithPath:attribPath],
  MTRDataKey : aData.GetDecodedObject()
  }];
- if ([resultSuccess count] == 0) {
- [resultSuccess addObject:[NSNumber numberWithBool:YES]];
- }
  };
 
- auto onFailureCb = [resultArray, resultFailure](
+ auto onFailureCb = [resultArray, interactionStatus](
  const app::ConcreteClusterPath * clusterPath, const uint32_t aValueId, CHIP_ERROR aError) {
  if (clusterPath) {
  app::ConcreteAttributePath attribPath(clusterPath->mEndpointId, clusterPath->mClusterId, aValueId);
  [resultArray addObject:@ {
  MTRAttributePathKey : [[MTRAttributePath alloc] initWithPath:attribPath],
  MTRErrorKey : [MTRError errorForCHIPErrorCode:aError]
  }];
- } else if ([resultFailure count] == 0) {
- [resultFailure addObject:[MTRError errorForCHIPErrorCode:aError]];
+ } else {
+ // This will only happen once per read interaction, and
+ // after that there will be no more calls to onFailureCb or
+ // onSuccessCb.
+ *interactionStatus = aError;
  }
  };
 
@@ -893,24 +896,14 @@ - (void)readAttributesWithEndpointID:(NSNumber * _Nullable)endpointID
  readParams.mpAttributePathParamsList = &attributePath;
  readParams.mAttributePathParamsListSize = 1;
 
- auto onDone = [resultArray, resultSuccess, resultFailure, bridge, successCb, failureCb](
+ auto onDone = [resultArray, interactionStatus, bridge, successCb, failureCb](
  BufferedReadClientCallback<MTRDataValueDictionaryDecodableType> * callback) {
- if ([resultFailure count] > 0 || [resultSuccess count] == 0) {
+ if (*interactionStatus != CHIP_NO_ERROR) {
  // Failure
- if (failureCb) {
- if ([resultFailure count] > 0) {
- failureCb(bridge, [MTRError errorToCHIPErrorCode:resultFailure[0]]);
- } else if ([resultArray count] > 0) {
- failureCb(bridge, [MTRError errorToCHIPErrorCode:resultArray[0][MTRErrorKey]]);
- } else {
- failureCb(bridge, CHIP_ERROR_READ_FAILED);
- }
- }
+ failureCb(bridge, *interactionStatus);
  } else {
  // Success
- if (successCb) {
- successCb(bridge, resultArray);
- }
+ successCb(bridge, resultArray);
  }
  chip::Platform::Delete(callback);
  };
@@ -1483,31 +1476,34 @@ - (void)readEventsWithEndpointID:(NSNumber * _Nullable)endpointID
  auto * bridge = new MTRDataValueDictionaryCallbackBridge(queue, completion,
  ^(ExchangeManager & exchangeManager, const SessionHandle & session, MTRDataValueDictionaryCallback successCb,
  MTRErrorCallback failureCb, MTRCallbackBridgeBase * bridge) {
+ // interactionStatus tracks whether the whole read interaction has failed.
+ //
+ // Make sure interactionStatus survives even if this block scope is destroyed.
+ auto interactionStatus = std::make_shared<CHIP_ERROR>(CHIP_NO_ERROR);
+
  auto resultArray = [[NSMutableArray alloc] init];
- auto resultSuccess = [[NSMutableArray alloc] init];
- auto resultFailure = [[NSMutableArray alloc] init];
- auto onSuccessCb = [resultArray, resultSuccess](const app::ConcreteClusterPath & clusterPath, const uint32_t aValueId,
+ auto onSuccessCb = [resultArray](const app::ConcreteClusterPath & clusterPath, const uint32_t aValueId,
  const MTRDataValueDictionaryDecodableType & aData) {
  app::ConcreteEventPath eventPath(clusterPath.mEndpointId, clusterPath.mClusterId, aValueId);
  [resultArray addObject:@ {
  MTREventPathKey : [[MTREventPath alloc] initWithPath:eventPath],
  MTRDataKey : aData.GetDecodedObject()
  }];
- if ([resultSuccess count] == 0) {
- [resultSuccess addObject:[NSNumber numberWithBool:YES]];
- }
  };
 
- auto onFailureCb = [resultArray, resultFailure](
+ auto onFailureCb = [resultArray, interactionStatus](
  const app::ConcreteClusterPath * clusterPath, const uint32_t aValueId, CHIP_ERROR aError) {
  if (clusterPath) {
  app::ConcreteEventPath eventPath(clusterPath->mEndpointId, clusterPath->mClusterId, aValueId);
  [resultArray addObject:@ {
  MTREventPathKey : [[MTREventPath alloc] initWithPath:eventPath],
  MTRErrorKey : [MTRError errorForCHIPErrorCode:aError]
  }];
- } else if ([resultFailure count] == 0) {
- [resultFailure addObject:[MTRError errorForCHIPErrorCode:aError]];
+ } else {
+ // This will only happen once per read interaction, and
+ // after that there will be no more calls to onFailureCb or
+ // onSuccessCb.
+ *interactionStatus = aError;
  }
  };
 
@@ -1529,24 +1525,13 @@ - (void)readEventsWithEndpointID:(NSNumber * _Nullable)endpointID
  readParams.mpEventPathParamsList = &eventPath;
  readParams.mEventPathParamsListSize = 1;
 
- auto onDone = [resultArray, resultSuccess, resultFailure, bridge, successCb, failureCb](
+ auto onDone = [resultArray, interactionStatus, bridge, successCb, failureCb](
  BufferedReadClientCallback<MTRDataValueDictionaryDecodableType> * callback) {
- if ([resultFailure count] > 0 || [resultSuccess count] == 0) {
+ if (*interactionStatus != CHIP_NO_ERROR) {
  // Failure
- if (failureCb) {
- if ([resultFailure count] > 0) {
- failureCb(bridge, [MTRError errorToCHIPErrorCode:resultFailure[0]]);
- } else if ([resultArray count] > 0) {
- failureCb(bridge, [MTRError errorToCHIPErrorCode:resultArray[0][MTRErrorKey]]);
- } else {
- failureCb(bridge, CHIP_ERROR_READ_FAILED);
- }
- }
+ failureCb(bridge, *interactionStatus);
  } else {
- // Success
- if (successCb) {
- successCb(bridge, resultArray);
- }
+ successCb(bridge, resultArray);
  }
  chip::Platform::Delete(callback);
  };

diff --git a/src/darwin/Framework/CHIP/MTRDeviceController+XPC.h b/src/darwin/Framework/CHIP/MTRDeviceController+XPC.h
@@ -76,6 +76,15 @@ typedef void (^MTRValuesHandler)(id _Nullable values, NSError * _Nullable error)
  */
 + (MTRSubscribeParams * _Nullable)decodeXPCSubscribeParams:(NSDictionary<NSString *, id> * _Nullable)params;
 
+/**
+ * Returns an NSXPCInterface configured for MTRDeviceControllerServerProtocol.
+ */
++ (NSXPCInterface *)xpcInterfaceForServerProtocol MTR_NEWLY_AVAILABLE;
+
+/**
+ * Returns an NSXPCInterface configured for MTRDeviceControllerClientProtocol.
+ */
++ (NSXPCInterface *)xpcInterfaceForClientProtocol MTR_NEWLY_AVAILABLE;
 @end
 
 /**

diff --git a/src/darwin/Framework/CHIP/MTRDeviceController+XPC.mm b/src/darwin/Framework/CHIP/MTRDeviceController+XPC.mm
@@ -29,6 +29,16 @@
 static NSString * const kMinIntervalKey = @"minInterval";
 static NSString * const kMaxIntervalKey = @"maxInterval";
 
+// Classes that are used by MTRDevice responses. In particular, needs to
+// include NSError.
+static NSSet * GetXPCAllowedClasses()
+{
+ static NSSet * const sXPCAllowedClasses = [NSSet setWithArray:@[
+ [NSString class], [NSNumber class], [NSData class], [NSArray class], [NSDictionary class], [NSError class]
+ ]];
+ return sXPCAllowedClasses;
+}
+
 static NSArray * _Nullable encodeAttributePath(MTRAttributePath * _Nullable path)
 {
  if (!path) {
@@ -190,6 +200,41 @@ + (MTRSubscribeParams * _Nullable)decodeXPCSubscribeParams:(NSDictionary<NSStrin
  return result;
 }
 
++ (NSXPCInterface *)xpcInterfaceForServerProtocol
+{
+ auto * xpcInterface = [NSXPCInterface interfaceWithProtocol:@protocol(MTRDeviceControllerServerProtocol)];
+ [xpcInterface setClasses:GetXPCAllowedClasses()
+ forSelector:@selector(readAttributeWithController:nodeId:endpointId:clusterId:attributeId:params:completion:)
+ argumentIndex:0
+ ofReply:YES];
+ [xpcInterface setClasses:GetXPCAllowedClasses()
+ forSelector:@selector
+ (writeAttributeWithController:nodeId:endpointId:clusterId:attributeId:value:timedWriteTimeout:completion:)
+ argumentIndex:0
+ ofReply:YES];
+ [xpcInterface setClasses:GetXPCAllowedClasses()
+ forSelector:@selector
+ (invokeCommandWithController:nodeId:endpointId:clusterId:commandId:fields:timedInvokeTimeout:completion:)
+ argumentIndex:0
+ ofReply:YES];
+
+ [xpcInterface setClasses:GetXPCAllowedClasses()
+ forSelector:@selector(readAttributeCacheWithController:nodeId:endpointId:clusterId:attributeId:completion:)
+ argumentIndex:0
+ ofReply:YES];
+ return xpcInterface;
+}
+
++ (NSXPCInterface *)xpcInterfaceForClientProtocol
+{
+ auto * xpcInterface = [NSXPCInterface interfaceWithProtocol:@protocol(MTRDeviceControllerClientProtocol)];
+ [xpcInterface setClasses:GetXPCAllowedClasses()
+ forSelector:@selector(handleReportWithController:nodeId:values:error:)
+ argumentIndex:2
+ ofReply:NO];
+ return xpcInterface;
+}
+
 @end
 
 @implementation MTRDeviceController (Deprecated_XPC)

diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerXPCConnection.mm b/src/darwin/Framework/CHIP/MTRDeviceControllerXPCConnection.mm
@@ -68,8 +68,8 @@ @implementation MTRDeviceControllerXPCConnection
 - (instancetype)initWithWorkQueue:(dispatch_queue_t)workQueue connectBlock:(NSXPCConnection * (^)(void) )connectBlock
 {
  if ([super init]) {
- _remoteDeviceServerProtocol = [NSXPCInterface interfaceWithProtocol:@protocol(MTRDeviceControllerServerProtocol)];
- _remoteDeviceClientProtocol = [NSXPCInterface interfaceWithProtocol:@protocol(MTRDeviceControllerClientProtocol)];
+ _remoteDeviceServerProtocol = [MTRDeviceController xpcInterfaceForServerProtocol];
+ _remoteDeviceClientProtocol = [MTRDeviceController xpcInterfaceForClientProtocol];
  _connectBlock = connectBlock;
  _workQueue = workQueue;
  _reportRegistry = [[NSMutableDictionary alloc] init];