Skip to content

Commit

Permalink
Make the error handling in readAttributesWithEndpointID more reasonab…
Browse files Browse the repository at this point in the history 
…le. (#25203)

* Make the error handling in readAttributesWithEndpointID more reasonable.

* Apply suggestions from code review

Co-authored-by: Karsten Sperling <[email protected]>

* Address review comments.

* Addressing more review comments.

---------

Co-authored-by: Karsten Sperling <[email protected]>
  • Loading branch information
@bzbarsky-apple @ksperling-apple @pull
2 people authored and pull[bot] committed Jan 8, 2024
1 parent 7fc42da commit 1100160
Show file tree
Hide file tree
Showing 9 changed files with 177 additions and 93 deletions.
10 changes: 10 additions & 0 deletions src/darwin/Framework/CHIP/MTRBaseDevice.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
79 changes: 32 additions & 47 deletions src/darwin/Framework/CHIP/MTRBaseDevice.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
};

Expand All @@ -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);
};
Expand Down Expand Up @@ -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;
}
};

Expand All @@ -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);
};
Expand Down
9 changes: 9 additions & 0 deletions src/darwin/Framework/CHIP/MTRDeviceController+XPC.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

/**
Expand Down
45 changes: 45 additions & 0 deletions src/darwin/Framework/CHIP/MTRDeviceController+XPC.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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)
Expand Down
4 changes: 2 additions & 2 deletions src/darwin/Framework/CHIP/MTRDeviceControllerXPCConnection.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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];
Expand Down
Loading

0 comments on commit 1100160

Please sign in to comment.