Improve chip-tool help for "synthetic" clusters. (#27665)

* Improve chip-tool help for "synthetic" clusters. 1. Makes it clear which things are actual clusters and which are command sets. 2. Makes sure all command sets have overall help text that describes what the commands in the set do. 3. Adds some help text to some specific commands. * Address review comment. * Address review comments.
project-chip · Jul 11, 2023 · 6f8d602 · 6f8d602
1 parent d0be501
commit 6f8d602
Show file tree

Hide file tree

Showing 27 changed files with 321 additions and 266 deletions.
diff --git a/examples/chip-tool/commands/clusters/SubscriptionsCommands.h b/examples/chip-tool/commands/clusters/SubscriptionsCommands.h
@@ -104,5 +104,5 @@ void registerCommandsSubscriptions(Commands & commands, CredentialIssuerCommands
         make_unique<ShutdownAllSubscriptions>(credsIssuerConfig),     //
     };
 
-    commands.Register(clusterName, clusterCommands, "Commands for shutting down subscriptions.");
+    commands.RegisterCommandSet(clusterName, clusterCommands, "Commands for shutting down subscriptions.");
 }
diff --git a/examples/chip-tool/commands/common/Commands.cpp b/examples/chip-tool/commands/common/Commands.cpp
@@ -119,12 +119,14 @@ static void DetectAndLogMismatchedDoubleQuotes(int argc, char ** argv)
 
 } // namespace
 
-void Commands::Register(const char * clusterName, commands_list commandsList, const char * helpText)
+void Commands::Register(const char * commandSetName, commands_list commandsList, const char * helpText, bool isCluster)
 {
-    mClusters[clusterName].second = helpText;
+    VerifyOrDieWithMsg(isCluster || helpText != nullptr, chipTool, "Non-cluster command sets must have help text");
+    mCommandSets[commandSetName].isCluster = isCluster;
+    mCommandSets[commandSetName].helpText  = helpText;
     for (auto & command : commandsList)
     {
-        mClusters[clusterName].first.push_back(std::move(command));
+        mCommandSets[commandSetName].commands.push_back(std::move(command));
     }
 }
 
@@ -192,26 +194,26 @@ CHIP_ERROR Commands::RunCommand(int argc, char ** argv, bool interactive,
 
     if (argc <= 1)
     {
-        ChipLogError(chipTool, "Missing cluster name");
-        ShowClusters(argv[0]);
+        ChipLogError(chipTool, "Missing cluster or command set name");
+        ShowCommandSets(argv[0]);
         return CHIP_ERROR_INVALID_ARGUMENT;
     }
 
-    auto clusterIter = GetCluster(argv[1]);
-    if (clusterIter == mClusters.end())
+    auto commandSetIter = GetCommandSet(argv[1]);
+    if (commandSetIter == mCommandSets.end())
     {
-        ChipLogError(chipTool, "Unknown cluster: %s", argv[1]);
-        ShowClusters(argv[0]);
+        ChipLogError(chipTool, "Unknown cluster or command set: %s", argv[1]);
+        ShowCommandSets(argv[0]);
         return CHIP_ERROR_INVALID_ARGUMENT;
     }
 
-    auto & commandList = clusterIter->second.first;
-    auto * clusterHelp = clusterIter->second.second;
+    auto & commandList = commandSetIter->second.commands;
+    auto * helpText    = commandSetIter->second.helpText;
 
     if (argc <= 2)
     {
         ChipLogError(chipTool, "Missing command name");
-        ShowCluster(argv[0], argv[1], commandList, clusterHelp);
+        ShowCommandSet(argv[0], argv[1], commandList, helpText);
         return CHIP_ERROR_INVALID_ARGUMENT;
     }
 
@@ -222,7 +224,7 @@ CHIP_ERROR Commands::RunCommand(int argc, char ** argv, bool interactive,
         if (command == nullptr)
         {
             ChipLogError(chipTool, "Unknown command: %s", argv[2]);
-            ShowCluster(argv[0], argv[1], commandList, clusterHelp);
+            ShowCommandSet(argv[0], argv[1], commandList, helpText);
             return CHIP_ERROR_INVALID_ARGUMENT;
         }
     }
@@ -293,19 +295,19 @@ CHIP_ERROR Commands::RunCommand(int argc, char ** argv, bool interactive,
     return command->Run();
 }
 
-Commands::ClusterMap::iterator Commands::GetCluster(std::string clusterName)
+Commands::CommandSetMap::iterator Commands::GetCommandSet(std::string commandSetName)
 {
-    for (auto & cluster : mClusters)
+    for (auto & commandSet : mCommandSets)
     {
-        std::string key(cluster.first);
+        std::string key(commandSet.first);
         std::transform(key.begin(), key.end(), key.begin(), ::tolower);
-        if (key.compare(clusterName) == 0)
+        if (key.compare(commandSetName) == 0)
         {
-            return mClusters.find(cluster.first);
+            return mCommandSets.find(commandSet.first);
         }
     }
 
-    return mClusters.end();
+    return mCommandSets.end();
 }
 
 Command * Commands::GetCommand(CommandsVector & commands, std::string commandName)
@@ -350,29 +352,53 @@ bool Commands::IsGlobalCommand(std::string commandName) const
     return IsAttributeCommand(commandName) || IsEventCommand(commandName);
 }
 
