Imporve Command Add API in IM Command

Summary of Changes:
-- Add PrepareCommand, GetCommandDataElementTLVWriter and FinishCommand
API to handle command add functionality, and remove old AddCommand which
create the redundant buffer to hold command data
-- Disable im in experimental build, and the ember change would happen
in seperate PR.

.github/workflows/build.yaml

@@ -58,7 +58,6 @@ jobs:
                      "gcc_release") GN_ARGS='is_debug=false';;
                      "clang") GN_ARGS='is_clang=true';;
                      "mbedtls") GN_ARGS='chip_crypto="mbedtls"';;
-                     "clang_experimental") GN_ARGS='is_clang=true chip_enable_interaction_model=true';;
                      *) ;;
                   esac
 

src/app/Command.cpp

@@ -67,7 +67,8 @@ CHIP_ERROR Command::Reset()
     SuccessOrExit(err);
 
     commandListBuilder = mInvokeCommandBuilder.CreateCommandListBuilder();
-    SuccessOrExit(commandListBuilder.GetError());
+    err                = commandListBuilder.GetError();
+    SuccessOrExit(err);
     MoveToState(CommandState::Initialized);
 
     mCommandIndex = 0;
@@ -123,6 +124,7 @@ CHIP_ERROR Command::ProcessCommandMessage(System::PacketBufferHandle && payload,
     }
 
 exit:
+    ChipLogFunctError(err);
     return err;
 }
 
@@ -136,86 +138,58 @@ void Command::Shutdown()
 
     mpExchangeMgr = nullptr;
     mpDelegate    = nullptr;
-    MoveToState(CommandState::Uninitialized);
+    ClearState();
 
     mCommandIndex = 0;
 exit:
     return;
 }
 
-chip::TLV::TLVWriter & Command::CreateCommandDataElementTLVWriter()
+CHIP_ERROR Command::PrepareCommand(const CommandParams * const apCommandParams)
 {
-    mCommandDataBuf = chip::System::PacketBufferHandle::New(chip::app::kMaxSecureSduLengthBytes);
-    if (mCommandDataBuf.IsNull())
+    CHIP_ERROR err = CHIP_NO_ERROR;
+
+    CommandDataElement::Builder commandDataElement =
+        mInvokeCommandBuilder.GetCommandListBuilder().CreateCommandDataElementBuilder();
+    err = commandDataElement.GetError();
+    SuccessOrExit(err);
+
+    if (apCommandParams != nullptr)
     {
-        ChipLogDetail(DataManagement, "Unable to allocate packet buffer");
+        err = ConstructCommandPath(*apCommandParams, commandDataElement);
+        SuccessOrExit(err);
     }
 
-    mCommandDataWriter.Init(mCommandDataBuf.Retain());
-
-    return mCommandDataWriter;
+    err = commandDataElement.GetWriter()->StartContainer(TLV::ContextTag(CommandDataElement::kCsTag_Data), TLV::kTLVType_Structure,
+                                                         mDataElementContainerType);
+exit:
+    ChipLogFunctError(err);
+    return err;
 }
 
-CHIP_ERROR Command::AddCommand(chip::EndpointId aEndpointId, chip::GroupId aGroupId, chip::ClusterId aClusterId,
-                               chip::CommandId aCommandId, BitFlags<CommandPathFlags> aFlags)
+TLV::TLVWriter * Command::GetCommandDataElementTLVWriter()
 {
-    CommandParams commandParams(aEndpointId, aGroupId, aClusterId, aCommandId, aFlags);
-
-    return AddCommand(commandParams);
+    return mInvokeCommandBuilder.GetCommandListBuilder().GetCommandDataElementBuilder().GetWriter();
 }
 
