Skip to content

Commit

Permalink
Cleanup status element in IM (#5721)
Browse files Browse the repository at this point in the history 
-- 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
  • Loading branch information
@yunhanw-google
yunhanw-google authored Apr 2, 2021
1 parent f3ec8e6 commit ea6dbf6
Show file tree
Hide file tree
Showing 9 changed files with 41 additions and 50 deletions.
6 changes: 3 additions & 3 deletions src/app/Command.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down
4 changes: 2 additions & 2 deletions src/app/Command.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
3 changes: 1 addition & 2 deletions src/app/CommandHandler.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down
16 changes: 5 additions & 11 deletions src/app/CommandSender.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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++;
Expand All @@ -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)
{
Expand All @@ -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);
}
Expand Down
3 changes: 2 additions & 1 deletion src/app/InteractionModelDelegate.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down
26 changes: 9 additions & 17 deletions src/app/MessageDef/StatusElement.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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;
Expand Down Expand Up @@ -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:
Expand Down
10 changes: 5 additions & 5 deletions src/app/MessageDef/StatusElement.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
18 changes: 11 additions & 7 deletions src/app/tests/TestMessageDef.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand All @@ -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)
Expand Down
5 changes: 3 additions & 2 deletions src/app/tests/integration/chip_im_initiator.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down

0 comments on commit ea6dbf6

Please sign in to comment.