Self review fixes

examples/common/pigweed/protos/fabric_admin_service.proto

@@ -6,7 +6,7 @@ package chip.rpc;
 
 // Define the message for a synchronized end device with necessary fields
 message DeviceCommissioningWindowInfo {
-  uint64 device_handle_id = 1;
+  uint64 handle = 1;
   uint32 commissioning_timeout = 2;
   uint32 discriminator = 3;
   uint32 iterations = 4;
@@ -25,7 +25,7 @@ message DeviceCommissioningInfo {
 }
 
 message KeepActiveParameters {
-  uint64 device_handle_id = 1;
+  uint64 handle = 1;
   uint32 stay_active_duration_ms = 2;
   uint32 timeout_ms = 3;
 }

examples/common/pigweed/protos/fabric_bridge_service.proto

@@ -6,9 +6,9 @@ package chip.rpc;
 
 // Define the message for a synchronized end device with necessary fields
 message SynchronizedDevice {
-  // Using device_handle_id instead of node_id because over multiple fabrics
+  // Using handle instead of node_id because over multiple fabrics
   // there can be overlapping node_ids that are not unique.
-  uint64 device_handle_id = 1;
+  uint64 handle = 1;
 
   optional string unique_id = 2;
   optional string vendor_name = 3;
@@ -24,12 +24,12 @@ message SynchronizedDevice {
 }
 
 message KeepActiveChanged {
-  uint64 device_handle_id = 1;
+  uint64 handle = 1;
   uint32 promised_active_duration_ms = 2;
 }
 
 message AdministratorCommissioningChanged {
-  uint64 device_handle_id = 1;
+  uint64 handle = 1;
   uint32 window_status = 2;
   optional uint32 opener_fabric_index = 3;
   optional uint32 opener_vendor_id = 4;

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

@@ -569,11 +569,11 @@ void PairingCommand::OnCurrentFabricRemove(void * context, NodeId nodeId, CHIP_E
 #if defined(PW_RPC_ENABLED)
         app::InteractionModelEngine::GetInstance()->ShutdownSubscriptions(command->CurrentCommissioner().GetFabricIndex(), nodeId);
         ScopedNodeId scopedNodeId(nodeId, command->CurrentCommissioner().GetFabricIndex());
-        auto optionalHandleId = DeviceMgr().BridgeToAdminDeviceMapper().GetHandleId(scopedNodeId);
-        if (optionalHandleId.has_value())
+        auto optionalHandle = DeviceMgr().BridgeToAdminDeviceMapper().GetHandleForBridge(scopedNodeId);
+        if (optionalHandle.has_value())
         {
-            RemoveSynchronizedDevice(optionalHandleId.value());
-            DeviceMgr().BridgeToAdminDeviceMapper().RemoveScopedNodeIdByHandleId(optionalHandleId.value());
+            RemoveSynchronizedDevice(optionalHandle.value());
+            DeviceMgr().BridgeToAdminDeviceMapper().RemoveScopedNodeIdByBridgeHandle(optionalHandle.value());
         }
 #endif
     }

examples/fabric-admin/device_manager/BridgeAdminDeviceMapper.cpp

@@ -18,41 +18,39 @@
 
 #include "BridgeAdminDeviceMapper.h"
 
-#include <limits>
-
 #include <lib/support/CodeUtils.h>
 
-std::optional<uint64_t> BridgeAdminDeviceMapper::AddScopedNodeId(const chip::ScopedNodeId & scopedNodeId)
+std::optional<uint64_t> BridgeAdminDeviceMapper::AddAdminScopedNodeId(const chip::ScopedNodeId & scopedNodeId)
 {
-    // We are assuming that we will never run out of HandleIds
-    VerifyOrDie(mNextHandleId != std::numeric_limits<uint64_t>::max());
-    VerifyOrReturnValue(mScopedNodeIdToHandleId.find(scopedNodeId) == mScopedNodeIdToHandleId.end(), std::nullopt);
-
-    uint64_t handleId                     = mNextHandleId;
-    mHandleIdToScopedNodeId[handleId]     = scopedNodeId;
-    mScopedNodeIdToHandleId[scopedNodeId] = handleId;
-    mNextHandleId++;
-    return handleId;
+    VerifyOrReturnValue(mScopedNodeIdToHandle.find(scopedNodeId) == mScopedNodeIdToHandle.end(), std::nullopt, ChipLogError(NotSpecified, "Duplicate ScopedNodeId alread exists in map"));
+
+    uint64_t handle                     = mNextHandle;
+    mHandleToScopedNodeId[handle]     = scopedNodeId;
+    mScopedNodeIdToHandle[scopedNodeId] = handle;
+    // We are assuming that we will never run out of Handles because we are using uint64_t here.
+    static_assert(sizeof(mNextHandle) == sizeof(uint64_t));
+    mNextHandle++;
+    return handle;
 }
 
-void BridgeAdminDeviceMapper::RemoveScopedNodeIdByHandleId(uint64_t handleId)
+void BridgeAdminDeviceMapper::RemoveScopedNodeIdByBridgeHandle(uint64_t handle)
 {
-    auto it = mHandleIdToScopedNodeId.find(handleId);
-    VerifyOrReturn(it != mHandleIdToScopedNodeId.end());
-    mScopedNodeIdToHandleId.erase(it->second);
-    mHandleIdToScopedNodeId.erase(handleId);
+    auto it = mHandleToScopedNodeId.find(handle);
+    VerifyOrReturn(it != mHandleToScopedNodeId.end());
+    mScopedNodeIdToHandle.erase(it->second);
+    mHandleToScopedNodeId.erase(handle);
 }
 
-std::optional<uint64_t> BridgeAdminDeviceMapper::GetHandleId(const chip::ScopedNodeId & scopedNodeId)
+std::optional<uint64_t> BridgeAdminDeviceMapper::GetHandleForBridge(const chip::ScopedNodeId & scopedNodeId)
 {
-    auto scopedNodeIterator = mScopedNodeIdToHandleId.find(scopedNodeId);
-    VerifyOrReturnValue(scopedNodeIterator != mScopedNodeIdToHandleId.end(), std::nullopt);
+    auto scopedNodeIterator = mScopedNodeIdToHandle.find(scopedNodeId);
+    VerifyOrReturnValue(scopedNodeIterator != mScopedNodeIdToHandle.end(), std::nullopt);
     return scopedNodeIterator->second;
 }
 
-std::optional<chip::ScopedNodeId> BridgeAdminDeviceMapper::GetScopedNodeId(uint64_t handleId)
+std::optional<chip::ScopedNodeId> BridgeAdminDeviceMapper::GetScopedNodeIdForAdmin(uint64_t handle)
 {
-    auto it = mHandleIdToScopedNodeId.find(handleId);
-    VerifyOrReturnValue(it != mHandleIdToScopedNodeId.end(), std::nullopt);
+    auto it = mHandleToScopedNodeId.find(handle);
+    VerifyOrReturnValue(it != mHandleToScopedNodeId.end(), std::nullopt);
     return it->second;
 }

examples/fabric-admin/device_manager/BridgeAdminDeviceMapper.h

@@ -32,27 +32,27 @@ struct ScopedNodeIdHasher
     {
         std::size_t h1 = std::hash<uint64_t>{}(scopedNodeId.GetFabricIndex());
         std::size_t h2 = std::hash<uint64_t>{}(scopedNodeId.GetNodeId());
-        // Bitshifting h2 reduces collisions where fabricIndex == nodeId resulting
-        // in hash return of 0.
+        // Bitshifting h2 reduces collisions when fabricIndex == nodeId.
         return h1 ^ (h2 << 1);
     }
 };
 
 // Bi-directional translation between handle for aggregator and information about the
-// the device required for fabric admin to communicate with local device.
+// the device required for fabric admin to communicate with device.
 class BridgeAdminDeviceMapper
 {
 public:
-    std::optional<uint64_t> AddScopedNodeId(const chip::ScopedNodeId & scopedNodeId);
-    void RemoveScopedNodeIdByHandleId(uint64_t handleId);
+    std::optional<uint64_t> AddAdminScopedNodeId(const chip::ScopedNodeId & scopedNodeId);
+    void RemoveScopedNodeIdByBridgeHandle(uint64_t handle);
 
-    std::optional<uint64_t> GetHandleId(const chip::ScopedNodeId & scopedNodeId);
-    std::optional<chip::ScopedNodeId> GetScopedNodeId(uint64_t handleId);
+    std::optional<uint64_t> GetHandleForBridge(const chip::ScopedNodeId & scopedNodeId);
+    std::optional<chip::ScopedNodeId> GetScopedNodeIdForAdmin(uint64_t handle);
 
 private:
-    uint64_t mNextHandleId = 0;
-    // If we ever need more data other than ScopedNodeId we can change
-    // mHandleIdToScopedNodeId value from ScopedNodeId to AggregatorDeviceInfo.
-    std::unordered_map<uint64_t, chip::ScopedNodeId> mHandleIdToScopedNodeId;
-    std::unordered_map<chip::ScopedNodeId, uint64_t, ScopedNodeIdHasher> mScopedNodeIdToHandleId;
+    uint64_t mNextHandle = 0;
+    // If admin side ever needs more data other than ScopedNodeId we can change
+    // mHandleToScopedNodeId value type from ScopedNodeId to AdminDeviceInfo (or something
+    // of that nature).
+    std::unordered_map<uint64_t, chip::ScopedNodeId> mHandleToScopedNodeId;
+    std::unordered_map<chip::ScopedNodeId, uint64_t, ScopedNodeIdHasher> mScopedNodeIdToHandle;
 };

examples/fabric-admin/device_manager/DeviceSubscription.cpp

@@ -207,7 +207,7 @@ void DeviceSubscription::OnDeviceConnectionFailure(const ScopedNodeId & peerId,
 }
 
 CHIP_ERROR DeviceSubscription::StartSubscription(OnDoneCallback onDoneCallback, Controller::DeviceController & controller,
-                                                 NodeId nodeId, uint64_t handleId)
+                                                 NodeId nodeId, uint64_t handle)
 {
     assertChipStackLockedByCurrentThread();
     VerifyOrDie(mState == State::Idle);
@@ -216,7 +216,7 @@ CHIP_ERROR DeviceSubscription::StartSubscription(OnDoneCallback onDoneCallback,
 
 #if defined(PW_RPC_ENABLED)
     mCurrentAdministratorCommissioningAttributes                  = chip_rpc_AdministratorCommissioningChanged_init_default;
-    mCurrentAdministratorCommissioningAttributes.device_handle_id = handleId;
+    mCurrentAdministratorCommissioningAttributes.handle = handle;
     mCurrentAdministratorCommissioningAttributes.window_status =
         static_cast<uint32_t>(Clusters::AdministratorCommissioning::CommissioningWindowStatusEnum::kWindowNotOpen);
 #endif

examples/fabric-admin/device_manager/DeviceSubscription.h

@@ -44,7 +44,7 @@ class DeviceSubscription : public chip::app::ReadClient::Callback
     DeviceSubscription();
 
     CHIP_ERROR StartSubscription(OnDoneCallback onDoneCallback, chip::Controller::DeviceController & controller,
-                                 chip::NodeId nodeId, uint64_t handleId);
+                                 chip::NodeId nodeId, uint64_t handle);
 
     /// 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.

examples/fabric-admin/device_manager/DeviceSubscriptionManager.cpp

@@ -38,7 +38,7 @@ DeviceSubscriptionManager & DeviceSubscriptionManager::Instance()
     return instance;
 }
 
-CHIP_ERROR DeviceSubscriptionManager::StartSubscription(Controller::DeviceController & controller, NodeId nodeId, uint64_t handleId)
+CHIP_ERROR DeviceSubscriptionManager::StartSubscription(Controller::DeviceController & controller, NodeId nodeId, uint64_t handle)
 {
     assertChipStackLockedByCurrentThread();
     auto it = mDeviceSubscriptionMap.find(nodeId);
@@ -47,7 +47,7 @@ CHIP_ERROR DeviceSubscriptionManager::StartSubscription(Controller::DeviceContro
     auto deviceSubscription = std::make_unique<DeviceSubscription>();
     VerifyOrReturnError(deviceSubscription, CHIP_ERROR_NO_MEMORY);
     ReturnErrorOnFailure(deviceSubscription->StartSubscription(
-        [this](NodeId aNodeId) { this->DeviceSubscriptionTerminated(aNodeId); }, controller, nodeId, handleId));
+        [this](NodeId aNodeId) { this->DeviceSubscriptionTerminated(aNodeId); }, controller, nodeId, handle));
 
     mDeviceSubscriptionMap[nodeId] = std::move(deviceSubscription);
     return CHIP_NO_ERROR;

examples/fabric-admin/device_manager/DeviceSubscriptionManager.h

@@ -32,7 +32,7 @@ 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, uint64_t handleId);
+    CHIP_ERROR StartSubscription(chip::Controller::DeviceController & controller, chip::NodeId nodeId, uint64_t handle);
 
     CHIP_ERROR RemoveSubscription(chip::NodeId nodeId);
 

