From 7cab1aab819185d71f7c968dd52ef7e7993adbf9 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Thu, 17 Oct 2024 14:22:10 -0400 Subject: [PATCH] Use ScopedNodeId for fabric sync CADMIN subscription (#36121) * Use ScopedNodeId for fabric sync CADMIN subscription * Restyled by clang-format --------- Co-authored-by: Restyled.io --- .../device_manager/DeviceSubscription.cpp | 22 ++++++++++--------- .../device_manager/DeviceSubscription.h | 6 ++--- .../DeviceSubscriptionManager.cpp | 18 +++++++-------- .../DeviceSubscriptionManager.h | 21 +++++++++++++----- .../device_manager/DeviceSynchronization.cpp | 3 ++- 5 files changed, 42 insertions(+), 28 deletions(-) diff --git a/examples/fabric-admin/device_manager/DeviceSubscription.cpp b/examples/fabric-admin/device_manager/DeviceSubscription.cpp index a5625707a5363b..0da2bdddd4fbf5 100644 --- 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(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 index 5d8403777364b7..8db31d6e9037fc 100644 --- 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; + using OnDoneCallback = std::function; 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 mClient; diff --git a/examples/fabric-admin/device_manager/DeviceSubscriptionManager.cpp b/examples/fabric-admin/device_manager/DeviceSubscriptionManager.cpp index 8dd43f28751591..0410c162a8e43a 100644 --- 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(); 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 index 5f4e1158634a29..1b5c45fd05ae16 100644 --- 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> mDeviceSubscriptionMap; + struct ScopedNodeIdHasher + { + std::size_t operator()(const chip::ScopedNodeId & scopedNodeId) const + { + std::size_t h1 = std::hash{}(scopedNodeId.GetFabricIndex()); + std::size_t h2 = std::hash{}(scopedNodeId.GetNodeId()); + // Bitshifting h2 reduces collisions when fabricIndex == nodeId. + return h1 ^ (h2 << 1); + } + }; + + void DeviceSubscriptionTerminated(chip::ScopedNodeId scopedNodeId); + + std::unordered_map, ScopedNodeIdHasher> mDeviceSubscriptionMap; }; diff --git a/examples/fabric-admin/device_manager/DeviceSynchronization.cpp b/examples/fabric-admin/device_manager/DeviceSynchronization.cpp index 26f4d7e91bc479..6c2284018e0d19 100644 --- 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));