Address follow-up PR comments for fabric-admin UniqueIdGetter (#35405)

--------- Co-authored-by: Restyled.io <[email protected]> Co-authored-by: saurabhst <[email protected]>
project-chip · Oct 3, 2024 · 2602241 · 2602241
1 parent 829778a
commit 2602241
Show file tree

Hide file tree

Showing 5 changed files with 71 additions and 46 deletions.
diff --git a/examples/fabric-admin/BUILD.gn b/examples/fabric-admin/BUILD.gn
@@ -88,8 +88,8 @@ static_library("fabric-admin-utils") {
     "device_manager/DeviceSubscriptionManager.h",
     "device_manager/DeviceSynchronization.cpp",
     "device_manager/DeviceSynchronization.h",
-    "device_manager/UidGetter.cpp",
-    "device_manager/UidGetter.h",
+    "device_manager/UniqueIdGetter.cpp",
+    "device_manager/UniqueIdGetter.h",
   ]
 
   deps = [ "${chip_root}/src/app:events" ]

diff --git a/examples/fabric-admin/device_manager/DeviceSynchronization.cpp b/examples/fabric-admin/device_manager/DeviceSynchronization.cpp
@@ -136,15 +136,11 @@ void DeviceSynchronizer::OnDone(chip::app::ReadClient * apReadClient)
 #if defined(PW_RPC_ENABLED)
     if (mState == State::ReceivedResponse && !DeviceMgr().IsCurrentBridgeDevice(mCurrentDeviceData.node_id))
     {
-        auto * device = DeviceMgr().FindDeviceByNode(mCurrentDeviceData.node_id);
-        if (!mCurrentDeviceData.has_unique_id && device)
+        GetUniqueId();
+        if (mState == State::GettingUid)
         {
-            GetUid(device->GetEndpointId());
-            if (mState == State::GettingUid)
-            {
-                // GetUid was successful and we rely on callback to call SynchronizationCompleteAddDevice.
-                return;
-            }
+            // GetUniqueId was successful and we rely on callback to call SynchronizationCompleteAddDevice.
+            return;
         }
         SynchronizationCompleteAddDevice();
     }
@@ -211,32 +207,49 @@ void DeviceSynchronizer::StartDeviceSynchronization(chip::Controller::DeviceCont
     MoveToState(State::Connecting);
 }
 
-void DeviceSynchronizer::GetUid(EndpointId remoteEndpointIdOfInterest)
+void DeviceSynchronizer::GetUniqueId()
 {
     VerifyOrDie(mState == State::ReceivedResponse);
     VerifyOrDie(mController);
+
+    // If we have a UniqueId we can return leaving state in ReceivedResponse.
+    VerifyOrReturn(!mCurrentDeviceData.has_unique_id, ChipLogDetail(NotSpecified, "We already have UniqueId"));
+
+    auto * device = DeviceMgr().FindDeviceByNode(mCurrentDeviceData.node_id);
+    // If there is no associated remote Fabric Sync Aggregator there is no other place for us to try
+    // getting the UniqueId from and can return leaving the state in ReceivedResponse.
+    VerifyOrReturn(device, ChipLogDetail(NotSpecified, "No remote Fabric Sync Aggregator to get UniqueId from"));
+
+    // Because device is not-null we expect IsFabricSyncReady to be true. IsFabricSyncReady indicates we have a
+    // connection to the remote Fabric Sync Aggregator.
     VerifyOrDie(DeviceMgr().IsFabricSyncReady());
-    auto remoteBridgeNodeId = DeviceMgr().GetRemoteBridgeNodeId();
-
-    CHIP_ERROR err = mUidGetter.GetUid(
-        [this](std::optional<CharSpan> aUniqueId) {
-            if (aUniqueId.has_value())
-            {
-                this->mCurrentDeviceData.has_unique_id = true;
-                memcpy(this->mCurrentDeviceData.unique_id, aUniqueId.value().data(), aUniqueId.value().size());
-            }
-            else
-            {
-                ChipLogError(NotSpecified, "We expected to get UniqueId from remote fabric sync bridge");
-            }
-            this->SynchronizationCompleteAddDevice();
-        },
-        *mController, remoteBridgeNodeId, remoteEndpointIdOfInterest);
+    auto remoteBridgeNodeId               = DeviceMgr().GetRemoteBridgeNodeId();
+    EndpointId remoteEndpointIdOfInterest = device->GetEndpointId();
+
+    ChipLogDetail(NotSpecified, "Attempting to get UniqueId from remote Fabric Sync Aggregator") CHIP_ERROR err =
+        mUniqueIdGetter.GetUniqueId(
+            [this](std::optional<CharSpan> aUniqueId) {
+                if (aUniqueId.has_value())
+                {
+                    this->mCurrentDeviceData.has_unique_id = true;
+                    memcpy(this->mCurrentDeviceData.unique_id, aUniqueId.value().data(), aUniqueId.value().size());
+                }
+                else
+                {
+                    ChipLogError(NotSpecified, "We expected to get UniqueId from remote Fabric Sync Aggregator, but failed");
+                }
+                this->SynchronizationCompleteAddDevice();
+            },
+            *mController, remoteBridgeNodeId, remoteEndpointIdOfInterest);
 
     if (err == CHIP_NO_ERROR)
     {
         MoveToState(State::GettingUid);
     }
+    else
+    {
+        ChipLogDetail(NotSpecified, "Failed to get UniqueId from remote Fabric Sync Aggregator")
+    }
 }
 
 void DeviceSynchronizer::SynchronizationCompleteAddDevice()

diff --git a/examples/fabric-admin/device_manager/DeviceSynchronization.h b/examples/fabric-admin/device_manager/DeviceSynchronization.h
@@ -17,7 +17,7 @@
  */
 #pragma once
 
-#include "UidGetter.h"
+#include "UniqueIdGetter.h"
 
 #include <app/ReadClient.h>
 #include <controller/CHIPDeviceController.h>
@@ -77,7 +77,7 @@ class DeviceSynchronizer : public chip::app::ReadClient::Callback
         GettingUid,       ///< We are getting UniqueId from the remote fabric sync bridge.
     };
 