examples/fabric-admin/device_manager/DeviceSynchronization.cpp

@@ -272,15 +272,15 @@ void DeviceSynchronizer::SynchronizationCompleteAddDevice()
 #if defined(PW_RPC_ENABLED)
     VerifyOrDie(mController);
     ScopedNodeId scopedNodeId(mNodeId, mController->GetFabricIndex());
-    auto handleId = DeviceMgr().BridgeToAdminDeviceMapper().AddScopedNodeId(scopedNodeId);
-    VerifyOrDie(handleId.has_value());
-    mCurrentDeviceData.device_handle_id = handleId.value();
+    auto handle = DeviceMgr().BridgeToAdminDeviceMapper().AddAdminScopedNodeId(scopedNodeId);
+    VerifyOrDie(handle.has_value());
+    mCurrentDeviceData.handle = handle.value();
     AddSynchronizedDevice(mCurrentDeviceData);
     // TODO(#35077) Figure out how we should reflect CADMIN values of ICD.
     if (!mCurrentDeviceData.is_icd)
     {
         // TODO(#35333) Figure out how we should recover in this circumstance.
-        CHIP_ERROR err = DeviceSubscriptionManager::Instance().StartSubscription(*mController, mNodeId, handleId.value());
+        CHIP_ERROR err = DeviceSubscriptionManager::Instance().StartSubscription(*mController, mNodeId, handle.value());
         if (err != CHIP_NO_ERROR)
         {
             ChipLogError(NotSpecified, "Failed start subscription to NodeId:" ChipLogFormatX64, ChipLogValueX64(mNodeId));

examples/fabric-admin/rpc/RpcClient.cpp

@@ -144,12 +144,12 @@ CHIP_ERROR AddSynchronizedDevice(const chip_rpc_SynchronizedDevice & data)
     return WaitForResponse(call);
 }
 
-CHIP_ERROR RemoveSynchronizedDevice(uint64_t handleId)
+CHIP_ERROR RemoveSynchronizedDevice(uint64_t handle)
 {
     ChipLogProgress(NotSpecified, "RemoveSynchronizedDevice");
 
     chip_rpc_SynchronizedDevice device = chip_rpc_SynchronizedDevice_init_default;
-    device.device_handle_id            = handleId;
+    device.handle            = handle;
 
     // 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 +164,12 @@ CHIP_ERROR RemoveSynchronizedDevice(uint64_t handleId)
     return WaitForResponse(call);
 }
 
-CHIP_ERROR ActiveChanged(uint64_t handleId, uint32_t promisedActiveDurationMs)
+CHIP_ERROR ActiveChanged(uint64_t handle, uint32_t promisedActiveDurationMs)
 {
     ChipLogProgress(NotSpecified, "ActiveChanged");
 
     chip_rpc_KeepActiveChanged parameters;
-    parameters.device_handle_id            = handleId;
+    parameters.handle            = handle;
     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

@@ -57,25 +57,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 handleId The device handle ID of the device to be removed.
+ * @param handle The device handle 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(uint64_t handleId);
+CHIP_ERROR RemoveSynchronizedDevice(uint64_t handle);
 
 /**
  * @brief Received StayActiveResponse on behalf of client that previously called KeepActive
  *
- * @param handleId The device handle ID of the device we recieved a StayActiveResponse.
+ * @param handle The device handle 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(uint64_t handleId, uint32_t promisedActiveDurationMs);
+CHIP_ERROR ActiveChanged(uint64_t handle, uint32_t promisedActiveDurationMs);
 
 /**
  * @brief CADMIN attribute has changed of one of the bridged devices that was previously added.

examples/fabric-admin/rpc/RpcServer.cpp

@@ -68,22 +68,22 @@ class FabricAdmin final : public rpc::FabricAdmin, public IcdManager::Delegate
             return;
         }
 
-        auto optionalHandleId = DeviceMgr().BridgeToAdminDeviceMapper().GetHandleId(scopedNodeId);
-        if (!optionalHandleId.has_value())
+        auto optionalHandle = DeviceMgr().BridgeToAdminDeviceMapper().GetHandleForBridge(scopedNodeId);
+        if (!optionalHandle.has_value())
         {
             ChipLogError(NotSpecified, "ICD check-in for device for no longer on aggregator. Request dropped for Node ID: 0x%lx",
                          scopedNodeId.GetNodeId());
             return;
         }
-        uint64_t handleId = optionalHandleId.value();
+        uint64_t handle = optionalHandle.value();
 
         // TODO https://github.com/CHIP-Specifications/connectedhomeip-spec/issues/10448. Spec does
         // not define what to do if we fail to send the StayActiveRequest. We are assuming that any
         // further attempts to send a StayActiveRequest will result in a similar failure. Because
         // 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(handleId, promisedActiveDuration); };
+        auto onDone    = [=](uint32_t promisedActiveDuration) { ActiveChanged(handle, promisedActiveDuration); };
         CHIP_ERROR err = StayActiveSender::SendStayActiveCommand(checkInData.mStayActiveDurationMs, clientInfo.peer_node,
                                                                  app::InteractionModelEngine::GetInstance(), onDone);
         if (err != CHIP_NO_ERROR)
@@ -95,7 +95,7 @@ class FabricAdmin final : public rpc::FabricAdmin, public IcdManager::Delegate
     pw::Status OpenCommissioningWindow(const chip_rpc_DeviceCommissioningWindowInfo & request,
                                        chip_rpc_OperationStatus & response) override
     {
-        auto optionalScopedNode = DeviceMgr().BridgeToAdminDeviceMapper().GetScopedNodeId(request.device_handle_id);
+        auto optionalScopedNode = DeviceMgr().BridgeToAdminDeviceMapper().GetScopedNodeIdForAdmin(request.handle);
         VerifyOrReturnValue(optionalScopedNode.has_value(), pw::Status::InvalidArgument());
 
         NodeId nodeId                    = optionalScopedNode.value().GetNodeId();
@@ -161,7 +161,7 @@ class FabricAdmin final : public rpc::FabricAdmin, public IcdManager::Delegate
 
     pw::Status KeepActive(const chip_rpc_KeepActiveParameters & request, pw_protobuf_Empty & response) override
     {
-        auto optionalScopedNode = DeviceMgr().BridgeToAdminDeviceMapper().GetScopedNodeId(request.device_handle_id);
+        auto optionalScopedNode = DeviceMgr().BridgeToAdminDeviceMapper().GetScopedNodeIdForAdmin(request.handle);
         VerifyOrReturnValue(optionalScopedNode.has_value(), pw::Status::InvalidArgument());
 
         ChipLogProgress(NotSpecified, "Received KeepActive request: 0x%lx, %u", optionalScopedNode.value().GetNodeId(),

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

@@ -49,7 +49,7 @@ class BridgedDevice
         std::optional<chip::VendorId> openerVendorId       = std::nullopt;
     };
 
-    BridgedDevice(uint64_t handleId);
+    BridgedDevice(uint64_t handle);
     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 uint64_t GetHandleId() { return mHandleId; };
+    inline uint64_t GetHandle() { return mHandle; };
     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;
 
-    uint64_t mHandleId                 = 0;
+    uint64_t mHandle                 = 0;
     chip::EndpointId mEndpointId       = 0;
     chip::EndpointId mParentEndpointId = 0;