-void Commands::ShowClusters(std::string executable)
+void Commands::ShowCommandSetOverview(std::string commandSetName, const CommandSet & commandSet)
+{
+    std::transform(commandSetName.begin(), commandSetName.end(), commandSetName.begin(),
+                   [](unsigned char c) { return std::tolower(c); });
+    fprintf(stderr, "  | * %-82s|\n", commandSetName.c_str());
+    ShowHelpText(commandSet.helpText);
+}
+
+void Commands::ShowCommandSets(std::string executable)
 {
     fprintf(stderr, "Usage:\n");
     fprintf(stderr, "  %s cluster_name command_name [param1 param2 ...]\n", executable.c_str());
+    fprintf(stderr, "or:\n");
+    fprintf(stderr, "  %s command_set_name command_name [param1 param2 ...]\n", executable.c_str());
     fprintf(stderr, "\n");
+    // Table of clusters
     fprintf(stderr, "  +-------------------------------------------------------------------------------------+\n");
     fprintf(stderr, "  | Clusters:                                                                           |\n");
     fprintf(stderr, "  +-------------------------------------------------------------------------------------+\n");
-    for (auto & cluster : mClusters)
+    for (auto & commandSet : mCommandSets)
+    {
+        if (commandSet.second.isCluster)
+        {
+            ShowCommandSetOverview(commandSet.first, commandSet.second);
+        }
+    }
+    fprintf(stderr, "  +-------------------------------------------------------------------------------------+\n");
+    fprintf(stderr, "\n");
+
+    // Table of command sets
+    fprintf(stderr, "  +-------------------------------------------------------------------------------------+\n");
+    fprintf(stderr, "  | Command sets:                                                                       |\n");
+    fprintf(stderr, "  +-------------------------------------------------------------------------------------+\n");
+    for (auto & commandSet : mCommandSets)
     {
-        std::string clusterName(cluster.first);
-        std::transform(clusterName.begin(), clusterName.end(), clusterName.begin(),
-                       [](unsigned char c) { return std::tolower(c); });
-        fprintf(stderr, "  | * %-82s|\n", clusterName.c_str());
-        ShowHelpText(cluster.second.second);
+        if (!commandSet.second.isCluster)
+        {
+            ShowCommandSetOverview(commandSet.first, commandSet.second);
+        }
     }
     fprintf(stderr, "  +-------------------------------------------------------------------------------------+\n");
 }
 
