Skip to content

Commit

Permalink
Simplify the logic in invokeCommandWithEndpointID in MTRBaseDevice. (#…
Browse files Browse the repository at this point in the history 
…25199)

* Fix MTRInvokeCallback to guarantee callback delivery, similar to
  #25197
* Fix NSObjectCommandCallback to guarantee callback delivery and ensure that it
  only calls a single callback.
* Simplify the logic in invokeCommandWithEndpointID by relying on the
  now-enforced invariants around callbacks.
* Document how errors are delivered for invokeCommandWithEndpointID.
  • Loading branch information
@bzbarsky-apple
bzbarsky-apple authored Feb 22, 2023
1 parent ec2b5e3 commit c6cadc8
Show file tree
Hide file tree
Showing 5 changed files with 64 additions and 35 deletions.
14 changes: 13 additions & 1 deletion src/darwin/Framework/CHIP/MTRBaseClusterUtils.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -368,7 +368,19 @@ template <typename InvokeBridgeType, typename ResponseType> class MTRInvokeCallb
mOnError(mBridge, error);
}

void OnDone(chip::app::CommandSender * commandSender) override { chip::Platform::Delete(this); }
void OnDone(chip::app::CommandSender * commandSender) override
{
if (!mCalledCallback) {
// This can happen if the server sends a response with an empty
// InvokeResponses list. Since we are not sending wildcard command
// paths, that's not a valid response and we should treat it as an
// error. Use the error we would have gotten if we in fact expected
// a nonempty list.
OnError(commandSender, CHIP_END_OF_TLV);
}

chip::Platform::Delete(this);
}

InvokeBridgeType * _Nonnull mBridge;

Expand Down
5 changes: 4 additions & 1 deletion src/darwin/Framework/CHIP/MTRBaseDevice.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -261,7 +261,10 @@ typedef NS_ENUM(uint8_t, MTRTransportType) {
*
* @param timeoutMs timeout in milliseconds for timed invoke, or nil.
*
* @param completion response handler will receive either values or error.
* @param completion response handler will receive either values or error. A
* path-specific error status from the command invocation
* will result in an error being passed to the completion, so
* values will only be passed in when the command succeeds.
*/
- (void)invokeCommandWithEndpointID:(NSNumber *)endpointID
clusterID:(NSNumber *)clusterID
Expand Down
68 changes: 36 additions & 32 deletions src/darwin/Framework/CHIP/MTRBaseDevice.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1001,21 +1001,47 @@ - (void)writeAttributeWithEndpointID:(NSNumber *)endpointID
void OnResponse(app::CommandSender * apCommandSender, const app::ConcreteCommandPath & aCommandPath,
const app::StatusIB & aStatus, TLV::TLVReader * aReader) override;

void OnError(const app::CommandSender * apCommandSender, CHIP_ERROR aError) override { mOnError(aError); }
void OnError(const app::CommandSender * apCommandSender, CHIP_ERROR aError) override
{
if (mCalledCallback) {
return;
}
mCalledCallback = true;

mOnError(aError);
}

void OnDone(app::CommandSender * apCommandSender) override { mOnDone(apCommandSender); }
void OnDone(app::CommandSender * apCommandSender) override
{
if (!mCalledCallback) {
// This can happen if the server sends a response with an empty
// InvokeResponses list. Since we are not sending wildcard command
// paths, that's not a valid response and we should treat it as an
// error. Use the error we would have gotten if we in fact expected
// a nonempty list.
OnError(apCommandSender, CHIP_END_OF_TLV);
}

mOnDone(apCommandSender);
}

OnSuccessCallbackType mOnSuccess;
OnErrorCallbackType mOnError;
OnDoneCallbackType mOnDone;
chip::ClusterId mClusterId;
// Id of the command we send.
chip::CommandId mCommandId;
bool mCalledCallback = false;
};

