Add support for more attributes in Bridged Device Basic Information c…

…luster in FabricBridge example (#34851) * Add extra attributes to the bridged device basic info structures, remove nonsense comments * Make use of AAI for BridgedDeviceBasicInformation cluster * Restyled by gn * Fix sizes for software version * Update the synchronized device proto to have more data in it * Switch to unique ptr in the registry, making sure memory management works (fixed memory leak on remove device) * Use more std::optional * Bump revision to 4 * Forward attributes from the create call into the bridged device * Make attribute mapping actually work * Restyle * Ensure unique IDs are generated * Restyle * Increase size to 33 to allow for a null terminator * make sure that the rpc structures are initialized * Restyle * Add some fake data to test moving the data around * Remove unused members that were likely just copied over * make the attributes optional * Restyled by clang-format * Fix string size for HW and software versions --------- Co-authored-by: Restyled.io <[email protected]>
project-chip · Aug 9, 2024 · 2363016 · 2363016
1 parent 2211ddc
commit 2363016
Show file tree

Hide file tree

Showing 10 changed files with 279 additions and 87 deletions.
diff --git a/examples/common/pigweed/BUILD.gn b/examples/common/pigweed/BUILD.gn
@@ -90,6 +90,7 @@ pw_proto_library("fabric_admin_service") {
 
 pw_proto_library("fabric_bridge_service") {
   sources = [ "protos/fabric_bridge_service.proto" ]
+  inputs = [ "protos/fabric_bridge_service.options" ]
   deps = [ "$dir_pw_protobuf:common_protos" ]
   strip_prefix = "protos"
   prefix = "fabric_bridge_service"

diff --git a/examples/common/pigweed/protos/fabric_bridge_service.options b/examples/common/pigweed/protos/fabric_bridge_service.options
@@ -0,0 +1,6 @@
+chip.rpc.SynchronizedDevice.unique_id max_size:33
+chip.rpc.SynchronizedDevice.vendor_name max_size:33
+chip.rpc.SynchronizedDevice.product_name max_size:33
+chip.rpc.SynchronizedDevice.node_label max_size:33
+chip.rpc.SynchronizedDevice.hardware_version_string max_size:65
+chip.rpc.SynchronizedDevice.software_version_string max_size:65
diff --git a/examples/common/pigweed/protos/fabric_bridge_service.proto b/examples/common/pigweed/protos/fabric_bridge_service.proto
@@ -7,6 +7,17 @@ package chip.rpc;
 // Define the message for a synchronized end device with necessary fields
 message SynchronizedDevice {
   uint64 node_id = 1;
+
+  optional string unique_id = 2;
+  optional string vendor_name = 3;
+  optional uint32 vendor_id = 4;
+  optional string product_name = 5;
+  optional uint32 product_id = 6;
+  optional string node_label = 7;
+  optional uint32 hardware_version = 8;
+  optional string hardware_version_string = 9;
+  optional uint32 software_version = 10;
+  optional string software_version_string = 11;
 }
 
 service FabricBridge {

diff --git a/examples/fabric-admin/rpc/RpcClient.cpp b/examples/fabric-admin/rpc/RpcClient.cpp
@@ -117,8 +117,36 @@ CHIP_ERROR AddSynchronizedDevice(chip::NodeId nodeId)
 {
     ChipLogProgress(NotSpecified, "AddSynchronizedDevice");
 
-    chip_rpc_SynchronizedDevice device;
-    device.node_id = nodeId;
+    chip_rpc_SynchronizedDevice device = chip_rpc_SynchronizedDevice_init_default;
+    device.node_id                     = nodeId;
+
+    // TODO: fill this with real data. For now we just add things for testing
+    strcpy(device.vendor_name, "Test Vendor");
+    device.has_vendor_name = true;
+
+    device.vendor_id     = 123;
+    device.has_vendor_id = true;
+
+    strcpy(device.product_name, "Test Product");
+    device.has_product_name = true;
+
+    device.product_id     = 234;
+    device.has_product_id = true;
+
+    strcpy(device.node_label, "Device Label");
+    device.has_node_label = true;
+
+    device.hardware_version     = 11;
+    device.has_hardware_version = true;
+
+    strcpy(device.hardware_version_string, "Hardware");
+    device.has_hardware_version_string = true;
+
+    device.software_version     = 22;
+    device.has_software_version = true;
+
+    strcpy(device.software_version_string, "Test 1.4.22");
+    device.has_software_version_string = true;
 
     // The RPC call is kept alive until it completes. When a response is received, it will be logged by the handler
     // function and the call will complete.
@@ -137,8 +165,8 @@ CHIP_ERROR RemoveSynchronizedDevice(chip::NodeId nodeId)
 {
     ChipLogProgress(NotSpecified, "RemoveSynchronizedDevice");
 
-    chip_rpc_SynchronizedDevice device;
-    device.node_id = nodeId;
+    chip_rpc_SynchronizedDevice device = chip_rpc_SynchronizedDevice_init_default;
+    device.node_id                     = nodeId;
 
     // The RPC call is kept alive until it completes. When a response is received, it will be logged by the handler
     // function and the call will complete.

diff --git a/examples/fabric-bridge-app/fabric-bridge-common/include/BridgedDevice.h b/examples/fabric-bridge-app/fabric-bridge-common/include/BridgedDevice.h
@@ -20,41 +20,50 @@
 
 #include <app/util/attribute-storage.h>
 
-#include <stdbool.h>
-#include <stdint.h>
-
-#include <functional>
 #include <string>
-#include <vector>
 
 class BridgedDevice
 {
 public:
-    static const int kDeviceNameSize = 32;
+    /// Defines all attributes that we keep track of for a bridged device
+    struct BridgedAttributes
+    {
+        std::string uniqueId;
+        std::string vendorName;
+        uint16_t vendorId = 0;
+        std::string productName;
+        uint16_t productId = 0;
+        std::string nodeLabel;
+        uint16_t hardwareVersion = 0;
+        std::string hardwareVersionString;
+        uint32_t softwareVersion = 0;
+        std::string softwareVersionString;
+    };
 
     BridgedDevice(chip::NodeId nodeId);
-    virtual ~BridgedDevice() {}
+    virtual ~BridgedDevice() = default;
 
     bool IsReachable();
     void SetReachable(bool reachable);
-    void SetName(const char * name);
-    void SetLocation(std::string location) { mLocation = location; };
+
     inline void SetEndpointId(chip::EndpointId id) { mEndpointId = id; };
     inline chip::EndpointId GetEndpointId() { return mEndpointId; };
     inline chip::NodeId GetNodeId() { return mNodeId; };
     inline void SetParentEndpointId(chip::EndpointId id) { mParentEndpointId = id; };
     inline chip::EndpointId GetParentEndpointId() { return mParentEndpointId; };
-    inline char * GetName() { return mName; };
-    inline std::string GetLocation() { return mLocation; };
-    inline std::string GetZone() { return mZone; };
-    inline void SetZone(std::string zone) { mZone = zone; };
+
+    [[nodiscard]] const BridgedAttributes & GetBridgedAttributes() const { return mAttributes; }
+    void SetBridgedAttributes(const BridgedAttributes & value) { mAttributes = value; }
+
+    /// Convenience method to set just the unique id of a bridged device as it
+    /// is one of the few attributes that is not always bulk-set
+    void SetUniqueId(const std::string & value) { mAttributes.uniqueId = value; }
 
 protected:
     bool mReachable;
-    char mName[kDeviceNameSize];
-    std::string mLocation;
     chip::NodeId mNodeId;
     chip::EndpointId mEndpointId;
     chip::EndpointId mParentEndpointId;
-    std::string mZone;
+
+    BridgedAttributes mAttributes;
 };
diff --git a/examples/fabric-bridge-app/fabric-bridge-common/include/BridgedDeviceManager.h b/examples/fabric-bridge-app/fabric-bridge-common/include/BridgedDeviceManager.h
@@ -22,6 +22,8 @@
 
 #include "BridgedDevice.h"
 
+#include <memory>
+
 class BridgedDeviceManager
 {
 public:
@@ -41,13 +43,19 @@ class BridgedDeviceManager
      * This function attempts to add a device to a dynamic endpoint. It tries to find an available
      * endpoint slot and retries the addition process up to a specified maximum number of times if
      * the endpoint already exists. If the addition is successful, it returns the index of the
-     * dynamic endpoint; otherwise, it returns -1.
+     * dynamic endpoint;
+     *
+     * Ensures that the device has a unique id:
+     *   - device MUST set its unique id if any BEFORE calling this method
+     *   - if no unique id (i.e. empty string), a new unique id will be generated
+     *   - Add will fail if the unique id is not unique
      *
      * @param dev A pointer to the device to be added.
      * @param parentEndpointId The parent endpoint ID. Defaults to an invalid endpoint ID.
-     * @return int The index of the dynamic endpoint if successful, -1 otherwise.
+     * @return int The index of the dynamic endpoint if successful, nullopt otherwise
      */
-    int AddDeviceEndpoint(BridgedDevice * dev, chip::EndpointId parentEndpointId = chip::kInvalidEndpointId);
+    std::optional<unsigned> AddDeviceEndpoint(std::unique_ptr<BridgedDevice> dev,
+                                              chip::EndpointId parentEndpointId = chip::kInvalidEndpointId);
 
     /**
      * @brief Removes a device from a dynamic endpoint.
@@ -95,16 +103,26 @@ class BridgedDeviceManager
      * @param nodeId The NodeId of the device to be removed.
      * @return int The index of the removed dynamic endpoint if successful, -1 otherwise.
      */
-    int RemoveDeviceByNodeId(chip::NodeId nodeId);
+    std::optional<unsigned> RemoveDeviceByNodeId(chip::NodeId nodeId);
+
+    /**
+     * Finds the device with the given unique id (if any)
+     */
+    BridgedDevice * GetDeviceByUniqueId(const std::string & id);
 
 private:
     friend BridgedDeviceManager & BridgeDeviceMgr();
 
+    /**
+     * Creates a new unique ID that is not used by any other mDevice
+     */
+    std::string GenerateUniqueId();
+
     static BridgedDeviceManager sInstance;
 
     chip::EndpointId mCurrentEndpointId;
     chip::EndpointId mFirstDynamicEndpointId;
-    BridgedDevice * mDevices[CHIP_DEVICE_CONFIG_DYNAMIC_ENDPOINT_COUNT + 1];
+    std::unique_ptr<BridgedDevice> mDevices[CHIP_DEVICE_CONFIG_DYNAMIC_ENDPOINT_COUNT + 1];
 };
 
 /**

diff --git a/examples/fabric-bridge-app/fabric-bridge-common/src/BridgedDevice.cpp b/examples/fabric-bridge-app/fabric-bridge-common/src/BridgedDevice.cpp
@@ -43,17 +43,10 @@ void BridgedDevice::SetReachable(bool reachable)
 
     if (reachable)
     {
-        ChipLogProgress(NotSpecified, "BridgedDevice[%s]: ONLINE", mName);
+        ChipLogProgress(NotSpecified, "BridgedDevice[%s]: ONLINE", mAttributes.uniqueId.c_str());
     }
     else
     {
-        ChipLogProgress(NotSpecified, "BridgedDevice[%s]: OFFLINE", mName);
+        ChipLogProgress(NotSpecified, "BridgedDevice[%s]: OFFLINE", mAttributes.uniqueId.c_str());
     }
 }
-
-void BridgedDevice::SetName(const char * name)
-{
-    ChipLogProgress(NotSpecified, "BridgedDevice[%s]: New Name=\"%s\"", mName, name);
-
-    chip::Platform::CopyString(mName, name);
-}
diff --git a/examples/fabric-bridge-app/fabric-bridge-common/src/BridgedDeviceBasicInformationImpl.cpp b/examples/fabric-bridge-app/fabric-bridge-common/src/BridgedDeviceBasicInformationImpl.cpp
@@ -44,14 +44,41 @@ CHIP_ERROR BridgedDeviceBasicInformationImpl::Read(const ConcreteReadAttributePa
         encoder.Encode(dev->IsReachable());
         break;
     case BasicInformation::Attributes::NodeLabel::Id:
-        encoder.Encode(CharSpan::fromCharString(dev->GetName()));
+        encoder.Encode(CharSpan::fromCharString(dev->GetBridgedAttributes().nodeLabel.c_str()));
         break;
     case BasicInformation::Attributes::ClusterRevision::Id:
         encoder.Encode(kBridgedDeviceBasicInformationClusterRevision);
         break;
     case BasicInformation::Attributes::FeatureMap::Id:
         encoder.Encode(kBridgedDeviceBasicInformationFeatureMap);
         break;
+    case BasicInformation::Attributes::UniqueID::Id:
+        encoder.Encode(CharSpan::fromCharString(dev->GetBridgedAttributes().uniqueId.c_str()));
+        break;
+    case BasicInformation::Attributes::VendorName::Id:
+        encoder.Encode(CharSpan::fromCharString(dev->GetBridgedAttributes().vendorName.c_str()));
+        break;
+    case BasicInformation::Attributes::VendorID::Id:
+        encoder.Encode(dev->GetBridgedAttributes().vendorId);
+        break;
+    case BasicInformation::Attributes::ProductName::Id:
+        encoder.Encode(CharSpan::fromCharString(dev->GetBridgedAttributes().productName.c_str()));
+        break;
+    case BasicInformation::Attributes::ProductID::Id:
+        encoder.Encode(dev->GetBridgedAttributes().productId);
+        break;
+    case BasicInformation::Attributes::HardwareVersion::Id:
+        encoder.Encode(dev->GetBridgedAttributes().hardwareVersion);
+        break;
+    case BasicInformation::Attributes::HardwareVersionString::Id:
+        encoder.Encode(CharSpan::fromCharString(dev->GetBridgedAttributes().hardwareVersionString.c_str()));
+        break;
+    case BasicInformation::Attributes::SoftwareVersion::Id:
+        encoder.Encode(dev->GetBridgedAttributes().softwareVersion);
+        break;
+    case BasicInformation::Attributes::SoftwareVersionString::Id:
+        encoder.Encode(CharSpan::fromCharString(dev->GetBridgedAttributes().softwareVersionString.c_str()));
+        break;
     default:
         return CHIP_ERROR_INVALID_ARGUMENT;
     }