[Group] Added ondone logic for command sending (#13584)

* Added ondone logic for group command * generated files * generation correction * block time invoke when group command * Removed error * break out SendGroupCommandRequest * Apply suggestions from code review Co-authored-by: Boris Zbarsky <[email protected]> * review comments * Apply suggestions from code review Co-authored-by: Boris Zbarsky <[email protected]> Co-authored-by: Boris Zbarsky <[email protected]>
project-chip · Jan 21, 2022 · 7abbe5f · 7abbe5f
1 parent 8ee96e4
commit 7abbe5f
Show file tree

Hide file tree

Showing 6 changed files with 133 additions and 16 deletions.
diff --git a/examples/chip-tool/commands/common/CommandInvoker.h b/examples/chip-tool/commands/common/CommandInvoker.h
@@ -34,12 +34,13 @@ class ResponseReceiver : public app::CommandSender::Callback
 public:
     using SuccessCallback = void (*)(void * context, const ResponseType & data);
     using FailureCallback = void (*)(void * context, EmberAfStatus status);
+    using DoneCallback    = void (*)(void * context);
 
     virtual ~ResponseReceiver() {}
 
 protected:
-    ResponseReceiver(void * aContext, SuccessCallback aOnSuccess, FailureCallback aOnError) :
-        mContext(aContext), mOnSuccess(aOnSuccess), mOnError(aOnError)
+    ResponseReceiver(void * aContext, SuccessCallback aOnSuccess, FailureCallback aOnError, DoneCallback aOnDone) :
+        mContext(aContext), mOnSuccess(aOnSuccess), mOnError(aOnError), mOnDone(aOnDone)
     {}
 
     inline void OnResponse(app::CommandSender * aCommandSender, const app::ConcreteCommandPath & aPath,
@@ -52,6 +53,11 @@ class ResponseReceiver : public app::CommandSender::Callback
 
     void OnDone(app::CommandSender * aCommandSender) override
     {
+        if (mOnDone != nullptr)
+        {
+            mOnDone(mContext);
+        }
+
         Platform::Delete(aCommandSender);
         Platform::Delete(this);
     }
@@ -60,6 +66,7 @@ class ResponseReceiver : public app::CommandSender::Callback
     void * mContext;
     SuccessCallback mOnSuccess;
     FailureCallback mOnError;
+    DoneCallback mOnDone = nullptr;
 };
 
 template <typename RequestType>
@@ -68,8 +75,9 @@ class CommandInvoker final : public ResponseReceiver<typename RequestType::Respo
     using Super = ResponseReceiver<typename RequestType::ResponseType>;
 
 public:
-    CommandInvoker(void * aContext, typename Super::SuccessCallback aOnSuccess, typename Super::FailureCallback aOnError) :
-        Super(aContext, aOnSuccess, aOnError)
+    CommandInvoker(void * aContext, typename Super::SuccessCallback aOnSuccess, typename Super::FailureCallback aOnError,
+                   typename Super::DoneCallback aOnDone) :
+        Super(aContext, aOnSuccess, aOnError, aOnDone)
     {}
 
     /**
@@ -80,9 +88,10 @@ class CommandInvoker final : public ResponseReceiver<typename RequestType::Respo
      *   ReturnErrorOnFailure(invoker->InvokeCommand(args));
      *   invoker.release(); // The invoker will deallocate itself now.
      */
-    static auto Alloc(void * aContext, typename Super::SuccessCallback aOnSuccess, typename Super::FailureCallback aOnError)
+    static auto Alloc(void * aContext, typename Super::SuccessCallback aOnSuccess, typename Super::FailureCallback aOnError,
+                      typename Super::DoneCallback aOnDone)
     {
-        return Platform::MakeUnique<CommandInvoker>(aContext, aOnSuccess, aOnError);
+        return Platform::MakeUnique<CommandInvoker>(aContext, aOnSuccess, aOnError, aOnDone);
     }
 
     CHIP_ERROR InvokeCommand(DeviceProxy * aDevice, EndpointId aEndpoint, const RequestType & aRequestData,
@@ -115,8 +124,12 @@ class CommandInvoker final : public ResponseReceiver<typename RequestType::Respo
         {
             return CHIP_ERROR_NO_MEMORY;
         }
-        ReturnErrorOnFailure(commandSender->SendCommandRequest(session.Value()));
 
+        // this (invoker) and commandSender will be deleted by the onDone call before the return of SendGroupCommandRequest
+        // this (invoker) should not be used after the SendGroupCommandRequest call
+        ReturnErrorOnFailure(commandSender->SendGroupCommandRequest(session.Value()));
+
+        // this (invoker) and commandSender are already deleted and are not to be used
         commandSender.release();
         exchangeManager->GetSessionManager()->RemoveGroupSession(session.Value()->AsGroupSession());
 
@@ -181,7 +194,8 @@ CHIP_ERROR InvokeCommand(DeviceProxy * aDevice, void * aContext,
                          typename detail::CommandInvoker<RequestType>::FailureCallback aFailureCallback, EndpointId aEndpoint,
                          const RequestType & aRequestData, const Optional<uint16_t> & aTimedInvokeTimeoutMs)
 {
-    auto invoker = detail::CommandInvoker<RequestType>::Alloc(aContext, aSuccessCallback, aFailureCallback);
+    auto invoker =
+        detail::CommandInvoker<RequestType>::Alloc(aContext, aSuccessCallback, aFailureCallback, nullptr /* aDoneCallback */);
     VerifyOrReturnError(invoker != nullptr, CHIP_ERROR_NO_MEMORY);
     ReturnErrorOnFailure(invoker->InvokeCommand(aDevice, aEndpoint, aRequestData, aTimedInvokeTimeoutMs));
     invoker.release();
@@ -211,12 +225,18 @@ CHIP_ERROR InvokeCommand(DeviceProxy * aDevice, void * aContext,
 template <typename RequestType, typename std::enable_if_t<!RequestType::MustUseTimedInvoke(), int> = 0>
 CHIP_ERROR InvokeGroupCommand(DeviceProxy * aDevice, void * aContext,
                               typename detail::CommandInvoker<RequestType>::SuccessCallback aSuccessCallback,
-                              typename detail::CommandInvoker<RequestType>::FailureCallback aFailureCallback, GroupId groupId,
+                              typename detail::CommandInvoker<RequestType>::FailureCallback aFailureCallback,
+                              typename detail::CommandInvoker<RequestType>::DoneCallback aDoneCallback, GroupId groupId,
                               const RequestType & aRequestData)
 {
-    auto invoker = detail::CommandInvoker<RequestType>::Alloc(aContext, aSuccessCallback, aFailureCallback);
+    auto invoker = detail::CommandInvoker<RequestType>::Alloc(aContext, aSuccessCallback, aFailureCallback, aDoneCallback);
     VerifyOrReturnError(invoker != nullptr, CHIP_ERROR_NO_MEMORY);
-    ReturnErrorOnFailure(invoker->InvokeGroupCommand(aDevice, groupId, aRequestData));
+
+    // invoker will be deleted by the onDone call before the return of InvokeGroupCommand
+    // invoker should not be used after the InvokeGroupCommand call
+    ReturnErrorOnFailure(invoker->InvokeGroupCommand(aDevice->GetExchangeManager(), groupId, aRequestData));
+
+    //  invoker is already deleted and is not to be used
     invoker.release();
     return CHIP_NO_ERROR;
 }

diff --git a/examples/chip-tool/templates/partials/test_cluster.zapt b/examples/chip-tool/templates/partials/test_cluster.zapt
@@ -251,7 +251,13 @@ class {{filename}}: public TestCommand
             (static_cast<{{filename}} *>(context))->{{>failureResponse}}(status);
         };
 
-        ReturnErrorOnFailure(chip::Controller::{{#if isGroupCommand}}InvokeGroupCommand{{else}}InvokeCommand{{/if}}({{>device}}, this, success, failure,
+        {{#if isGroupCommand}}
+        auto done = [](void * context) {
+            (static_cast<{{filename}} *>(context))->OnDoneResponse_{{index}}();
+        };
+        {{/if}}
+
+        ReturnErrorOnFailure(chip::Controller::{{#if isGroupCommand}}InvokeGroupCommand{{else}}InvokeCommand{{/if}}({{>device}}, this, success, failure, {{#if isGroupCommand}}done,{{/if}}
           {{#if isGroupCommand}}groupId{{else}}endpoint{{/if}},
           request
           {{> maybeTimedInteractionTimeout }}

diff --git a/src/app/CommandSender.cpp b/src/app/CommandSender.cpp
@@ -69,6 +69,7 @@ CHIP_ERROR CommandSender::SendCommandRequest(const SessionHandle & session, Opti
     // Create a new exchange context.
     mpExchangeCtx = mpExchangeMgr->NewContext(session, this);
     VerifyOrReturnError(mpExchangeCtx != nullptr, CHIP_ERROR_NO_MEMORY);
+    VerifyOrReturnError(!mpExchangeCtx->IsGroupExchangeContext(), CHIP_ERROR_INVALID_MESSAGE_TYPE);
 
     mpExchangeCtx->SetResponseTimeout(timeout.ValueOr(kImMessageTimeout));
 
@@ -82,6 +83,23 @@ CHIP_ERROR CommandSender::SendCommandRequest(const SessionHandle & session, Opti
     return SendInvokeRequest();
 }
 
+CHIP_ERROR CommandSender::SendGroupCommandRequest(const SessionHandle & session)
+{
+    VerifyOrReturnError(mState == State::AddedCommand, CHIP_ERROR_INCORRECT_STATE);
+
+    ReturnErrorOnFailure(Finalize(mPendingInvokeData));
+
+    // Create a new exchange context.
+    mpExchangeCtx = mpExchangeMgr->NewContext(session, this);
+    VerifyOrReturnError(mpExchangeCtx != nullptr, CHIP_ERROR_NO_MEMORY);
+    VerifyOrReturnError(mpExchangeCtx->IsGroupExchangeContext(), CHIP_ERROR_INVALID_MESSAGE_TYPE);
+
+    ReturnErrorOnFailure(SendInvokeRequest());
+
+    Close();
+    return CHIP_NO_ERROR;
+}
+
 CHIP_ERROR CommandSender::SendInvokeRequest()
 {
     using namespace Protocols::InteractionModel;

diff --git a/src/app/CommandSender.h b/src/app/CommandSender.h
@@ -123,7 +123,8 @@ class CommandSender final : public Messaging::ExchangeDelegate
          * This function will:
          *      - Always be called exactly *once* for a given CommandSender instance.
          *      - Be called even in error circumstances.
-         *      - Only be called after a successful call to SendCommandRequest as been made.
+         *      - Only be called after a successful call to SendCommandRequest returns, if SendCommandRequest is used.
+         *      - Always be called before a successful return from SendGroupCommandRequest, if SendGroupCommandRequest is used.
          *
          * This function must be implemented to destroy the CommandSender object.
          *
@@ -217,6 +218,13 @@ class CommandSender final : public Messaging::ExchangeDelegate
     //
     CHIP_ERROR SendCommandRequest(const SessionHandle & session, Optional<System::Clock::Timeout> timeout = NullOptional);
 
+    // Sends a queued up group command request to the target encapsulated by the secureSession handle.
+    //
+    // If this function is successful, it will invoke the OnDone callback before returning to indicate
+    // to the application that it can destroy and free this object.
+    //
+    CHIP_ERROR SendGroupCommandRequest(const SessionHandle & session);
+
 private:
     friend class TestCommandInteraction;
 

diff --git a/src/app/tests/suites/TestGroupMessaging.yaml b/src/app/tests/suites/TestGroupMessaging.yaml
@@ -67,13 +67,12 @@ tests:
       cluster: "On/Off"
       command: "On"
       groupId: "1234"
-      disabled: true
 
       # Test Pair 3 : Validates previous group command with a unicast to read
     - label: "Check on/off attribute value is true after on command"
       cluster: "On/Off"
       command: "readAttribute"
       attribute: "OnOff"
-      disabled: true
+      endpoint: 1
       response:
           value: 1
diff --git a/zzz_generated/chip-tool/zap-generated/test/Commands.h b/zzz_generated/chip-tool/zap-generated/test/Commands.h