Cleanup IM (#12606)

--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.

src/app/AttributeAccessInterface.cpp

@@ -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);
 
@@ -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()
@@ -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;

src/app/AttributeAccessInterface.h

@@ -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;
 };
 
 /**
@@ -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);
     }
 
     /**

src/app/Command.cpp

@@ -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()
 {
     //

src/app/Command.h

@@ -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;

src/app/CommandHandler.cpp

@@ -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
@@ -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();
 }
 
@@ -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)),
@@ -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;
 }

src/app/CommandHandler.h

@@ -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;
@@ -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));
@@ -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);
 

src/app/CommandSender.cpp

@@ -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);

src/app/EventManagement.cpp

@@ -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)
@@ -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())
@@ -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,