Pass ScopedNodeId between Bridge and Admin as Matter unique identifier

examples/common/pigweed/BUILD.gn

@@ -80,18 +80,30 @@ pw_proto_library("button_service") {
   prefix = "button_service"
 }
 
+pw_proto_library("fabric_sync_common") {
+  sources = [ "protos/fabric_sync_common.proto" ]
+  strip_prefix = "protos"
+  prefix = "fabric_sync_common"
+}
+
 pw_proto_library("fabric_admin_service") {
   sources = [ "protos/fabric_admin_service.proto" ]
   inputs = [ "protos/fabric_admin_service.options" ]
-  deps = [ "$dir_pw_protobuf:common_protos" ]
+  deps = [
+    ":fabric_sync_common",
+    "$dir_pw_protobuf:common_protos",
+  ]
   strip_prefix = "protos"
   prefix = "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" ]
+  deps = [
+    ":fabric_sync_common",
+    "$dir_pw_protobuf:common_protos",
+  ]
   strip_prefix = "protos"
   prefix = "fabric_bridge_service"
 }

examples/common/pigweed/protos/fabric_admin_service.proto

@@ -1,12 +1,13 @@
 syntax = "proto3";
 
 import 'pw_protobuf_protos/common.proto';
+import 'fabric_sync_common/fabric_sync_common.proto';
 
 package chip.rpc;
 
 // Define the message for a synchronized end device with necessary fields
 message DeviceCommissioningWindowInfo {
-  uint64 node_id = 1;
+  ScopedNode id = 1;
   uint32 commissioning_timeout = 2;
   uint32 discriminator = 3;
   uint32 iterations = 4;
@@ -25,7 +26,7 @@ message DeviceCommissioningInfo {
 }
 
 message KeepActiveParameters {
-  uint64 node_id = 1;
+  ScopedNode id = 1;
   uint32 stay_active_duration_ms = 2;
   uint32 timeout_ms = 3;
 }

examples/common/pigweed/protos/fabric_bridge_service.proto

@@ -1,12 +1,13 @@
 syntax = "proto3";
 
 import 'pw_protobuf_protos/common.proto';
+import 'fabric_sync_common/fabric_sync_common.proto';
 
 package chip.rpc;
 
 // Define the message for a synchronized end device with necessary fields
 message SynchronizedDevice {
-  uint64 node_id = 1;
+  ScopedNode id = 1;
 
   optional string unique_id = 2;
   optional string vendor_name = 3;
@@ -22,12 +23,12 @@ message SynchronizedDevice {
 }
 
 message KeepActiveChanged {
-  uint64 node_id = 1;
+  ScopedNode id = 1;
   uint32 promised_active_duration_ms = 2;
 }
 
 message AdministratorCommissioningChanged {
-  uint64 node_id = 1;
+  ScopedNode id = 1;
   uint32 window_status = 2;
   optional uint32 opener_fabric_index = 3;
   optional uint32 opener_vendor_id = 4;

examples/common/pigweed/protos/fabric_sync_common.proto

@@ -0,0 +1,6 @@
+syntax = "proto3";
+
+message ScopedNode {
+  uint64 node_id = 1;
+  uint32 fabric_index = 2;
+}

examples/common/pigweed/rpc_console/py/BUILD.gn

@@ -48,6 +48,7 @@ pw_python_package("chip_rpc") {
     "${chip_root}/examples/common/pigweed:echo_service.python",
     "${chip_root}/examples/common/pigweed:fabric_admin_service.python",
     "${chip_root}/examples/common/pigweed:fabric_bridge_service.python",
+    "${chip_root}/examples/common/pigweed:fabric_sync_common.python",
     "${chip_root}/examples/common/pigweed:lighting_service.python",
     "${chip_root}/examples/common/pigweed:locking_service.python",
     "${chip_root}/examples/common/pigweed:ot_cli_service.python",

examples/fabric-admin/commands/pairing/PairingCommand.cpp

@@ -567,7 +567,8 @@ void PairingCommand::OnCurrentFabricRemove(void * context, NodeId nodeId, CHIP_E
 
 #if defined(PW_RPC_ENABLED)
         app::InteractionModelEngine::GetInstance()->ShutdownSubscriptions(command->CurrentCommissioner().GetFabricIndex(), nodeId);
-        RemoveSynchronizedDevice(nodeId);
+        ScopedNodeId scopedNodeId(nodeId, command->CurrentCommissioner().GetFabricIndex());
+        RemoveSynchronizedDevice(scopedNodeId);
 #endif
     }
     else

examples/fabric-admin/device_manager/DeviceSubscription.cpp

@@ -215,8 +215,10 @@ CHIP_ERROR DeviceSubscription::StartSubscription(OnDoneCallback onDoneCallback,
     mNodeId = nodeId;
 
 #if defined(PW_RPC_ENABLED)
-    mCurrentAdministratorCommissioningAttributes         = chip_rpc_AdministratorCommissioningChanged_init_default;
-    mCurrentAdministratorCommissioningAttributes.node_id = nodeId;
+    mCurrentAdministratorCommissioningAttributes                 = chip_rpc_AdministratorCommissioningChanged_init_default;
+    mCurrentAdministratorCommissioningAttributes.has_id          = true;
+    mCurrentAdministratorCommissioningAttributes.id.node_id      = nodeId;
+    mCurrentAdministratorCommissioningAttributes.id.fabric_index = controller.GetFabricIndex();
     mCurrentAdministratorCommissioningAttributes.window_status =
         static_cast<uint32_t>(Clusters::AdministratorCommissioning::CommissioningWindowStatusEnum::kWindowNotOpen);
 #endif

examples/fabric-admin/device_manager/DeviceSynchronization.cpp

@@ -205,10 +205,12 @@ void DeviceSynchronizer::StartDeviceSynchronization(Controller::DeviceController
     mNodeId = nodeId;
 
 #if defined(PW_RPC_ENABLED)
-    mCurrentDeviceData            = chip_rpc_SynchronizedDevice_init_default;
-    mCurrentDeviceData.node_id    = nodeId;
-    mCurrentDeviceData.has_is_icd = true;
-    mCurrentDeviceData.is_icd     = deviceIsIcd;
+    mCurrentDeviceData                 = chip_rpc_SynchronizedDevice_init_default;
+    mCurrentDeviceData.has_id          = true;
+    mCurrentDeviceData.id.node_id      = nodeId;
+    mCurrentDeviceData.id.fabric_index = controller->GetFabricIndex();
+    mCurrentDeviceData.has_is_icd      = true;
+    mCurrentDeviceData.is_icd          = deviceIsIcd;
 #endif
 
     ReturnOnFailure(controller->GetConnectedDevice(nodeId, &mOnDeviceConnectedCallback, &mOnDeviceConnectionFailureCallback));

examples/fabric-admin/rpc/RpcClient.cpp

@@ -144,12 +144,14 @@ CHIP_ERROR AddSynchronizedDevice(const chip_rpc_SynchronizedDevice & data)
     return WaitForResponse(call);
 }
 
-CHIP_ERROR RemoveSynchronizedDevice(NodeId nodeId)
+CHIP_ERROR RemoveSynchronizedDevice(ScopedNodeId scopedNodeId)
 {
     ChipLogProgress(NotSpecified, "RemoveSynchronizedDevice");
 
     chip_rpc_SynchronizedDevice device = chip_rpc_SynchronizedDevice_init_default;
-    device.node_id                     = nodeId;
+    device.has_id                      = true;
+    device.id.node_id                  = scopedNodeId.GetNodeId();
+    device.id.fabric_index             = scopedNodeId.GetFabricIndex();
 
     // 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.
@@ -164,12 +166,14 @@ CHIP_ERROR RemoveSynchronizedDevice(NodeId nodeId)
     return WaitForResponse(call);
 }
 
-CHIP_ERROR ActiveChanged(NodeId nodeId, uint32_t promisedActiveDurationMs)
+CHIP_ERROR ActiveChanged(ScopedNodeId scopedNodeId, uint32_t promisedActiveDurationMs)
 {
     ChipLogProgress(NotSpecified, "ActiveChanged");
 
     chip_rpc_KeepActiveChanged parameters;
-    parameters.node_id                     = nodeId;
+    parameters.has_id                      = true;
+    parameters.id.node_id                  = scopedNodeId.GetNodeId();
+    parameters.id.fabric_index             = scopedNodeId.GetFabricIndex();
     parameters.promised_active_duration_ms = promisedActiveDurationMs;
 
     // The RPC call is kept alive until it completes. When a response is received, it will be logged by the handler

examples/fabric-admin/rpc/RpcClient.h

@@ -18,6 +18,7 @@
 
 #pragma once
 
+#include <lib/core/ScopedNodeId.h>
 #include <platform/CHIPDeviceLayer.h>
 
 #include "fabric_bridge_service/fabric_bridge_service.rpc.pb.h"
@@ -57,25 +58,25 @@ CHIP_ERROR AddSynchronizedDevice(const chip_rpc_SynchronizedDevice & data);
  * It logs the progress and checks if a `RemoveSynchronizedDevice` operation is already in progress.
  * If an operation is in progress, it returns `CHIP_ERROR_BUSY`.
  *
- * @param nodeId The Node ID of the device to be removed.
+ * @param scopedNodeId The Scoped Node ID of the device to be removed.
  * @return CHIP_ERROR An error code indicating the success or failure of the operation.
  * - CHIP_NO_ERROR: The RPC command was successfully processed.
  * - CHIP_ERROR_BUSY: Another operation is currently in progress.
  * - CHIP_ERROR_INTERNAL: An internal error occurred while activating the RPC call.
  */
-CHIP_ERROR RemoveSynchronizedDevice(chip::NodeId nodeId);
+CHIP_ERROR RemoveSynchronizedDevice(chip::ScopedNodeId scopedNodeId);
 
 /**
  * @brief Received StayActiveResponse on behalf of client that previously called KeepActive
  *
- * @param nodeId The Node ID of the device we recieved a StayActiveResponse.
+ * @param scopedNodeId The Scoped Node ID of the device we recieved a StayActiveResponse.
  * @param promisedActiveDurationMs the computed duration (in milliseconds) that the ICD intends to stay active for.
  * @return CHIP_ERROR An error code indicating the success or failure of the operation.
  * - CHIP_NO_ERROR: The RPC command was successfully processed.
  * - CHIP_ERROR_BUSY: Another operation is currently in progress.
  * - CHIP_ERROR_INTERNAL: An internal error occurred while activating the RPC call.
  */
-CHIP_ERROR ActiveChanged(chip::NodeId nodeId, uint32_t promisedActiveDurationMs);
+CHIP_ERROR ActiveChanged(chip::ScopedNodeId scopedNodeId, uint32_t promisedActiveDurationMs);
 
 /**
  * @brief CADMIN attribute has changed of one of the bridged devices that was previously added.

examples/fabric-admin/rpc/RpcServer.cpp

@@ -42,29 +42,40 @@ namespace {
 
 #if defined(PW_RPC_FABRIC_ADMIN_SERVICE) && PW_RPC_FABRIC_ADMIN_SERVICE
 
+struct ScopedNodeIdHasher
+{
+    std::size_t operator()(const chip::ScopedNodeId & scopedNodeId) const
+    {
+        std::size_t h1 = std::hash<uint64_t>{}(scopedNodeId.GetFabricIndex());
+        std::size_t h2 = std::hash<uint64_t>{}(scopedNodeId.GetNodeId());
+        // Bitshifting h2 reduces collisions when fabricIndex == nodeId.
+        return h1 ^ (h2 << 1);
+    }
+};
+
 class FabricAdmin final : public rpc::FabricAdmin, public IcdManager::Delegate
 {
 public:
     void OnCheckInCompleted(const app::ICDClientInfo & clientInfo) override
     {
         // Accessing mPendingCheckIn should only be done while holding ChipStackLock
         assertChipStackLockedByCurrentThread();
-        NodeId nodeId = clientInfo.peer_node.GetNodeId();
-        auto it       = mPendingCheckIn.find(nodeId);
+        ScopedNodeId scopedNodeId = clientInfo.peer_node;
+        auto it                   = mPendingCheckIn.find(scopedNodeId);
         VerifyOrReturn(it != mPendingCheckIn.end());
 
         KeepActiveDataForCheckIn checkInData = it->second;
         // Removed from pending map as check-in from this node has occured and we will handle the pending KeepActive
         // request.
-        mPendingCheckIn.erase(nodeId);
+        mPendingCheckIn.erase(scopedNodeId);
 
         auto timeNow = System::SystemClock().GetMonotonicTimestamp();
         if (timeNow > checkInData.mRequestExpiryTimestamp)
         {
-            ChipLogError(
-                NotSpecified,
-                "ICD check-in for device we have been waiting, came after KeepActive expiry. Reqeust dropped for Node ID: 0x%lx",
-                nodeId);
+            ChipLogError(NotSpecified,
+                         "ICD check-in for device we have been waiting, came after KeepActive expiry. Request dropped for ID: "
+                         "[%d:0x " ChipLogFormatX64 "]",
+                         scopedNodeId.GetFabricIndex(), ChipLogValueX64(scopedNodeId.GetNodeId()));
             return;
         }
 
@@ -74,7 +85,7 @@ class FabricAdmin final : public rpc::FabricAdmin, public IcdManager::Delegate
         // there is no mechanism for us to communicate with the client that sent out the KeepActive
         // command that there was a failure, we simply fail silently. After spec issue is
         // addressed, we can implement what spec defines here.
-        auto onDone    = [=](uint32_t promisedActiveDuration) { ActiveChanged(nodeId, promisedActiveDuration); };
+        auto onDone    = [=](uint32_t promisedActiveDuration) { ActiveChanged(scopedNodeId, promisedActiveDuration); };
         CHIP_ERROR err = StayActiveSender::SendStayActiveCommand(checkInData.mStayActiveDurationMs, clientInfo.peer_node,
                                                                  app::InteractionModelEngine::GetInstance(), onDone);
         if (err != CHIP_NO_ERROR)
@@ -86,7 +97,10 @@ class FabricAdmin final : public rpc::FabricAdmin, public IcdManager::Delegate
     pw::Status OpenCommissioningWindow(const chip_rpc_DeviceCommissioningWindowInfo & request,
                                        chip_rpc_OperationStatus & response) override
     {
-        NodeId nodeId                    = request.node_id;
+        VerifyOrReturnValue(request.has_id, pw::Status::InvalidArgument());
+        // TODO(#35875): OpenDeviceCommissioningWindow uses the same controller every time and doesn't currently accept
+        // FabricIndex. For now we are dropping fabric index from the scoped node id.
+        NodeId nodeId                    = request.id.node_id;
         uint32_t commissioningTimeoutSec = request.commissioning_timeout;
         uint32_t iterations              = request.iterations;
         uint16_t discriminator           = request.discriminator;
@@ -149,18 +163,19 @@ class FabricAdmin final : public rpc::FabricAdmin, public IcdManager::Delegate
 
     pw::Status KeepActive(const chip_rpc_KeepActiveParameters & request, pw_protobuf_Empty & response) override
     {
-        ChipLogProgress(NotSpecified, "Received KeepActive request: 0x%lx, %u", request.node_id, request.stay_active_duration_ms);
-        // TODO(#33221): We should really be using ScopedNode, but that requires larger fix in communication between
-        // fabric-admin and fabric-bridge. For now we make the assumption that there is only one fabric used by
-        // fabric-admin.
+        VerifyOrReturnValue(request.has_id, pw::Status::InvalidArgument());
+        ScopedNodeId scopedNodeId(request.id.node_id, request.id.fabric_index);
+        ChipLogProgress(NotSpecified, "Received KeepActive request: Id[%d, 0x" ChipLogFormatX64 "], %u",
+                        scopedNodeId.GetFabricIndex(), ChipLogValueX64(scopedNodeId.GetNodeId()), request.stay_active_duration_ms);
+
         KeepActiveWorkData * data =
-            Platform::New<KeepActiveWorkData>(this, request.node_id, request.stay_active_duration_ms, request.timeout_ms);
+            Platform::New<KeepActiveWorkData>(this, scopedNodeId, request.stay_active_duration_ms, request.timeout_ms);
         VerifyOrReturnValue(data, pw::Status::Internal());
         DeviceLayer::PlatformMgr().ScheduleWork(KeepActiveWork, reinterpret_cast<intptr_t>(data));
         return pw::OkStatus();
     }
 
-    void ScheduleSendingKeepActiveOnCheckIn(NodeId nodeId, uint32_t stayActiveDurationMs, uint32_t timeoutMs)
+    void ScheduleSendingKeepActiveOnCheckIn(ScopedNodeId scopedNodeId, uint32_t stayActiveDurationMs, uint32_t timeoutMs)
     {
         // Accessing mPendingCheckIn should only be done while holding ChipStackLock
         assertChipStackLockedByCurrentThread();
@@ -170,14 +185,14 @@ class FabricAdmin final : public rpc::FabricAdmin, public IcdManager::Delegate
         KeepActiveDataForCheckIn checkInData     = { .mStayActiveDurationMs   = stayActiveDurationMs,
                                                      .mRequestExpiryTimestamp = expiryTimestamp };
 
-        auto it = mPendingCheckIn.find(nodeId);
+        auto it = mPendingCheckIn.find(scopedNodeId);
         if (it != mPendingCheckIn.end())
         {
             checkInData.mStayActiveDurationMs   = std::max(checkInData.mStayActiveDurationMs, it->second.mStayActiveDurationMs);
             checkInData.mRequestExpiryTimestamp = std::max(checkInData.mRequestExpiryTimestamp, it->second.mRequestExpiryTimestamp);
         }
 
-        mPendingCheckIn[nodeId] = checkInData;
+        mPendingCheckIn[scopedNodeId] = checkInData;
     }
 
 private:
@@ -189,27 +204,29 @@ class FabricAdmin final : public rpc::FabricAdmin, public IcdManager::Delegate
 
     struct KeepActiveWorkData
     {
-        KeepActiveWorkData(FabricAdmin * fabricAdmin, NodeId nodeId, uint32_t stayActiveDurationMs, uint32_t timeoutMs) :
-            mFabricAdmin(fabricAdmin), mNodeId(nodeId), mStayActiveDurationMs(stayActiveDurationMs), mTimeoutMs(timeoutMs)
+        KeepActiveWorkData(FabricAdmin * fabricAdmin, ScopedNodeId scopedNodeId, uint32_t stayActiveDurationMs,
+                           uint32_t timeoutMs) :
+            mFabricAdmin(fabricAdmin),
+            mScopedNodeId(scopedNodeId), mStayActiveDurationMs(stayActiveDurationMs), mTimeoutMs(timeoutMs)
         {}
 
         FabricAdmin * mFabricAdmin;
-        NodeId mNodeId;
+        ScopedNodeId mScopedNodeId;
         uint32_t mStayActiveDurationMs;
         uint32_t mTimeoutMs;
     };
 
     static void KeepActiveWork(intptr_t arg)
     {
         KeepActiveWorkData * data = reinterpret_cast<KeepActiveWorkData *>(arg);
-        data->mFabricAdmin->ScheduleSendingKeepActiveOnCheckIn(data->mNodeId, data->mStayActiveDurationMs, data->mTimeoutMs);
+        data->mFabricAdmin->ScheduleSendingKeepActiveOnCheckIn(data->mScopedNodeId, data->mStayActiveDurationMs, data->mTimeoutMs);
         Platform::Delete(data);
     }
 
     // Modifications to mPendingCheckIn should be done on the MatterEventLoop thread
     // otherwise we would need a mutex protecting this data to prevent race as this
     // data is accessible by both RPC thread and Matter eventloop.
-    std::unordered_map<NodeId, KeepActiveDataForCheckIn> mPendingCheckIn;
+    std::unordered_map<ScopedNodeId, KeepActiveDataForCheckIn, ScopedNodeIdHasher> mPendingCheckIn;
 };
 
 FabricAdmin fabric_admin_service;

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

@@ -49,7 +49,7 @@ class BridgedDevice
         std::optional<chip::VendorId> openerVendorId       = std::nullopt;
     };
 
-    BridgedDevice(chip::NodeId nodeId);
+    BridgedDevice(chip::ScopedNodeId scopedNodeId);
     virtual ~BridgedDevice() = default;
 
     [[nodiscard]] bool IsReachable() const { return mReachable; }
@@ -62,7 +62,7 @@ class BridgedDevice
 
     inline void SetEndpointId(chip::EndpointId id) { mEndpointId = id; };
     inline chip::EndpointId GetEndpointId() { return mEndpointId; };
-    inline chip::NodeId GetNodeId() { return mNodeId; };
+    inline chip::ScopedNodeId GetScopedNodeId() { return mScopedNodeId; };
     inline void SetParentEndpointId(chip::EndpointId id) { mParentEndpointId = id; };
     inline chip::EndpointId GetParentEndpointId() { return mParentEndpointId; };
 
@@ -80,7 +80,7 @@ class BridgedDevice
     bool mReachable = false;
     bool mIsIcd     = false;
 
-    chip::NodeId mNodeId               = 0;
+    chip::ScopedNodeId mScopedNodeId;
     chip::EndpointId mEndpointId       = 0;
     chip::EndpointId mParentEndpointId = 0;