[group] Replace manual onSuccess call for group message by onDone cal…

…lback (#12121) * Replace manual onSuccess by onDone callback * generated files * merge conflict fix * pr comments * regenerate code
project-chip · Apr 8, 2022 · cca46e0 · cca46e0
1 parent c9bcf36
commit cca46e0
Show file tree

Hide file tree

Showing 5 changed files with 100 additions and 24 deletions.
diff --git 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::app::Clusters::{{asUpperCamelCase cluster}}::Attributes::{{asUpperCamelCase attribute}}::TypeInfo>({{#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::app::Clusters::{{asUpperCamelCase cluster}}::Attributes::{{asUpperCamelCase attribute}}::TypeInfo>({{#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
@@ -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
@@ -39,8 +39,10 @@ namespace Controller {
 template <typename T>
 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 <typename T>
 using ReadResponseSuccessCallback = void (*)(void * context, T responseData);
 using ReadResponseFailureCallback = void (*)(void * context, EmberAfStatus status);
@@ -93,6 +95,14 @@ class DLL_EXPORT ClusterBase
     template <typename AttrType>
     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 /* doneCb */);
+    }
+
+    template <typename AttrType>
+    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<AttrType>((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<AttrType>(
+            (mSessionHandle.HasValue() ? mSessionHandle.Value() : mDevice->GetSecureSession().Value()), mEndpoint, clusterId,
+            attributeId, requestData, onSuccessCb, onFailureCb, onDoneCb);
     }
 
     template <typename AttributeInfo>
@@ -124,6 +141,15 @@ class DLL_EXPORT ClusterBase
                               failureCb);
     }
 
+    template <typename AttributeInfo>
+    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
@@ -47,7 +47,7 @@ class WriteCallback final : public app::WriteClient::Callback
     //
     using OnErrorCallbackType =
         std::function<void(const app::ConcreteAttributePath * path, app::StatusIB status, CHIP_ERROR aError)>;
-    using OnDoneCallbackType = std::function<void(app::WriteClient *, WriteCallback *)>;
+    using OnDoneCallbackType = std::function<void(app::WriteClient *)>;
 
     WriteCallback(OnSuccessCallbackType aOnSuccess, OnErrorCallbackType aOnError, OnDoneCallbackType aOnDone) :
         mOnSuccess(aOnSuccess), mOnError(aOnError), mOnDone(aOnDone)
@@ -70,12 +70,21 @@ 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 +96,11 @@ class WriteCallback final : public app::WriteClient::Callback
 template <typename AttrType>
 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<WriteCallback>(onSuccessCb, onErrorCb, onDone);
+    auto callback = Platform::MakeUnique<WriteCallback>(onSuccessCb, onErrorCb, onDoneCb);
     VerifyOrReturnError(callback != nullptr, CHIP_ERROR_NO_MEMORY);
 
     ReturnErrorOnFailure(app::InteractionModelEngine::GetInstance()->NewWriteClient(handle, callback.get()));
@@ -111,14 +118,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 +127,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 <typename AttributeInfo>
+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

diff --git a/zzz_generated/chip-tool/zap-generated/test/Commands.h b/zzz_generated/chip-tool/zap-generated/test/Commands.h