Skip to content

Commit

Permalink
Cleanup IM (#12606)
Browse files Browse the repository at this point in the history
--Move ConstructCommandPath function into CommandPathIB, update all usage across the codes.
--Use reference delcartion for usages on all Create* functions from IM MessageDef, update all usage across the code.
--Update the missing error-check for IM messageDef.
  • Loading branch information
yunhanw-google authored and pull[bot] committed Jan 21, 2022
1 parent 5938166 commit 1078608
Show file tree
Hide file tree
Showing 38 changed files with 450 additions and 477 deletions.
24 changes: 12 additions & 12 deletions src/app/AttributeAccessInterface.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -24,16 +24,16 @@ namespace app {
CHIP_ERROR AttributeReportBuilder::PrepareAttribute(AttributeReportIBs::Builder & aAttributeReportIBsBuilder,
const ConcreteDataAttributePath & aPath, DataVersion aDataVersion)
{
mAttributeReportIBBuilder = aAttributeReportIBsBuilder.CreateAttributeReport();
AttributeReportIB::Builder & attributeReportIBBuilder = aAttributeReportIBsBuilder.CreateAttributeReport();
ReturnErrorOnFailure(aAttributeReportIBsBuilder.GetError());

mAttributeDataIBBuilder = mAttributeReportIBBuilder.CreateAttributeData();
ReturnErrorOnFailure(mAttributeReportIBBuilder.GetError());
AttributeDataIB::Builder & attributeDataIBBuilder = attributeReportIBBuilder.CreateAttributeData();
ReturnErrorOnFailure(attributeReportIBBuilder.GetError());

mAttributeDataIBBuilder.DataVersion(aDataVersion);
attributeDataIBBuilder.DataVersion(aDataVersion);

auto attributePathIBBuilder = mAttributeDataIBBuilder.CreatePath();
ReturnErrorOnFailure(mAttributeDataIBBuilder.GetError());
AttributePathIB::Builder & attributePathIBBuilder = attributeDataIBBuilder.CreatePath();
ReturnErrorOnFailure(attributeDataIBBuilder.GetError());

attributePathIBBuilder.Endpoint(aPath.mEndpointId).Cluster(aPath.mClusterId).Attribute(aPath.mAttributeId);

Expand All @@ -46,13 +46,13 @@ CHIP_ERROR AttributeReportBuilder::PrepareAttribute(AttributeReportIBs::Builder

ReturnErrorOnFailure(attributePathIBBuilder.EndOfAttributePathIB().GetError());

return mAttributeDataIBBuilder.GetError();
return attributeDataIBBuilder.GetError();
}

CHIP_ERROR AttributeReportBuilder::FinishAttribute()
CHIP_ERROR AttributeReportBuilder::FinishAttribute(AttributeReportIBs::Builder & aAttributeReportIBsBuilder)
{
ReturnErrorOnFailure(mAttributeDataIBBuilder.EndOfAttributeDataIB().GetError());
return mAttributeReportIBBuilder.EndOfAttributeReportIB().GetError();
ReturnErrorOnFailure(aAttributeReportIBsBuilder.GetAttributeReport().GetAttributeData().EndOfAttributeDataIB().GetError());
return aAttributeReportIBsBuilder.GetAttributeReport().EndOfAttributeReportIB().GetError();
}

CHIP_ERROR AttributeValueEncoder::EncodeEmptyList()
Expand All @@ -70,9 +70,9 @@ CHIP_ERROR AttributeValueEncoder::EncodeEmptyList()
AttributeReportBuilder builder;

ReturnErrorOnFailure(builder.PrepareAttribute(mAttributeReportIBsBuilder, mPath, mDataVersion));
ReturnErrorOnFailure(builder.EncodeValue(DataModel::List<uint8_t>()));
ReturnErrorOnFailure(builder.EncodeValue(mAttributeReportIBsBuilder, DataModel::List<uint8_t>()));

ReturnErrorOnFailure(builder.FinishAttribute());
ReturnErrorOnFailure(builder.FinishAttribute(mAttributeReportIBsBuilder));
mEncodeState.mCurrentEncodingListIndex = 0;
}
mCurrentEncodingListIndex = 0;
Expand Down
16 changes: 6 additions & 10 deletions src/app/AttributeAccessInterface.h
Original file line number Diff line number Diff line change
Expand Up @@ -68,21 +68,17 @@ class AttributeReportBuilder
/**
* FinishAttribute encodes the "footer" part of an attribute report (it closes the containers opened in PrepareAttribute)
*/
CHIP_ERROR FinishAttribute();
CHIP_ERROR FinishAttribute(AttributeReportIBs::Builder & aAttributeReportIBs);

/**
* EncodeValue encodes the value field of the report, it should be called exactly once.
*/
template <typename... Ts>
CHIP_ERROR EncodeValue(Ts... aArgs)
CHIP_ERROR EncodeValue(AttributeReportIBs::Builder & aAttributeReportIBs, Ts... aArgs)
{
return DataModel::Encode(*mAttributeDataIBBuilder.GetWriter(), TLV::ContextTag(to_underlying(AttributeDataIB::Tag::kData)),
std::forward<Ts>(aArgs)...);
return DataModel::Encode(*(aAttributeReportIBs.GetAttributeReport().GetAttributeData().GetWriter()),
TLV::ContextTag(to_underlying(AttributeDataIB::Tag::kData)), std::forward<Ts>(aArgs)...);
}

private:
AttributeReportIB::Builder mAttributeReportIBBuilder;
AttributeDataIB::Builder mAttributeDataIBBuilder;
};

/**
Expand Down Expand Up @@ -252,9 +248,9 @@ class AttributeValueEncoder
AttributeReportBuilder builder;

ReturnErrorOnFailure(builder.PrepareAttribute(mAttributeReportIBsBuilder, mPath, mDataVersion));
ReturnErrorOnFailure(builder.EncodeValue(std::forward<Ts>(aArgs)...));
ReturnErrorOnFailure(builder.EncodeValue(mAttributeReportIBsBuilder, std::forward<Ts>(aArgs)...));

return builder.FinishAttribute();
return builder.FinishAttribute(mAttributeReportIBsBuilder);
}

/**
Expand Down
11 changes: 0 additions & 11 deletions src/app/Command.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -37,17 +37,6 @@ CHIP_ERROR Command::Finalize(System::PacketBufferHandle & commandPacket)
return mCommandMessageWriter.Finalize(&commandPacket);
}

CHIP_ERROR Command::ConstructCommandPath(const CommandPathParams & aCommandPathParams, CommandPathIB::Builder & aCommandPath)
{
if (aCommandPathParams.mFlags.Has(CommandPathFlags::kEndpointIdValid))
{
aCommandPath.EndpointId(aCommandPathParams.mEndpointId);
}

aCommandPath.ClusterId(aCommandPathParams.mClusterId).CommandId(aCommandPathParams.mCommandId).EndOfCommandPathIB();
return aCommandPath.GetError();
}

void Command::Abort()
{
//
Expand Down
1 change: 0 additions & 1 deletion src/app/Command.h
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,6 @@ class Command
void Close();

void MoveToState(const CommandState aTargetState);
CHIP_ERROR ConstructCommandPath(const CommandPathParams & aCommandPathParams, CommandPathIB::Builder & aCommandPath);
const char * GetStateStr() const;

Messaging::ExchangeContext * mpExchangeCtx = nullptr;
Expand Down
47 changes: 22 additions & 25 deletions src/app/CommandHandler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -295,17 +295,11 @@ CHIP_ERROR CommandHandler::AddStatusInternal(const ConcreteCommandPath & aComman
const Protocols::InteractionModel::Status aStatus,
const Optional<ClusterStatus> & aClusterStatus)
{
StatusIB::Builder statusIBBuilder;
StatusIB statusIB;

CommandPathParams commandPathParams = { aCommandPath.mEndpointId,
0, // GroupId
aCommandPath.mClusterId, aCommandPath.mCommandId,
chip::app::CommandPathFlags::kEndpointIdValid };

ReturnLogErrorOnFailure(PrepareStatus(commandPathParams));
statusIBBuilder = mInvokeResponseBuilder.GetInvokeResponses().GetInvokeResponse().GetStatus().CreateErrorStatus();

ReturnLogErrorOnFailure(PrepareStatus(aCommandPath));
CommandStatusIB::Builder & commandStatus = mInvokeResponseBuilder.GetInvokeResponses().GetInvokeResponse().GetStatus();
StatusIB::Builder & statusIBBuilder = commandStatus.CreateErrorStatus();
ReturnErrorOnFailure(commandStatus.GetError());
//
// TODO: Most of the callers are incorrectly passing SecureChannel as the protocol ID, when in fact, the status code provided
// above is always an IM code. Instead of fixing all the callers (which is a fairly sizeable change), we'll embark on fixing
Expand All @@ -314,7 +308,7 @@ CHIP_ERROR CommandHandler::AddStatusInternal(const ConcreteCommandPath & aComman
statusIB.mStatus = aStatus;
statusIB.mClusterStatus = aClusterStatus;
statusIBBuilder.EncodeStatusIB(statusIB);
ReturnLogErrorOnFailure(statusIBBuilder.GetError());
ReturnErrorOnFailure(statusIBBuilder.GetError());
return FinishStatus();
}

Expand All @@ -336,24 +330,22 @@ CHIP_ERROR CommandHandler::AddClusterSpecificFailure(const ConcreteCommandPath &
return AddStatusInternal(aCommandPath, Protocols::InteractionModel::Status::Failure, clusterStatus);
}

CHIP_ERROR CommandHandler::PrepareResponse(const ConcreteCommandPath & aRequestCommandPath, CommandId aResponseCommand)
{
CommandPathParams params = { aRequestCommandPath.mEndpointId,
0, // GroupId
aRequestCommandPath.mClusterId, aResponseCommand, (CommandPathFlags::kEndpointIdValid) };
return PrepareCommand(params, false /* aStartDataStruct */);
}

