Add new AddStatus overloads using ClusterStatusCode (project-chip#33904)

* Add new AddStatus overloads using ClusterStatusCode

- CommandHandler and WriteHandler did not have a way to cleanly just `AddStatus` with
  a cluster-specific status code (to eventually harmonize handling methods to
  always return just a ClusterStatusCode).

This PR:

- Introduces an `AddStatus` overload for `ClusterStatusCode` to CommandHandler
  and WriteHandler.
- Removes the unimplemented `WriteClient::Shutdown` method.
- Adds notes to the `AddClusterSpecificSuccess` and `AddClusterSpecificFailure`.
- Removes implicit conversion from `ClusterStatusCode` to `Status`
  - Was never used and was error-prone/lossy

Fixes project-chip#31120

Testing done:
- Updated necessary unit tests.
- Added unit tests for cluster specific statuses on writes.
- Integration tests still pass.

* Restyled by clang-format

* Improve type safety of test

* Address review comments

- Make StatusIB initializable from ClusterStatusCode
- Clean-ups requested
- CommandResponseHelper loses the error-prone cluster-specific-code on success
  (can be added back if ever needed).

* Restyled by clang-format

---------

Co-authored-by: Restyled.io <commits@restyled.io>

src/app/CommandHandler.h

@@ -25,6 +25,7 @@
 #include <lib/support/CodeUtils.h>
 #include <lib/support/IntrusiveList.h>
 #include <lib/support/logging/CHIPLogging.h>
+#include <protocols/interaction_model/StatusCode.h>
 
 namespace chip {
 namespace app {
@@ -116,21 +117,51 @@ class CommandHandler
 
     /**
      * Adds the given command status and returns any failures in adding statuses (e.g. out
-     * of buffer space) to the caller
+     * of buffer space) to the caller. `context` is an optional (if not nullptr)
+     * debug string to include in logging.
      */
     virtual CHIP_ERROR FallibleAddStatus(const ConcreteCommandPath & aRequestCommandPath,
-                                         const Protocols::InteractionModel::Status aStatus, const char * context = nullptr) = 0;
+                                         const Protocols::InteractionModel::ClusterStatusCode & aStatus,
+                                         const char * context = nullptr) = 0;
+    CHIP_ERROR FallibleAddStatus(const ConcreteCommandPath & aRequestCommandPath, const Protocols::InteractionModel::Status aStatus,
+                                 const char * context = nullptr)
+    {
+        return FallibleAddStatus(aRequestCommandPath, Protocols::InteractionModel::ClusterStatusCode{ aStatus }, context);
+    }
 
     /**
-     * Adds a status when the caller is unable to handle any failures. Logging is performed
-     * and failure to register the status is checked with VerifyOrDie.
+     * Adds an IM global or Cluster status when the caller is unable to handle any failures. Logging is performed
+     * and failure to register the status is checked with VerifyOrDie. `context` is an optional (if not nullptr)
+     * debug string to include in logging.
      */
-    virtual void AddStatus(const ConcreteCommandPath & aCommandPath, const Protocols::InteractionModel::Status aStatus,
-                           const char * context = nullptr) = 0;
+    virtual void AddStatus(const ConcreteCommandPath & aRequestCommandPath,
+                           const Protocols::InteractionModel::ClusterStatusCode & aStatus, const char * context = nullptr) = 0;
+    void AddStatus(const ConcreteCommandPath & aRequestCommandPath, const Protocols::InteractionModel::Status aStatus,
+                   const char * context = nullptr)
+    {
+        AddStatus(aRequestCommandPath, Protocols::InteractionModel::ClusterStatusCode{ aStatus }, context);
+    }
 
-    virtual CHIP_ERROR AddClusterSpecificSuccess(const ConcreteCommandPath & aRequestCommandPath, ClusterStatus aClusterStatus) = 0;
+    /**
+     * Sets the response to indicate Success with a cluster-specific status code `aClusterStatus` included.
+     *
+     * NOTE: For regular success, what you want is AddStatus/FailibleAddStatus(aRequestCommandPath,
+     * InteractionModel::Status::Success).
+     */
+    virtual CHIP_ERROR AddClusterSpecificSuccess(const ConcreteCommandPath & aRequestCommandPath, ClusterStatus aClusterStatus)
+    {
+        return FallibleAddStatus(aRequestCommandPath,
+                                 Protocols::InteractionModel::ClusterStatusCode::ClusterSpecificSuccess(aClusterStatus));
+    }
 
-    virtual CHIP_ERROR AddClusterSpecificFailure(const ConcreteCommandPath & aRequestCommandPath, ClusterStatus aClusterStatus) = 0;
+    /**
+     * Sets the response to indicate Failure with a cluster-specific status code `aClusterStatus` included.
+     */
+    virtual CHIP_ERROR AddClusterSpecificFailure(const ConcreteCommandPath & aRequestCommandPath, ClusterStatus aClusterStatus)
+    {
+        return FallibleAddStatus(aRequestCommandPath,
+                                 Protocols::InteractionModel::ClusterStatusCode::ClusterSpecificFailure(aClusterStatus));
+    }
 
     /**
      * GetAccessingFabricIndex() may only be called during synchronous command

src/app/CommandHandlerImpl.cpp

@@ -19,6 +19,7 @@
 
 #include <access/AccessControl.h>
 #include <app-common/zap-generated/cluster-objects.h>
+#include <app/MessageDef/StatusIB.h>
 #include <app/RequiredPrivilege.h>
 #include <app/StatusResponse.h>
 #include <app/util/MatterCallbacks.h>
@@ -30,6 +31,7 @@
 #include <lib/support/TypeTraits.h>
 #include <messaging/ExchangeContext.h>
 #include <platform/LockTracker.h>
+#include <protocols/interaction_model/StatusCode.h>
 #include <protocols/secure_channel/Constants.h>
 
 namespace chip {
@@ -592,11 +594,11 @@ CHIP_ERROR CommandHandlerImpl::AddStatusInternal(const ConcreteCommandPath & aCo
     return TryAddingResponse([&]() -> CHIP_ERROR { return TryAddStatusInternal(aCommandPath, aStatus); });
 }
 
-void CommandHandlerImpl::AddStatus(const ConcreteCommandPath & aCommandPath, const Protocols::InteractionModel::Status aStatus,
-                                   const char * context)
+void CommandHandlerImpl::AddStatus(const ConcreteCommandPath & aCommandPath,
+                                   const Protocols::InteractionModel::ClusterStatusCode & status, const char * context)
 {
 
-    CHIP_ERROR error = FallibleAddStatus(aCommandPath, aStatus, context);
+    CHIP_ERROR error = FallibleAddStatus(aCommandPath, status, context);
 
     if (error != CHIP_NO_ERROR)
     {
@@ -610,33 +612,37 @@ void CommandHandlerImpl::AddStatus(const ConcreteCommandPath & aCommandPath, con
     }
 }
 
-CHIP_ERROR CommandHandlerImpl::FallibleAddStatus(const ConcreteCommandPath & path, const Protocols::InteractionModel::Status status,
+CHIP_ERROR CommandHandlerImpl::FallibleAddStatus(const ConcreteCommandPath & path,
+                                                 const Protocols::InteractionModel::ClusterStatusCode & status,
                                                  const char * context)
 {
-    if (status != Status::Success)
+    if (!status.IsSuccess())
     {
         if (context == nullptr)
         {
             context = "no additional context";
         }
 
-        ChipLogError(DataManagement,
-                     "Endpoint=%u Cluster=" ChipLogFormatMEI " Command=" ChipLogFormatMEI " status " ChipLogFormatIMStatus " (%s)",
-                     path.mEndpointId, ChipLogValueMEI(path.mClusterId), ChipLogValueMEI(path.mCommandId),
-                     ChipLogValueIMStatus(status), context);
+        if (status.HasClusterSpecificCode())
+        {
+            ChipLogError(DataManagement,
+                         "Endpoint=%u Cluster=" ChipLogFormatMEI " Command=" ChipLogFormatMEI " status " ChipLogFormatIMStatus
+                         " ClusterSpecificCode=%u (%s)",
+                         path.mEndpointId, ChipLogValueMEI(path.mClusterId), ChipLogValueMEI(path.mCommandId),
+                         ChipLogValueIMStatus(status.GetStatus()), static_cast<unsigned>(status.GetClusterSpecificCode().Value()),
+                         context);
+        }
+        else
+        {
+            ChipLogError(DataManagement,
+                         "Endpoint=%u Cluster=" ChipLogFormatMEI " Command=" ChipLogFormatMEI " status " ChipLogFormatIMStatus
+                         " (%s)",
+                         path.mEndpointId, ChipLogValueMEI(path.mClusterId), ChipLogValueMEI(path.mCommandId),
+                         ChipLogValueIMStatus(status.GetStatus()), context);
+        }
     }
 
-    return AddStatusInternal(path, StatusIB(status));
-}
-
-CHIP_ERROR CommandHandlerImpl::AddClusterSpecificSuccess(const ConcreteCommandPath & aCommandPath, ClusterStatus aClusterStatus)
-{
-    return AddStatusInternal(aCommandPath, StatusIB(Status::Success, aClusterStatus));
-}
-
-CHIP_ERROR CommandHandlerImpl::AddClusterSpecificFailure(const ConcreteCommandPath & aCommandPath, ClusterStatus aClusterStatus)
-{
-    return AddStatusInternal(aCommandPath, StatusIB(Status::Failure, aClusterStatus));
+    return AddStatusInternal(path, StatusIB{ status });
 }
 
 CHIP_ERROR CommandHandlerImpl::PrepareInvokeResponseCommand(const ConcreteCommandPath & aResponseCommandPath,

src/app/CommandHandlerImpl.h

@@ -29,6 +29,7 @@
 #include <messaging/Flags.h>
 #include <protocols/Protocols.h>
 #include <protocols/interaction_model/Constants.h>
+#include <protocols/interaction_model/StatusCode.h>
 #include <system/SystemPacketBuffer.h>
 #include <system/TLVPacketBufferBackingStore.h>
 
@@ -114,15 +115,16 @@ class CommandHandlerImpl : public CommandHandler
     /**************** CommandHandler interface implementation ***********************/
 
     using CommandHandler::AddResponseData;
+    using CommandHandler::AddStatus;
+    using CommandHandler::FallibleAddStatus;
 
     void FlushAcksRightAwayOnSlowCommand() override;
 
-    CHIP_ERROR FallibleAddStatus(const ConcreteCommandPath & aRequestCommandPath, const Protocols::InteractionModel::Status aStatus,
+    CHIP_ERROR FallibleAddStatus(const ConcreteCommandPath & aRequestCommandPath,
+                                 const Protocols::InteractionModel::ClusterStatusCode & aStatus,
                                  const char * context = nullptr) override;
-    void AddStatus(const ConcreteCommandPath & aCommandPath, const Protocols::InteractionModel::Status aStatus,
+    void AddStatus(const ConcreteCommandPath & aCommandPath, const Protocols::InteractionModel::ClusterStatusCode & aStatus,
                    const char * context = nullptr) override;
-    CHIP_ERROR AddClusterSpecificSuccess(const ConcreteCommandPath & aRequestCommandPath, ClusterStatus aClusterStatus) override;
-    CHIP_ERROR AddClusterSpecificFailure(const ConcreteCommandPath & aRequestCommandPath, ClusterStatus aClusterStatus) override;
 
     CHIP_ERROR AddResponseData(const ConcreteCommandPath & aRequestCommandPath, CommandId aResponseCommandId,
                                const DataModel::EncodableToTLV & aEncodable) override;

src/app/CommandResponseHelper.h

@@ -21,6 +21,7 @@
 #include <app/CommandHandler.h>
 #include <app/ConcreteCommandPath.h>
 #include <protocols/interaction_model/Constants.h>
+#include <protocols/interaction_model/StatusCode.h>
 
 namespace chip {
 namespace app {
@@ -38,35 +39,27 @@ class CommandResponseHelper
         mCommandHandler->AddResponse(mCommandPath, aResponse);
         mSentResponse = true;
         return CHIP_NO_ERROR;
-    };
+    }
 
-    CHIP_ERROR Success(ClusterStatus aClusterStatus)
+    CHIP_ERROR Success()
     {
-        CHIP_ERROR err = mCommandHandler->AddClusterSpecificSuccess(mCommandPath, aClusterStatus);
-        if (err == CHIP_NO_ERROR)
-        {
-            mSentResponse = true;
-        }
+        CHIP_ERROR err = mCommandHandler->FallibleAddStatus(mCommandPath, Protocols::InteractionModel::Status::Success);
+        mSentResponse  = (err == CHIP_NO_ERROR);
         return err;
     }
 
     CHIP_ERROR Failure(Protocols::InteractionModel::Status aStatus)
     {
         CHIP_ERROR err = mCommandHandler->FallibleAddStatus(mCommandPath, aStatus);
-        if (err == CHIP_NO_ERROR)
-        {
-            mSentResponse = true;
-        }
+        mSentResponse  = (err == CHIP_NO_ERROR);
         return err;
     }
 
     CHIP_ERROR Failure(ClusterStatus aClusterStatus)
     {
-        CHIP_ERROR err = mCommandHandler->AddClusterSpecificFailure(mCommandPath, aClusterStatus);
-        if (err == CHIP_NO_ERROR)
-        {
-            mSentResponse = true;
-        }
+        CHIP_ERROR err = mCommandHandler->FallibleAddStatus(
+            mCommandPath, Protocols::InteractionModel::ClusterStatusCode::ClusterSpecificFailure(aClusterStatus));
+        mSentResponse = (err == CHIP_NO_ERROR);
         return err;
     }
 

src/app/MessageDef/StatusIB.h

@@ -34,17 +34,32 @@
 #include <lib/support/CodeUtils.h>
 #include <lib/support/logging/CHIPLogging.h>
 #include <protocols/interaction_model/Constants.h>
+#include <protocols/interaction_model/StatusCode.h>
 #include <protocols/secure_channel/Constants.h>
 
 namespace chip {
 namespace app {
 struct StatusIB
 {
     StatusIB() = default;
-    StatusIB(Protocols::InteractionModel::Status imStatus) : mStatus(imStatus) {}
-    StatusIB(Protocols::InteractionModel::Status imStatus, ClusterStatus clusterStatus) :
+    explicit StatusIB(Protocols::InteractionModel::Status imStatus) : mStatus(imStatus) {}
+
+    explicit StatusIB(Protocols::InteractionModel::Status imStatus, ClusterStatus clusterStatus) :
         mStatus(imStatus), mClusterStatus(clusterStatus)
     {}
+
+    explicit StatusIB(const Protocols::InteractionModel::ClusterStatusCode & statusCode) : mStatus(statusCode.GetStatus())
+    {
+        // NOTE: Cluster-specific codes are only valid on SUCCESS/FAILURE IM status (7.10.7. Status Codes)
+        chip::Optional<ClusterStatus> clusterStatus = statusCode.GetClusterSpecificCode();
+        if (clusterStatus.HasValue())
+        {
+            mStatus        = statusCode.IsSuccess() ? Protocols::InteractionModel::Status::Success
+                                                    : Protocols::InteractionModel::Status::Failure;
+            mClusterStatus = clusterStatus;
+        }
+    }
+
     explicit StatusIB(CHIP_ERROR error) { InitFromChipError(error); }
 
     enum class Tag : uint8_t

src/app/WriteClient.h

@@ -228,12 +228,6 @@ class WriteClient : public Messaging::ExchangeDelegate
      */
     CHIP_ERROR SendWriteRequest(const SessionHandle & session, System::Clock::Timeout timeout = System::Clock::kZero);
 
-    /**
-     *  Shutdown the WriteClient. This terminates this instance
-     *  of the object and releases all held resources.
-     */
-    void Shutdown();
-
 private:
     friend class TestWriteInteraction;
     friend class InteractionModelEngine;

src/app/WriteHandler.cpp

@@ -21,13 +21,15 @@
 #include <app/AttributeAccessInterfaceRegistry.h>
 #include <app/InteractionModelEngine.h>
 #include <app/MessageDef/EventPathIB.h>
+#include <app/MessageDef/StatusIB.h>
 #include <app/StatusResponse.h>
 #include <app/WriteHandler.h>
 #include <app/reporting/Engine.h>
 #include <app/util/MatterCallbacks.h>
 #include <app/util/ember-compatibility-functions.h>
 #include <credentials/GroupDataProvider.h>
 #include <lib/support/TypeTraits.h>
+#include <protocols/interaction_model/StatusCode.h>
 
 namespace chip {
 namespace app {
@@ -315,7 +317,7 @@ CHIP_ERROR WriteHandler::ProcessAttributeDataIBs(TLV::TLVReader & aAttributeData
             // it with Busy status code.
             (dataAttributePath.IsListItemOperation() && !IsSameAttribute(mProcessingAttributePath, dataAttributePath)))
         {
-            err = AddStatus(dataAttributePath, StatusIB(Status::Busy));
+            err = AddStatusInternal(dataAttributePath, StatusIB(Status::Busy));
             continue;
         }
 
@@ -353,7 +355,7 @@ CHIP_ERROR WriteHandler::ProcessAttributeDataIBs(TLV::TLVReader & aAttributeData
         if (err != CHIP_NO_ERROR)
         {
             mWriteResponseBuilder.GetWriteResponses().Rollback(backup);
-            err = AddStatus(dataAttributePath, StatusIB(err));
+            err = AddStatusInternal(dataAttributePath, StatusIB(err));
         }
 
         DataModelCallbacks::GetInstance()->AttributeOperation(DataModelCallbacks::OperationType::Write,
@@ -616,22 +618,23 @@ Status WriteHandler::ProcessWriteRequest(System::PacketBufferHandle && aPayload,
     return status;
 }
 
-CHIP_ERROR WriteHandler::AddStatus(const ConcreteDataAttributePath & aPath, const Protocols::InteractionModel::Status aStatus)
+CHIP_ERROR WriteHandler::AddStatus(const ConcreteDataAttributePath & aPath,
+                                   const Protocols::InteractionModel::ClusterStatusCode & aStatus)
 {
-    return AddStatus(aPath, StatusIB(aStatus));
+    return AddStatusInternal(aPath, StatusIB{ aStatus });
 }
 
 CHIP_ERROR WriteHandler::AddClusterSpecificSuccess(const ConcreteDataAttributePath & aPath, ClusterStatus aClusterStatus)
 {
-    return AddStatus(aPath, StatusIB(Status::Success, aClusterStatus));
+    return AddStatus(aPath, Protocols::InteractionModel::ClusterStatusCode::ClusterSpecificSuccess(aClusterStatus));
 }
 
 CHIP_ERROR WriteHandler::AddClusterSpecificFailure(const ConcreteDataAttributePath & aPath, ClusterStatus aClusterStatus)
 {
-    return AddStatus(aPath, StatusIB(Status::Failure, aClusterStatus));
+    return AddStatus(aPath, Protocols::InteractionModel::ClusterStatusCode::ClusterSpecificFailure(aClusterStatus));
 }
 
-CHIP_ERROR WriteHandler::AddStatus(const ConcreteDataAttributePath & aPath, const StatusIB & aStatus)
+CHIP_ERROR WriteHandler::AddStatusInternal(const ConcreteDataAttributePath & aPath, const StatusIB & aStatus)
 {
     AttributeStatusIBs::Builder & writeResponses   = mWriteResponseBuilder.GetWriteResponses();
     AttributeStatusIB::Builder & attributeStatusIB = writeResponses.CreateAttributeStatus();

src/app/WriteHandler.h

@@ -31,6 +31,7 @@
 #include <messaging/Flags.h>
 #include <protocols/Protocols.h>
 #include <protocols/interaction_model/Constants.h>
+#include <protocols/interaction_model/StatusCode.h>
 #include <system/SystemPacketBuffer.h>
 #include <system/TLVPacketBufferBackingStore.h>
 
@@ -100,11 +101,14 @@ class WriteHandler : public Messaging::ExchangeDelegate
     CHIP_ERROR ProcessAttributeDataIBs(TLV::TLVReader & aAttributeDataIBsReader);
     CHIP_ERROR ProcessGroupAttributeDataIBs(TLV::TLVReader & aAttributeDataIBsReader);
 
-    CHIP_ERROR AddStatus(const ConcreteDataAttributePath & aPath, const Protocols::InteractionModel::Status aStatus);
-
-    CHIP_ERROR AddClusterSpecificSuccess(const ConcreteDataAttributePath & aAttributePathParams, uint8_t aClusterStatus);
+    CHIP_ERROR AddStatus(const ConcreteDataAttributePath & aPath, const Protocols::InteractionModel::ClusterStatusCode & aStatus);
+    CHIP_ERROR AddStatus(const ConcreteDataAttributePath & aPath, const Protocols::InteractionModel::Status aStatus)
+    {
+        return AddStatus(aPath, Protocols::InteractionModel::ClusterStatusCode{ aStatus });
+    }
 
-    CHIP_ERROR AddClusterSpecificFailure(const ConcreteDataAttributePath & aAttributePathParams, uint8_t aClusterStatus);
+    CHIP_ERROR AddClusterSpecificSuccess(const ConcreteDataAttributePath & aAttributePathParams, ClusterStatus aClusterStatus);
+    CHIP_ERROR AddClusterSpecificFailure(const ConcreteDataAttributePath & aAttributePathParams, ClusterStatus aClusterStatus);
 
     FabricIndex GetAccessingFabricIndex() const;
 
@@ -166,7 +170,7 @@ class WriteHandler : public Messaging::ExchangeDelegate
     // ProcessGroupAttributeDataIBs.
     CHIP_ERROR DeliverFinalListWriteEndForGroupWrite(bool writeWasSuccessful);
 
-    CHIP_ERROR AddStatus(const ConcreteDataAttributePath & aPath, const StatusIB & aStatus);
+    CHIP_ERROR AddStatusInternal(const ConcreteDataAttributePath & aPath, const StatusIB & aStatus);
 
 private:
     // ExchangeDelegate

src/app/data-model-interface/InvokeResponder.h

@@ -138,7 +138,7 @@ class AutoCompleteInvokeResponder
     {
         if (mCompleteState != CompleteState::kComplete)
         {
-            mWriter->Complete(Protocols::InteractionModel::Status::Failure);
+            mWriter->Complete(StatusIB{ Protocols::InteractionModel::Status::Failure });
         }
     }