-CHIP_ERROR Command::AddCommand(CommandParams & aCommandParams)
+CHIP_ERROR Command::FinishCommand()
 {
-    CHIP_ERROR err              = CHIP_NO_ERROR;
-    const uint8_t * commandData = nullptr;
-    uint32_t commandLen         = 0;
-
-    if (!mCommandDataBuf.IsNull())
-    {
-        commandData = mCommandDataBuf->Start();
-        commandLen  = mCommandDataBuf->DataLength();
-    }
-
-    if (commandLen > 0 && commandData != nullptr)
-    {
-        // Command argument list can be empty.
-        VerifyOrExit(commandLen >= 2, err = CHIP_ERROR_INVALID_ARGUMENT);
-        VerifyOrExit(commandData[0] == chip::TLV::kTLVType_Structure, err = CHIP_ERROR_INVALID_ARGUMENT);
-
-        commandData += 1;
-        commandLen -= 1;
-    }
-
-    {
-        CommandDataElement::Builder commandDataElement =
-            mInvokeCommandBuilder.GetCommandListBuilder().CreateCommandDataElementBuilder();
-
-        err = Command::ConstructCommandPath(aCommandParams, commandDataElement);
-        SuccessOrExit(err);
+    CHIP_ERROR err = CHIP_NO_ERROR;
 
-        if (commandLen > 0 && commandData != nullptr)
-        {
-            // Copy the application data into a new TLV structure field contained with the
-            // command structure.  NOTE: The TLV writer will take care of moving the app data
-            // to the correct location within the buffer.
-            err = mInvokeCommandBuilder.GetWriter()->PutPreEncodedContainer(chip::TLV::ContextTag(CommandDataElement::kCsTag_Data),
-                                                                            chip::TLV::kTLVType_Structure, commandData, commandLen);
-            SuccessOrExit(err);
-        }
-        commandDataElement.EndOfCommandDataElement();
-
-        err = commandDataElement.GetError();
-        SuccessOrExit(err);
-    }
+    CommandDataElement::Builder commandDataElement = mInvokeCommandBuilder.GetCommandListBuilder().GetCommandDataElementBuilder();
+    err                                            = commandDataElement.GetWriter()->EndContainer(mDataElementContainerType);
+    SuccessOrExit(err);
+    commandDataElement.EndOfCommandDataElement();
+    err = commandDataElement.GetError();
+    SuccessOrExit(err);
     MoveToState(CommandState::AddCommand);
 
 exit:
-    mCommandDataBuf = nullptr;
     ChipLogFunctError(err);
     return err;
 }
 
-CHIP_ERROR Command::ConstructCommandPath(const CommandParams & aCommandParams, CommandDataElement::Builder & aCommandDataElement)
+CHIP_ERROR Command::ConstructCommandPath(const CommandParams & aCommandParams, CommandDataElement::Builder aCommandDataElement)
 {
     CommandPath::Builder commandPath = aCommandDataElement.CreateCommandPathBuilder();
     if (aCommandParams.Flags.Has(CommandPathFlags::kEndpointIdValid))
@@ -249,20 +223,18 @@ CHIP_ERROR Command::ClearExistingExchangeContext()
 CHIP_ERROR Command::FinalizeCommandsMessage()
 {
     CHIP_ERROR err = CHIP_NO_ERROR;
-
-    CommandList::Builder commandListBuilder = mInvokeCommandBuilder.GetCommandListBuilder().EndOfCommandList();
-    SuccessOrExit(commandListBuilder.GetError());
+    CommandList::Builder commandListBuilder;
+    VerifyOrExit(mState == CommandState::AddCommand, err = CHIP_ERROR_INCORRECT_STATE);
+    commandListBuilder = mInvokeCommandBuilder.GetCommandListBuilder().EndOfCommandList();
+    err                = commandListBuilder.GetError();
+    SuccessOrExit(err);
 
     mInvokeCommandBuilder.EndOfInvokeCommand();
     err = mInvokeCommandBuilder.GetError();
     SuccessOrExit(err);
 
     err = mCommandMessageWriter.Finalize(&mCommandMessageBuf);
     SuccessOrExit(err);
-
-    VerifyOrExit(mCommandMessageBuf->EnsureReservedSize(System::PacketBuffer::kDefaultHeaderReserve),
-                 err = CHIP_ERROR_BUFFER_TOO_SMALL);
-
 exit:
     ChipLogFunctError(err);
     return err;

src/app/Command.h

@@ -117,10 +117,9 @@ class Command
      */
     CHIP_ERROR FinalizeCommandsMessage();
 
-    chip::TLV::TLVWriter & CreateCommandDataElementTLVWriter();
-    CHIP_ERROR AddCommand(chip::EndpointId aEndpintId, chip::GroupId aGroupId, chip::ClusterId aClusterId,
-                          chip::CommandId aCommandId, BitFlags<CommandPathFlags> Flags);
-    CHIP_ERROR AddCommand(CommandParams & aCommandParams);
+    CHIP_ERROR PrepareCommand(const CommandParams * const apCommandParams);
+    TLV::TLVWriter * GetCommandDataElementTLVWriter();
+    CHIP_ERROR FinishCommand();
     virtual CHIP_ERROR AddStatusCode(const CommandParams * apCommandParams,
                                      const Protocols::SecureChannel::GeneralStatusCode aGeneralCode,
                                      const Protocols::Id aProtocolId, const uint16_t aProtocolCode)
@@ -141,14 +140,14 @@ class Command
 
     virtual ~Command() = default;
 
-    bool IsFree() const { return (nullptr == mpExchangeCtx); };
+    bool IsFree() const { return mState == CommandState::Uninitialized; };
     virtual CHIP_ERROR ProcessCommandDataElement(CommandDataElement::Parser & aCommandElement) = 0;
 
 protected:
     CHIP_ERROR ClearExistingExchangeContext();
     void MoveToState(const CommandState aTargetState);
     CHIP_ERROR ProcessCommandMessage(System::PacketBufferHandle && payload, CommandRoleId aCommandRoleId);
-    CHIP_ERROR ConstructCommandPath(const CommandParams & aCommandParams, CommandDataElement::Builder & aCommandDataElement);
+    CHIP_ERROR ConstructCommandPath(const CommandParams & aCommandParams, CommandDataElement::Builder aCommandDataElement);
     void ClearState();
     const char * GetStateStr() const;
 
@@ -161,12 +160,9 @@ class Command
 
 private:
     friend class TestCommandInteraction;
-    chip::System::PacketBufferHandle mpBufHandle;
-    CommandState mState;
-
-    chip::System::PacketBufferHandle mCommandDataBuf;
+    CommandState mState                    = CommandState::Uninitialized;
+    TLV::TLVType mDataElementContainerType = TLV::kTLVType_NotSpecified;
     chip::System::PacketBufferTLVWriter mCommandMessageWriter;
-    chip::System::PacketBufferTLVWriter mCommandDataWriter;
 };
 } // namespace app
 } // namespace chip

