From 783dc0ff2d19f09a2275e77f05a961b527b551bb Mon Sep 17 00:00:00 2001 From: yunhanw Date: Tue, 30 Mar 2021 22:57:57 -0700 Subject: [PATCH] Cleanup status element in IM -- 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 #5494 #5547 #5756 --- src/app/Command.cpp | 6 ++--- src/app/Command.h | 4 +-- src/app/CommandHandler.cpp | 3 +-- src/app/CommandSender.cpp | 16 ++++-------- src/app/InteractionModelDelegate.h | 3 ++- src/app/MessageDef/StatusElement.cpp | 26 +++++++------------ src/app/MessageDef/StatusElement.h | 10 +++---- src/app/tests/TestMessageDef.cpp | 18 ++++++++----- .../tests/integration/chip_im_initiator.cpp | 5 ++-- 9 files changed, 41 insertions(+), 50 deletions(-) diff --git a/src/app/Command.cpp b/src/app/Command.cpp index d044c711e6c7f8..fd624a65c24ca6 100644 --- a/src/app/Command.cpp +++ b/src/app/Command.cpp @@ -225,8 +225,8 @@ 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; @@ -234,7 +234,7 @@ CHIP_ERROR Command::AddStatusCode(const uint16_t aGeneralCode, Protocols::Id aPr 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 index 5a92b7e0df2b50..e1ef21afb750ca 100644 --- 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 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 index e7114d4d23f2e8..1ed06b20c708bc 100644 --- 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(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 index 87d26c8852bad8..c5b48e44559b11 100644 --- 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(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 index 2e2a48d90aa8b2..280afabe5da9b5 100644 --- 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 index 7476f1a18a3bb3..fe89afb71b8d0e 100644 --- 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(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(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 index e74a59165c9dc1..c23c689ed53d88 100644 --- a/src/app/MessageDef/StatusElement.h +++ b/src/app/MessageDef/StatusElement.h @@ -31,6 +31,7 @@ #include #include +#include #include #include #include @@ -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 index 4a8cf2b39bac77..f0c46804ee64d5 100644 --- 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(generalCode) == + static_cast(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 index 7f88ed30f46bf6..4df145ddc4da1f 100644 --- 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(aGeneralCode), aProtocolId, aProtocolCode, aEndpointId, aClusterId, aCommandId, aCommandIndex); return CHIP_NO_ERROR; }