From fc3824f74c4558414410c64d6cef992cc615e7cc Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Mon, 20 Feb 2023 13:38:56 -0500 Subject: [PATCH] Simplify the logic in invokeCommandWithEndpointID in MTRBaseDevice. * 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 5317d1e3e15e59..bc05f0cecdea16 100644 --- a/src/darwin/Framework/CHIP/MTRBaseDevice.h +++ b/src/darwin/Framework/CHIP/MTRBaseDevice.h @@ -256,7 +256,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 1234edb4e2b1e8..1c4c5205afed69 100644 --- a/src/darwin/Framework/CHIP/MTRBaseDevice.mm +++ b/src/darwin/Framework/CHIP/MTRBaseDevice.mm @@ -1036,9 +1036,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; @@ -1046,11 +1066,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; @@ -1096,11 +1122,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], @@ -1109,16 +1135,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]), @@ -1129,23 +1149,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); {