-void Commands::ShowCluster(std::string executable, std::string clusterName, CommandsVector & commands, const char * helpText)
+void Commands::ShowCommandSet(std::string executable, std::string commandSetName, CommandsVector & commands, const char * helpText)
 {
     fprintf(stderr, "Usage:\n");
-    fprintf(stderr, "  %s %s command_name [param1 param2 ...]\n", executable.c_str(), clusterName.c_str());
+    fprintf(stderr, "  %s %s command_name [param1 param2 ...]\n", executable.c_str(), commandSetName.c_str());
 
     if (helpText)
     {
@@ -554,11 +580,11 @@ bool Commands::DecodeArgumentsFromBase64EncodedJson(const char * json, std::vect
     auto commandName = jsonValue[kJsonCommandKey].asString();
     auto arguments   = jsonValue[kJsonArgumentsKey].asString();
 
-    auto clusterIter = GetCluster(clusterName);
-    VerifyOrReturnValue(clusterIter != mClusters.end(), false,
+    auto clusterIter = GetCommandSet(clusterName);
+    VerifyOrReturnValue(clusterIter != mCommandSets.end(), false,
                         ChipLogError(chipTool, "Cluster '%s' is not supported.", clusterName.c_str()));
 
-    auto & commandList = clusterIter->second.first;
+    auto & commandList = clusterIter->second.commands;
 
     auto command = GetCommand(commandList, commandName);
 

diff --git a/examples/chip-tool/commands/common/Commands.h b/examples/chip-tool/commands/common/Commands.h
@@ -30,25 +30,44 @@ class Commands
 public:
     using CommandsVector = ::std::vector<std::unique_ptr<Command>>;
 
-    void Register(const char * clusterName, commands_list commandsList, const char * helpText = nullptr);
+    void RegisterCluster(const char * clusterName, commands_list commandsList)
+    {
+        Register(clusterName, commandsList, nullptr, true);
+    }
+    // Command sets represent chip-tool functionality that is not actually
+    // XML-defined clusters.  All command sets should have help text explaining
+    // what sort of commands one should expect to find in the set.
+    void RegisterCommandSet(const char * commandSetName, commands_list commandsList, const char * helpText)
+    {
+        Register(commandSetName, commandsList, helpText, false);
+    }
     int Run(int argc, char ** argv);
     int RunInteractive(const char * command, const chip::Optional<char *> & storageDirectory = chip::NullOptional);
 
 private:
-    using ClusterMap = std::map<std::string, std::pair<CommandsVector, const char *>>;
+    struct CommandSet
+    {
+        CommandsVector commands;
+        bool isCluster        = false;
+        const char * helpText = nullptr;
+    };
+    // The tuple contains the commands, whether it's a synthetic cluster, and
+    // the help text for the cluster (which may be null).
+    using CommandSetMap = std::map<std::string, CommandSet>;
 
     CHIP_ERROR RunCommand(int argc, char ** argv, bool interactive = false,
                           const chip::Optional<char *> & interactiveStorageDirectory = chip::NullOptional);
 
-    ClusterMap::iterator GetCluster(std::string clusterName);
+    CommandSetMap::iterator GetCommandSet(std::string commandSetName);
     Command * GetCommand(CommandsVector & commands, std::string commandName);
     Command * GetGlobalCommand(CommandsVector & commands, std::string commandName, std::string attributeName);
     bool IsAttributeCommand(std::string commandName) const;
     bool IsEventCommand(std::string commandName) const;
     bool IsGlobalCommand(std::string commandName) const;
 
-    void ShowClusters(std::string executable);
-    void ShowCluster(std::string executable, std::string clusterName, CommandsVector & commands, const char * helpText);
+    void ShowCommandSets(std::string executable);
+    static void ShowCommandSetOverview(std::string commandSetName, const CommandSet & commandSet);
+    void ShowCommandSet(std::string executable, std::string commandSetName, CommandsVector & commands, const char * helpText);
     void ShowClusterAttributes(std::string executable, std::string clusterName, std::string commandName, CommandsVector & commands);
     void ShowClusterEvents(std::string executable, std::string clusterName, std::string commandName, CommandsVector & commands);
     void ShowCommand(std::string executable, std::string clusterName, Command * command);
@@ -60,7 +79,9 @@ class Commands
     // helpText may be null, in which case it's not shown.
     static void ShowHelpText(const char * helpText);
 
-    ClusterMap mClusters;
+    void Register(const char * commandSetName, commands_list commandsList, const char * helpText, bool isCluster);
+
+    CommandSetMap mCommandSets;
 #ifdef CONFIG_USE_LOCAL_STORAGE
     PersistentStorage mStorage;
 #endif // CONFIG_USE_LOCAL_STORAGE

diff --git a/examples/chip-tool/commands/delay/Commands.h b/examples/chip-tool/commands/delay/Commands.h
@@ -30,5 +30,5 @@ void registerCommandsDelay(Commands & commands, CredentialIssuerCommands * creds
         make_unique<WaitForCommissioneeCommand>(credsIssuerConfig), //
     };
 
-    commands.Register(clusterName, clusterCommands);
+    commands.RegisterCommandSet(clusterName, clusterCommands, "Commands for waiting for something to happen.");
 }
diff --git a/examples/chip-tool/commands/delay/WaitForCommissioneeCommand.h b/examples/chip-tool/commands/delay/WaitForCommissioneeCommand.h
@@ -26,8 +26,8 @@ class WaitForCommissioneeCommand : public CHIPCommand
 {
 public:
     WaitForCommissioneeCommand(CredentialIssuerCommands * credIssuerCommands) :
-        CHIPCommand("wait-for-commissionee", credIssuerCommands), mOnDeviceConnectedCallback(OnDeviceConnectedFn, this),
-        mOnDeviceConnectionFailureCallback(OnDeviceConnectionFailureFn, this)
+        CHIPCommand("wait-for-commissionee", credIssuerCommands, "Establish a CASE session to the provided node id."),
+        mOnDeviceConnectedCallback(OnDeviceConnectedFn, this), mOnDeviceConnectionFailureCallback(OnDeviceConnectionFailureFn, this)
     {
         AddArgument("nodeId", 0, UINT64_MAX, &mNodeId);
         AddArgument("expire-existing-session", 0, 1, &mExpireExistingSession);

diff --git a/examples/chip-tool/commands/discover/Commands.h b/examples/chip-tool/commands/discover/Commands.h
@@ -85,5 +85,5 @@ void registerCommandsDiscover(Commands & commands, CredentialIssuerCommands * cr
         make_unique<DiscoverCommissionersCommand>(credsIssuerConfig),
     };
 
-    commands.Register(clusterName, clusterCommands);
+    commands.RegisterCommandSet(clusterName, clusterCommands, "Commands for device discovery.");
 }
diff --git a/examples/chip-tool/commands/group/Commands.h b/examples/chip-tool/commands/group/Commands.h
@@ -358,5 +358,6 @@ void registerCommandsGroup(Commands & commands, CredentialIssuerCommands * creds
         make_unique<RemoveKeySet>(credsIssuerConfig),
     };
 
-    commands.Register(clusterName, clusterCommands);
+    commands.RegisterCommandSet(clusterName, clusterCommands,
+                                "Commands for manipulating group keys and memberships for chip-tool itself.");
 }
diff --git a/examples/chip-tool/commands/interactive/Commands.h b/examples/chip-tool/commands/interactive/Commands.h
@@ -33,5 +33,5 @@ void registerCommandsInteractive(Commands & commands, CredentialIssuerCommands *
 #endif // CONFIG_USE_INTERACTIVE_MODE
     };
 
-    commands.Register(clusterName, clusterCommands);
+    commands.RegisterCommandSet(clusterName, clusterCommands, "Commands for starting long-lived interactive modes.");
 }
diff --git a/examples/chip-tool/commands/interactive/InteractiveCommands.h b/examples/chip-tool/commands/interactive/InteractiveCommands.h
@@ -29,8 +29,10 @@ class Commands;
 class InteractiveCommand : public CHIPCommand
 {
 public:
-    InteractiveCommand(const char * name, Commands * commandsHandler, CredentialIssuerCommands * credsIssuerConfig) :
-        CHIPCommand(name, credsIssuerConfig), mHandler(commandsHandler)
+    InteractiveCommand(const char * name, Commands * commandsHandler, const char * helpText,
+                       CredentialIssuerCommands * credsIssuerConfig) :
+        CHIPCommand(name, credsIssuerConfig, helpText),
+        mHandler(commandsHandler)
     {
         AddArgument("advertise-operational", 0, 1, &mAdvertiseOperational,
                     "Advertise operational node over DNS-SD and accept incoming CASE sessions.");
@@ -51,7 +53,8 @@ class InteractiveStartCommand : public InteractiveCommand
 {
 public:
     InteractiveStartCommand(Commands * commandsHandler, CredentialIssuerCommands * credsIssuerConfig) :
-        InteractiveCommand("start", commandsHandler, credsIssuerConfig)
+        InteractiveCommand("start", commandsHandler, "Start an interactive shell that can then run other commands.",
+                           credsIssuerConfig)
     {}
 
     /////////// CHIPCommand Interface /////////
@@ -62,7 +65,8 @@ class InteractiveServerCommand : public InteractiveCommand, public WebSocketServ
 {
 public:
     InteractiveServerCommand(Commands * commandsHandler, CredentialIssuerCommands * credsIssuerConfig) :
-        InteractiveCommand("server", commandsHandler, credsIssuerConfig)
+        InteractiveCommand("server", commandsHandler, "Start a websocket server that can receive commands sent by another process.",
+                           credsIssuerConfig)
     {
         AddArgument("port", 0, UINT16_MAX, &mPort, "Port the websocket will listen to. Defaults to 9002.");
     }

diff --git a/examples/chip-tool/commands/pairing/Commands.h b/examples/chip-tool/commands/pairing/Commands.h
@@ -255,5 +255,5 @@ void registerCommandsPairing(Commands & commands, CredentialIssuerCommands * cre
         make_unique<IssueNOCChainCommand>(credsIssuerConfig),
     };
 
-    commands.Register(clusterName, clusterCommands);
+    commands.RegisterCommandSet(clusterName, clusterCommands, "Commands for commissioning devices.");
 }
diff --git a/examples/chip-tool/commands/payload/Commands.h b/examples/chip-tool/commands/payload/Commands.h
@@ -36,5 +36,5 @@ void registerCommandsPayload(Commands & commands)
         make_unique<SetupPayloadVerhoeffGenerate>(),          //
     };
 
-    commands.Register(clusterName, clusterCommands);
+    commands.RegisterCommandSet(clusterName, clusterCommands, "Commands for parsing and generating setup payloads.");
 }
diff --git a/examples/chip-tool/commands/session-management/Commands.h b/examples/chip-tool/commands/session-management/Commands.h
@@ -30,5 +30,5 @@ void registerCommandsSessionManagement(Commands & commands, CredentialIssuerComm
         make_unique<EvictLocalCASESessionsCommand>(credsIssuerConfig),
     };
 
-    commands.Register(clusterName, clusterCommands, "Commands for managing CASE and PASE session state");
+    commands.RegisterCommandSet(clusterName, clusterCommands, "Commands for managing CASE and PASE session state.");
 }
diff --git a/examples/chip-tool/commands/storage/Commands.h b/examples/chip-tool/commands/storage/Commands.h
@@ -27,5 +27,5 @@ void registerCommandsStorage(Commands & commands)
 
     commands_list clusterCommands = { make_unique<StorageClearAll>() };
 
-    commands.Register(clusterName, clusterCommands);
+    commands.RegisterCommandSet(clusterName, clusterCommands, "Commands for managing persistent data stored by chip-tool.");
 }
diff --git a/examples/chip-tool/templates/commands.zapt b/examples/chip-tool/templates/commands.zapt
@@ -117,7 +117,7 @@ void registerCluster{{asUpperCamelCase name}}(Commands & commands, CredentialIss
         {{/zcl_events}}
     };
 
-    commands.Register(clusterName, clusterCommands);
+    commands.RegisterCluster(clusterName, clusterCommands);
 }
 {{/zcl_clusters}}
 
