Fix according to PR comments

project-chip#33255
erwinpan1 · May 17, 2024 · fd15ba9 · fd15ba9
1 parent 5c7b946
commit fd15ba9
Show file tree

Hide file tree

Showing 7 changed files with 98 additions and 58 deletions.
diff --git a/examples/chef/common/chef-rpc-actions-worker.cpp b/examples/chef/common/chef-rpc-actions-worker.cpp
@@ -89,38 +89,38 @@ void ChefRpcActionsWorker::ProcessActionQueue()
         ChipLogError(NotSpecified,
                      "Cannot run action due to not finding delegate: endpointId=%d, clusterId=%04lx, attributeId=%04lx \033[0m \n",
                      task.endpointId, static_cast<unsigned long>(task.clusterId), static_cast<unsigned long>(task.actionId));
+        return;
     }
-    else
-    {
-        ActionType type = static_cast<ActionType>(task.type);
-
-        switch (type)
-        {
-        case ActionType::WRITE_ATTRIBUTE: {
-            ChipLogProgress(NotSpecified, "Writing Attribute: endpointId=%d, clusterId=%04lx, attributeId=%04lx, args.size=%lu",
-                            task.endpointId, static_cast<unsigned long>(task.clusterId), static_cast<unsigned long>(task.actionId),
-                            static_cast<unsigned long>(task.args.size()));
-            delegate->AttributeWriteHandler(task.endpointId, static_cast<chip::AttributeId>(task.actionId), task.args);
-        }
-        break;
-        case ActionType::RUN_COMMAND: {
-            ChipLogProgress(NotSpecified, "Running Command: endpointId=%d, clusterId=%04lx, commandId=%04lx, args.size=%lu",
-                            task.endpointId, static_cast<unsigned long>(task.clusterId), static_cast<unsigned long>(task.actionId),
-                            static_cast<unsigned long>(task.args.size()));
-            delegate->CommandHandler(task.endpointId, static_cast<chip::CommandId>(task.actionId), task.args);
-        }
-        break;
-        case ActionType::EMIT_EVENT: {
-            ChipLogProgress(NotSpecified, "Emitting Event: endpointId=%d, clusterId=%04lx, eventIdId=%04lx, args.size=%lu",
-                            task.endpointId, static_cast<unsigned long>(task.clusterId), static_cast<unsigned long>(task.actionId),
-                            static_cast<unsigned long>(task.args.size()));
-            delegate->EventHandler(task.endpointId, static_cast<chip::EventId>(task.actionId), task.args);
-        }
-        break;
-        default:
-            break;
-        }
-    }
+
+   ActionType type = static_cast<ActionType>(task.type);
+
+   switch (type)
+   {
+   case ActionType::WRITE_ATTRIBUTE: {
+       ChipLogProgress(NotSpecified, "Writing Attribute: endpointId=%d, clusterId=%04lx, attributeId=%04lx, args.size=%lu",
+                       task.endpointId, static_cast<unsigned long>(task.clusterId), static_cast<unsigned long>(task.actionId),
+                       static_cast<unsigned long>(task.args.size()));
+       delegate->AttributeWriteHandler(task.endpointId, static_cast<chip::AttributeId>(task.actionId), task.args);
+   }
+   break;
+   case ActionType::RUN_COMMAND: {
+       ChipLogProgress(NotSpecified, "Running Command: endpointId=%d, clusterId=%04lx, commandId=%04lx, args.size=%lu",
+                       task.endpointId, static_cast<unsigned long>(task.clusterId), static_cast<unsigned long>(task.actionId),
+                       static_cast<unsigned long>(task.args.size()));
+       delegate->CommandHandler(task.endpointId, static_cast<chip::CommandId>(task.actionId), task.args);
+   }
+   break;
+   case ActionType::EMIT_EVENT: {
+       ChipLogProgress(NotSpecified, "Emitting Event: endpointId=%d, clusterId=%04lx, eventIdId=%04lx, args.size=%lu",
+                       task.endpointId, static_cast<unsigned long>(task.clusterId), static_cast<unsigned long>(task.actionId),
+                       static_cast<unsigned long>(task.args.size()));
+       delegate->EventHandler(task.endpointId, static_cast<chip::EventId>(task.actionId), task.args);
+   }
+   break;
+   default:
+       break;
+   }
+
 
     if (queue.empty())
     {

diff --git a/examples/chef/common/chef-rpc-actions-worker.h b/examples/chef/common/chef-rpc-actions-worker.h
@@ -37,9 +37,9 @@ class ActionsDelegate
 
     virtual ~ActionsDelegate() = default;
 
-    virtual void AttributeWriteHandler(chip::EndpointId endpointId, chip::AttributeId attributeId, std::vector<uint32_t> args){};
-    virtual void CommandHandler(chip::EndpointId endpointId, chip::CommandId commandId, std::vector<uint32_t> args){};
-    virtual void EventHandler(chip::EndpointId endpointId, chip::EventId eventId, std::vector<uint32_t> args){};
+    virtual void AttributeWriteHandler(chip::EndpointId endpointId, chip::AttributeId attributeId, std::vector<uint32_t> args) = 0;
+    virtual void CommandHandler(chip::EndpointId endpointId, chip::CommandId commandId, std::vector<uint32_t> args) = 0;
+    virtual void EventHandler(chip::EndpointId endpointId, chip::EventId eventId, std::vector<uint32_t> args) = 0;
 
 protected:
     ClusterId mClusterId;
@@ -53,8 +53,8 @@ struct ActionTask
     uint32_t delayMs;
     uint32_t actionId;
     std::vector<uint32_t> args;
-    ActionTask(chip::EndpointId e, chip::ClusterId c, chip::rpc::ActionType t, uint32_t d, uint32_t i, std::vector<uint32_t> a) :
-        endpointId(e), clusterId(c), type(t), delayMs(d), actionId(i), args(a){};
+    ActionTask(chip::EndpointId endpoint, chip::ClusterId cluster, chip::rpc::ActionType actionType, uint32_t delay, uint32_t id, std::vector<uint32_t> arg) :
+        endpointId(endpoint), clusterId(cluster), type(actionType), delayMs(delay), actionId(id), args(arg){};
     ~ActionTask(){};
 };
 

