[im] Fix error code cases in IM[TE3] (#7002)

* [im] Return error when some cluster is not enabled on endpoint

The current code may return wrong success or unexpected error code when
one cluster is not enabled on endpoint, this commit added a function
thay will query cluster from ember cluster catalog, the function will be
replaced by cluster catalog.

* Add tests

* Add UT

* Address comments

* Address comments

* Address comments

* Update comments

* Fix test

src/app/CommandHandler.cpp

@@ -92,6 +92,8 @@ CHIP_ERROR CommandHandler::ProcessCommandDataElement(CommandDataElement::Parser
     err = commandPath.GetEndpointId(&endpointId);
     SuccessOrExit(err);
 
+    VerifyOrExit(ServerClusterCommandExists(clusterId, commandId, endpointId), err = CHIP_ERROR_INVALID_PROFILE_ID);
+
     err = aCommandElement.GetData(&commandDataReader);
     if (CHIP_END_OF_TLV == err)
     {
@@ -106,13 +108,21 @@ CHIP_ERROR CommandHandler::ProcessCommandDataElement(CommandDataElement::Parser
 exit:
     if (err != CHIP_NO_ERROR)
     {
-        // The Path is not present when there is an error to be conveyed back. ResponseCommandElement would only have status code,
-        // set the error with CHIP_NO_ERROR, then continue to process rest of commands
-        AddStatusCode(nullptr, GeneralStatusCode::kInvalidArgument, Protocols::SecureChannel::Id,
-                      Protocols::SecureChannel::kProtocolCodeGeneralFailure);
-        err = CHIP_NO_ERROR;
+        chip::app::CommandPathParams returnStatusParam = { endpointId,
+                                                           0, // GroupId
+                                                           clusterId, commandId, (chip::app::CommandPathFlags::kEndpointIdValid) };
+
+        // The Path is the path in the request if there are any error occurred before we dispatch the command to clusters.
+        // Currently, it could be failed to decode Path or failed to find cluster / command on desired endpoint.
+        // TODO: The behavior when receiving a malformed message is not clear in the Spec. (Spec#3259)
+        // TODO: The error code should be updated after #7072 added error codes required by IM.
+        AddStatusCode(&returnStatusParam,
+                      err == CHIP_ERROR_INVALID_PROFILE_ID ? GeneralStatusCode::kNotFound : GeneralStatusCode::kInvalidArgument,
+                      Protocols::SecureChannel::Id, Protocols::SecureChannel::kProtocolCodeGeneralFailure);
     }
-    return err;
+    // We have handled the error status above and put the error status in response, now return success status so we can process
+    // other commands in the invoke request.
+    return CHIP_NO_ERROR;
 }
 
 CHIP_ERROR CommandHandler::AddStatusCode(const CommandPathParams * apCommandPathParams,

src/app/CommandSender.cpp

@@ -163,6 +163,7 @@ CHIP_ERROR CommandSender::ProcessCommandDataElement(CommandDataElement::Parser &
     }
     else if (CHIP_END_OF_TLV == err)
     {
+        // TODO(Spec#3258): The endpoint id in response command is not clear, so we cannot do "ClientClusterCommandExists" check.
         err = aCommandElement.GetData(&commandDataReader);
         SuccessOrExit(err);
         // TODO(#4503): Should call callbacks of cluster that sends the command.

src/app/InteractionModelEngine.h

@@ -164,6 +164,18 @@ class InteractionModelEngine : public Messaging::ExchangeDelegate
 
 void DispatchSingleClusterCommand(chip::ClusterId aClusterId, chip::CommandId aCommandId, chip::EndpointId aEndPointId,
                                   chip::TLV::TLVReader & aReader, Command * apCommandObj);
+
+/**
+ *  Check whether the given cluster exists on the given endpoint and supports the given command.
+ *  TODO: The implementation lives in ember-compatibility-functions.cpp, this should be replaced by IM command catalog look up
+ * function after we have a cluster catalog in interaction model engine.
+ *  TODO: The endpoint id on response command (client side command) is unclear, so we don't have a ClientClusterCommandExists
+ * function. (Spec#3258)
+ *
+ *  @retval  True if the endpoint contains the server side of the given cluster and that cluster implements the given command, false
+ * otherwise.
+ */
+bool ServerClusterCommandExists(chip::ClusterId aClusterId, chip::CommandId aCommandId, chip::EndpointId aEndPointId);
 CHIP_ERROR ReadSingleClusterData(ClusterInfo & aClusterInfo, TLV::TLVWriter & aWriter);
 CHIP_ERROR WriteSingleClusterData(ClusterInfo & aClusterInfo, TLV::TLVReader & aReader);
 } // namespace app

src/app/tests/TestCommandInteraction.cpp

@@ -54,6 +54,12 @@ static secure_channel::MessageCounterManager gMessageCounterManager;
 static Transport::AdminId gAdminId = 0;
 static bool isCommandDispatched    = false;
 
+namespace {
+constexpr EndpointId kTestEndpointId = 1;
+constexpr ClusterId kTestClusterId   = 3;
+constexpr CommandId kTestCommandId   = 4;
+} // namespace
+
 namespace app {
 
 void DispatchSingleClusterCommand(chip::ClusterId aClusterId, chip::CommandId aCommandId, chip::EndpointId aEndPointId,
@@ -64,6 +70,7 @@ void DispatchSingleClusterCommand(chip::ClusterId aClusterId, chip::CommandId aC
                                                        aClusterId,  // ClusterId
                                                        aCommandId,  // CommandId
                                                        (chip::app::CommandPathFlags::kEndpointIdValid) };
+
     ChipLogDetail(Controller, "Received Cluster Command: Cluster=%" PRIx16 " Command=%" PRIx8 " Endpoint=%" PRIx8, aClusterId,
                   aCommandId, aEndPointId);
 
@@ -73,6 +80,12 @@ void DispatchSingleClusterCommand(chip::ClusterId aClusterId, chip::CommandId aC
     chip::isCommandDispatched = true;
 }
 
+bool ServerClusterCommandExists(chip::ClusterId aClusterId, chip::CommandId aCommandId, chip::EndpointId aEndPointId)
+{
+    // Mock cluster catalog, only support one command on one cluster on one endpoint.
+    return (aEndPointId == kTestEndpointId && aClusterId == kTestClusterId && aCommandId == kTestCommandId);
+}
+
 class TestCommandInteraction
 {
 public:
@@ -81,6 +94,7 @@ class TestCommandInteraction
     static void TestCommandSenderWithSendCommand(nlTestSuite * apSuite, void * apContext);
     static void TestCommandHandlerWithSendEmptyCommand(nlTestSuite * apSuite, void * apContext);
     static void TestCommandSenderWithProcessReceivedMsg(nlTestSuite * apSuite, void * apContext);
+    static void TestCommandHandlerWithProcessReceivedNotExistCommand(nlTestSuite * apSuite, void * apContext);
     static void TestCommandHandlerWithSendSimpleCommandData(nlTestSuite * apSuite, void * apContext);
     static void TestCommandHandlerWithSendSimpleStatusCode(nlTestSuite * apSuite, void * apContext);
     static void TestCommandHandlerWithSendEmptyResponse(nlTestSuite * apSuite, void * apContext);
@@ -89,7 +103,8 @@ class TestCommandInteraction
 
 private:
     static void GenerateReceivedCommand(nlTestSuite * apSuite, void * apContext, System::PacketBufferHandle & aPayload,
-                                        bool aNeedCommandData);
+                                        bool aNeedCommandData, EndpointId aEndpointId = kTestEndpointId,
+                                        ClusterId aClusterId = kTestClusterId, CommandId aCommandId = kTestCommandId);
     static void AddCommandDataElement(nlTestSuite * apSuite, void * apContext, Command * apCommand, bool aNeedStatusCode,
                                       bool aIsEmptyResponse);
     static void ValidateCommandHandlerWithSendCommand(nlTestSuite * apSuite, void * apContext, bool aNeedStatusCode,
@@ -106,7 +121,9 @@ class TestExchangeDelegate : public Messaging::ExchangeDelegate
 };
 
 void TestCommandInteraction::GenerateReceivedCommand(nlTestSuite * apSuite, void * apContext, System::PacketBufferHandle & aPayload,
-                                                     bool aNeedCommandData)
+                                                     bool aNeedCommandData, EndpointId aEndpointId, ClusterId aClusterId,
+                                                     CommandId aCommandId)
+
 {
     CHIP_ERROR err = CHIP_NO_ERROR;
     InvokeCommand::Builder invokeCommandBuilder;
@@ -123,7 +140,7 @@ void TestCommandInteraction::GenerateReceivedCommand(nlTestSuite * apSuite, void
     NL_TEST_ASSERT(apSuite, commandList.GetError() == CHIP_NO_ERROR);
     CommandPath::Builder commandPathBuilder = commandDataElementBuilder.CreateCommandPathBuilder();
     NL_TEST_ASSERT(apSuite, commandDataElementBuilder.GetError() == CHIP_NO_ERROR);
-    commandPathBuilder.EndpointId(1).ClusterId(3).CommandId(4).EndOfCommandPath();
+    commandPathBuilder.EndpointId(aEndpointId).ClusterId(aClusterId).CommandId(aCommandId).EndOfCommandPath();
     NL_TEST_ASSERT(apSuite, commandPathBuilder.GetError() == CHIP_NO_ERROR);
 
     if (aNeedCommandData)
@@ -361,6 +378,25 @@ void TestCommandInteraction::TestCommandHandlerWithProcessReceivedMsg(nlTestSuit
     commandHandler.Shutdown();
 }
 
+void TestCommandInteraction::TestCommandHandlerWithProcessReceivedNotExistCommand(nlTestSuite * apSuite, void * apContext)
+{
+    CHIP_ERROR err = CHIP_NO_ERROR;
+    app::CommandHandler commandHandler;
+    System::PacketBufferHandle commandDatabuf = System::PacketBufferHandle::New(System::PacketBuffer::kMaxSize);
+    err                                       = commandHandler.Init(&chip::gExchangeManager, nullptr);
+    NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
+    // Use some invalid endpoint / cluster / command.
+    GenerateReceivedCommand(apSuite, apContext, commandDatabuf, false /*aNeedCommandData*/, 0xDE /* endpoint */,
+                            0xADBE /* cluster */, 0xEF /* command */);
+
+    // TODO: Need to find a way to get the response instead of only check if a function on key path is called.
+    // We should not reach CommandDispatch if requested command does not exist.
+    chip::isCommandDispatched = false;
+    err                       = commandHandler.ProcessCommandMessage(std::move(commandDatabuf), Command::CommandRoleId::HandlerId);
+    NL_TEST_ASSERT(apSuite, !chip::isCommandDispatched);
+    commandHandler.Shutdown();
+}
+
 void TestCommandInteraction::TestCommandHandlerWithProcessReceivedEmptyDataMsg(nlTestSuite * apSuite, void * apContext)
 {
     CHIP_ERROR err = CHIP_NO_ERROR;
@@ -420,6 +456,7 @@ const nlTest sTests[] =
     NL_TEST_DEF("TestCommandHandlerWithSendSimpleStatusCode", chip::app::TestCommandInteraction::TestCommandHandlerWithSendSimpleStatusCode),
     NL_TEST_DEF("TestCommandHandlerWithSendEmptyResponse", chip::app::TestCommandInteraction::TestCommandHandlerWithSendEmptyResponse),
     NL_TEST_DEF("TestCommandHandlerWithProcessReceivedMsg", chip::app::TestCommandInteraction::TestCommandHandlerWithProcessReceivedMsg),
+    NL_TEST_DEF("TestCommandHandlerWithProcessReceivedNotExistCommand", chip::app::TestCommandInteraction::TestCommandHandlerWithProcessReceivedNotExistCommand),
     NL_TEST_DEF("TestCommandHandlerWithProcessReceivedEmptyDataMsg", chip::app::TestCommandInteraction::TestCommandHandlerWithProcessReceivedEmptyDataMsg),
     NL_TEST_SENTINEL()
 };

src/app/tests/integration/chip_im_initiator.cpp

@@ -45,10 +45,11 @@
 namespace {
 // Max value for the number of message request sent.
 
-constexpr size_t kMaxCommandMessageCount    = 3;
-constexpr size_t kMaxReadMessageCount       = 3;
-constexpr int32_t gMessageIntervalSeconds   = 1;
-constexpr chip::Transport::AdminId gAdminId = 0;
+constexpr size_t kMaxCommandMessageCount          = 3;
+constexpr size_t kTotalFailureCommandMessageCount = 1;
+constexpr size_t kMaxReadMessageCount             = 3;
+constexpr int32_t gMessageIntervalSeconds         = 1;
+constexpr chip::Transport::AdminId gAdminId       = 0;
 
 // The ReadClient object.
 chip::app::ReadClient * gpReadClient = nullptr;
@@ -74,6 +75,16 @@ uint64_t gReadCount = 0;
 // Count of the number of CommandResponses received.
 uint64_t gReadRespCount = 0;
 
+// Whether the last command successed.
+enum class TestCommandResult : uint8_t
+{
+    kUndefined,
+    kSuccess,
+    kFailure
+};
+
+TestCommandResult gLastCommandResult = TestCommandResult::kUndefined;
+
 std::condition_variable gCond;
 
 CHIP_ERROR SendCommandRequest(chip::app::CommandSender * commandSender)
@@ -126,6 +137,43 @@ CHIP_ERROR SendCommandRequest(chip::app::CommandSender * commandSender)
     return err;
 }
 
+CHIP_ERROR SendBadCommandRequest(chip::app::CommandSender * commandSender)
+{
+    CHIP_ERROR err = CHIP_NO_ERROR;
+
+    VerifyOrReturnError(commandSender != nullptr, CHIP_ERROR_INCORRECT_STATE);
+
+    gLastMessageTime = chip::System::Timer::GetCurrentEpoch();
+
+    printf("\nSend invoke command request message to Node: %" PRIu64 "\n", chip::kTestDeviceNodeId);
+
+    chip::app::CommandPathParams commandPathParams = { 0xDE,   // Bad Endpoint
+                                                       0xADBE, // Bad GroupId
+                                                       0xEFCA, // Bad ClusterId
+                                                       0xFE,   // Bad CommandId
+                                                       chip::app::CommandPathFlags::kEndpointIdValid };
+
+    err = commandSender->PrepareCommand(&commandPathParams);
+    SuccessOrExit(err);
+
+    err = commandSender->FinishCommand();
+    SuccessOrExit(err);
+
+    err = commandSender->SendCommandRequest(chip::kTestDeviceNodeId, gAdminId);
+    SuccessOrExit(err);
+
+    if (err == CHIP_NO_ERROR)
+    {
+        gCommandCount++;
+    }
+    else
+    {
+        printf("Send invoke command request failed, err: %s\n", chip::ErrorStr(err));
+    }
+exit:
+    return err;
+}
+
 CHIP_ERROR SendReadRequest(void)
 {
     CHIP_ERROR err           = CHIP_NO_ERROR;
@@ -220,6 +268,9 @@ class MockInteractionModelApp : public chip::app::InteractionModelDelegate
         printf("CommandResponseStatus with GeneralCode %d, ProtocolId %d, ProtocolCode %d, EndpointId %d, ClusterId %d, CommandId "
                "%d, CommandIndex %d",
                static_cast<uint16_t>(aGeneralCode), aProtocolId, aProtocolCode, aEndpointId, aClusterId, aCommandId, aCommandIndex);
+        gLastCommandResult = (aGeneralCode == chip::Protocols::SecureChannel::GeneralStatusCode::kSuccess && aProtocolCode == 0)
+            ? TestCommandResult::kSuccess
+            : TestCommandResult::kFailure;
         return CHIP_NO_ERROR;
     }
 
@@ -255,6 +306,12 @@ class MockInteractionModelApp : public chip::app::InteractionModelDelegate
 namespace chip {
 namespace app {
 
+bool ServerClusterCommandExists(chip::ClusterId aClusterId, chip::CommandId aCommandId, chip::EndpointId aEndPointId)
+{
+    // Always return true in test.
+    return true;
+}
+
 void DispatchSingleClusterCommand(chip::ClusterId aClusterId, chip::CommandId aCommandId, chip::EndpointId aEndPointId,
                                   chip::TLV::TLVReader & aReader, Command * apCommandObj)
 {
@@ -267,6 +324,8 @@ void DispatchSingleClusterCommand(chip::ClusterId aClusterId, chip::CommandId aC
     {
         chip::TLV::Debug::Dump(aReader, TLVPrettyPrinter);
     }
+
+    gLastCommandResult = TestCommandResult::kSuccess;
 }
 
 CHIP_ERROR WriteSingleClusterData(ClusterInfo & aClusterInfo, TLV::TLVReader & aReader)
@@ -347,13 +406,35 @@ int main(int argc, char * argv[])
         if (err != CHIP_NO_ERROR)
         {
             printf("Send command request failed: %s\n", chip::ErrorStr(err));
-            goto exit;
+            ExitNow();
         }
 
         if (gCond.wait_for(lock, std::chrono::seconds(gMessageIntervalSeconds)) == std::cv_status::timeout)
         {
             printf("Invoke Command: No response received\n");
         }
+
+        VerifyOrExit(gLastCommandResult == TestCommandResult::kSuccess, err = CHIP_ERROR_INCORRECT_STATE);
+    }
+
+    // Test with invalid endpoint / cluster / command combination.
+    {
+        chip::app::CommandSender * commandSender;
+        err = chip::app::InteractionModelEngine::GetInstance()->NewCommandSender(&commandSender);
+        SuccessOrExit(err);
+        err = SendBadCommandRequest(commandSender);
+        if (err != CHIP_NO_ERROR)
+        {
+            printf("Send command request failed: %s\n", chip::ErrorStr(err));
+            ExitNow();
+        }
+
+        if (gCond.wait_for(lock, std::chrono::seconds(gMessageIntervalSeconds)) == std::cv_status::timeout)
+        {
+            printf("Invoke Command: No response received\n");
+        }
+
+        VerifyOrExit(gLastCommandResult == TestCommandResult::kFailure, err = CHIP_ERROR_INCORRECT_STATE);
     }
 
     // Connection has been established. Now send the ReadRequests.
@@ -377,7 +458,7 @@ int main(int argc, char * argv[])
     ShutdownChip();
 
 exit:
-    if (err != CHIP_NO_ERROR || (gCommandRespCount != kMaxCommandMessageCount))
+    if (err != CHIP_NO_ERROR || (gCommandRespCount != kMaxCommandMessageCount + kTotalFailureCommandMessageCount))
     {
         printf("ChipCommandSender failed: %s\n", chip::ErrorStr(err));
         exit(EXIT_FAILURE);

src/app/tests/integration/chip_im_responder.cpp

@@ -44,6 +44,13 @@
 
 namespace chip {
 namespace app {
+
+bool ServerClusterCommandExists(chip::ClusterId aClusterId, chip::CommandId aCommandId, chip::EndpointId aEndPointId)
+{
+    // The Mock cluster catalog -- only have one command on one cluster on one endpoint.
+    return (aEndPointId == kTestEndpointId && aClusterId == kTestClusterId && aCommandId == kTestCommandId);
+}
+
 void DispatchSingleClusterCommand(chip::ClusterId aClusterId, chip::CommandId aCommandId, chip::EndpointId aEndPointId,
                                   chip::TLV::TLVReader & aReader, Command * apCommandObj)
 {

src/app/util/ember-compatibility-functions.cpp

@@ -21,9 +21,9 @@
  *          when calling ember callbacks.
  */
 
-#include <app/util/ember-compatibility-functions.h>
-
 #include <app/Command.h>
+#include <app/InteractionModelEngine.h>
+#include <app/util/ember-compatibility-functions.h>
 #include <app/util/util.h>
 #include <lib/core/CHIPCore.h>
 #include <lib/core/CHIPTLV.h>
@@ -65,15 +65,17 @@ bool IMEmberAfSendDefaultResponseWithCallback(EmberAfStatus status)
         return false;
     }
 
-    chip::app::CommandPathParams returnStatusParam = { imCompatibilityEmberApsFrame.sourceEndpoint,
+    chip::app::CommandPathParams returnStatusParam = { imCompatibilityEmberApsFrame.destinationEndpoint,
                                                        0, // GroupId
                                                        imCompatibilityEmberApsFrame.clusterId,
                                                        imCompatibilityEmberAfCluster.commandId,
                                                        (chip::app::CommandPathFlags::kEndpointIdValid) };
 
-    CHIP_ERROR err =
-        currentCommandObject->AddStatusCode(&returnStatusParam, chip::Protocols::SecureChannel::GeneralStatusCode::kSuccess,
-                                            chip::Protocols::InteractionModel::Id, status);
+    CHIP_ERROR err = currentCommandObject->AddStatusCode(&returnStatusParam,
+                                                         status == EMBER_ZCL_STATUS_SUCCESS
+                                                             ? chip::Protocols::SecureChannel::GeneralStatusCode::kSuccess
+                                                             : chip::Protocols::SecureChannel::GeneralStatusCode::kFailure,
+                                                         chip::Protocols::InteractionModel::Id, status);
     return CHIP_NO_ERROR == err;
 }
 
@@ -84,5 +86,13 @@ void ResetEmberAfObjects()
 }
 
 } // namespace Compatibility
+
+bool ServerClusterCommandExists(chip::ClusterId aClusterId, chip::CommandId aCommandId, chip::EndpointId aEndPointId)
+{
+    // TODO: Currently, we are using cluster catalog from the ember library, this should be modified or replaced after several
+    // updates to Commands.
+    return emberAfContainsServer(aEndPointId, aClusterId);
+}
+
 } // namespace app
 } // namespace chip