Cleanup status element in IM (project-chip#5721)

-- Remove extra protocolcode typo in EncodeStatusElement(aGeneralCode, aProtocolId.ToFullyQualifiedSpecForm(), aProtocolCode, aProtocolCode) -- Replace aGeneralCode with chip::Protocols::SecureChannel::GeneralStatusCode. -- Remove extra status code insertion in command sender since status code is only present for command handler. fixed project-chip#5494 project-chip#5547 project-chip#5756
jimlyall-q · Apr 12, 2021 · 9462bee · 9462bee
1 parent 112ef7c
commit 9462bee
Show file tree

Hide file tree

Showing 9 changed files with 41 additions and 50 deletions.
diff --git a/src/app/Command.cpp b/src/app/Command.cpp
@@ -225,16 +225,16 @@ CHIP_ERROR Command::AddCommand(CommandParams & aCommandParams)
     return err;
 }
 
-CHIP_ERROR Command::AddStatusCode(const uint16_t aGeneralCode, Protocols::Id aProtocolId, const uint16_t aProtocolCode,
-                                  const chip::ClusterId aClusterId)
+CHIP_ERROR Command::AddStatusCode(const Protocols::SecureChannel::GeneralStatusCode aGeneralCode, Protocols::Id aProtocolId,
+                                  const uint16_t aProtocolCode)
 {
     CHIP_ERROR err = CHIP_NO_ERROR;
     StatusElement::Builder statusElementBuilder;
 
     err = statusElementBuilder.Init(mInvokeCommandBuilder.GetWriter());
     SuccessOrExit(err);
 
-    statusElementBuilder.EncodeStatusElement(aGeneralCode, aProtocolId.ToFullyQualifiedSpecForm(), aProtocolCode, aProtocolCode)
+    statusElementBuilder.EncodeStatusElement(aGeneralCode, aProtocolId.ToFullyQualifiedSpecForm(), aProtocolCode)
         .EndOfStatusElement();
     err = statusElementBuilder.GetError();
 

diff --git a/src/app/Command.h b/src/app/Command.h
@@ -121,8 +121,8 @@ class Command
     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 AddStatusCode(const uint16_t aGeneralCode, Protocols::Id aProtocolId, const uint16_t aProtocolCode,
-                             const chip::ClusterId aClusterId);
+    CHIP_ERROR AddStatusCode(const Protocols::SecureChannel::GeneralStatusCode aGeneralCode, Protocols::Id aProtocolId,
+                             const uint16_t aProtocolCode);
 
     /**
      * Gets the inner exchange context object, without ownership.

diff --git a/src/app/CommandHandler.cpp b/src/app/CommandHandler.cpp
@@ -93,8 +93,7 @@ CHIP_ERROR CommandHandler::ProcessCommandDataElement(CommandDataElement::Parser
         // Empty Command, Add status code in invoke command response, notify cluster handler to hand it further.
         err = CHIP_NO_ERROR;
         ChipLogDetail(DataManagement, "Add Status code for empty command, cluster Id is %d", clusterId);
-        AddStatusCode(static_cast<uint16_t>(GeneralStatusCode::kSuccess), Protocols::SecureChannel::Id,
-                      Protocols::SecureChannel::kProtocolCodeSuccess, clusterId);
+        AddStatusCode(GeneralStatusCode::kSuccess, Protocols::SecureChannel::Id, Protocols::SecureChannel::kProtocolCodeSuccess);
     }
     else if (CHIP_NO_ERROR == err)
     {

diff --git a/src/app/CommandSender.cpp b/src/app/CommandSender.cpp
@@ -118,9 +118,9 @@ CHIP_ERROR CommandSender::ProcessCommandDataElement(CommandDataElement::Parser &
     chip::ClusterId clusterId;
     chip::CommandId commandId;
     chip::EndpointId endpointId;
-    uint16_t generalCode  = 0;
-    uint32_t protocolId   = 0;
-    uint16_t protocolCode = 0;
+    Protocols::SecureChannel::GeneralStatusCode generalCode = Protocols::SecureChannel::GeneralStatusCode::kSuccess;
+    uint32_t protocolId                                     = 0;
+    uint16_t protocolCode                                   = 0;
     StatusElement::Parser statusElementParser;
 
     mCommandIndex++;
@@ -144,7 +144,7 @@ CHIP_ERROR CommandSender::ProcessCommandDataElement(CommandDataElement::Parser &
         err = statusElementParser.CheckSchemaValidity();
         SuccessOrExit(err);
 
-        err = statusElementParser.DecodeStatusElement(&generalCode, &protocolId, &protocolCode, &clusterId);
+        err = statusElementParser.DecodeStatusElement(&generalCode, &protocolId, &protocolCode);
         SuccessOrExit(err);
         if (mpDelegate != nullptr)
         {
@@ -155,13 +155,7 @@ CHIP_ERROR CommandSender::ProcessCommandDataElement(CommandDataElement::Parser &
     else if (CHIP_END_OF_TLV == err)
     {
         err = aCommandElement.GetData(&commandDataReader);
-        if (CHIP_END_OF_TLV == err)
-        {
-            err = CHIP_NO_ERROR;
-            ChipLogDetail(DataManagement, "Add Status code for empty command, cluster Id is %d", clusterId);
-            AddStatusCode(static_cast<uint16_t>(GeneralStatusCode::kSuccess), Protocols::SecureChannel::Id,
-                          Protocols::SecureChannel::kProtocolCodeSuccess, clusterId);
-        }
+        SuccessOrExit(err);
         // TODO(#4503): Should call callbacks of cluster that sends the command.
         DispatchSingleClusterCommand(clusterId, commandId, endpointId, commandDataReader, this);
     }

diff --git a/src/app/InteractionModelDelegate.h b/src/app/InteractionModelDelegate.h
@@ -90,7 +90,8 @@ class InteractionModelDelegate
      * same command Id
      * @retval # CHIP_ERROR_NOT_IMPLEMENTED if not implemented
      */
-    virtual CHIP_ERROR CommandResponseStatus(const CommandSender * apCommandSender, const uint16_t aGeneralCode,
+    virtual CHIP_ERROR CommandResponseStatus(const CommandSender * apCommandSender,
+                                             const Protocols::SecureChannel::GeneralStatusCode aGeneralCode,
                                              const uint32_t aProtocolId, const uint16_t aProtocolCode, chip::EndpointId aEndpointId,
                                              const chip::ClusterId aClusterId, chip::CommandId aCommandId, uint8_t aCommandIndex)
     {

diff --git a/src/app/MessageDef/StatusElement.cpp b/src/app/MessageDef/StatusElement.cpp
@@ -51,19 +51,21 @@ CHIP_ERROR StatusElement::Parser::Init(const chip::TLV::TLVReader & aReader)
     return err;
 }
 
-CHIP_ERROR StatusElement::Parser::DecodeStatusElement(uint16_t * apGeneralCode, uint32_t * apProtocolId, uint16_t * apProtocolCode,
-                                                      chip::ClusterId * apClusterId) const
+CHIP_ERROR StatusElement::Parser::DecodeStatusElement(Protocols::SecureChannel::GeneralStatusCode * apGeneralCode,
+                                                      uint32_t * apProtocolId, uint16_t * apProtocolCode) const
 {
     CHIP_ERROR err = CHIP_NO_ERROR;
     chip::TLV::TLVReader lReader;
 
+    uint16_t generalCode;
     lReader.Init(mReader);
 
     err = lReader.Next();
     SuccessOrExit(err);
     VerifyOrExit(lReader.GetType() == chip::TLV::kTLVType_UnsignedInteger, err = CHIP_ERROR_WRONG_TLV_TYPE);
-    err = lReader.Get(*apGeneralCode);
+    err = lReader.Get(generalCode);
     SuccessOrExit(err);
+    *apGeneralCode = static_cast<Protocols::SecureChannel::GeneralStatusCode>(generalCode);
 
     err = lReader.Next();
     SuccessOrExit(err);
@@ -77,12 +79,6 @@ CHIP_ERROR StatusElement::Parser::DecodeStatusElement(uint16_t * apGeneralCode,
     err = lReader.Get(*apProtocolCode);
     SuccessOrExit(err);
 
-    err = lReader.Next();
-    SuccessOrExit(err);
-    VerifyOrExit(lReader.GetType() == chip::TLV::kTLVType_UnsignedInteger, err = CHIP_ERROR_WRONG_TLV_TYPE);
-    err = lReader.Get(*apClusterId);
-    SuccessOrExit(err);
-
 exit:
     ChipLogFunctError(err);
     return err;
@@ -208,24 +204,20 @@ CHIP_ERROR StatusElement::Builder::Init(chip::TLV::TLVWriter * const apWriter, c
     return ListBuilder::Init(apWriter, aContextTagToUse);
 }
 
-StatusElement::Builder & StatusElement::Builder::EncodeStatusElement(const uint16_t aGeneralCode, const uint32_t aProtocolId,
-                                                                     const uint16_t aStatusElement,
-                                                                     const chip::ClusterId aClusterId)
+StatusElement::Builder & StatusElement::Builder::EncodeStatusElement(const Protocols::SecureChannel::GeneralStatusCode aGeneralCode,
+                                                                     const uint32_t aProtocolId, const uint16_t aProtocolCode)
 {
     uint64_t tag = chip::TLV::AnonymousTag;
 
     SuccessOrExit(mError);
 
-    mError = mpWriter->Put(tag, aGeneralCode);
+    mError = mpWriter->Put(tag, static_cast<uint16_t>(aGeneralCode));
     SuccessOrExit(mError);
 
     mError = mpWriter->Put(tag, aProtocolId);
     SuccessOrExit(mError);
 
-    mError = mpWriter->Put(tag, aStatusElement);
-    SuccessOrExit(mError);
-
-    mError = mpWriter->Put(tag, aClusterId);
+    mError = mpWriter->Put(tag, aProtocolCode);
     SuccessOrExit(mError);
 
 exit:

diff --git a/src/app/MessageDef/StatusElement.h b/src/app/MessageDef/StatusElement.h
@@ -31,6 +31,7 @@
 
 #include <core/CHIPCore.h>
 #include <core/CHIPTLV.h>
+#include <protocols/secure_channel/Constants.h>
 #include <support/CodeUtils.h>
 #include <support/logging/CHIPLogging.h>
 #include <util/basic-types.h>
@@ -85,8 +86,8 @@ class Parser : public ListParser
      *               element is missing. CHIP_ERROR_WRONG_TLV_TYPE if the elements are of the wrong
      *               type.
      */
-    CHIP_ERROR DecodeStatusElement(uint16_t * apGeneralCode, uint32_t * apProtocolId, uint16_t * apProtocolCode,
-                                   chip::ClusterId * apClusterId) const;
+    CHIP_ERROR DecodeStatusElement(Protocols::SecureChannel::GeneralStatusCode * apGeneralCode, uint32_t * apProtocolId,
+                                   uint16_t * apProtocolCode) const;
 };
 
 class Builder : public ListBuilder
@@ -118,14 +119,13 @@ class Builder : public ListBuilder
      * @param[in]   aGeneralCode    General status code
      * @param[in]   aProtocolId     A protocol ID (32-bit integer composed of a 16-bit vendor id and 16-bit Scoped id)
      * @param[in]   aProtocolCode   16-bit protocol-specific error code
-     * @param[in]   aClusterId      Cluster Id for ZCL
      *
      * @return       CHIP_ERROR codes returned by chip::TLV objects. CHIP_END_OF_TLV if either
      *               element is missing. CHIP_ERROR_WRONG_TLV_TYPE if the elements are of the wrong
      *               type.
      */
-    StatusElement::Builder & EncodeStatusElement(const uint16_t aGeneralCode, const uint32_t aProtocolId,
-                                                 const uint16_t aProtocolCode, const chip::ClusterId aClusterId);
+    StatusElement::Builder & EncodeStatusElement(const Protocols::SecureChannel::GeneralStatusCode aGeneralCode,
+                                                 const uint32_t aProtocolId, const uint16_t aProtocolCode);
 
     /**
      *  @brief Mark the end of this StatusElement

diff --git a/src/app/tests/TestMessageDef.cpp b/src/app/tests/TestMessageDef.cpp
@@ -324,7 +324,8 @@ void BuildStatusElement(nlTestSuite * apSuite, StatusElement::Builder & aStatusE
 {
     CHIP_ERROR err = CHIP_NO_ERROR;
 
-    aStatusElementBuilder.EncodeStatusElement(1, 2, 3, 4).EndOfStatusElement();
+    aStatusElementBuilder.EncodeStatusElement(chip::Protocols::SecureChannel::GeneralStatusCode::kFailure, 2, 3)
+        .EndOfStatusElement();
     err = aStatusElementBuilder.GetError();
     NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
 }
@@ -334,16 +335,19 @@ void ParseStatusElement(nlTestSuite * apSuite, StatusElement::Parser & aStatusEl
     CHIP_ERROR err = CHIP_NO_ERROR;
     StatusElement::Parser StatusElementParser;
 
-    uint16_t generalCode      = 0;
-    uint32_t protocolId       = 0;
-    uint16_t protocolCode     = 0;
-    chip::ClusterId clusterId = 0;
+    chip::Protocols::SecureChannel::GeneralStatusCode generalCode = chip::Protocols::SecureChannel::GeneralStatusCode::kFailure;
+    uint32_t protocolId                                           = 0;
+    uint16_t protocolCode                                         = 0;
 
     err = aStatusElementParser.CheckSchemaValidity();
     NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
 
-    err = aStatusElementParser.DecodeStatusElement(&generalCode, &protocolId, &protocolCode, &clusterId);
-    NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR && generalCode == 1 && protocolId == 2 && protocolCode == 3 && clusterId == 4);
+    err = aStatusElementParser.DecodeStatusElement(&generalCode, &protocolId, &protocolCode);
+    NL_TEST_ASSERT(apSuite,
+                   err == CHIP_NO_ERROR &&
+                       static_cast<uint16_t>(generalCode) ==
+                           static_cast<uint16_t>(chip::Protocols::SecureChannel::GeneralStatusCode::kFailure) &&
+                       protocolId == 2 && protocolCode == 3);
 }
 
 void BuildAttributeStatusElement(nlTestSuite * apSuite, AttributeStatusElement::Builder & aAttributeStatusElementBuilder)

diff --git a/src/app/tests/integration/chip_im_initiator.cpp b/src/app/tests/integration/chip_im_initiator.cpp
@@ -229,14 +229,15 @@ class MockInteractionModelApp : public chip::app::InteractionModelDelegate
         printf("ReportError with err %d", aError);
         return CHIP_NO_ERROR;
     }
-    CHIP_ERROR CommandResponseStatus(const chip::app::CommandSender * apCommandSender, const uint16_t aGeneralCode,
+    CHIP_ERROR CommandResponseStatus(const chip::app::CommandSender * apCommandSender,
+                                     const chip::Protocols::SecureChannel::GeneralStatusCode aGeneralCode,
                                      const uint32_t aProtocolId, const uint16_t aProtocolCode, const chip::EndpointId aEndpointId,
                                      const chip::ClusterId aClusterId, const chip::CommandId aCommandId,
                                      uint8_t aCommandIndex) override
     {
         printf("CommandResponseStatus with GeneralCode %d, ProtocolId %d, ProtocolCode %d, EndpointId %d, ClusterId %d, CommandId "
                "%d, CommandIndex %d",
-               aGeneralCode, aProtocolId, aProtocolCode, aEndpointId, aClusterId, aCommandId, aCommandIndex);
+               static_cast<uint16_t>(aGeneralCode), aProtocolId, aProtocolCode, aEndpointId, aClusterId, aCommandId, aCommandIndex);
         return CHIP_NO_ERROR;
     }