Use ScopedNodeId for fabric sync CADMIN subscription (project-chip#36121

) * Use ScopedNodeId for fabric sync CADMIN subscription * Restyled by clang-format --------- Co-authored-by: Restyled.io <[email protected]>
rquidute · Oct 17, 2024 · 7cab1aa · 7cab1aa
1 parent 8f4fd1d
commit 7cab1aa
Show file tree

Hide file tree

Showing 5 changed files with 42 additions and 28 deletions.
diff --git a/examples/fabric-admin/device_manager/DeviceSubscription.cpp b/examples/fabric-admin/device_manager/DeviceSubscription.cpp
@@ -120,7 +120,7 @@ void DeviceSubscription::OnDone(ReadClient * apReadClient)
     // After calling mOnDoneCallback we are indicating that `this` is deleted and we shouldn't do anything else with
     // DeviceSubscription.
     MoveToState(State::AwaitingDestruction);
-    mOnDoneCallback(mNodeId);
+    mOnDoneCallback(mScopedNodeId);
 }
 
 void DeviceSubscription::OnError(CHIP_ERROR error)
@@ -135,7 +135,7 @@ void DeviceSubscription::OnDeviceConnected(Messaging::ExchangeManager & exchange
         // After calling mOnDoneCallback we are indicating that `this` is deleted and we shouldn't do anything else with
         // DeviceSubscription.
         MoveToState(State::AwaitingDestruction);
-        mOnDoneCallback(mNodeId);
+        mOnDoneCallback(mScopedNodeId);
         return;
     }
     VerifyOrDie(mState == State::Connecting);
@@ -160,7 +160,7 @@ void DeviceSubscription::OnDeviceConnected(Messaging::ExchangeManager & exchange
         // After calling mOnDoneCallback we are indicating that `this` is deleted and we shouldn't do anything else with
         // DeviceSubscription.
         MoveToState(State::AwaitingDestruction);
-        mOnDoneCallback(mNodeId);
+        mOnDoneCallback(mScopedNodeId);
         return;
     }
     MoveToState(State::SubscriptionStarted);