-    void GetUid(chip::EndpointId endpointId);
+    void GetUniqueId();
     void SynchronizationCompleteAddDevice();
 
     void MoveToState(const State targetState);
@@ -93,5 +93,5 @@ class DeviceSynchronizer : public chip::app::ReadClient::Callback
     // mState != Idle).
     chip::Controller::DeviceController * mController = nullptr;
     chip_rpc_SynchronizedDevice mCurrentDeviceData   = chip_rpc_SynchronizedDevice_init_default;
-    UidGetter mUidGetter;
+    UniqueIdGetter mUniqueIdGetter;
 };
diff --git a/...fabric-admin/device_manager/UidGetter.cpp → ...c-admin/device_manager/UniqueIdGetter.cpp b/...fabric-admin/device_manager/UidGetter.cpp → ...c-admin/device_manager/UniqueIdGetter.cpp
@@ -16,7 +16,7 @@
  *
  */
 
-#include "UidGetter.h"
+#include "UniqueIdGetter.h"
 
 using namespace ::chip;
 using namespace ::chip::app;
@@ -26,12 +26,12 @@ namespace {
 
 void OnDeviceConnectedWrapper(void * context, Messaging::ExchangeManager & exchangeMgr, const SessionHandle & sessionHandle)
 {
-    reinterpret_cast<UidGetter *>(context)->OnDeviceConnected(exchangeMgr, sessionHandle);
+    reinterpret_cast<UniqueIdGetter *>(context)->OnDeviceConnected(exchangeMgr, sessionHandle);
 }
 
 void OnDeviceConnectionFailureWrapper(void * context, const ScopedNodeId & peerId, CHIP_ERROR error)
 {
-    reinterpret_cast<UidGetter *>(context)->OnDeviceConnectionFailure(peerId, error);
+    reinterpret_cast<UniqueIdGetter *>(context)->OnDeviceConnectionFailure(peerId, error);
 }
 
 bool SuccessOrLog(CHIP_ERROR err, const char * name)
@@ -48,13 +48,13 @@ bool SuccessOrLog(CHIP_ERROR err, const char * name)
 
 } // namespace
 
-UidGetter::UidGetter() :
+UniqueIdGetter::UniqueIdGetter() :
     mOnDeviceConnectedCallback(OnDeviceConnectedWrapper, this),
     mOnDeviceConnectionFailureCallback(OnDeviceConnectionFailureWrapper, this)
 {}
 
