From 11657312892bbfd4378a45d7a6fd61d77dae7951 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Wed, 22 Feb 2023 13:30:02 -0500 Subject: [PATCH] Simplify the logic in invokeCommandWithEndpointID in MTRBaseDevice. (#25199) * Fix MTRInvokeCallback to guarantee callback delivery, similar to https://github.com/project-chip/connectedhomeip/pull/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. --- .../Framework/CHIP/MTRBaseClusterUtils.h | 14 +++- src/darwin/Framework/CHIP/MTRBaseDevice.h | 5 +- src/darwin/Framework/CHIP/MTRBaseDevice.mm | 68 ++++++++++--------- src/darwin/Framework/CHIP/MTRDevice.h | 5 +- .../Framework/CHIPTests/MTRDeviceTests.m | 7 ++ 5 files changed, 64 insertions(+), 35 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRBaseClusterUtils.h b/src/darwin/Framework/CHIP/MTRBaseClusterUtils.h index 41ed337db6e0b6..36a7687e8b1a25 100644 --- a/src/darwin/Framework/CHIP/MTRBaseClusterUtils.h +++ b/src/darwin/Framework/CHIP/MTRBaseClusterUtils.h @@ -368,7 +368,19 @@ template 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; diff --git a/src/darwin/Framework/CHIP/MTRBaseDevice.h b/src/darwin/Framework/CHIP/MTRBaseDevice.h index 4846986c9bea9b..b43145c27852ba 100644 --- a/src/darwin/Framework/CHIP/MTRBaseDevice.h +++ b/src/darwin/Framework/CHIP/MTRBaseDevice.h @@ -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 diff --git a/src/darwin/Framework/CHIP/MTRBaseDevice.mm b/src/darwin/Framework/CHIP/MTRBaseDevice.mm index 550dd2728b5613..c55be43d5538ac 100644 --- a/src/darwin/Framework/CHIP/MTRBaseDevice.mm +++ b/src/darwin/Framework/CHIP/MTRBaseDevice.mm @@ -1001,9 +1001,29 @@ - (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; @@ -1011,11 +1031,17 @@ void OnResponse(app::CommandSender * apCommandSender, const app::ConcreteCommand 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; @@ -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], @@ -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([endpointID unsignedShortValue]), 0, static_cast([clusterID unsignedLongValue]), @@ -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); }; diff --git a/src/darwin/Framework/CHIP/MTRDevice.h b/src/darwin/Framework/CHIP/MTRDevice.h index 4ae66999f52f64..418500efd93883 100644 --- a/src/darwin/Framework/CHIP/MTRDevice.h +++ b/src/darwin/Framework/CHIP/MTRDevice.h @@ -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 diff --git a/src/darwin/Framework/CHIPTests/MTRDeviceTests.m b/src/darwin/Framework/CHIPTests/MTRDeviceTests.m index 1f9f0bae6121d0..52a54ac2e7bc72 100644 --- a/src/darwin/Framework/CHIPTests/MTRDeviceTests.m +++ b/src/darwin/Framework/CHIPTests/MTRDeviceTests.m @@ -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); { @@ -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); { @@ -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); { @@ -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); { @@ -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); { @@ -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); { @@ -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); {