diff --git a/examples/chef/common/clusters/switch/SwitchEventHandler.cpp b/examples/chef/common/clusters/switch/SwitchEventHandler.cpp
@@ -32,57 +32,49 @@ using namespace chip::DeviceLayer;
 
 void SwitchEventHandler::OnSwitchLatched(EndpointId endpointId, uint8_t newPosition)
 {
-    ChipLogDetail(NotSpecified, "The latching switch is moved to a new position:%d", newPosition);
+    ChipLogDetail(NotSpecified, "%s: endpointId=%d, newPosition=%d", __func__, endpointId, newPosition);
 
     Clusters::SwitchServer::Instance().OnSwitchLatch(endpointId, newPosition);
 }
 
 void SwitchEventHandler::OnInitialPress(EndpointId endpointId, uint8_t newPosition)
 {
-    ChipLogDetail(NotSpecified, "The new position when the momentary switch starts to be pressed:%d", newPosition);
+    ChipLogDetail(NotSpecified, "%s: endpointId=%d, newPosition=%d", __func__, endpointId, newPosition);
 
     Clusters::SwitchServer::Instance().OnInitialPress(endpointId, newPosition);
 }
 
 void SwitchEventHandler::OnLongPress(EndpointId endpointId, uint8_t newPosition)
 {
-    ChipLogDetail(NotSpecified, "The new position when the momentary switch has been pressed for a long time:%d", newPosition);
+    ChipLogDetail(NotSpecified, "%s: endpointId=%d, newPosition=%d", __func__, endpointId, newPosition);
 
     Clusters::SwitchServer::Instance().OnLongPress(endpointId, newPosition);
 }
 
 void SwitchEventHandler::OnShortRelease(EndpointId endpointId, uint8_t previousPosition)
 {
-    ChipLogDetail(NotSpecified, "The the previous value of the CurrentPosition when the momentary switch has been released:%d",
-                  previousPosition);
+    ChipLogDetail(NotSpecified, "%s: endpointId=%d, previousPosition=%d", __func__, endpointId, previousPosition);
 
     Clusters::SwitchServer::Instance().OnShortRelease(endpointId, previousPosition);
 }
 
 void SwitchEventHandler::OnLongRelease(EndpointId endpointId, uint8_t previousPosition)
 {
-    ChipLogDetail(NotSpecified,
-                  "The the previous value of the CurrentPosition when the momentary switch has been released after having been "
-                  "pressed for a long time:%d",
-                  previousPosition);
+    ChipLogDetail(NotSpecified, "%s: endpointId=%d, previousPosition=%d", __func__, endpointId, previousPosition);
 
     Clusters::SwitchServer::Instance().OnLongRelease(endpointId, previousPosition);
 }
 
 void SwitchEventHandler::OnMultiPressOngoing(EndpointId endpointId, uint8_t newPosition, uint8_t count)
 {
-    ChipLogDetail(NotSpecified, "The new position when the momentary switch has been pressed in a multi-press sequence:%d",
-                  newPosition);
-    ChipLogDetail(NotSpecified, "%d times the momentary switch has been pressed", count);
+    ChipLogDetail(NotSpecified, "%s: endpointId=%d, newPosition=%d, count=%d", __func__, endpointId, newPosition, count);
 
     Clusters::SwitchServer::Instance().OnMultiPressOngoing(endpointId, newPosition, count);
 }
 
 void SwitchEventHandler::OnMultiPressComplete(EndpointId endpointId, uint8_t previousPosition, uint8_t count)
 {
-    ChipLogDetail(NotSpecified, "The previous position when the momentary switch has been pressed in a multi-press sequence:%d",
-                  previousPosition);
-    ChipLogDetail(NotSpecified, "%d times the momentary switch has been pressed", count);
+    ChipLogDetail(NotSpecified, "%s: endpointId=%d, previousPosition=%d, count=%d", __func__, endpointId, previousPosition, count);
 
     Clusters::SwitchServer::Instance().OnMultiPressComplete(endpointId, previousPosition, count);
 }
