Fabric Bridge: use AttributeAccessInterface for BridgedDeviceBasicInf…

…ormationCluster, expose more attributes (#34845)

* 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

* Bump revision to 4

---------

Co-authored-by: Restyled.io <[email protected]>

examples/fabric-bridge-app/fabric-bridge-common/BUILD.gn

@@ -30,9 +30,11 @@ source_set("fabric-bridge-lib") {
 
   sources = [
     "include/BridgedDevice.h",
+    "include/BridgedDeviceBasicInformationImpl.h",
     "include/BridgedDeviceManager.h",
     "include/CHIPProjectAppConfig.h",
     "src/BridgedDevice.cpp",
+    "src/BridgedDeviceBasicInformationImpl.cpp",
     "src/BridgedDeviceManager.cpp",
     "src/ZCLCallbacks.cpp",
   ]

examples/fabric-bridge-app/fabric-bridge-common/include/BridgedDeviceBasicInformationImpl.h

@@ -0,0 +1,32 @@
+/*
+ *    Copyright (c) 2024 Project CHIP Authors
+ *
+ *    Licensed under the Apache License, Version 2.0 (the "License");
+ *    you may not use this file except in compliance with the License.
+ *    You may obtain a copy of the License at
+ *
+ *        http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *    Unless required by applicable law or agreed to in writing, software
+ *    distributed under the License is distributed on an "AS IS" BASIS,
+ *    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *    See the License for the specific language governing permissions and
+ *    limitations under the License.
+ */
+#pragma once
+
+#include <app-common/zap-generated/ids/Clusters.h>
+#include <app/AttributeAccessInterface.h>
+
+class BridgedDeviceBasicInformationImpl : public chip::app::AttributeAccessInterface
+{
+public:
+    BridgedDeviceBasicInformationImpl() :
+        chip::app::AttributeAccessInterface(chip::NullOptional /* endpointId */,
+                                            chip::app::Clusters::BridgedDeviceBasicInformation::Id)
+    {}
+
+    // AttributeAccessInterface implementation
+    CHIP_ERROR Read(const chip::app::ConcreteReadAttributePath & path, chip::app::AttributeValueEncoder & encoder) override;
+    CHIP_ERROR Write(const chip::app::ConcreteDataAttributePath & path, chip::app::AttributeValueDecoder & decoder) override;
+};

examples/fabric-bridge-app/fabric-bridge-common/src/BridgedDeviceBasicInformationImpl.cpp

@@ -0,0 +1,78 @@
+/*
+ *    Copyright (c) 2024 Project CHIP Authors
+ *
+ *    Licensed under the Apache License, Version 2.0 (the "License");
+ *    you may not use this file except in compliance with the License.
+ *    You may obtain a copy of the License at
+ *
+ *        http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *    Unless required by applicable law or agreed to in writing, software
+ *    distributed under the License is distributed on an "AS IS" BASIS,
+ *    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *    See the License for the specific language governing permissions and
+ *    limitations under the License.
+ */
+#include "BridgedDeviceBasicInformationImpl.h"
+
+#include "BridgedDeviceManager.h"
+
+#include <app-common/zap-generated/cluster-objects.h>
+#include <app-common/zap-generated/ids/Attributes.h>
+#include <app-common/zap-generated/ids/Clusters.h>
+
+#include <app/AttributeAccessInterfaceRegistry.h>
+
+static constexpr unsigned kBridgedDeviceBasicInformationClusterRevision = 4;
+static constexpr unsigned kBridgedDeviceBasicInformationFeatureMap      = 0;
+
+using namespace ::chip;
+using namespace ::chip::app;
+using namespace ::chip::app::Clusters;
+
+CHIP_ERROR BridgedDeviceBasicInformationImpl::Read(const ConcreteReadAttributePath & path, AttributeValueEncoder & encoder)
+{
+    // Registration is done for the bridged device basic information only
+    VerifyOrDie(path.mClusterId == app::Clusters::BridgedDeviceBasicInformation::Id);
+
+    BridgedDevice * dev = BridgeDeviceMgr().GetDevice(path.mEndpointId);
+    VerifyOrReturnError(dev != nullptr, CHIP_ERROR_NOT_FOUND);
+
+    switch (path.mAttributeId)
+    {
+    case BasicInformation::Attributes::Reachable::Id:
+        encoder.Encode(dev->IsReachable());
+        break;
+    case BasicInformation::Attributes::NodeLabel::Id:
+        encoder.Encode(CharSpan::fromCharString(dev->GetName()));
+        break;
+    case BasicInformation::Attributes::ClusterRevision::Id:
+        encoder.Encode(kBridgedDeviceBasicInformationClusterRevision);
+        break;
+    case BasicInformation::Attributes::FeatureMap::Id:
+        encoder.Encode(kBridgedDeviceBasicInformationFeatureMap);
+        break;
+    default:
+        return CHIP_ERROR_INVALID_ARGUMENT;
+    }
+    return CHIP_NO_ERROR;
+}
+
+CHIP_ERROR BridgedDeviceBasicInformationImpl::Write(const ConcreteDataAttributePath & path, AttributeValueDecoder & decoder)
+{
+    VerifyOrDie(path.mClusterId == app::Clusters::BridgedDeviceBasicInformation::Id);
+
+    BridgedDevice * dev = BridgeDeviceMgr().GetDevice(path.mEndpointId);
+    VerifyOrReturnError(dev != nullptr, CHIP_ERROR_NOT_FOUND);
+
+    if (!dev->IsReachable())
+    {
+        return CHIP_ERROR_NOT_CONNECTED;
+    }
+
+    ChipLogProgress(NotSpecified, "Bridged device basic information attempt to write attribute: ep=%d", path.mAttributeId);
+
+    // nothing writable right now ...
+
+    return CHIP_ERROR_INVALID_ARGUMENT;
+}

examples/fabric-bridge-app/fabric-bridge-common/src/BridgedDeviceManager.cpp

@@ -45,8 +45,13 @@ using namespace chip::app::Clusters;
 
 namespace {
 
-constexpr uint8_t kMaxRetries = 10;
-constexpr int kNodeLabelSize  = 32;
+constexpr uint8_t kMaxRetries      = 10;
+constexpr int kNodeLabelSize       = 32;
+constexpr int kUniqueIdSize        = 32;
+constexpr int kVendorNameSize      = 32;
+constexpr int kProductNameSize     = 32;
+constexpr int kHardwareVersionSize = 32;
+constexpr int kSoftwareVersionSize = 32;
 
 // Current ZCL implementation of Struct uses a max-size array of 254 bytes
 constexpr int kDescriptorAttributeArraySize = 254;
@@ -76,27 +81,44 @@ constexpr int kDescriptorAttributeArraySize = 254;
 //   - Bridged Device Basic Information
 //   - Administrator Commissioning
 
+// clang-format off
 // Declare Descriptor cluster attributes
 DECLARE_DYNAMIC_ATTRIBUTE_LIST_BEGIN(descriptorAttrs)
-DECLARE_DYNAMIC_ATTRIBUTE(Descriptor::Attributes::DeviceTypeList::Id, ARRAY, kDescriptorAttributeArraySize, 0), /* device list */
-    DECLARE_DYNAMIC_ATTRIBUTE(Descriptor::Attributes::ServerList::Id, ARRAY, kDescriptorAttributeArraySize, 0), /* server list */
-    DECLARE_DYNAMIC_ATTRIBUTE(Descriptor::Attributes::ClientList::Id, ARRAY, kDescriptorAttributeArraySize, 0), /* client list */
-    DECLARE_DYNAMIC_ATTRIBUTE(Descriptor::Attributes::PartsList::Id, ARRAY, kDescriptorAttributeArraySize, 0),  /* parts list */
-    DECLARE_DYNAMIC_ATTRIBUTE_LIST_END();
+  DECLARE_DYNAMIC_ATTRIBUTE(Descriptor::Attributes::DeviceTypeList::Id, ARRAY, kDescriptorAttributeArraySize, 0), /* device list */
+  DECLARE_DYNAMIC_ATTRIBUTE(Descriptor::Attributes::ServerList::Id, ARRAY, kDescriptorAttributeArraySize, 0), /* server list */
+  DECLARE_DYNAMIC_ATTRIBUTE(Descriptor::Attributes::ClientList::Id, ARRAY, kDescriptorAttributeArraySize, 0), /* client list */
+  DECLARE_DYNAMIC_ATTRIBUTE(Descriptor::Attributes::PartsList::Id, ARRAY, kDescriptorAttributeArraySize, 0),  /* parts list */
+DECLARE_DYNAMIC_ATTRIBUTE_LIST_END();
 
 // Declare Bridged Device Basic Information cluster attributes
 DECLARE_DYNAMIC_ATTRIBUTE_LIST_BEGIN(bridgedDeviceBasicAttrs)
-DECLARE_DYNAMIC_ATTRIBUTE(BridgedDeviceBasicInformation::Attributes::NodeLabel::Id, CHAR_STRING, kNodeLabelSize, 0), /* NodeLabel */
-    DECLARE_DYNAMIC_ATTRIBUTE(BridgedDeviceBasicInformation::Attributes::Reachable::Id, BOOLEAN, 1, 0),              /* Reachable */
-    DECLARE_DYNAMIC_ATTRIBUTE(BridgedDeviceBasicInformation::Attributes::FeatureMap::Id, BITMAP32, 4, 0), /* feature map */
-    DECLARE_DYNAMIC_ATTRIBUTE_LIST_END();
+  // The attributes below are MANDATORY in the Bridged Device Information Cluster
+  DECLARE_DYNAMIC_ATTRIBUTE(BridgedDeviceBasicInformation::Attributes::Reachable::Id, BOOLEAN, 1, 0),
+  DECLARE_DYNAMIC_ATTRIBUTE(BridgedDeviceBasicInformation::Attributes::UniqueID::Id, CHAR_STRING, kUniqueIdSize, 0),
+  DECLARE_DYNAMIC_ATTRIBUTE(BridgedDeviceBasicInformation::Attributes::FeatureMap::Id, BITMAP32, 4, 0),
+
+  // The attributes below are OPTIONAL in the bridged device, however they are MANDATORY in the BasicInformation cluster
+  // so they can always be provided if desired
+  DECLARE_DYNAMIC_ATTRIBUTE(BridgedDeviceBasicInformation::Attributes::VendorName::Id, CHAR_STRING, kVendorNameSize, 0),
+  DECLARE_DYNAMIC_ATTRIBUTE(BridgedDeviceBasicInformation::Attributes::VendorID::Id, INT16U, 2, 0),
+  DECLARE_DYNAMIC_ATTRIBUTE(BridgedDeviceBasicInformation::Attributes::ProductName::Id, CHAR_STRING, kProductNameSize, 0),
+  DECLARE_DYNAMIC_ATTRIBUTE(BridgedDeviceBasicInformation::Attributes::ProductID::Id, INT16U, 2, 0),
+  DECLARE_DYNAMIC_ATTRIBUTE(BridgedDeviceBasicInformation::Attributes::NodeLabel::Id, CHAR_STRING, kNodeLabelSize, 0),
+  DECLARE_DYNAMIC_ATTRIBUTE(BridgedDeviceBasicInformation::Attributes::HardwareVersion::Id, INT16U, 2, 0),
+  DECLARE_DYNAMIC_ATTRIBUTE(BridgedDeviceBasicInformation::Attributes::HardwareVersionString::Id, CHAR_STRING,
+                            kHardwareVersionSize, 0),
+  DECLARE_DYNAMIC_ATTRIBUTE(BridgedDeviceBasicInformation::Attributes::SoftwareVersion::Id, INT32U, 4, 0),
+  DECLARE_DYNAMIC_ATTRIBUTE(BridgedDeviceBasicInformation::Attributes::SoftwareVersionString::Id, CHAR_STRING,
+                            kSoftwareVersionSize, 0),
+DECLARE_DYNAMIC_ATTRIBUTE_LIST_END();
 
 // Declare Administrator Commissioning cluster attributes
 DECLARE_DYNAMIC_ATTRIBUTE_LIST_BEGIN(AdministratorCommissioningAttrs)
-DECLARE_DYNAMIC_ATTRIBUTE(AdministratorCommissioning::Attributes::WindowStatus::Id, ENUM8, 1, 0),              /* NodeLabel */
-    DECLARE_DYNAMIC_ATTRIBUTE(AdministratorCommissioning::Attributes::AdminFabricIndex::Id, FABRIC_IDX, 1, 0), /* Reachable */
-    DECLARE_DYNAMIC_ATTRIBUTE(AdministratorCommissioning::Attributes::AdminVendorId::Id, VENDOR_ID, 2, 0),     /* Reachable */
-    DECLARE_DYNAMIC_ATTRIBUTE_LIST_END();
+  DECLARE_DYNAMIC_ATTRIBUTE(AdministratorCommissioning::Attributes::WindowStatus::Id, ENUM8, 1, 0),
+  DECLARE_DYNAMIC_ATTRIBUTE(AdministratorCommissioning::Attributes::AdminFabricIndex::Id, FABRIC_IDX, 1, 0),
+  DECLARE_DYNAMIC_ATTRIBUTE(AdministratorCommissioning::Attributes::AdminVendorId::Id, VENDOR_ID, 2, 0),
+DECLARE_DYNAMIC_ATTRIBUTE_LIST_END();
+// clang-format on
 
 constexpr CommandId administratorCommissioningCommands[] = {
     app::Clusters::AdministratorCommissioning::Commands::OpenCommissioningWindow::Id,

examples/fabric-bridge-app/fabric-bridge-common/src/ZCLCallbacks.cpp

@@ -25,88 +25,26 @@
 using namespace ::chip;
 using namespace ::chip::app::Clusters;
 
-#define ZCL_DESCRIPTOR_CLUSTER_REVISION (1u)
 #define ZCL_ADMINISTRATOR_COMMISSIONING_CLUSTER_REVISION (1u)
-#define ZCL_BRIDGED_DEVICE_BASIC_INFORMATION_CLUSTER_REVISION (2u)
-#define ZCL_BRIDGED_DEVICE_BASIC_INFORMATION_FEATURE_MAP (0u)
 
 // External attribute read callback function
 Protocols::InteractionModel::Status emberAfExternalAttributeReadCallback(EndpointId endpoint, ClusterId clusterId,
                                                                          const EmberAfAttributeMetadata * attributeMetadata,
                                                                          uint8_t * buffer, uint16_t maxReadLength)
 {
-    AttributeId attributeId = attributeMetadata->attributeId;
-
-    BridgedDevice * dev = BridgeDeviceMgr().GetDevice(endpoint);
-    if (dev == nullptr)
-    {
-        return Protocols::InteractionModel::Status::Failure;
-    }
-
-    if (clusterId == BridgedDeviceBasicInformation::Id)
-    {
-        using namespace BridgedDeviceBasicInformation::Attributes;
-        ChipLogProgress(NotSpecified, "HandleReadBridgedDeviceBasicAttribute: attrId=%d, maxReadLength=%d", attributeId,
-                        maxReadLength);
-
-        if ((attributeId == Reachable::Id) && (maxReadLength == 1))
-        {
-            *buffer = dev->IsReachable() ? 1 : 0;
-        }
-        else if ((attributeId == NodeLabel::Id) && (maxReadLength == 32))
-        {
-            MutableByteSpan zclNameSpan(buffer, maxReadLength);
-            MakeZclCharString(zclNameSpan, dev->GetName());
-        }
-        else if ((attributeId == ClusterRevision::Id) && (maxReadLength == 2))
-        {
-            uint16_t rev = ZCL_BRIDGED_DEVICE_BASIC_INFORMATION_CLUSTER_REVISION;
-            memcpy(buffer, &rev, sizeof(rev));
-        }
-        else if ((attributeId == FeatureMap::Id) && (maxReadLength == 4))
-        {
-            uint32_t featureMap = ZCL_BRIDGED_DEVICE_BASIC_INFORMATION_FEATURE_MAP;
-            memcpy(buffer, &featureMap, sizeof(featureMap));
-        }
-        else
-        {
-            return Protocols::InteractionModel::Status::Failure;
-        }
-        return Protocols::InteractionModel::Status::Success;
-    }
-
     if (clusterId == AdministratorCommissioning::Id)
     {
         // TODO(#34791) This is a workaround to prevent crash. CADMIN is still reading incorrect
         // Attribute values on dynamic endpoint as it only reads the root node and not the actual bridge
         // device we are representing here, when addressing the issue over there we can more easily
         // resolve this workaround.
-        if ((attributeId == AdministratorCommissioning::Attributes::ClusterRevision::Id) && (maxReadLength == 2))
+        if ((attributeMetadata->attributeId == AdministratorCommissioning::Attributes::ClusterRevision::Id) && (maxReadLength == 2))
         {
             uint16_t rev = ZCL_ADMINISTRATOR_COMMISSIONING_CLUSTER_REVISION;
             memcpy(buffer, &rev, sizeof(rev));
             return Protocols::InteractionModel::Status::Success;
         }
-        return Protocols::InteractionModel::Status::Failure;
     }
 
     return Protocols::InteractionModel::Status::Failure;
 }
-
-// External attribute write callback function
-Protocols::InteractionModel::Status emberAfExternalAttributeWriteCallback(EndpointId endpoint, ClusterId clusterId,
-                                                                          const EmberAfAttributeMetadata * attributeMetadata,
-                                                                          uint8_t * buffer)
-{
-    uint16_t endpointIndex                  = emberAfGetDynamicIndexFromEndpoint(endpoint);
-    Protocols::InteractionModel::Status ret = Protocols::InteractionModel::Status::Failure;
-
-    BridgedDevice * dev = BridgeDeviceMgr().GetDevice(endpointIndex);
-    if (dev != nullptr && dev->IsReachable())
-    {
-        ChipLogProgress(NotSpecified, "emberAfExternalAttributeWriteCallback: ep=%d, clusterId=%d", endpoint, clusterId);
-        ret = Protocols::InteractionModel::Status::Success;
-    }
-
-    return ret;
-}

examples/fabric-bridge-app/linux/main.cpp

@@ -19,6 +19,7 @@
 #include <AppMain.h>
 
 #include "BridgedDevice.h"
+#include "BridgedDeviceBasicInformationImpl.h"
 #include "BridgedDeviceManager.h"
 #include "CommissionableInit.h"
 
@@ -48,6 +49,8 @@ constexpr uint16_t kPollIntervalMs = 100;
 constexpr uint16_t kRetryIntervalS = 3;
 #endif
 
+BridgedDeviceBasicInformationImpl gBridgedDeviceBasicInformationAttributes;
+
 bool KeyboardHit()
 {
     int bytesWaiting;
@@ -175,6 +178,7 @@ void ApplicationInit()
     ChipLogDetail(NotSpecified, "Fabric-Bridge: ApplicationInit()");
 
     CommandHandlerInterfaceRegistry::RegisterCommandHandler(&gAdministratorCommissioningCommandHandler);
+    registerAttributeAccessOverride(&gBridgedDeviceBasicInformationAttributes);
 
 #if defined(PW_RPC_FABRIC_BRIDGE_SERVICE) && PW_RPC_FABRIC_BRIDGE_SERVICE
     InitRpcServer(kFabricBridgeServerPort);