From b69deec309711f54fffb7ff3608d818659997816 Mon Sep 17 00:00:00 2001 From: Mathieu Kardous Date: Mon, 22 Nov 2021 16:23:56 -0500 Subject: [PATCH 1/5] Replace manual onSuccess by onDone callback --- .../templates/partials/test_cluster.zapt | 34 ++++++++++++++- src/app/WriteClient.cpp | 2 + src/controller/CHIPCluster.h | 32 ++++++++++++-- src/controller/WriteInteraction.h | 42 +++++++++++-------- 4 files changed, 88 insertions(+), 22 deletions(-) diff --git a/examples/chip-tool/templates/partials/test_cluster.zapt b/examples/chip-tool/templates/partials/test_cluster.zapt index d733267856b329..e8a3ba9bf7b23b 100644 --- a/examples/chip-tool/templates/partials/test_cluster.zapt +++ b/examples/chip-tool/templates/partials/test_cluster.zapt @@ -59,11 +59,23 @@ class {{filename}}: public TestCommand {{#*inline "failureCallback"}}mOnFailureCallback_{{index}}{{/inline}} {{#*inline "successCallback"}}mOnSuccessCallback_{{index}}{{/inline}} + + {{#if isGroupCommand}} + {{#*inline "doneCallback"}}mOnDoneCallback_{{index}}{{/inline}} + {{/if}} + {{#*inline "failureResponse"}}OnFailureCallback_{{index}}{{/inline}} {{#*inline "successResponse"}}OnSuccessCallback_{{index}}{{/inline}} + + {{#*inline "doneResponse"}}OnDoneCallback_{{index}}{{/inline}} + {{#*inline "successArguments"}}void * context{{#chip_tests_item_response_parameters}}, {{zapTypeToDecodableClusterObjectType type ns=parent.cluster isArgument=true}} {{asLowerCamelCase name}}{{/chip_tests_item_response_parameters}}{{/inline}} {{#*inline "failureArguments"}}void * context, uint8_t status{{/inline}} + {{#if isGroupCommand}} + {{#*inline "doneArguments"}}void * context{{/inline}} + {{/if}} + {{#chip_tests_items}} {{#unless (isTestOnlyCluster cluster)}} {{#unless isWait}} @@ -88,6 +100,15 @@ class {{filename}}: public TestCommand { (static_cast<{{filename}} *>(context))->OnFailureResponse_{{index}}(chip::to_underlying(status)); } + + {{#if isGroupCommand}} + static void {{>doneResponse}}(void * context) + { + (static_cast<{{filename}} *>(context))->OnDoneResponse_{{index}}(); + + } + {{/if}} + {{else if isReadAttribute}} static void {{>failureResponse}}(void * context, EmberAfStatus status) { @@ -141,8 +162,11 @@ class {{filename}}: public TestCommand {{else}} {{#*inline "failureResponse"}}OnFailureResponse_{{index}}{{/inline}} {{#*inline "successResponse"}}OnSuccessResponse_{{index}}{{/inline}} + {{#*inline "doneResponse"}}OnDoneResponse_{{index}}{{/inline}} + {{#*inline "failureArguments"}}uint8_t status{{/inline}} {{#*inline "successArguments"}}{{#chip_tests_item_response_parameters}}{{#not_first}}, {{/not_first}}{{zapTypeToDecodableClusterObjectType type ns=parent.cluster isArgument=true}} {{asLowerCamelCase name}}{{/chip_tests_item_response_parameters}}{{/inline}} + {{#*inline "doneArguments"}}{{/inline}} CHIP_ERROR {{>testCommand}}() { @@ -185,7 +209,8 @@ class {{filename}}: public TestCommand {{#if isWriteAttribute}} {{#*inline "failureResponse"}}OnFailureCallback_{{index}}{{/inline}} {{#*inline "successResponse"}}OnSuccessCallback_{{index}}{{/inline}} - {{#if async}}ReturnErrorOnFailure({{else}}return {{/if}}cluster.WriteAttribute({{#chip_tests_item_parameters}}{{asLowerCamelCase name}}Argument, {{/chip_tests_item_parameters}}this, {{>successResponse}}, {{>failureResponse}}){{#if async}}){{/if}}; + {{#*inline "doneResponse"}}OnDoneCallback_{{index}}{{/inline}} + {{#if async}}ReturnErrorOnFailure({{else}}return {{/if}}cluster.WriteAttribute({{#chip_tests_item_parameters}}{{asLowerCamelCase name}}Argument, {{/chip_tests_item_parameters}}this, {{>successResponse}}, {{>failureResponse}}{{#if isGroupCommand}}, {{>doneResponse}}{{/if}}){{#if async}}){{/if}}; {{else if isReadAttribute}} {{#*inline "failureResponse"}}OnFailureCallback_{{index}}{{/inline}} {{#*inline "successResponse"}}OnSuccessCallback_{{index}}{{/inline}} @@ -248,6 +273,13 @@ class {{filename}}: public TestCommand {{/if}} } + {{#if isGroupCommand}} + void {{>doneResponse}}({{>doneArguments}}) + { + NextTest(); + } + {{/if}} + {{/if}} {{/chip_tests_items}} }; diff --git a/src/app/WriteClient.cpp b/src/app/WriteClient.cpp index 2419a9cc0b8cf1..ab48f1022919fa 100644 --- a/src/app/WriteClient.cpp +++ b/src/app/WriteClient.cpp @@ -254,6 +254,8 @@ CHIP_ERROR WriteClient::SendWriteRequest(SessionHandle session, System::Clock::T { // Always shutdown on Group communication ChipLogDetail(DataManagement, "Closing on group Communication "); + + // onDone is called ShutdownInternal(); } diff --git a/src/controller/CHIPCluster.h b/src/controller/CHIPCluster.h index 43dd636466f655..72d5df855bacf5 100644 --- a/src/controller/CHIPCluster.h +++ b/src/controller/CHIPCluster.h @@ -39,8 +39,10 @@ namespace Controller { template using CommandResponseSuccessCallback = void(void * context, const T & responseObject); using CommandResponseFailureCallback = void(void * context, EmberAfStatus status); +using CommandResponseDoneCallback = void(); using WriteResponseSuccessCallback = void (*)(void * context); using WriteResponseFailureCallback = void (*)(void * context, EmberAfStatus status); +using WriteResponseDoneCallback = void (*)(void * context); template using ReadResponseSuccessCallback = void (*)(void * context, T responseData); using ReadResponseFailureCallback = void (*)(void * context, EmberAfStatus status); @@ -93,6 +95,14 @@ class DLL_EXPORT ClusterBase template CHIP_ERROR WriteAttribute(const AttrType & requestData, void * context, ClusterId clusterId, AttributeId attributeId, WriteResponseSuccessCallback successCb, WriteResponseFailureCallback failureCb) + { + return WriteAttribute(requestData, context, clusterId, attributeId, successCb, failureCb, nullptr); + } + + template + CHIP_ERROR WriteAttribute(const AttrType & requestData, void * context, ClusterId clusterId, AttributeId attributeId, + WriteResponseSuccessCallback successCb, WriteResponseFailureCallback failureCb, + WriteResponseDoneCallback doneCb) { VerifyOrReturnError(mDevice != nullptr, CHIP_ERROR_INCORRECT_STATE); @@ -111,9 +121,16 @@ class DLL_EXPORT ClusterBase } }; - return chip::Controller::WriteAttribute((mSessionHandle.HasValue()) ? mSessionHandle.Value() - : mDevice->GetSecureSession().Value(), - mEndpoint, clusterId, attributeId, requestData, onSuccessCb, onFailureCb); + auto onDoneCb = [context, doneCb](app::WriteClient * pWriteClient) { + if (doneCb != nullptr) + { + doneCb(context); + } + }; + + return chip::Controller::WriteAttribute( + (mSessionHandle.HasValue() ? mSessionHandle.Value() : mDevice->GetSecureSession().Value()), mEndpoint, clusterId, + attributeId, requestData, onSuccessCb, onFailureCb, onDoneCb); } template @@ -124,6 +141,15 @@ class DLL_EXPORT ClusterBase failureCb); } + template + CHIP_ERROR WriteAttribute(const typename AttributeInfo::Type & requestData, void * context, + WriteResponseSuccessCallback successCb, WriteResponseFailureCallback failureCb, + WriteResponseDoneCallback doneCb) + { + return WriteAttribute(requestData, context, AttributeInfo::GetClusterId(), AttributeInfo::GetAttributeId(), successCb, + failureCb, doneCb); + } + /** * Read an attribute and get a type-safe callback with the attribute value. */ diff --git a/src/controller/WriteInteraction.h b/src/controller/WriteInteraction.h index ac1bf55b978cd7..68d401e4b5a82a 100644 --- a/src/controller/WriteInteraction.h +++ b/src/controller/WriteInteraction.h @@ -47,7 +47,7 @@ class WriteCallback final : public app::WriteClient::Callback // using OnErrorCallbackType = std::function; - using OnDoneCallbackType = std::function; + using OnDoneCallbackType = std::function; WriteCallback(OnSuccessCallbackType aOnSuccess, OnErrorCallbackType aOnError, OnDoneCallbackType aOnDone) : mOnSuccess(aOnSuccess), mOnError(aOnError), mOnDone(aOnDone) @@ -70,12 +70,19 @@ class WriteCallback final : public app::WriteClient::Callback mOnError(nullptr, app::StatusIB(Protocols::InteractionModel::Status::Failure), aError); } - void OnDone(app::WriteClient * apWriteClient) override { mOnDone(apWriteClient, this); } + void OnDone(app::WriteClient * apWriteClient) override + { + if (mOnDone != nullptr) + mOnDone(apWriteClient); + + // Always needs to be the last call + chip::Platform::Delete(this); + } private: - OnSuccessCallbackType mOnSuccess; - OnErrorCallbackType mOnError; - OnDoneCallbackType mOnDone; + OnSuccessCallbackType mOnSuccess = nullptr; + OnErrorCallbackType mOnError = nullptr; + OnDoneCallbackType mOnDone = nullptr; }; /** @@ -87,13 +94,11 @@ class WriteCallback final : public app::WriteClient::Callback template CHIP_ERROR WriteAttribute(SessionHandle sessionHandle, chip::EndpointId endpointId, ClusterId clusterId, AttributeId attributeId, const AttrType & requestData, WriteCallback::OnSuccessCallbackType onSuccessCb, - WriteCallback::OnErrorCallbackType onErrorCb) + WriteCallback::OnErrorCallbackType onErrorCb, WriteCallback::OnDoneCallbackType onDoneCb) { app::WriteClientHandle handle; - auto onDone = [](app::WriteClient * apWriteClient, WriteCallback * callback) { chip::Platform::Delete(callback); }; - - auto callback = Platform::MakeUnique(onSuccessCb, onErrorCb, onDone); + auto callback = Platform::MakeUnique(onSuccessCb, onErrorCb, onDoneCb); VerifyOrReturnError(callback != nullptr, CHIP_ERROR_NO_MEMORY); ReturnErrorOnFailure(app::InteractionModelEngine::GetInstance()->NewWriteClient(handle, callback.get())); @@ -111,14 +116,6 @@ CHIP_ERROR WriteAttribute(SessionHandle sessionHandle, chip::EndpointId endpoint ReturnErrorOnFailure(handle.SendWriteRequest(sessionHandle)); callback.release(); - - if (sessionHandle.IsGroupSession()) - { - // Manually call success callback since OnReponse won't be called in WriteClient for group - app::ConcreteAttributePath aPath; - onSuccessCb(aPath); - } - return CHIP_NO_ERROR; } @@ -128,7 +125,16 @@ CHIP_ERROR WriteAttribute(SessionHandle sessionHandle, chip::EndpointId endpoint WriteCallback::OnErrorCallbackType onErrorCb) { return WriteAttribute(sessionHandle, endpointId, AttributeInfo::GetClusterId(), AttributeInfo::GetAttributeId(), requestData, - onSuccessCb, onErrorCb); + onSuccessCb, onErrorCb, nullptr); +} + +template +CHIP_ERROR WriteAttribute(SessionHandle sessionHandle, chip::EndpointId endpointId, + const typename AttributeInfo::Type & requestData, WriteCallback::OnSuccessCallbackType onSuccessCb, + WriteCallback::OnErrorCallbackType onErrorCb, WriteCallback::OnDoneCallbackType onDoneCb) +{ + return WriteAttribute(sessionHandle, endpointId, AttributeInfo::GetClusterId(), AttributeInfo::GetAttributeId(), requestData, + onSuccessCb, onErrorCb, onDoneCb); } } // namespace Controller From a5ae70024fb7eb500a5200ed48d54fedf561491a Mon Sep 17 00:00:00 2001 From: Mathieu Kardous Date: Mon, 22 Nov 2021 16:24:05 -0500 Subject: [PATCH 2/5] generated files --- zzz_generated/chip-tool/zap-generated/test/Commands.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/zzz_generated/chip-tool/zap-generated/test/Commands.h b/zzz_generated/chip-tool/zap-generated/test/Commands.h index 7429e78970ca92..d8538ab8c5922b 100644 --- a/zzz_generated/chip-tool/zap-generated/test/Commands.h +++ b/zzz_generated/chip-tool/zap-generated/test/Commands.h @@ -43457,6 +43457,8 @@ class TestGroupMessaging : public TestCommand (static_cast(context))->OnFailureResponse_0(chip::to_underlying(status)); } + static void OnDoneCallback_0(void * context) { (static_cast(context))->OnDoneResponse_0(); } + static void OnSuccessCallback_0(void * context) { (static_cast(context))->OnSuccessResponse_0(); } static void OnFailureCallback_1(void * context, EmberAfStatus status) @@ -43500,7 +43502,7 @@ class TestGroupMessaging : public TestCommand locationArgument = chip::Span("usgarbage: not in length on purpose", 2); return cluster.WriteAttribute( - locationArgument, this, OnSuccessCallback_0, OnFailureCallback_0); + locationArgument, this, OnSuccessCallback_0, OnFailureCallback_0, OnDoneCallback_0); } void OnFailureResponse_0(uint8_t status) { ThrowFailureResponse(); } From 8007489206ee32f5fc483a5223957ce4f778db36 Mon Sep 17 00:00:00 2001 From: Mathieu Kardous Date: Mon, 22 Nov 2021 16:41:33 -0500 Subject: [PATCH 3/5] merge conflict fix --- zzz_generated/chip-tool/zap-generated/test/Commands.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/zzz_generated/chip-tool/zap-generated/test/Commands.h b/zzz_generated/chip-tool/zap-generated/test/Commands.h index d8538ab8c5922b..161b4619664c99 100644 --- a/zzz_generated/chip-tool/zap-generated/test/Commands.h +++ b/zzz_generated/chip-tool/zap-generated/test/Commands.h @@ -43476,6 +43476,8 @@ class TestGroupMessaging : public TestCommand (static_cast(context))->OnFailureResponse_2(chip::to_underlying(status)); } + static void OnDoneCallback_2(void * context) { (static_cast(context))->OnDoneResponse_2(); } + static void OnSuccessCallback_2(void * context) { (static_cast(context))->OnSuccessResponse_2(); } static void OnFailureCallback_3(void * context, EmberAfStatus status) @@ -43524,7 +43526,6 @@ class TestGroupMessaging : public TestCommand void OnSuccessResponse_1(chip::CharSpan location) { VerifyOrReturn(CheckValueAsString("location", location, chip::CharSpan("us", 2))); - NextTest(); } @@ -43560,7 +43561,6 @@ class TestGroupMessaging : public TestCommand void OnSuccessResponse_3(chip::CharSpan location) { VerifyOrReturn(CheckValueAsString("location", location, chip::CharSpan("", 0))); - NextTest(); } }; From 37fd4ad2ed8c3509d5149241a5441fced0070b1d Mon Sep 17 00:00:00 2001 From: Mathieu Kardous Date: Thu, 25 Nov 2021 11:33:10 -0500 Subject: [PATCH 4/5] pr comments --- src/controller/CHIPCluster.h | 2 +- src/controller/WriteInteraction.h | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/controller/CHIPCluster.h b/src/controller/CHIPCluster.h index 72d5df855bacf5..2c6d3786b7b6fd 100644 --- a/src/controller/CHIPCluster.h +++ b/src/controller/CHIPCluster.h @@ -96,7 +96,7 @@ class DLL_EXPORT ClusterBase CHIP_ERROR WriteAttribute(const AttrType & requestData, void * context, ClusterId clusterId, AttributeId attributeId, WriteResponseSuccessCallback successCb, WriteResponseFailureCallback failureCb) { - return WriteAttribute(requestData, context, clusterId, attributeId, successCb, failureCb, nullptr); + return WriteAttribute(requestData, context, clusterId, attributeId, successCb, failureCb, nullptr /* doneCb */); } template diff --git a/src/controller/WriteInteraction.h b/src/controller/WriteInteraction.h index 68d401e4b5a82a..d844302472f4dc 100644 --- a/src/controller/WriteInteraction.h +++ b/src/controller/WriteInteraction.h @@ -73,7 +73,9 @@ class WriteCallback final : public app::WriteClient::Callback void OnDone(app::WriteClient * apWriteClient) override { if (mOnDone != nullptr) + { mOnDone(apWriteClient); + } // Always needs to be the last call chip::Platform::Delete(this); From 31e82737af14b1b7a95d50aa6d75d414cc5d6662 Mon Sep 17 00:00:00 2001 From: Mathieu Kardous Date: Thu, 25 Nov 2021 11:54:29 -0500 Subject: [PATCH 5/5] regenerate code --- zzz_generated/chip-tool/zap-generated/test/Commands.h | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/zzz_generated/chip-tool/zap-generated/test/Commands.h b/zzz_generated/chip-tool/zap-generated/test/Commands.h index 161b4619664c99..8d7d45577f6a83 100644 --- a/zzz_generated/chip-tool/zap-generated/test/Commands.h +++ b/zzz_generated/chip-tool/zap-generated/test/Commands.h @@ -43511,6 +43511,8 @@ class TestGroupMessaging : public TestCommand void OnSuccessResponse_0() { NextTest(); } + void OnDoneResponse_0() { NextTest(); } + CHIP_ERROR TestReadBackAttribute_1() { const chip::EndpointId endpoint = mEndpointId.HasValue() ? mEndpointId.Value() : 0; @@ -43526,6 +43528,7 @@ class TestGroupMessaging : public TestCommand void OnSuccessResponse_1(chip::CharSpan location) { VerifyOrReturn(CheckValueAsString("location", location, chip::CharSpan("us", 2))); + NextTest(); } @@ -43539,13 +43542,15 @@ class TestGroupMessaging : public TestCommand locationArgument = chip::Span("garbage: not in length on purpose", 0); return cluster.WriteAttribute( - locationArgument, this, OnSuccessCallback_2, OnFailureCallback_2); + locationArgument, this, OnSuccessCallback_2, OnFailureCallback_2, OnDoneCallback_2); } void OnFailureResponse_2(uint8_t status) { ThrowFailureResponse(); } void OnSuccessResponse_2() { NextTest(); } + void OnDoneResponse_2() { NextTest(); } + CHIP_ERROR TestReadBackAttribute_3() { const chip::EndpointId endpoint = mEndpointId.HasValue() ? mEndpointId.Value() : 0; @@ -43561,6 +43566,7 @@ class TestGroupMessaging : public TestCommand void OnSuccessResponse_3(chip::CharSpan location) { VerifyOrReturn(CheckValueAsString("location", location, chip::CharSpan("", 0))); + NextTest(); } };