diff --git a/examples/chef/common/clusters/switch/SwitchManager.cpp b/examples/chef/common/clusters/switch/SwitchManager.cpp
@@ -44,6 +44,7 @@ class SwitchActionsDelegate : public chip::app::ActionsDelegate
     ~SwitchActionsDelegate() override{};
 
     void AttributeWriteHandler(chip::EndpointId endpointId, chip::AttributeId attributeId, std::vector<uint32_t> args) override;
+    void CommandHandler(chip::EndpointId endpointId, chip::AttributeId attributeId, std::vector<uint32_t> args) override {};
     void EventHandler(chip::EndpointId endpointId, chip::EventId eventId, std::vector<uint32_t> args) override;
 
 private:
@@ -118,7 +119,7 @@ void SwitchActionsDelegate::EventHandler(chip::EndpointId endpointId, chip::Even
     case Events::MultiPressOngoing::Id: {
         if (args.size() < 2)
         {
-            ChipLogError(NotSpecified, "MultiPressOngoing to few arguments");
+            ChipLogError(NotSpecified, "MultiPressOngoing has too few arguments");
             return;
         }
         uint8_t newPosition                   = static_cast<uint8_t>(args[0]);
@@ -129,7 +130,7 @@ void SwitchActionsDelegate::EventHandler(chip::EndpointId endpointId, chip::Even
     case Events::MultiPressComplete::Id: {
         if (args.size() < 2)
         {
-            ChipLogError(NotSpecified, "MultiPressComplete to few arguments");
+            ChipLogError(NotSpecified, "MultiPressComplete has too few arguments");
             return;
         }
         uint8_t previousPosition            = static_cast<uint8_t>(args[0]);
@@ -157,11 +158,13 @@ const Clusters::Descriptor::Structs::SemanticTagStruct::Type gLatchingSwitch[] =
           { chip::app::DataModel::MakeNullable(chip::CharSpan("High", 4)) }) }
 };
 
+static SwitchEventHandler * gSwitchEventHandler = new SwitchEventHandler();
+static SwitchActionsDelegate * gSwitchActionsDelegate = new SwitchActionsDelegate(Clusters::Switch::Id, gSwitchEventHandler);
+
 void emberAfSwitchClusterInitCallback(EndpointId endpointId)
 {
     ChipLogProgress(Zcl, "Chef: emberAfSwitchClusterInitCallback");
 
-    ChefRpcActionsWorker::Instance().RegisterRpcActionsDelegate(
-        Clusters::Switch::Id, new SwitchActionsDelegate(Clusters::Switch::Id, new SwitchEventHandler()));
+    ChefRpcActionsWorker::Instance().RegisterRpcActionsDelegate(Clusters::Switch::Id, gSwitchActionsDelegate);
     SetTagList(/* endpoint= */ 1, Span<const Clusters::Descriptor::Structs::SemanticTagStruct::Type>(gLatchingSwitch));
 }
diff --git a/examples/chef/devices/rootnode_genericswitch_2dfff6e516.zap b/examples/chef/devices/rootnode_genericswitch_2dfff6e516.zap
@@ -1,6 +1,6 @@
 {
   "fileFormat": 2,
-  "featureLevel": 100,
+  "featureLevel": 102,
   "creator": "zap",
   "keyValuePairs": [
     {
@@ -29,6 +29,7 @@
       "pathRelativity": "relativeToZap",
       "path": "../../../src/app/zap-templates/app-templates.json",
       "type": "gen-templates-json",
+      "category": "matter",
       "version": "chip-v1"
     }
   ],

diff --git a/examples/chef/devices/rootnode_genericswitch_9866e35d0b.zap b/examples/chef/devices/rootnode_genericswitch_9866e35d0b.zap
@@ -1,6 +1,6 @@
 {
   "fileFormat": 2,
-  "featureLevel": 100,
+  "featureLevel": 102,
   "creator": "zap",
   "keyValuePairs": [
     {
@@ -29,6 +29,7 @@
       "pathRelativity": "relativeToZap",
       "path": "../../../src/app/zap-templates/app-templates.json",
       "type": "gen-templates-json",
+      "category": "matter",
       "version": "chip-v1"
     }
   ],

diff --git a/examples/common/pigweed/rpc_services/Actions.h b/examples/common/pigweed/rpc_services/Actions.h
@@ -35,6 +35,48 @@ namespace rpc {
 
 class Actions final : public pw_rpc::nanopb::Actions::Service<Actions>
 {
+    /*
+     * RPC Actions Service is a debugging interface for writting Attributes, emitting Events, or
+     * running Commands batchwisely. Each action will be execute one by one according to the
+     * delayed time in ms.
+     *
+     * An Action is a abstract type of Attribute, Command, Event
+     *  type                : this defines the action is either a Attribute, Command, or Event
+     *  delayMs             : this define the relative delayed time in ms after last action
+     *  actionId            : this defines the ID of Attribute, Command or Event
+     *  arg1, arg2, arg3    : it has up to 3 optional arguments which could be mapped to Attribute/Command/Event aruments
+     *
+     * An Action Request is composed of a batch of Actions
+     *
+     * The usage of Actions Service:
+     *      1. Create an Action Request by protos.chip.rpc.ActionsWrite
+     *      2. Append Actions one by one to the action request
+     *      3. After all action added to the request, call rpcs.chip.rpc.Actions.Set to execute that
+     *
+     * e.g. In rpc_console:
+     *
+     *      # Init a RPC Action request which is for endpoint 1 and Switch Cluster (0x003B)
+     *      In [1]: request=protos.chip.rpc.ActionsWrite(endpoint_id=1, cluster_id=int("0x003B", 16))
+     *
+     *      # Add an action to write attribute id 0x1 (CurrentPosition) with data 2 after 1000 ms (meaning right away)
+     *      In [2]: request.actions.append(protos.chip.rpc.Action(type=protos.chip.rpc.ActionType.WRITE_ATTRIBUTE, delayMs=1000, actionId=1, arg1=2))
+     *
+     *      # Define the follow action to emit an event id 0x1 (InitialPress) with data 2 (NewPosition) after 1000 ms
+     *      In [3]: message.actions.append(protos.chip.rpc.Action(type=protos.chip.rpc.ActionType.EMIT_EVENT, delayMs=1000, actionId=1, arg1=2))
+     *
+     *      # Define the follow action to emit an event id 0x2 (LongPress) with data 2 (NewPosition) after 2000 ms
+     *      In [4]: message.actions.append(protos.chip.rpc.Action(type=protos.chip.rpc.ActionType.EMIT_EVENT, delayMs=2000, actionId=2, arg1=2))
+     *
+     *      # Define an action to write attribute id 0x1 (CurrentPosition) with data 0 after 2000 ms (meaning button bouncing back)
+     *      In [5]: message.actions.append(protos.chip.rpc.Action(type=protos.chip.rpc.ActionType.WRITE_ATTRIBUTE, delayMs=2000, actionId=1, arg1=0))
+     *
+     *      # Define the follow action to emit an event id 0x4 (LongRelease) with data 2 (PreviousPosition) after 1000 ms
+     *      In [6]: message.actions.append(protos.chip.rpc.Action(type=protos.chip.rpc.ActionType.EMIT_EVENT, delayMs=1000, actionId=4, arg1=2))
+     *
+     *      # Set the actions to device
+     *      In [7]: rpcs.chip.rpc.Actions.Set(message, pw_rpc_timeout_s=10000)
+     *
+     */
 public:
     enum class Type : uint8_t
     {
@@ -44,9 +86,10 @@ class Actions final : public pw_rpc::nanopb::Actions::Service<Actions>
     };
 
     ::pw::Status Set(const chip_rpc_ActionsRequest & request, ::pw_protobuf_Empty & response)
+
     {
         DeviceLayer::StackLock lock;
-        ChipLogError(NotSpecified, " request.endpoint_id=%d, request.cluster_id=%d", request.endpoint_id, request.cluster_id);
+        ChipLogProgress(NotSpecified, " request.endpoint_id=%d, request.cluster_id=%d", request.endpoint_id, request.cluster_id);
 
         for (int i = 0; i < request.actions_count; i++)
         {