@@ -203,29 +203,31 @@ void DeviceSubscription::OnDeviceConnectionFailure(const ScopedNodeId & peerId,
     // After calling mOnDoneCallback we are indicating that `this` is deleted and we shouldn't do anything else with
     // DeviceSubscription.
     MoveToState(State::AwaitingDestruction);
-    mOnDoneCallback(mNodeId);
+    mOnDoneCallback(mScopedNodeId);
 }
 
 CHIP_ERROR DeviceSubscription::StartSubscription(OnDoneCallback onDoneCallback, Controller::DeviceController & controller,
-                                                 NodeId nodeId)
+                                                 ScopedNodeId scopedNodeId)
 {
     assertChipStackLockedByCurrentThread();
     VerifyOrDie(mState == State::Idle);
+    VerifyOrReturnError(controller.GetFabricIndex() == scopedNodeId.GetFabricIndex(), CHIP_ERROR_INVALID_ARGUMENT);
 
-    mNodeId = nodeId;
+    mScopedNodeId = scopedNodeId;
 
 #if defined(PW_RPC_ENABLED)
     mCurrentAdministratorCommissioningAttributes                 = chip_rpc_AdministratorCommissioningChanged_init_default;
     mCurrentAdministratorCommissioningAttributes.has_id          = true;
-    mCurrentAdministratorCommissioningAttributes.id.node_id      = nodeId;
-    mCurrentAdministratorCommissioningAttributes.id.fabric_index = controller.GetFabricIndex();
+    mCurrentAdministratorCommissioningAttributes.id.node_id      = scopedNodeId.GetNodeId();
+    mCurrentAdministratorCommissioningAttributes.id.fabric_index = scopedNodeId.GetFabricIndex();
     mCurrentAdministratorCommissioningAttributes.window_status =
         static_cast<uint32_t>(Clusters::AdministratorCommissioning::CommissioningWindowStatusEnum::kWindowNotOpen);
 #endif
 
     mOnDoneCallback = onDoneCallback;
     MoveToState(State::Connecting);
-    CHIP_ERROR err = controller.GetConnectedDevice(nodeId, &mOnDeviceConnectedCallback, &mOnDeviceConnectionFailureCallback);
+    CHIP_ERROR err =
+        controller.GetConnectedDevice(scopedNodeId.GetNodeId(), &mOnDeviceConnectedCallback, &mOnDeviceConnectionFailureCallback);
     if (err != CHIP_NO_ERROR)
     {
         MoveToState(State::Idle);
@@ -258,5 +260,5 @@ void DeviceSubscription::StopSubscription()
     // After calling mOnDoneCallback we are indicating that `this` is deleted and we shouldn't do anything else with
     // DeviceSubscription.
     MoveToState(State::AwaitingDestruction);
-    mOnDoneCallback(mNodeId);
+    mOnDoneCallback(mScopedNodeId);
 }
diff --git a/examples/fabric-admin/device_manager/DeviceSubscription.h b/examples/fabric-admin/device_manager/DeviceSubscription.h
@@ -39,12 +39,12 @@ class DeviceSubscriptionManager;
 class DeviceSubscription : public chip::app::ReadClient::Callback
 {
 public:
-    using OnDoneCallback = std::function<void(chip::NodeId)>;
+    using OnDoneCallback = std::function<void(chip::ScopedNodeId)>;
 
     DeviceSubscription();
 
     CHIP_ERROR StartSubscription(OnDoneCallback onDoneCallback, chip::Controller::DeviceController & controller,
-                                 chip::NodeId nodeId);
+                                 chip::ScopedNodeId nodeId);
 
     /// This will trigger stopping the subscription. Once subscription is stopped the OnDoneCallback
     /// provided in StartSubscription will be called to indicate that subscription have been terminated.
@@ -80,7 +80,7 @@ class DeviceSubscription : public chip::app::ReadClient::Callback
     void MoveToState(const State aTargetState);
     const char * GetStateStr() const;
 
-    chip::NodeId mNodeId = chip::kUndefinedNodeId;
+    chip::ScopedNodeId mScopedNodeId;
 
     OnDoneCallback mOnDoneCallback;
     std::unique_ptr<chip::app::ReadClient> mClient;

diff --git a/examples/fabric-admin/device_manager/DeviceSubscriptionManager.cpp b/examples/fabric-admin/device_manager/DeviceSubscriptionManager.cpp
@@ -38,25 +38,25 @@ DeviceSubscriptionManager & DeviceSubscriptionManager::Instance()
     return instance;
 }
 
-CHIP_ERROR DeviceSubscriptionManager::StartSubscription(Controller::DeviceController & controller, NodeId nodeId)
+CHIP_ERROR DeviceSubscriptionManager::StartSubscription(Controller::DeviceController & controller, ScopedNodeId scopedNodeId)
 {
     assertChipStackLockedByCurrentThread();
-    auto it = mDeviceSubscriptionMap.find(nodeId);
+    auto it = mDeviceSubscriptionMap.find(scopedNodeId);
     VerifyOrReturnError((it == mDeviceSubscriptionMap.end()), CHIP_ERROR_INCORRECT_STATE);
 
     auto deviceSubscription = std::make_unique<DeviceSubscription>();
     VerifyOrReturnError(deviceSubscription, CHIP_ERROR_NO_MEMORY);
     ReturnErrorOnFailure(deviceSubscription->StartSubscription(
-        [this](NodeId aNodeId) { this->DeviceSubscriptionTerminated(aNodeId); }, controller, nodeId));
+        [this](ScopedNodeId aNodeId) { this->DeviceSubscriptionTerminated(aNodeId); }, controller, scopedNodeId));
 
-    mDeviceSubscriptionMap[nodeId] = std::move(deviceSubscription);
+    mDeviceSubscriptionMap[scopedNodeId] = std::move(deviceSubscription);
     return CHIP_NO_ERROR;
 }
 
