[im] Fix error code cases in IM[TE3]

src/app/CommandHandler.cpp

@@ -90,6 +90,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)
     {
@@ -108,13 +110,20 @@ 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)
+        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 puts the error status in response, now return success status so we can process
+    // other commands in the invoke request (by caller).
+    return CHIP_NO_ERROR;
 }
 
 CHIP_ERROR CommandHandler::AddStatusCode(const CommandPathParams * apCommandPathParams,

src/app/CommandSender.cpp

@@ -161,6 +161,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,17 @@ 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  If the endpoint contains the cluster and command.
+ */
+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

@@ -53,28 +53,46 @@ static TransportMgr<Transport::UDP> gTransportManager;
 static secure_channel::MessageCounterManager gMessageCounterManager;
 static Transport::AdminId gAdminId = 0;
 
+namespace {
+constexpr EndpointId kTestEndpointId = 1;
+constexpr ClusterId kTestClusterId   = 3;
+constexpr CommandId kTestCommandId   = 4;
+
+bool gCommandReachedDispatch = false;
+} // namespace
+
 namespace app {
 
 void DispatchSingleClusterCommand(chip::ClusterId aClusterId, chip::CommandId aCommandId, chip::EndpointId aEndPointId,
                                   chip::TLV::TLVReader & aReader, Command * apCommandObj)
 {
+    gCommandReachedDispatch = true;
     ChipLogDetail(Controller, "Received Cluster Command: Cluster=%" PRIx16 " Command=%" PRIx8 " Endpoint=%" PRIx8, aClusterId,
                   aCommandId, aEndPointId);
 }
 
+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 == kTestEndpointId);
+}
+
 class TestCommandInteraction
 {
 public:
     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);
     static void TestCommandHandlerWithProcessReceivedMsg(nlTestSuite * apSuite, void * apContext);
 
 private:
-    static void GenerateReceivedCommand(nlTestSuite * apSuite, void * apContext, System::PacketBufferHandle & aPayload);
+    static void GenerateReceivedCommand(nlTestSuite * apSuite, void * apContext, System::PacketBufferHandle & aPayload,
+                                        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,
@@ -90,7 +108,8 @@ class TestExchangeDelegate : public Messaging::ExchangeDelegate
     void OnResponseTimeout(Messaging::ExchangeContext * ec) override {}
 };
 
-void TestCommandInteraction::GenerateReceivedCommand(nlTestSuite * apSuite, void * apContext, System::PacketBufferHandle & aPayload)
+void TestCommandInteraction::GenerateReceivedCommand(nlTestSuite * apSuite, void * apContext, System::PacketBufferHandle & aPayload,
+                                                     EndpointId aEndpointId, ClusterId aClusterId, CommandId aCommandId)
 {
     CHIP_ERROR err = CHIP_NO_ERROR;
     InvokeCommand::Builder invokeCommandBuilder;
@@ -107,7 +126,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);
 
     chip::TLV::TLVWriter * pWriter = commandDataElementBuilder.GetWriter();
@@ -294,6 +313,24 @@ 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, 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.
+    gCommandReachedDispatch = false;
+    err                     = commandHandler.ProcessCommandMessage(std::move(commandDatabuf), Command::CommandRoleId::HandlerId);
+    NL_TEST_ASSERT(apSuite, !gCommandReachedDispatch);
+    commandHandler.Shutdown();
+}
+
 } // namespace app
 } // namespace chip
 
@@ -337,6 +374,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_SENTINEL()
 };
 // clang-format on

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