-CHIP_ERROR UidGetter::GetUid(OnDoneCallback onDoneCallback, chip::Controller::DeviceController & controller, chip::NodeId nodeId,
-                             chip::EndpointId endpointId)
+CHIP_ERROR UniqueIdGetter::GetUniqueId(OnDoneCallback onDoneCallback, chip::Controller::DeviceController & controller,
+                                       chip::NodeId nodeId, chip::EndpointId endpointId)
 {
     assertChipStackLockedByCurrentThread();
     VerifyOrDie(!mCurrentlyGettingUid);
@@ -74,7 +74,7 @@ CHIP_ERROR UidGetter::GetUid(OnDoneCallback onDoneCallback, chip::Controller::De
     return err;
 }
 
-void UidGetter::OnAttributeData(const ConcreteDataAttributePath & path, TLV::TLVReader * data, const StatusIB & status)
+void UniqueIdGetter::OnAttributeData(const ConcreteDataAttributePath & path, TLV::TLVReader * data, const StatusIB & status)
 {
     VerifyOrDie(path.mClusterId == Clusters::BridgedDeviceBasicInformation::Id);
 
@@ -95,23 +95,23 @@ void UidGetter::OnAttributeData(const ConcreteDataAttributePath & path, TLV::TLV
     }
 }
 
-void UidGetter::OnReportEnd()
+void UniqueIdGetter::OnReportEnd()
 {
     // We will call mOnDoneCallback in OnDone.
 }
 
-void UidGetter::OnError(CHIP_ERROR error)
+void UniqueIdGetter::OnError(CHIP_ERROR error)
 {
     ChipLogProgress(NotSpecified, "Error Getting UID: %" CHIP_ERROR_FORMAT, error.Format());
 }
 
-void UidGetter::OnDone(ReadClient * apReadClient)
+void UniqueIdGetter::OnDone(ReadClient * apReadClient)
 {
     mCurrentlyGettingUid = false;
     mOnDoneCallback(mUniqueIdHasValue ? std::make_optional<CharSpan>(mUniqueId) : std::nullopt);
 }
 
-void UidGetter::OnDeviceConnected(Messaging::ExchangeManager & exchangeMgr, const SessionHandle & sessionHandle)
+void UniqueIdGetter::OnDeviceConnected(Messaging::ExchangeManager & exchangeMgr, const SessionHandle & sessionHandle)
 {
     VerifyOrDie(mCurrentlyGettingUid);
     mClient = std::make_unique<ReadClient>(app::InteractionModelEngine::GetInstance(), &exchangeMgr, *this /* callback */,
@@ -137,7 +137,7 @@ void UidGetter::OnDeviceConnected(Messaging::ExchangeManager & exchangeMgr, cons
     }
 }
 
-void UidGetter::OnDeviceConnectionFailure(const ScopedNodeId & peerId, CHIP_ERROR error)
+void UniqueIdGetter::OnDeviceConnectionFailure(const ScopedNodeId & peerId, CHIP_ERROR error)
 {
     VerifyOrDie(mCurrentlyGettingUid);
     ChipLogError(NotSpecified, "DeviceSubscription failed to connect to " ChipLogFormatX64, ChipLogValueX64(peerId.GetNodeId()));

diff --git a/...s/fabric-admin/device_manager/UidGetter.h → ...ric-admin/device_manager/UniqueIdGetter.h b/...s/fabric-admin/device_manager/UidGetter.h → ...ric-admin/device_manager/UniqueIdGetter.h
@@ -24,15 +24,27 @@
 #include <memory>
 #include <optional>
 
-class UidGetter : public chip::app::ReadClient::Callback
+/**
+ * @brief Class used to get UniqueID from Bridged Device Basic Information Cluster
+ *
+ * When syncing a device from another fabric that does not have a UniqueID, spec
+ * dictates:
+ *    When a Fabric Synchronizing Administrator commissions a Synchronized Device,
+ *    it SHALL persist and maintain an association with the UniqueID in the Bridged
+ *    Device Basic Information Cluster exposed by another Fabric Synchronizing
+ *    Administrator.
+ *
+ * This class assists in retrieving the UniqueId in the above situation.
+ */
+class UniqueIdGetter : public chip::app::ReadClient::Callback
 {
 public:
     using OnDoneCallback = std::function<void(std::optional<chip::CharSpan>)>;
 
-    UidGetter();
+    UniqueIdGetter();
 
-    CHIP_ERROR GetUid(OnDoneCallback onDoneCallback, chip::Controller::DeviceController & controller, chip::NodeId nodeId,
-                      chip::EndpointId endpointId);
+    CHIP_ERROR GetUniqueId(OnDoneCallback onDoneCallback, chip::Controller::DeviceController & controller, chip::NodeId nodeId,
+                           chip::EndpointId endpointId);
 
     ///////////////////////////////////////////////////////////////
     // ReadClient::Callback implementation