Switch Interaction Model client error reporting to just CHIP_ERROR. (#…

…13888)

* Switch Interaction Model client error reporting to just CHIP_ERROR.

Now that we can put a StatusIB inside a CHIP_ERROR, we can stop
passing both, or passing just EmberAfStatus, and pass a CHIP_ERROR
that either contains a StatusIB or contains the actual client-side
error we ran into (e.g. failure to decode).

* Address review comments.

examples/all-clusters-app/all-clusters-common/src/binding-handler.cpp

@@ -82,7 +82,7 @@ static void BoundDeviceChangedHandler(const EmberBindingTableEntry * binding, ch
         auto onSuccess = [](const ConcreteCommandPath & commandPath, const StatusIB & status, const auto & dataResponse) {
             ChipLogProgress(NotSpecified, "OnOff command succeeds");
         };
-        auto onFailure = [](const StatusIB & status, CHIP_ERROR error) {
+        auto onFailure = [](CHIP_ERROR error) {
             ChipLogError(NotSpecified, "OnOff command failed: %" CHIP_ERROR_FORMAT, error.Format());
         };
 

examples/chip-tool/commands/common/CommandInvoker.h

@@ -33,7 +33,7 @@ class ResponseReceiver : public app::CommandSender::Callback
 {
 public:
     using SuccessCallback = void (*)(void * context, const ResponseType & data);
-    using FailureCallback = void (*)(void * context, EmberAfStatus status);
+    using FailureCallback = void (*)(void * context, CHIP_ERROR err);
     using DoneCallback    = void (*)(void * context);
 
     virtual ~ResponseReceiver() {}
@@ -46,10 +46,7 @@ class ResponseReceiver : public app::CommandSender::Callback
     inline void OnResponse(app::CommandSender * aCommandSender, const app::ConcreteCommandPath & aPath,
                            const app::StatusIB & aStatus, TLV::TLVReader * aData) override;
 
-    void OnError(const app::CommandSender * aCommandSender, const app::StatusIB & aStatus, CHIP_ERROR aError) override
-    {
-        mOnError(mContext, app::ToEmberAfStatus(aStatus.mStatus));
-    }
+    void OnError(const app::CommandSender * aCommandSender, CHIP_ERROR aError) override { mOnError(mContext, aError); }
 
     void OnDone(app::CommandSender * aCommandSender) override
     {
@@ -164,7 +161,7 @@ void ResponseReceiver<ResponseType>::OnResponse(app::CommandSender * aCommandSen
 exit:
     if (err != CHIP_NO_ERROR)
     {
-        mOnError(mContext, app::ToEmberAfStatus(Protocols::InteractionModel::Status::Failure));
+        mOnError(mContext, err);
     }
 }
 
@@ -178,7 +175,7 @@ inline void ResponseReceiver<app::DataModel::NullObjectType>::OnResponse(app::Co
     //
     if (aData != nullptr)
     {
-        mOnError(mContext, app::ToEmberAfStatus(Protocols::InteractionModel::Status::Failure));
+        mOnError(mContext, CHIP_ERROR_SCHEMA_MISMATCH);
         return;
     }
 

examples/chip-tool/templates/commands.zapt

@@ -203,9 +203,9 @@ static void OnDefaultSuccessResponse(void * context)
     command->SetCommandExitStatus(CHIP_NO_ERROR);
 }
 
-static void OnDefaultFailure(void * context, EmberAfStatus status)
+static void OnDefaultFailure(void * context, CHIP_ERROR err)
 {
-    ChipLogProgress(chipTool, "Default Failure Response: 0x%02x", chip::to_underlying(status));
+    ChipLogProgress(chipTool, "Default Failure Response: %" CHIP_ERROR_FORMAT, err.Format());
 
     ModelCommand * command = static_cast<ModelCommand *>(context);
     command->SetCommandExitStatus(CHIP_ERROR_INTERNAL);

examples/chip-tool/templates/partials/test_cluster.zapt

@@ -131,7 +131,8 @@ class {{filename}}: public TestCommand
     {{#*inline "staticDoneResponse"}}OnDoneCallback_{{index}}{{/inline}}
 
     {{#*inline "successArguments"}}{{#chip_tests_item_response_parameters}}{{#first}}{{#if ../leadingComma}}, {{/if}}{{/first}} {{zapTypeToDecodableClusterObjectType type ns=parent.cluster isArgument=true}} {{asLowerCamelCase name}}{{#not_last}}, {{/not_last}}{{/chip_tests_item_response_parameters}}{{/inline}}
-    {{#*inline "failureArguments"}}{{#if leadingComma}}, {{/if}}EmberAfStatus status{{/inline}}
+    {{! TODO: Temporary if cascade until everything is converted to the new status setup }}
+    {{#*inline "failureArguments"}}{{#if leadingComma}}, {{/if}}CHIP_ERROR error{{/inline}}
     {{#*inline "staticSuccessArguments"}}void * context{{> successArguments leadingComma=true}}{{/inline}}
     {{#*inline "staticFailureArguments"}}void * context{{> failureArguments leadingComma=true}}{{/inline}}
     {{#*inline "staticDoneArguments"}}void * context{{/inline}}
@@ -155,7 +156,7 @@ class {{filename}}: public TestCommand
 
     static void {{>staticFailureResponse}}({{>staticFailureArguments}})
     {
-        (static_cast<{{filename}} *>(context))->{{>failureResponse}}(status);
+        (static_cast<{{filename}} *>(context))->{{>failureResponse}}(error);
     }
 
     static void {{>staticSuccessResponse}}({{> staticSuccessArguments}})
@@ -247,8 +248,8 @@ class {{filename}}: public TestCommand
             (static_cast<{{filename}} *>(context))->{{>successResponse}}({{#chip_tests_item_response_parameters}}{{#not_first}}, {{/not_first}}data.{{asLowerCamelCase name}}{{/chip_tests_item_response_parameters}});
         };
 
-        auto failure = [](void * context, EmberAfStatus status) {
-            (static_cast<{{filename}} *>(context))->{{>failureResponse}}(status);
+        auto failure = [](void * context, CHIP_ERROR error) {
+            (static_cast<{{filename}} *>(context))->{{>failureResponse}}(error);
         };
 
         {{#if isGroupCommand}}
@@ -305,14 +306,15 @@ class {{filename}}: public TestCommand
 
     void {{>failureResponse}}({{>failureArguments}})
     {
+        chip::app::StatusIB status(error);
         {{#if response.error}}
-          VerifyOrReturn(CheckValue("status", status, {{response.error}}));
+          VerifyOrReturn(CheckValue("status", chip::to_underlying(status.mStatus), {{response.error}}));
           {{#unless async}}NextTest();{{/unless}}
         {{else if response.errorWrongValue}}
-          VerifyOrReturn(CheckConstraintNotValue("status", status, 0));
+          VerifyOrReturn(CheckConstraintNotValue("status", chip::to_underlying(status.mStatus), 0));
           {{#unless async}}NextTest();{{/unless}}
         {{else}}
-          {{#if optional}}(status == EMBER_ZCL_STATUS_UNSUPPORTED_ATTRIBUTE) ? NextTest() : {{/if}}ThrowFailureResponse();
+          {{#if optional}}(status.mStatus == chip::Protocols::InteractionModel::Status::UnsupportedAttribute) ? NextTest() : {{/if}}ThrowFailureResponse();
         {{/if}}
     }
 

examples/tv-casting-app/linux/main.cpp

@@ -204,9 +204,9 @@ void OnContentLauncherSuccessResponse(void * context, const LaunchResponse::Deco
     ChipLogProgress(AppServer, "ContentLauncher: Default Success Response");
 }
 
-void OnContentLauncherFailureResponse(void * context, EmberAfStatus status)
+void OnContentLauncherFailureResponse(void * context, CHIP_ERROR error)
 {
-    ChipLogError(AppServer, "ContentLauncher: Default Failure Response: %" PRIu8, status);
+    ChipLogError(AppServer, "ContentLauncher: Default Failure Response: %" CHIP_ERROR_FORMAT, error.Format());
 }
 
 void DeviceEventCallback(const DeviceLayer::ChipDeviceEvent * event, intptr_t arg)

src/app/CommandSender.cpp

@@ -121,12 +121,11 @@ CHIP_ERROR CommandSender::OnMessageReceived(Messaging::ExchangeContext * apExcha
     }
 
     CHIP_ERROR err = CHIP_NO_ERROR;
-    StatusIB status(Protocols::InteractionModel::Status::Failure);
     VerifyOrExit(apExchangeContext == mpExchangeCtx, err = CHIP_ERROR_INCORRECT_STATE);
 
     if (mState == State::AwaitingTimedStatus)
     {
-        err = HandleTimedStatus(aPayloadHeader, std::move(aPayload), status);
+        err = HandleTimedStatus(aPayloadHeader, std::move(aPayload));
         // Skip all other processing here (which is for the response to the
         // invoke request), no matter whether err is success or not.
         goto exit;
@@ -136,11 +135,10 @@ CHIP_ERROR CommandSender::OnMessageReceived(Messaging::ExchangeContext * apExcha
     {
         err = ProcessInvokeResponse(std::move(aPayload));
         SuccessOrExit(err);
-        status.mStatus = Protocols::InteractionModel::Status::Success;
     }
     else if (aPayloadHeader.HasMessageType(Protocols::InteractionModel::MsgType::StatusResponse))
     {
-        err = StatusResponse::ProcessStatusResponse(std::move(aPayload), status);
+        err = StatusResponse::ProcessStatusResponse(std::move(aPayload));
         SuccessOrExit(err);
     }
     else
@@ -153,7 +151,7 @@ CHIP_ERROR CommandSender::OnMessageReceived(Messaging::ExchangeContext * apExcha
     {
         if (err != CHIP_NO_ERROR)
         {
-            mpCallback->OnError(this, status, err);
+            mpCallback->OnError(this, err);
         }
     }
 
@@ -210,9 +208,7 @@ void CommandSender::OnResponseTimeout(Messaging::ExchangeContext * apExchangeCon
 
     if (mpCallback != nullptr)
     {
-        StatusIB status;
-        status.mStatus = Protocols::InteractionModel::Status::Failure;
-        mpCallback->OnError(this, status, CHIP_ERROR_TIMEOUT);
+        mpCallback->OnError(this, CHIP_ERROR_TIMEOUT);
     }
 
     Close();
@@ -309,14 +305,14 @@ CHIP_ERROR CommandSender::ProcessInvokeResponseIB(InvokeResponseIB::Parser & aIn
 
         if (mpCallback != nullptr)
         {
-            if (statusIB.mStatus == Protocols::InteractionModel::Status::Success)
+            if (statusIB.IsSuccess())
             {
                 mpCallback->OnResponse(this, ConcreteCommandPath(endpointId, clusterId, commandId), statusIB,
                                        hasDataResponse ? &commandDataReader : nullptr);
             }
             else
             {
-                mpCallback->OnError(this, statusIB, CHIP_ERROR_IM_STATUS_CODE_RECEIVED);
+                mpCallback->OnError(this, statusIB.ToChipError());
             }
         }
     }
@@ -382,10 +378,9 @@ TLV::TLVWriter * CommandSender::GetCommandDataIBTLVWriter()
     }
 }
 
-CHIP_ERROR CommandSender::HandleTimedStatus(const PayloadHeader & aPayloadHeader, System::PacketBufferHandle && aPayload,
-                                            StatusIB & aStatusIB)
+CHIP_ERROR CommandSender::HandleTimedStatus(const PayloadHeader & aPayloadHeader, System::PacketBufferHandle && aPayload)
 {
-    ReturnErrorOnFailure(TimedRequest::HandleResponse(aPayloadHeader, std::move(aPayload), aStatusIB));
+    ReturnErrorOnFailure(TimedRequest::HandleResponse(aPayloadHeader, std::move(aPayload)));
 
     return SendInvokeRequest();
 }

src/app/CommandSender.h

@@ -102,9 +102,9 @@ class CommandSender final : public Messaging::ExchangeDelegate
          *
          * - CHIP_ERROR_TIMEOUT: A response was not received within the expected response timeout.
          * - CHIP_ERROR_*TLV*: A malformed, non-compliant response was received from the server.
-         * - CHIP_ERROR_IM_STATUS_CODE_RECEIVED: An invoke response containing a status code denoting an error was received.
-         *                  When the protocol ID in the received status is IM, aInteractionModelStatus will contain the IM status
-         *                  code. Otherwise, aInteractionModelStatus will always be set to IM::Status::Failure.
+         * - CHIP_ERROR encapsulating a StatusIB: If we got a non-path-specific
+         *   status response from the server.  In that case,
+         *   StatusIB::InitFromChipError can be used to extract the status.
          * - CHIP_ERROR*: All other cases.
          *
          * The CommandSender object MUST continue to exist after this call is completed. The application shall wait until it
@@ -114,7 +114,7 @@ class CommandSender final : public Messaging::ExchangeDelegate
          * @param[in] aStatusIB       The status code including IM status code and optional cluster status code
          * @param[in] aError          A system error code that conveys the overall error code.
          */
-        virtual void OnError(const CommandSender * apCommandSender, const StatusIB & aStatusIB, CHIP_ERROR aError) {}
+        virtual void OnError(const CommandSender * apCommandSender, CHIP_ERROR aError) {}
 
         /**
          * OnDone will be called when CommandSender has finished all work and is safe to destroy and free the
@@ -279,10 +279,9 @@ class CommandSender final : public Messaging::ExchangeDelegate
     // Timed Request.  The caller is assumed to have already checked that our
     // exchange context member is the one the message came in on.
     //
-    // aStatusIB will be populated with the returned status if we can parse it
-    // successfully.
-    CHIP_ERROR HandleTimedStatus(const PayloadHeader & aPayloadHeader, System::PacketBufferHandle && aPayload,
-                                 StatusIB & aStatusIB);
+    // If the server returned an error status, that will be returned as an error
+    // value of CHIP_ERROR.
+    CHIP_ERROR HandleTimedStatus(const PayloadHeader & aPayloadHeader, System::PacketBufferHandle && aPayload);
 
     // Send our queued-up Invoke Request message.  Assumes the exchange is ready
     // and mPendingInvokeData is populated.

src/app/DeviceControllerInteractionModelDelegate.h

@@ -42,17 +42,17 @@ class DeviceControllerInteractionModelDelegate : public chip::app::CommandSender
         }
     }
 
-    void OnError(const app::CommandSender * apCommandSender, const chip::app::StatusIB & aStatus,
-                 CHIP_ERROR aProtocolError) override
+    void OnError(const app::CommandSender * apCommandSender, CHIP_ERROR aError) override
     {
         // The IMDefaultResponseCallback started out life as an Ember function, so it only accepted
-        // Ember status codes. Consequently, let's convert the IM code over to a meaningful Ember status before dispatching.
+        // Ember status codes. Consequently, let's convert the error over to a meaningful Ember status before dispatching.
         //
-        // This however, results in loss (aError is completely discarded). When full cluster-specific status codes are implemented
-        // as well, this will be an even bigger problem.
+        // This however, results in loss (non-IM errors and cluster-specific
+        // status codes are completely discarded).
         //
         // For now, #10331 tracks this issue.
-        IMDefaultResponseCallback(apCommandSender, app::ToEmberAfStatus(aStatus.mStatus));
+        app::StatusIB status(aError);
+        IMDefaultResponseCallback(apCommandSender, app::ToEmberAfStatus(status.mStatus));
     }
 
     void OnDone(app::CommandSender * apCommandSender) override { return chip::Platform::Delete(apCommandSender); }
@@ -63,9 +63,10 @@ class DeviceControllerInteractionModelDelegate : public chip::app::CommandSender
         IMWriteResponseCallback(apWriteClient, attributeStatus.mStatus);
     }
 
-    void OnError(const app::WriteClient * apWriteClient, const app::StatusIB & aStatus, CHIP_ERROR aError) override
+    void OnError(const app::WriteClient * apWriteClient, CHIP_ERROR aError) override
     {
-        IMWriteResponseCallback(apWriteClient, aStatus.mStatus);
+        app::StatusIB status(aError);
+        IMWriteResponseCallback(apWriteClient, status.mStatus);
     }
 
     void OnDone(app::WriteClient * apWriteClient) override {}

src/app/MessageDef/StatusIB.h

@@ -45,6 +45,7 @@ struct StatusIB
     StatusIB(Protocols::InteractionModel::Status imStatus, ClusterStatus clusterStatus) :
         mStatus(imStatus), mClusterStatus(clusterStatus)
     {}
+    explicit StatusIB(CHIP_ERROR error) { InitFromChipError(error); }
 
     enum class Tag : uint8_t
     {
@@ -104,12 +105,22 @@ struct StatusIB
     CHIP_ERROR ToChipError() const;
 
     /**
-     * Extract a CHIP_ERROR into this StatusIB.  If IsIMStatus is false for the
-     * error, this might do a best-effort attempt to come up with a
+     * Extract a CHIP_ERROR into this StatusIB.  If IsIMStatus() is false for
+     * the error, this might do a best-effort attempt to come up with a
      * corresponding StatusIB, defaulting to a generic Status::Failure.
      */
     void InitFromChipError(CHIP_ERROR aError);
 
+    /**
+     * Test whether this status is a success.
+     */
+    bool IsSuccess() const { return mStatus == Protocols::InteractionModel::Status::Success; }
+
+    /**
+     * Test whether this status is a failure.
+     */
+    bool IsFailure() const { return !IsSuccess(); }
+
     /**
      * Register the StatusIB error formatter.
      */

src/app/ReadClient.cpp

@@ -254,9 +254,8 @@ CHIP_ERROR ReadClient::OnMessageReceived(Messaging::ExchangeContext * apExchange
     }
     else if (aPayloadHeader.HasMessageType(Protocols::InteractionModel::MsgType::StatusResponse))
     {
-        StatusIB status;
         VerifyOrExit(apExchangeContext == mpExchangeCtx, err = CHIP_ERROR_INCORRECT_STATE);
-        err = StatusResponse::ProcessStatusResponse(std::move(aPayload), status);
+        err = StatusResponse::ProcessStatusResponse(std::move(aPayload));
         SuccessOrExit(err);
     }
     else

src/app/ReadClient.h

@@ -138,6 +138,9 @@ class ReadClient : public Messaging::ExchangeDelegate
          *
          * - CHIP_ERROR_TIMEOUT: A response was not received within the expected response timeout.
          * - CHIP_ERROR_*TLV*: A malformed, non-compliant response was received from the server.
+         * - CHIP_ERROR encapsulating a StatusIB: If we got a non-path-specific
+         *   status response from the server.  In that case,
+         *   StatusIB::InitFromChipError can be used to extract the status.
          * - CHIP_ERROR*: All other cases.
          *
          * This object MUST continue to exist after this call is completed. The application shall wait until it

src/app/ReadHandler.cpp

@@ -143,8 +143,7 @@ CHIP_ERROR ReadHandler::OnReadInitialRequest(System::PacketBufferHandle && aPayl
 CHIP_ERROR ReadHandler::OnStatusResponse(Messaging::ExchangeContext * apExchangeContext, System::PacketBufferHandle && aPayload)
 {
     CHIP_ERROR err = CHIP_NO_ERROR;
-    StatusIB status;
-    err = StatusResponse::ProcessStatusResponse(std::move(aPayload), status);
+    err            = StatusResponse::ProcessStatusResponse(std::move(aPayload));
     SuccessOrExit(err);
     switch (mState)
     {

src/app/StatusResponse.cpp

@@ -42,9 +42,8 @@ CHIP_ERROR StatusResponse::Send(Protocols::InteractionModel::Status aStatus, Mes
     return CHIP_NO_ERROR;
 }
 
-CHIP_ERROR StatusResponse::ProcessStatusResponse(System::PacketBufferHandle && aPayload, StatusIB & aStatus)
+CHIP_ERROR StatusResponse::ProcessStatusResponse(System::PacketBufferHandle && aPayload)
 {
-    CHIP_ERROR err = CHIP_NO_ERROR;
     StatusResponseMessage::Parser response;
     System::PacketBufferTLVReader reader;
     reader.Init(std::move(aPayload));
@@ -53,22 +52,16 @@ CHIP_ERROR StatusResponse::ProcessStatusResponse(System::PacketBufferHandle && a
 #if CHIP_CONFIG_IM_ENABLE_SCHEMA_CHECK
     ReturnErrorOnFailure(response.CheckSchemaValidity());
 #endif
-    ReturnErrorOnFailure(response.GetStatus(aStatus.mStatus));
-    ChipLogProgress(InteractionModel, "Received status response, status is %" PRIu8, to_underlying(aStatus.mStatus));
+    StatusIB status;
+    ReturnErrorOnFailure(response.GetStatus(status.mStatus));
+    ChipLogProgress(InteractionModel, "Received status response, status is %" PRIu8, to_underlying(status.mStatus));
 
-    if (aStatus.mStatus == Protocols::InteractionModel::Status::Success)
+    if (status.mStatus == Protocols::InteractionModel::Status::Success)
     {
-        err = CHIP_NO_ERROR;
+        return CHIP_NO_ERROR;
     }
-    else if (aStatus.mStatus == Protocols::InteractionModel::Status::ResourceExhausted)
-    {
-        err = CHIP_ERROR_NO_MEMORY;
-    }
-    else
-    {
-        err = CHIP_ERROR_INCORRECT_STATE;
-    }
-    return err;
+
+    return status.ToChipError();
 }
 } // namespace app
 } // namespace chip

src/app/StatusResponse.h

@@ -32,7 +32,7 @@ class StatusResponse
 public:
     static CHIP_ERROR Send(Protocols::InteractionModel::Status aStatus, Messaging::ExchangeContext * apExchangeContext,
                            bool aExpectResponse);
-    static CHIP_ERROR ProcessStatusResponse(System::PacketBufferHandle && aPayload, StatusIB & aStatus);
+    static CHIP_ERROR ProcessStatusResponse(System::PacketBufferHandle && aPayload);
 };
 } // namespace app
 } // namespace chip