CHIP_ERROR CommandHandler::PrepareCommand(const CommandPathParams & aCommandPathParams, bool aStartDataStruct)
CHIP_ERROR CommandHandler::PrepareCommand(const ConcreteCommandPath & aCommandPath, bool aStartDataStruct)
{
ReturnErrorOnFailure(AllocateBuffer());
//
// We must not be in the middle of preparing a command, or having prepared or sent one.
//
VerifyOrReturnError(mState == CommandState::Idle, CHIP_ERROR_INCORRECT_STATE);
CommandDataIB::Builder commandData = mInvokeResponseBuilder.GetInvokeResponses().CreateInvokeResponse().CreateCommand();
InvokeResponseIBs::Builder & invokeResponses = mInvokeResponseBuilder.GetInvokeResponses();
InvokeResponseIB::Builder & invokeResponse = invokeResponses.CreateInvokeResponse();
ReturnErrorOnFailure(invokeResponses.GetError());

CommandDataIB::Builder & commandData = invokeResponse.CreateCommand();
ReturnErrorOnFailure(commandData.GetError());
ReturnErrorOnFailure(ConstructCommandPath(aCommandPathParams, commandData.CreatePath()));
CommandPathIB::Builder & path = commandData.CreatePath();
ReturnErrorOnFailure(commandData.GetError());
ReturnErrorOnFailure(path.Encode(aCommandPath));
if (aStartDataStruct)
{
ReturnErrorOnFailure(commandData.GetWriter()->StartContainer(TLV::ContextTag(to_underlying(CommandDataIB::Tag::kData)),
Expand All @@ -379,16 +371,21 @@ CHIP_ERROR CommandHandler::FinishCommand(bool aStartDataStruct)
return CHIP_NO_ERROR;
}

CHIP_ERROR CommandHandler::PrepareStatus(const CommandPathParams & aCommandPathParams)
CHIP_ERROR CommandHandler::PrepareStatus(const ConcreteCommandPath & aCommandPath)
{
ReturnErrorOnFailure(AllocateBuffer());
//
// We must not be in the middle of preparing a command, or having prepared or sent one.
//
VerifyOrReturnError(mState == CommandState::Idle, CHIP_ERROR_INCORRECT_STATE);
CommandStatusIB::Builder commandStatus = mInvokeResponseBuilder.GetInvokeResponses().CreateInvokeResponse().CreateStatus();
InvokeResponseIBs::Builder & invokeResponses = mInvokeResponseBuilder.GetInvokeResponses();
InvokeResponseIB::Builder & invokeResponse = invokeResponses.CreateInvokeResponse();
ReturnErrorOnFailure(invokeResponses.GetError());
CommandStatusIB::Builder & commandStatus = invokeResponse.CreateStatus();
ReturnErrorOnFailure(commandStatus.GetError());
CommandPathIB::Builder & path = commandStatus.CreatePath();
ReturnErrorOnFailure(commandStatus.GetError());
ReturnErrorOnFailure(ConstructCommandPath(aCommandPathParams, commandStatus.CreatePath()));
ReturnErrorOnFailure(path.Encode(aCommandPath));
MoveToState(CommandState::AddingCommand);
return CHIP_NO_ERROR;
}
Expand Down
8 changes: 4 additions & 4 deletions src/app/CommandHandler.h
Original file line number Diff line number Diff line change
Expand Up @@ -142,9 +142,9 @@ class CommandHandler : public Command
CHIP_ERROR AddClusterSpecificFailure(const ConcreteCommandPath & aCommandPath, ClusterStatus aClusterStatus) override;

CHIP_ERROR ProcessInvokeRequest(System::PacketBufferHandle && payload, bool isTimedInvoke);
CHIP_ERROR PrepareCommand(const CommandPathParams & aCommandPathParams, bool aStartDataStruct = true);
CHIP_ERROR PrepareCommand(const ConcreteCommandPath & aCommandPath, bool aStartDataStruct = true);
CHIP_ERROR FinishCommand(bool aStartDataStruct = true);
CHIP_ERROR PrepareStatus(const CommandPathParams & aCommandPathParams);
CHIP_ERROR PrepareStatus(const ConcreteCommandPath & aCommandPath);
CHIP_ERROR FinishStatus();
TLV::TLVWriter * GetCommandDataIBTLVWriter();
FabricIndex GetAccessingFabricIndex() const;
Expand All @@ -162,7 +162,8 @@ class CommandHandler : public Command
template <typename CommandData>
CHIP_ERROR AddResponseData(const ConcreteCommandPath & aRequestCommandPath, const CommandData & aData)
{
ReturnErrorOnFailure(PrepareResponse(aRequestCommandPath, CommandData::GetCommandId()));
ConcreteCommandPath path = { aRequestCommandPath.mEndpointId, aRequestCommandPath.mClusterId, CommandData::GetCommandId() };
ReturnErrorOnFailure(PrepareCommand(path, false));
TLV::TLVWriter * writer = GetCommandDataIBTLVWriter();
VerifyOrReturnError(writer != nullptr, CHIP_ERROR_INCORRECT_STATE);
ReturnErrorOnFailure(DataModel::Encode(*writer, TLV::ContextTag(to_underlying(CommandDataIB::Tag::kData)), aData));
Expand Down Expand Up @@ -210,7 +211,6 @@ class CommandHandler : public Command

CHIP_ERROR ProcessCommandDataIB(CommandDataIB::Parser & aCommandElement);
CHIP_ERROR SendCommandResponse();
CHIP_ERROR PrepareResponse(const ConcreteCommandPath & aRequestCommandPath, CommandId aResponseCommand);
CHIP_ERROR AddStatusInternal(const ConcreteCommandPath & aCommandPath, const Protocols::InteractionModel::Status aStatus,
const Optional<ClusterStatus> & aClusterStatus);

Expand Down
16 changes: 8 additions & 8 deletions src/app/CommandSender.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -298,23 +298,23 @@ CHIP_ERROR CommandSender::ProcessInvokeResponseIB(InvokeResponseIB::Parser & aIn

CHIP_ERROR CommandSender::PrepareCommand(const CommandPathParams & aCommandPathParams, bool aStartDataStruct)
{
CommandDataIB::Builder commandData;
ReturnLogErrorOnFailure(AllocateBuffer());

//
// We must not be in the middle of preparing a command, or having prepared or sent one.
//
VerifyOrReturnError(mState == CommandState::Idle, CHIP_ERROR_INCORRECT_STATE);

commandData = mInvokeRequestBuilder.GetInvokeRequests().CreateCommandData();
ReturnLogErrorOnFailure(commandData.GetError());

ReturnLogErrorOnFailure(ConstructCommandPath(aCommandPathParams, commandData.CreatePath()));
InvokeRequests::Builder & invokeRequests = mInvokeRequestBuilder.GetInvokeRequests();
CommandDataIB::Builder & invokeRequest = invokeRequests.CreateCommandData();
ReturnErrorOnFailure(invokeRequests.GetError());
CommandPathIB::Builder & path = invokeRequest.CreatePath();
ReturnErrorOnFailure(invokeRequest.GetError());
ReturnErrorOnFailure(path.Encode(aCommandPathParams));

if (aStartDataStruct)
{
ReturnLogErrorOnFailure(commandData.GetWriter()->StartContainer(TLV::ContextTag(to_underlying(CommandDataIB::Tag::kData)),
TLV::kTLVType_Structure, mDataElementContainerType));
ReturnLogErrorOnFailure(invokeRequest.GetWriter()->StartContainer(TLV::ContextTag(to_underlying(CommandDataIB::Tag::kData)),
TLV::kTLVType_Structure, mDataElementContainerType));
}

MoveToState(CommandState::AddingCommand);
Expand Down
82 changes: 32 additions & 50 deletions src/app/EventManagement.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -289,38 +289,43 @@ CHIP_ERROR EventManagement::CalculateEventSize(EventLoggingDelegate * apDelegate

ctxt.mCurrentEventNumber = GetPriorityBuffer(apOptions->mpEventSchema->mPriority)->GetLastEventNumber();
ctxt.mCurrentTime.mValue = GetPriorityBuffer(apOptions->mpEventSchema->mPriority)->GetLastEventTimestamp();
err = ConstructEvent(&ctxt, apDelegate, apOptions);
if (CHIP_NO_ERROR == err)

TLVWriter checkpoint = ctxt.mWriter;
err = ConstructEvent(&ctxt, apDelegate, apOptions);
if (err != CHIP_NO_ERROR)
{
requiredSize = writer.GetLengthWritten();
ctxt.mWriter = checkpoint;
}
else
{
// update these variables since ConstructEvent can be used to track the
// state of a set of events over multiple calls.
ctxt.mCurrentEventNumber++;
ctxt.mCurrentTime = apOptions->mTimestamp;
requiredSize = writer.GetLengthWritten();
}
return err;
}

CHIP_ERROR EventManagement::ConstructEvent(EventLoadOutContext * apContext, EventLoggingDelegate * apDelegate,
const EventOptions * apOptions)
{

CHIP_ERROR err = CHIP_NO_ERROR;
TLVWriter checkpoint = apContext->mWriter;
TLV::TLVType dataContainerType;
EventReportIB::Builder eventReportBuilder;
EventDataIB::Builder eventDataIBBuilder;
EventPathIB::Builder eventPathBuilder;
uint64_t deltatime = 0;

VerifyOrExit(apContext->mCurrentEventNumber >= apContext->mStartingEventNumber,
/* no-op: don't write event, but advance current event Number */);
VerifyOrReturnError(apContext->mCurrentEventNumber >= apContext->mStartingEventNumber, CHIP_NO_ERROR
/* no-op: don't write event, but advance current event Number */);

VerifyOrExit(apOptions != nullptr, err = CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrReturnError(apOptions != nullptr, CHIP_ERROR_INVALID_ARGUMENT);

EventReportIB::Builder eventReportBuilder;
eventReportBuilder.Init(&(apContext->mWriter));
// TODO: Update IsUrgent, issue 11386
// TODO: Update statusIB, issue 11388
eventDataIBBuilder = eventReportBuilder.CreateEventData();
eventPathBuilder = eventDataIBBuilder.CreatePath();
err = eventDataIBBuilder.GetError();
SuccessOrExit(err);
EventDataIB::Builder & eventDataIBBuilder = eventReportBuilder.CreateEventData();
ReturnErrorOnFailure(eventReportBuilder.GetError());
EventPathIB::Builder & eventPathBuilder = eventDataIBBuilder.CreatePath();
ReturnErrorOnFailure(eventDataIBBuilder.GetError());

// TODO: Revisit NodeId since the the encoding spec and the IM seem to disagree on how this stuff works
eventPathBuilder.Node(apOptions->mpEventSchema->mNodeId)
Expand All @@ -329,10 +334,9 @@ CHIP_ERROR EventManagement::ConstructEvent(EventLoadOutContext * apContext, Even
.Event(apOptions->mpEventSchema->mEventId)
.IsUrgent(false)
.EndOfEventPathIB();
err = eventPathBuilder.GetError();
SuccessOrExit(err);

ReturnErrorOnFailure(eventPathBuilder.GetError());
eventDataIBBuilder.Priority(chip::to_underlying(apContext->mPriority));
ReturnErrorOnFailure(eventDataIBBuilder.GetError());

deltatime = apOptions->mTimestamp.mValue - apContext->mCurrentTime.mValue;
if (apOptions->mTimestamp.IsSystem())
Expand All @@ -344,42 +348,20 @@ CHIP_ERROR EventManagement::ConstructEvent(EventLoadOutContext * apContext, Even
eventDataIBBuilder.DeltaEpochTimestamp(deltatime);
}

err = eventDataIBBuilder.GetError();
SuccessOrExit(err);
ReturnErrorOnFailure(eventDataIBBuilder.GetError());

err = apContext->mWriter.StartContainer(ContextTag(chip::to_underlying(EventDataIB::Tag::kData)), TLV::kTLVType_Structure,
dataContainerType);
SuccessOrExit(err);
ReturnErrorOnFailure(apContext->mWriter.StartContainer(ContextTag(chip::to_underlying(EventDataIB::Tag::kData)),
TLV::kTLVType_Structure, dataContainerType));
// Callback to write the EventData
err = apDelegate->WriteEvent(apContext->mWriter);
SuccessOrExit(err);

err = apContext->mWriter.EndContainer(dataContainerType);
SuccessOrExit(err);

ReturnErrorOnFailure(apDelegate->WriteEvent(apContext->mWriter));
ReturnErrorOnFailure(apContext->mWriter.EndContainer(dataContainerType));
eventDataIBBuilder.EndOfEventDataIB();
SuccessOrExit(err = eventDataIBBuilder.GetError());
ReturnErrorOnFailure(eventDataIBBuilder.GetError());
eventReportBuilder.EndOfEventReportIB();
SuccessOrExit(err = eventReportBuilder.GetError());

err = apContext->mWriter.Finalize();
SuccessOrExit(err);

ReturnErrorOnFailure(eventReportBuilder.GetError());
ReturnErrorOnFailure(apContext->mWriter.Finalize());
apContext->mFirst = false;

exit:
if (err != CHIP_NO_ERROR)
{
apContext->mWriter = checkpoint;
}
else
{
// update these variables since ConstructEvent can be used to track the
// state of a set of events over multiple calls.
apContext->mCurrentEventNumber++;
apContext->mCurrentTime = apOptions->mTimestamp;
}
return err;
return CHIP_NO_ERROR;
}

void EventManagement::CreateEventManagement(Messaging::ExchangeManager * apExchangeManager, uint32_t aNumBuffers,
Expand Down
Loading

0 comments on commit 1078608

Please sign in to comment.