Cleanup IM

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

@@ -275,17 +275,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
@@ -294,7 +288,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();
 }
 
@@ -316,24 +310,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)),
@@ -359,16 +351,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,

src/app/MessageDef/CommandPathIB.cpp

@@ -169,5 +169,25 @@ CommandPathIB::Builder & CommandPathIB::Builder::EndOfCommandPathIB()
     return *this;
 }
 
+CHIP_ERROR CommandPathIB::Builder::Encode(const CommandPathParams & aCommandPathParams)
+{
+    if (aCommandPathParams.mFlags.Has(CommandPathFlags::kEndpointIdValid))
+    {
+        EndpointId(aCommandPathParams.mEndpointId);
+    }
+
+    ClusterId(aCommandPathParams.mClusterId).CommandId(aCommandPathParams.mCommandId).EndOfCommandPathIB();
+    return GetError();
+}
+
+CHIP_ERROR CommandPathIB::Builder::Encode(const ConcreteCommandPath & aConcreteCommandPath)
+{
+    EndpointId(aConcreteCommandPath.mEndpointId)
+        .ClusterId(aConcreteCommandPath.mClusterId)
+        .CommandId(aConcreteCommandPath.mCommandId)
+        .EndOfCommandPathIB();
+    return GetError();
+}
+
 }; // namespace app
 }; // namespace chip

src/app/MessageDef/CommandPathIB.h

@@ -22,6 +22,8 @@
 #include "ListParser.h"
 
 #include <app/AppBuildConfig.h>
+#include <app/CommandPathParams.h>
+#include <app/ConcreteCommandPath.h>
 #include <app/util/basic-types.h>
 #include <lib/core/CHIPCore.h>
 #include <lib/core/CHIPTLV.h>
@@ -128,6 +130,9 @@ class Builder : public ListBuilder
      *  @return A reference to *this
      */
     CommandPathIB::Builder & EndOfCommandPathIB();
+
+    CHIP_ERROR Encode(const CommandPathParams & aCommandPathParams);
+    CHIP_ERROR Encode(const ConcreteCommandPath & aConcreteCommandPath);
 };
 } // namespace CommandPathIB
 } // namespace app