void NSObjectCommandCallback::OnResponse(app::CommandSender * apCommandSender, const app::ConcreteCommandPath & aCommandPath,
const app::StatusIB & aStatus, TLV::TLVReader * aReader)
{
if (mCalledCallback) {
return;
}
mCalledCallback = true;

MTRDataValueDictionaryDecodableType response;
CHIP_ERROR err = CHIP_NO_ERROR;

Expand Down Expand Up @@ -1061,11 +1087,11 @@ - (void)invokeCommandWithEndpointID:(NSNumber *)endpointID
auto * bridge = new MTRDataValueDictionaryCallbackBridge(queue, completion,
^(ExchangeManager & exchangeManager, const SessionHandle & session, MTRDataValueDictionaryCallback successCb,
MTRErrorCallback failureCb, MTRCallbackBridgeBase * bridge) {
auto resultArray = [[NSMutableArray alloc] init];
auto resultSuccess = [[NSMutableArray alloc] init];
auto resultFailure = [[NSMutableArray alloc] init];
auto onSuccessCb = [resultArray, resultSuccess](const app::ConcreteCommandPath & commandPath,
const app::StatusIB & status, const MTRDataValueDictionaryDecodableType & responseData) {
// NSObjectCommandCallback guarantees that there will be exactly one call to either the success callback or the failure
// callback.
auto onSuccessCb = [successCb, bridge](const app::ConcreteCommandPath & commandPath, const app::StatusIB & status,
const MTRDataValueDictionaryDecodableType & responseData) {
auto resultArray = [[NSMutableArray alloc] init];
if (responseData.GetDecodedObject()) {
[resultArray addObject:@ {
MTRCommandPathKey : [[MTRCommandPath alloc] initWithPath:commandPath],
Expand All @@ -1074,16 +1100,10 @@ - (void)invokeCommandWithEndpointID:(NSNumber *)endpointID
} else {
[resultArray addObject:@ { MTRCommandPathKey : [[MTRCommandPath alloc] initWithPath:commandPath] }];
}
if ([resultSuccess count] == 0) {
[resultSuccess addObject:[NSNumber numberWithBool:YES]];
}
successCb(bridge, resultArray);
};

auto onFailureCb = [resultFailure](CHIP_ERROR aError) {
if ([resultFailure count] == 0) {
[resultFailure addObject:[MTRError errorForCHIPErrorCode:aError]];
}
};
auto onFailureCb = [failureCb, bridge](CHIP_ERROR aError) { failureCb(bridge, aError); };

app::CommandPathParams commandPath = { static_cast<chip::EndpointId>([endpointID unsignedShortValue]), 0,
static_cast<chip::ClusterId>([clusterID unsignedLongValue]),
Expand All @@ -1094,23 +1114,7 @@ - (void)invokeCommandWithEndpointID:(NSNumber *)endpointID
VerifyOrReturnError(decoder != nullptr, CHIP_ERROR_NO_MEMORY);

auto rawDecoderPtr = decoder.get();
auto onDoneCb = [rawDecoderPtr, bridge, successCb, failureCb, resultArray, resultSuccess, resultFailure](
app::CommandSender * commandSender) {
if ([resultFailure count] > 0 || [resultSuccess count] == 0) {
// Failure
if (failureCb) {
if ([resultFailure count] > 0) {
failureCb(bridge, [MTRError errorToCHIPErrorCode:resultFailure[0]]);
} else {
failureCb(bridge, CHIP_ERROR_WRITE_FAILED);
}
}
} else {
// Success
if (successCb) {
successCb(bridge, resultArray);
}
}
auto onDoneCb = [rawDecoderPtr](app::CommandSender * commandSender) {
chip::Platform::Delete(commandSender);
chip::Platform::Delete(rawDecoderPtr);
};
Expand Down
5 changes: 4 additions & 1 deletion src/darwin/Framework/CHIP/MTRDevice.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,10 @@ typedef NS_ENUM(NSUInteger, MTRDeviceState) {
* @param timeout timeout in milliseconds for timed invoke, or nil. This value must be within [1, UINT16_MAX], and will be clamped
* to this range.
*
* @param completion response handler will receive either values or error.
* @param completion response handler will receive either values or error. A
* path-specific error status from the command invocation
* will result in an error being passed to the completion, so
* values will only be passed in when the command succeeds.
*/
- (void)invokeCommandWithEndpointID:(NSNumber *)endpointID
clusterID:(NSNumber *)clusterID
Expand Down
7 changes: 7 additions & 0 deletions src/darwin/Framework/CHIPTests/MTRDeviceTests.m
View file
Original file line number Diff line number Diff line change
Expand Up @@ -354,6 +354,7 @@ - (void)test003_InvokeCommand
completion:^(id _Nullable values, NSError * _Nullable error) {
NSLog(@"invoke command: MoveToLevelWithOnOff values: %@, error: %@", values, error);

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

{
Expand Down Expand Up @@ -485,6 +486,7 @@ - (void)test005_Subscribe
completion:^(id _Nullable values, NSError * _Nullable error) {
NSLog(@"invoke command: On values: %@, error: %@", values, error);

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

{
Expand Down Expand Up @@ -535,6 +537,7 @@ - (void)test005_Subscribe
completion:^(id _Nullable values, NSError * _Nullable error) {
NSLog(@"invoke command: On values: %@, error: %@", values, error);

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

{
Expand Down Expand Up @@ -1090,6 +1093,7 @@ - (void)test012_SubscriptionError
completion:^(id _Nullable values, NSError * _Nullable error) {
NSLog(@"invoke command: On values: %@, error: %@", values, error);

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

{
Expand Down Expand Up @@ -1681,6 +1685,7 @@ - (void)test900_SubscribeAllAttributes
completion:^(id _Nullable values, NSError * _Nullable error) {
NSLog(@"invoke command: On values: %@, error: %@", values, error);

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

{
Expand Down Expand Up @@ -1711,6 +1716,7 @@ - (void)test900_SubscribeAllAttributes
completion:^(id _Nullable values, NSError * _Nullable error) {
NSLog(@"invoke command: On values: %@, error: %@", values, error);

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

{
Expand Down Expand Up @@ -1760,6 +1766,7 @@ - (void)test900_SubscribeAllAttributes
completion:^(id _Nullable values, NSError * _Nullable error) {
NSLog(@"invoke command: On values: %@, error: %@", values, error);

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

{
Expand Down

0 comments on commit c6cadc8

Please sign in to comment.