Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make the error handling in readAttributesWithEndpointID more reasonable. #25203

Merged
merged 4 commits into from
Feb 24, 2023
Merged

Make the error handling in readAttributesWithEndpointID more reasonable. #25203

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c8f90ce
Make the error handling in readAttributesWithEndpointID more reasonable.
bzbarsky-apple Feb 20, 2023
f8693e7
Apply suggestions from code review
bzbarsky-apple Feb 21, 2023
b2c1254
Address review comments.
bzbarsky-apple Feb 22, 2023
bdb49dd
Addressing more review comments.
bzbarsky-apple Feb 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions src/darwin/Framework/CHIP/MTRBaseDevice.h
View file
Open in desktop
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.
*
* completion will be called with an error if the entire read interaction fails.
bzbarsky-apple marked this conversation as resolved.
Show resolved Hide resolved
* 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 @@ -336,6 +341,11 @@ typedef NS_ENUM(uint8_t, MTRTransportType) {
*
* If all of endpointID, clusterID, eventID are nil, all events on the
* device will be read.
*
* completion will be called with an error if the entire read interaction fails.
bzbarsky-apple marked this conversation as resolved.
Show resolved Hide resolved
* 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
85 changes: 38 additions & 47 deletions src/darwin/Framework/CHIP/MTRBaseDevice.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -847,31 +847,37 @@ - (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 = Platform::MakeShared<CHIP_ERROR>(CHIP_NO_ERROR);
if (interactionStatus == nullptr) {
bzbarsky-apple marked this conversation as resolved.
Show resolved Hide resolved
return CHIP_ERROR_NO_MEMORY;
}

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 +899,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 @@ -1514,31 +1510,37 @@ - (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 = Platform::MakeShared<CHIP_ERROR>(CHIP_NO_ERROR);
if (interactionStatus == nullptr) {
return CHIP_ERROR_NO_MEMORY;
}

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 @@ -1560,24 +1562,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
36 changes: 36 additions & 0 deletions src/darwin/Framework/CHIP/MTRDeviceControllerXPCConnection.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,9 +67,45 @@ @implementation MTRDeviceControllerXPCConnection

- (instancetype)initWithWorkQueue:(dispatch_queue_t)workQueue connectBlock:(NSXPCConnection * (^)(void) )connectBlock
{
// Classes that are used by MTRDevice responses. In particular, needs to
// include NSError.
//
// TODO: should this include NSOrderedSet, NSSet, NSDate, NSNull, even
// though we don't use them? Should we have public API for some sort for
// this set?
__auto_type * allowedClasses = [NSSet setWithArray:@[
bzbarsky-apple marked this conversation as resolved.
Show resolved Hide resolved
[NSString class], [NSNumber class], [NSData class], [NSArray class], [NSDictionary class], [NSError class]
]];
if ([super init]) {
_remoteDeviceServerProtocol = [NSXPCInterface interfaceWithProtocol:@protocol(MTRDeviceControllerServerProtocol)];
_remoteDeviceClientProtocol = [NSXPCInterface interfaceWithProtocol:@protocol(MTRDeviceControllerClientProtocol)];
[_remoteDeviceServerProtocol
setClasses:allowedClasses
forSelector:@selector(readAttributeWithController:nodeId:endpointId:clusterId:attributeId:params:completion:)
argumentIndex:0
ofReply:YES];
[_remoteDeviceServerProtocol
setClasses:allowedClasses
forSelector:@selector
(writeAttributeWithController:nodeId:endpointId:clusterId:attributeId:value:timedWriteTimeout:completion:)
argumentIndex:0
ofReply:YES];
[_remoteDeviceServerProtocol
setClasses:allowedClasses
forSelector:@selector
(invokeCommandWithController:nodeId:endpointId:clusterId:commandId:fields:timedInvokeTimeout:completion:)
argumentIndex:0
ofReply:YES];

[_remoteDeviceServerProtocol
setClasses:allowedClasses
forSelector:@selector(readAttributeCacheWithController:nodeId:endpointId:clusterId:attributeId:completion:)
argumentIndex:0
ofReply:YES];
[_remoteDeviceClientProtocol setClasses:allowedClasses
forSelector:@selector(handleReportWithController:nodeId:values:error:)
argumentIndex:2
ofReply:NO];
_connectBlock = connectBlock;
_workQueue = workQueue;
_reportRegistry = [[NSMutableDictionary alloc] init];
Expand Down
51 changes: 39 additions & 12 deletions src/darwin/Framework/CHIPTests/MTRDeviceTests.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,7 @@ - (void)test001_ReadAttribute
completion:^(id _Nullable values, NSError * _Nullable error) {
NSLog(@"read attribute: DeviceType values: %@, error: %@", values, error);

XCTAssertNil(error);
XCTAssertEqual([MTRErrorTestUtils errorToZCLErrorCode:error], 0);

{
Expand All @@ -272,6 +273,8 @@ - (void)test001_ReadAttribute
MTRAttributePath * path = result[@"attributePath"];
XCTAssertEqual([path.cluster unsignedIntegerValue], 29);
XCTAssertEqual([path.attribute unsignedIntegerValue], 0);
XCTAssertNotNil(result[@"data"]);
XCTAssertNil(result[@"error"]);
XCTAssertTrue([result[@"data"] isKindOfClass:[NSDictionary class]]);
XCTAssertTrue([result[@"data"][@"type"] isEqualToString:@"Array"]);
}
Expand Down Expand Up @@ -573,20 +576,36 @@ - (void)test006_ReadAttributeFailure
MTRBaseDevice * device = GetConnectedDevice();
dispatch_queue_t queue = dispatch_get_main_queue();

[device
readAttributesWithEndpointID:@0
clusterID:@10000
attributeID:@0
params:nil
queue:queue
completion:^(id _Nullable values, NSError * _Nullable error) {
NSLog(@"read attribute: DeviceType values: %@, error: %@", values, error);
[device readAttributesWithEndpointID:@0
clusterID:@10000
attributeID:@0
params:nil
queue:queue
completion:^(id _Nullable values, NSError * _Nullable error) {
NSLog(@"read attribute: DeviceType values: %@, error: %@", values, error);

XCTAssertNil(values);
XCTAssertEqual([MTRErrorTestUtils errorToZCLErrorCode:error], EMBER_ZCL_STATUS_UNSUPPORTED_CLUSTER);
XCTAssertNil(error);
XCTAssertNotNil(values);

[expectation fulfill];
}];
{
XCTAssertTrue([values isKindOfClass:[NSArray class]]);
NSArray * resultArray = values;
XCTAssertEqual([resultArray count], 1);
NSDictionary * result = resultArray[0];

MTRAttributePath * path = result[@"attributePath"];
XCTAssertEqual(path.endpoint.unsignedIntegerValue, 0);
XCTAssertEqual(path.cluster.unsignedIntegerValue, 10000);
XCTAssertEqual(path.attribute.unsignedIntegerValue, 0);
XCTAssertNotNil(result[@"error"]);
XCTAssertNil(result[@"data"]);
XCTAssertTrue([result[@"error"] isKindOfClass:[NSError class]]);
XCTAssertEqual([MTRErrorTestUtils errorToZCLErrorCode:result[@"error"]],
EMBER_ZCL_STATUS_UNSUPPORTED_CLUSTER);
}

[expectation fulfill];
}];

[self waitForExpectations:[NSArray arrayWithObject:expectation] timeout:kTimeoutInSeconds];
}
Expand Down Expand Up @@ -736,6 +755,7 @@ - (void)test010_ReadAllAttribute
completion:^(id _Nullable values, NSError * _Nullable error) {
NSLog(@"read attribute: DeviceType values: %@, error: %@", values, error);

XCTAssertNil(error);
XCTAssertEqual([MTRErrorTestUtils errorToZCLErrorCode:error], 0);

{
Expand All @@ -745,6 +765,8 @@ - (void)test010_ReadAllAttribute
MTRAttributePath * path = result[@"attributePath"];
XCTAssertEqual([path.cluster unsignedIntegerValue], 29);
XCTAssertEqual([path.endpoint unsignedIntegerValue], 1);
XCTAssertNotNil(result[@"data"]);
XCTAssertNil(result[@"error"]);
XCTAssertTrue([result[@"data"] isKindOfClass:[NSDictionary class]]);
}
XCTAssertTrue([resultArray count] > 0);
Expand Down Expand Up @@ -930,6 +952,7 @@ - (void)test011_ReadCachedAttribute
XCTAssertEqual([path.endpoint unsignedShortValue], 1);
XCTAssertEqual([path.cluster unsignedLongValue], 6);
XCTAssertEqual([path.attribute unsignedLongValue], 0);
XCTAssertNotNil(values[0][@"data"]);
XCTAssertNil(values[0][@"error"]);
XCTAssertTrue([values[0][@"data"][@"type"] isEqualToString:@"Boolean"]);
XCTAssertEqual([values[0][@"data"][@"value"] boolValue], NO);
Expand All @@ -954,6 +977,7 @@ - (void)test011_ReadCachedAttribute
XCTAssertEqual([path.cluster unsignedLongValue], 6);
XCTAssertEqual([path.attribute unsignedLongValue], 0);
XCTAssertNil(value[@"error"]);
XCTAssertNotNil(value[@"data"]);
}
[cacheExpectation fulfill];
}];
Expand All @@ -975,6 +999,8 @@ - (void)test011_ReadCachedAttribute
MTRAttributePath * path = value[@"attributePath"];
XCTAssertEqual([path.endpoint unsignedShortValue], 1);
XCTAssertEqual([path.attribute unsignedLongValue], 0);
XCTAssertNil(value[@"error"]);
XCTAssertNotNil(value[@"data"]);
}
[cacheExpectation fulfill];
}];
Expand All @@ -997,6 +1023,7 @@ - (void)test011_ReadCachedAttribute
XCTAssertEqual([path.endpoint unsignedShortValue], 1);
XCTAssertEqual([path.cluster unsignedLongValue], 6);
XCTAssertNil(value[@"error"]);
XCTAssertNotNil(value[@"data"]);
}
[cacheExpectation fulfill];
}];
Expand Down
2 changes: 1 addition & 1 deletion src/darwin/Framework/CHIPTests/MTRErrorTestUtils.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ + (uint8_t)errorToZCLErrorCode:(NSError * _Nullable)error
return EMBER_ZCL_STATUS_SUCCESS;
}

if (error.domain != MTRInteractionErrorDomain) {
if (![error.domain isEqualToString:MTRInteractionErrorDomain]) {
return EMBER_ZCL_STATUS_FAILURE;
}

Expand Down
Loading