src/app/CommandHandler.cpp

@@ -120,25 +120,18 @@ CHIP_ERROR CommandHandler::AddStatusCode(const CommandParams * apCommandParams,
 {
     CHIP_ERROR err = CHIP_NO_ERROR;
     StatusElement::Builder statusElementBuilder;
-    CommandDataElement::Builder commandDataElement =
-        mInvokeCommandBuilder.GetCommandListBuilder().CreateCommandDataElementBuilder();
 
-    if (apCommandParams != nullptr)
-    {
-        err = ConstructCommandPath(*apCommandParams, commandDataElement);
-        SuccessOrExit(err);
-    }
+    err = PrepareCommand(apCommandParams);
+    SuccessOrExit(err);
 
-    statusElementBuilder = commandDataElement.CreateStatusElementBuilder();
+    statusElementBuilder =
+        mInvokeCommandBuilder.GetCommandListBuilder().GetCommandDataElementBuilder().CreateStatusElementBuilder();
     statusElementBuilder.EncodeStatusElement(aGeneralCode, aProtocolId.ToFullyQualifiedSpecForm(), aProtocolCode)
         .EndOfStatusElement();
     err = statusElementBuilder.GetError();
     SuccessOrExit(err);
 
-    commandDataElement.EndOfCommandDataElement();
-    err = commandDataElement.GetError();
-    SuccessOrExit(err);
-    MoveToState(CommandState::AddCommand);
+    err = FinishCommand();
 
 exit:
     ChipLogFunctError(err);

src/app/MessageDef/CommandList.h

@@ -63,10 +63,15 @@ class Builder : public ListBuilder
     /**
      *  @brief Initialize a CommandDataElement::Builder for writing into the TLV stream
      *
-     *  @return A reference to AttributeDataList::Builder
+     *  @return A reference to CommandDataElement::Builder
      */
     CommandDataElement::Builder & CreateCommandDataElementBuilder();
 
+    /**
+     *  @return A reference to CommandDataElement::Builder
+     */
+    CommandDataElement::Builder & GetCommandDataElementBuilder() { return mCommandDataElementBuilder; };
+
     /**
      *  @brief Mark the end of this CommandList
      *

src/app/tests/TestCommandInteraction.cpp

@@ -146,21 +146,16 @@ void TestCommandInteraction::AddCommandDataElement(nlTestSuite * apSuite, void *
     }
     else
     {
-        chip::TLV::TLVType dummyType = chip::TLV::kTLVType_NotSpecified;
-        chip::TLV::TLVWriter writer  = apCommand->CreateCommandDataElementTLVWriter();
-        err                          = writer.StartContainer(chip::TLV::AnonymousTag, chip::TLV::kTLVType_Structure, dummyType);
+        err = apCommand->PrepareCommand(&commandParams);
         NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
 
-        err = writer.PutBoolean(chip::TLV::ContextTag(1), true);
-        NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
+        chip::TLV::TLVWriter * writer = apCommand->GetCommandDataElementTLVWriter();
 
-        err = writer.EndContainer(dummyType);
+        err = writer->PutBoolean(chip::TLV::ContextTag(1), true);
         NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
 
-        err = writer.Finalize();
+        err = apCommand->FinishCommand();
         NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
-
-        apCommand->AddCommand(commandParams);
     }
 }
 
@@ -198,9 +193,12 @@ void TestCommandInteraction::TestCommandHandlerWithSendEmptyCommand(nlTestSuite
     NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
 
     commandHandler.mpExchangeCtx = gExchangeManager.NewContext({ 0, 0, 0 }, nullptr);
+
     TestExchangeDelegate delegate;
     commandHandler.mpExchangeCtx->SetDelegate(&delegate);
-    err = commandHandler.AddCommand(commandParams);
+    err                          = commandHandler.PrepareCommand(&commandParams);
+    NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
+    err = commandHandler.FinishCommand();
     NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
     err = commandHandler.SendCommandResponse();
     NL_TEST_ASSERT(apSuite, err == CHIP_ERROR_NOT_CONNECTED);

src/app/tests/integration/chip_im_initiator.cpp

@@ -86,7 +86,7 @@ CHIP_ERROR SendCommandRequest(void)
 
     printf("\nSend invoke command request message to Node: %" PRIu64 "\n", chip::kTestDeviceNodeId);
 
-    chip::app::Command::CommandParams CommandParams = { kTestEndPointId, // Endpoint
+    chip::app::Command::CommandParams commandParams = { kTestEndPointId, // Endpoint
                                                         kTestGroupId,    // GroupId
                                                         kTestClusterId,  // ClusterId
                                                         kTestCommandId,  // CommandId
@@ -96,27 +96,19 @@ CHIP_ERROR SendCommandRequest(void)
 
     uint8_t effectIdentifier = 1; // Dying light
     uint8_t effectVariant    = 1;
+    chip::TLV::TLVWriter * writer;
 
-    chip::TLV::TLVType dummyType = chip::TLV::kTLVType_NotSpecified;
-
-    chip::TLV::TLVWriter writer = gpCommandSender->CreateCommandDataElementTLVWriter();
-
-    err = writer.StartContainer(chip::TLV::AnonymousTag, chip::TLV::kTLVType_Structure, dummyType);
-    SuccessOrExit(err);
-
-    err = writer.Put(chip::TLV::ContextTag(1), effectIdentifier);
-    SuccessOrExit(err);
-
-    err = writer.Put(chip::TLV::ContextTag(2), effectVariant);
+    err = gpCommandSender->PrepareCommand(&commandParams);
     SuccessOrExit(err);
 
-    err = writer.EndContainer(dummyType);
+    writer = gpCommandSender->GetCommandDataElementTLVWriter();
+    err    = writer->Put(chip::TLV::ContextTag(1), effectIdentifier);
     SuccessOrExit(err);
 
-    err = writer.Finalize();
+    err = writer->Put(chip::TLV::ContextTag(2), effectVariant);
     SuccessOrExit(err);
 
-    err = gpCommandSender->AddCommand(CommandParams);
+    err = gpCommandSender->FinishCommand();
     SuccessOrExit(err);
 
     err = gpCommandSender->SendCommandRequest(chip::kTestDeviceNodeId, gAdminId);

src/app/tests/integration/chip_im_responder.cpp

@@ -68,10 +68,6 @@ void DispatchSingleClusterCommand(chip::ClusterId aClusterId, chip::CommandId aC
     uint8_t effectIdentifier = 1; // Dying light
     uint8_t effectVariant    = 1;
 
-    chip::TLV::TLVType dummyType = chip::TLV::kTLVType_NotSpecified;
-
-    chip::TLV::TLVWriter writer = apCommandObj->CreateCommandDataElementTLVWriter();
-
     if (statusCodeFlipper)
     {
         printf("responder constructing status code in command");
@@ -81,22 +77,20 @@ void DispatchSingleClusterCommand(chip::ClusterId aClusterId, chip::CommandId aC
     else
     {
         printf("responder constructing command data in command");
-        err = writer.StartContainer(chip::TLV::AnonymousTag, chip::TLV::kTLVType_Structure, dummyType);
-        SuccessOrExit(err);
 
-        err = writer.Put(chip::TLV::ContextTag(1), effectIdentifier);
-        SuccessOrExit(err);
+        chip::TLV::TLVWriter * writer;
 
-        err = writer.Put(chip::TLV::ContextTag(2), effectVariant);
+        err = apCommandObj->PrepareCommand(&commandParams);
         SuccessOrExit(err);
 
-        err = writer.EndContainer(dummyType);
+        writer = apCommandObj->GetCommandDataElementTLVWriter();
+        err    = writer->Put(chip::TLV::ContextTag(1), effectIdentifier);
         SuccessOrExit(err);
 
-        err = writer.Finalize();
+        err = writer->Put(chip::TLV::ContextTag(2), effectVariant);
         SuccessOrExit(err);
 
-        err = apCommandObj->AddCommand(commandParams);
+        err = apCommandObj->FinishCommand();
         SuccessOrExit(err);
     }
     statusCodeFlipper = !statusCodeFlipper;