-CHIP_ERROR DeviceSubscriptionManager::RemoveSubscription(NodeId nodeId)
+CHIP_ERROR DeviceSubscriptionManager::RemoveSubscription(ScopedNodeId scopedNodeId)
 {
     assertChipStackLockedByCurrentThread();
-    auto it = mDeviceSubscriptionMap.find(nodeId);
+    auto it = mDeviceSubscriptionMap.find(scopedNodeId);
     VerifyOrReturnError((it != mDeviceSubscriptionMap.end()), CHIP_ERROR_NOT_FOUND);
     // We cannot safely erase the DeviceSubscription from mDeviceSubscriptionMap.
     // After calling StopSubscription we expect DeviceSubscription to eventually
@@ -67,14 +67,14 @@ CHIP_ERROR DeviceSubscriptionManager::RemoveSubscription(NodeId nodeId)
     return CHIP_NO_ERROR;
 }
 
-void DeviceSubscriptionManager::DeviceSubscriptionTerminated(NodeId nodeId)
+void DeviceSubscriptionManager::DeviceSubscriptionTerminated(ScopedNodeId scopedNodeId)
 {
     assertChipStackLockedByCurrentThread();
-    auto it = mDeviceSubscriptionMap.find(nodeId);
+    auto it = mDeviceSubscriptionMap.find(scopedNodeId);
     // DeviceSubscriptionTerminated is a private method that is expected to only
     // be called by DeviceSubscription when it is terminal and is ready to be
     // cleaned up and removed. If it is not mapped that means something has gone
     // really wrong and there is likely a memory leak somewhere.
     VerifyOrDie(it != mDeviceSubscriptionMap.end());
-    mDeviceSubscriptionMap.erase(nodeId);
+    mDeviceSubscriptionMap.erase(scopedNodeId);
 }
diff --git a/examples/fabric-admin/device_manager/DeviceSubscriptionManager.h b/examples/fabric-admin/device_manager/DeviceSubscriptionManager.h
@@ -32,12 +32,23 @@ class DeviceSubscriptionManager
 
     /// Usually called after we have added a synchronized device to fabric-bridge to monitor
     /// for any changes that need to be propagated to fabric-bridge.
-    CHIP_ERROR StartSubscription(chip::Controller::DeviceController & controller, chip::NodeId nodeId);
+    CHIP_ERROR StartSubscription(chip::Controller::DeviceController & controller, chip::ScopedNodeId scopedNodeId);
 
-    CHIP_ERROR RemoveSubscription(chip::NodeId nodeId);
+    CHIP_ERROR RemoveSubscription(chip::ScopedNodeId scopedNodeId);
 
 private:
-    void DeviceSubscriptionTerminated(chip::NodeId nodeId);
-
-    std::unordered_map<chip::NodeId, std::unique_ptr<DeviceSubscription>> mDeviceSubscriptionMap;
+    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);
+        }
+    };
+
+    void DeviceSubscriptionTerminated(chip::ScopedNodeId scopedNodeId);
+
+    std::unordered_map<chip::ScopedNodeId, std::unique_ptr<DeviceSubscription>, ScopedNodeIdHasher> mDeviceSubscriptionMap;
 };
diff --git a/examples/fabric-admin/device_manager/DeviceSynchronization.cpp b/examples/fabric-admin/device_manager/DeviceSynchronization.cpp
@@ -278,7 +278,8 @@ void DeviceSynchronizer::SynchronizationCompleteAddDevice()
     {
         VerifyOrDie(mController);
         // TODO(#35333) Figure out how we should recover in this circumstance.
-        CHIP_ERROR err = DeviceSubscriptionManager::Instance().StartSubscription(*mController, mNodeId);
+        ScopedNodeId scopedNodeId(mNodeId, mController->GetFabricIndex());
+        CHIP_ERROR err = DeviceSubscriptionManager::Instance().StartSubscription(*mController, scopedNodeId);
         if (err != CHIP_NO_ERROR)
         {
             ChipLogError(NotSpecified, "Failed start subscription to NodeId:" ChipLogFormatX64, ChipLogValueX64(mNodeId));