@@ -136,7 +136,7 @@ void registerClusterAny(Commands & commands, CredentialIssuerCommands * credsIss
         make_unique<SubscribeAll>(credsIssuerConfig),         //
     };
 
-    commands.Register(clusterName, clusterCommands);
+    commands.RegisterCommandSet(clusterName, clusterCommands, "Commands for sending IM messages based on cluster id, not cluster name.");
 }
 
 void registerClusters(Commands & commands, CredentialIssuerCommands * credsIssuerConfig)

diff --git a/examples/chip-tool/templates/tests/commands.zapt b/examples/chip-tool/templates/tests/commands.zapt
@@ -58,5 +58,5 @@ void registerCommandsTests(Commands & commands, CredentialIssuerCommands * creds
 #endif // CONFIG_ENABLE_YAML_TESTS
     };
 
-    commands.Register(clusterName, clusterCommands);
+    commands.RegisterCommandSet(clusterName, clusterCommands, "Commands for running YAML tests.");
 }
diff --git a/examples/darwin-framework-tool/commands/discover/Commands.h b/examples/darwin-framework-tool/commands/discover/Commands.h
@@ -32,5 +32,5 @@ void registerCommandsDiscover(Commands & commands)
         make_unique<DiscoverCommissionablesListCommand>(),
     };
 
-    commands.Register(clusterName, clusterCommands);
+    commands.RegisterCommandSet(clusterName, clusterCommands, "Commands for device discovery.");
 }
diff --git a/examples/darwin-framework-tool/commands/interactive/Commands.h b/examples/darwin-framework-tool/commands/interactive/Commands.h
@@ -35,5 +35,5 @@ void registerCommandsInteractive(Commands & commands)
 #endif // CONFIG_USE_INTERACTIVE_MODE
     };
 
-    commands.Register(clusterName, clusterCommands);
+    commands.RegisterCommandSet(clusterName, clusterCommands, "Commands for starting long-lived interactive modes.");
 }
diff --git a/examples/darwin-framework-tool/commands/pairing/Commands.h b/examples/darwin-framework-tool/commands/pairing/Commands.h
@@ -102,5 +102,5 @@ void registerCommandsPairing(Commands & commands)
         make_unique<GetCommissionerNodeIdCommand>(),
     };
 
-    commands.Register(clusterName, clusterCommands);
+    commands.RegisterCommandSet(clusterName, clusterCommands, "Commands for commissioning devices.");
 }
diff --git a/examples/darwin-framework-tool/commands/payload/Commands.h b/examples/darwin-framework-tool/commands/payload/Commands.h
@@ -27,5 +27,5 @@ void registerCommandsPayload(Commands & commands)
         make_unique<SetupPayloadParseCommand>(), //
     };
 
-    commands.Register(clusterName, clusterCommands);
+    commands.RegisterCommandSet(clusterName, clusterCommands, "Commands for parsing and generating setup payloads.");
 }
diff --git a/examples/darwin-framework-tool/commands/provider/Commands.h b/examples/darwin-framework-tool/commands/provider/Commands.h
@@ -10,5 +10,5 @@ void registerClusterOtaSoftwareUpdateProviderInteractive(Commands & commands)
         make_unique<OTASoftwareUpdateSetParams>(),   //
     };
 
-    commands.Register(clusterName, clusterCommands);
+    commands.RegisterCommandSet(clusterName, clusterCommands, "Command for configuring darwin-framework-tool as an OTA provider.");
 }