[Fabric-Admin] Set remote bridge after reverse pair the bridge device (…

…project-chip#36451)

* [Fabric-Admin] Set remote bridge after reverse pair the bridge devide

* Add remote bridge to local bridge after pairing

* Set the reverse commissioning flag

* Do not add device as a bridged device after reverse pairing

* Fix compare error

* Address review comments

examples/fabric-admin/commands/fabric-sync/FabricSyncCommand.cpp

@@ -57,6 +57,7 @@ void FabricSyncAddBridgeCommand::OnCommissioningComplete(NodeId deviceId, CHIP_E
 
         DeviceManager::Instance().UpdateLastUsedNodeId(mBridgeNodeId);
         DeviceManager::Instance().SubscribeRemoteFabricBridge();
+        DeviceManager::Instance().InitCommissionerControl();
 
         if (DeviceManager::Instance().IsLocalBridgeReady())
         {
@@ -284,7 +285,7 @@ void FabricSyncDeviceCommand::OnCommissioningComplete(NodeId deviceId, CHIP_ERRO
 
     if (err == CHIP_NO_ERROR)
     {
-        DeviceManager::Instance().AddSyncedDevice(Device(mAssignedNodeId, mRemoteEndpointId));
+        DeviceManager::Instance().AddSyncedDevice(SyncedDevice(mAssignedNodeId, mRemoteEndpointId));
     }
     else
     {

examples/fabric-admin/device_manager/DeviceManager.cpp

@@ -72,39 +72,35 @@ void DeviceManager::UpdateLastUsedNodeId(NodeId nodeId)
 void DeviceManager::SetRemoteBridgeNodeId(chip::NodeId nodeId)
 {
     mRemoteBridgeNodeId = nodeId;
-
-    if (mRemoteBridgeNodeId != kUndefinedNodeId)
-    {
-        mCommissionerControl.Init(PairingManager::Instance().CurrentCommissioner(), mRemoteBridgeNodeId, kAggregatorEndpointId);
-    }
+    ChipLogProgress(NotSpecified, "Set remote bridge NodeId:" ChipLogFormatX64, ChipLogValueX64(mRemoteBridgeNodeId));
 }
 
-void DeviceManager::AddSyncedDevice(const Device & device)
+void DeviceManager::AddSyncedDevice(const SyncedDevice & device)
 {
     mSyncedDevices.insert(device);
     ChipLogProgress(NotSpecified, "Added synced device: NodeId:" ChipLogFormatX64 ", EndpointId %u",
                     ChipLogValueX64(device.GetNodeId()), device.GetEndpointId());
 }
 
-Device * DeviceManager::FindDeviceByEndpoint(EndpointId endpointId)
+SyncedDevice * DeviceManager::FindDeviceByEndpoint(EndpointId endpointId)
 {
     for (auto & device : mSyncedDevices)
     {
         if (device.GetEndpointId() == endpointId)
         {
-            return const_cast<Device *>(&device);
+            return const_cast<SyncedDevice *>(&device);
         }
     }
     return nullptr;
 }
 
-Device * DeviceManager::FindDeviceByNode(NodeId nodeId)
+SyncedDevice * DeviceManager::FindDeviceByNode(NodeId nodeId)
 {
     for (auto & device : mSyncedDevices)
     {
         if (device.GetNodeId() == nodeId)
         {
-            return const_cast<Device *>(&device);
+            return const_cast<SyncedDevice *>(&device);
         }
     }
     return nullptr;
@@ -116,8 +112,8 @@ void DeviceManager::RemoveSyncedDevice(chip::ScopedNodeId scopedNodeId)
     RemoveSynchronizedDevice(scopedNodeId);
 #endif
 
-    NodeId nodeId   = scopedNodeId.GetNodeId();
-    Device * device = FindDeviceByNode(nodeId);
+    NodeId nodeId         = scopedNodeId.GetNodeId();
+    SyncedDevice * device = FindDeviceByNode(nodeId);
     if (device == nullptr)
     {
         ChipLogProgress(NotSpecified, "No device found with NodeId:" ChipLogFormatX64, ChipLogValueX64(nodeId));
@@ -129,6 +125,18 @@ void DeviceManager::RemoveSyncedDevice(chip::ScopedNodeId scopedNodeId)
                     ChipLogValueX64(device->GetNodeId()), device->GetEndpointId());
 }
 
+void DeviceManager::InitCommissionerControl()
+{
+    if (mRemoteBridgeNodeId != kUndefinedNodeId)
+    {
+        mCommissionerControl.Init(PairingManager::Instance().CurrentCommissioner(), mRemoteBridgeNodeId, kAggregatorEndpointId);
+    }
+    else
+    {
+        ChipLogError(NotSpecified, "Failed to initialize the Commissioner Control delegate");
+    }
+}
+
 void DeviceManager::OpenDeviceCommissioningWindow(ScopedNodeId scopedNodeId, uint32_t iterations, uint16_t commissioningTimeoutSec,
                                                   uint16_t discriminator, const ByteSpan & salt, const ByteSpan & verifier)
 {
@@ -212,10 +220,10 @@ void DeviceManager::UnpairLocalFabricBridge()
 
 void DeviceManager::SubscribeRemoteFabricBridge()
 {
-    ChipLogProgress(NotSpecified, "Start subscription to the remote bridge.")
+    ChipLogProgress(NotSpecified, "Start subscription to the remote bridge.");
 
-        CHIP_ERROR error = mBridgeSubscriber.StartSubscription(PairingManager::Instance().CurrentCommissioner(),
-                                                               mRemoteBridgeNodeId, kAggregatorEndpointId);
+    CHIP_ERROR error = mBridgeSubscriber.StartSubscription(PairingManager::Instance().CurrentCommissioner(), mRemoteBridgeNodeId,
+                                                           kAggregatorEndpointId);
 
     if (error != CHIP_NO_ERROR)
     {
@@ -354,7 +362,7 @@ void DeviceManager::HandleAttributePartsListUpdate(TLV::TLVReader & data)
     std::vector<EndpointId> removedEndpoints;
 
     // Note: We're using vectors and manual searches instead of set operations
-    // because we need to work with the Device objects in mSyncedDevices,
+    // because we need to work with the SyncedDevice objects in mSyncedDevices,
     // not just their EndpointIds. This approach allows us to access the full
     // Device information when processing changes.
 
@@ -381,15 +389,16 @@ void DeviceManager::HandleAttributePartsListUpdate(TLV::TLVReader & data)
     for (const auto & endpoint : addedEndpoints)
     {
         // print to console
-        fprintf(stderr, "A new device is added on Endpoint: %u\n", endpoint);
+        fprintf(stderr, "A new endpoint %u is added on the remote bridge\n", endpoint);
     }
 
     // Process removed endpoints
     for (const auto & endpoint : removedEndpoints)
     {
-        ChipLogProgress(NotSpecified, "Endpoint removed: %u", endpoint);
+        // print to console
+        fprintf(stderr, "Endpoint %u removed from the remote bridge\n", endpoint);
 
-        Device * device = FindDeviceByEndpoint(endpoint);
+        SyncedDevice * device = FindDeviceByEndpoint(endpoint);
 
         if (device == nullptr)
         {
@@ -467,6 +476,7 @@ void DeviceManager::HandleCommandResponse(const app::ConcreteCommandPath & path,
     if (path.mClusterId == app::Clusters::CommissionerControl::Id &&
         path.mCommandId == app::Clusters::CommissionerControl::Commands::ReverseOpenCommissioningWindow::Id)
     {
+        VerifyOrDie(path.mEndpointId == kAggregatorEndpointId);
         HandleReverseOpenCommissioningWindow(data);
     }
 }

examples/fabric-admin/device_manager/DeviceManager.h

@@ -32,15 +32,15 @@ constexpr uint32_t kDefaultSetupPinCode    = 20202021;
 constexpr uint16_t kDefaultLocalBridgePort = 5540;
 constexpr uint16_t kResponseTimeoutSeconds = 30;
 
-class Device
+class SyncedDevice
 {
 public:
-    Device(chip::NodeId nodeId, chip::EndpointId endpointId) : mNodeId(nodeId), mEndpointId(endpointId) {}
+    SyncedDevice(chip::NodeId nodeId, chip::EndpointId endpointId) : mNodeId(nodeId), mEndpointId(endpointId) {}
 
     chip::NodeId GetNodeId() const { return mNodeId; }
     chip::EndpointId GetEndpointId() const { return mEndpointId; }
 
-    bool operator<(const Device & other) const
+    bool operator<(const SyncedDevice & other) const
     {
         return mNodeId < other.mNodeId || (mNodeId == other.mNodeId && mEndpointId < other.mEndpointId);
     }
@@ -81,10 +81,15 @@ class DeviceManager
 
     bool IsLocalBridgeReady() const { return mLocalBridgeNodeId != chip::kUndefinedNodeId; }
 
-    void AddSyncedDevice(const Device & device);
+    void AddSyncedDevice(const SyncedDevice & device);
 
     void RemoveSyncedDevice(chip::ScopedNodeId scopedNodeId);
 
+    /**
+     * @brief Initializes the CommissionerControl for fabric sync setup process.
+     */
+    void InitCommissionerControl();
+
     /**
      * @brief Determines whether a given nodeId corresponds to the "current bridge device," either local or remote.
      *
@@ -177,8 +182,8 @@ class DeviceManager
 
     void HandleCommandResponse(const chip::app::ConcreteCommandPath & path, chip::TLV::TLVReader & data);
 
-    Device * FindDeviceByEndpoint(chip::EndpointId endpointId);
-    Device * FindDeviceByNode(chip::NodeId nodeId);
+    SyncedDevice * FindDeviceByEndpoint(chip::EndpointId endpointId);
+    SyncedDevice * FindDeviceByNode(chip::NodeId nodeId);
 
 private:
     void RequestCommissioningApproval();
@@ -193,8 +198,6 @@ class DeviceManager
 
     void HandleReverseOpenCommissioningWindow(chip::TLV::TLVReader & data);
 
-    static DeviceManager sInstance;
-
     chip::NodeId mLastUsedNodeId = 0;
 
     // The Node ID of the remote bridge used for Fabric-Sync
@@ -207,7 +210,7 @@ class DeviceManager
     // This represents the bridge within its own ecosystem.
     chip::NodeId mLocalBridgeNodeId = chip::kUndefinedNodeId;
 
-    std::set<Device> mSyncedDevices;
+    std::set<SyncedDevice> mSyncedDevices;
     bool mInitialized   = false;
     uint64_t mRequestId = 0;
 

examples/fabric-admin/device_manager/DeviceSynchronization.cpp

@@ -142,13 +142,16 @@ void DeviceSynchronizer::OnReportEnd()
 
 void DeviceSynchronizer::OnDone(app::ReadClient * apReadClient)
 {
+    ChipLogProgress(NotSpecified, "Synchronization complete for NodeId:" ChipLogFormatX64, ChipLogValueX64(mNodeId));
+
 #if defined(PW_RPC_ENABLED)
     if (mState == State::ReceivedResponse && !DeviceManager::Instance().IsCurrentBridgeDevice(mNodeId))
     {
         GetUniqueId();
         if (mState == State::GettingUid)
         {
-            // GetUniqueId was successful and we rely on callback to call SynchronizationCompleteAddDevice.
+            ChipLogProgress(NotSpecified,
+                            "GetUniqueId was successful and we rely on callback to call SynchronizationCompleteAddDevice.");
             return;
         }
         SynchronizationCompleteAddDevice();
@@ -206,6 +209,8 @@ void DeviceSynchronizer::StartDeviceSynchronization(Controller::DeviceController
 
     mNodeId = nodeId;
 
+    ChipLogProgress(NotSpecified, "Start device synchronization for NodeId:" ChipLogFormatX64, ChipLogValueX64(mNodeId));
+
 #if defined(PW_RPC_ENABLED)
     mCurrentDeviceData                 = chip_rpc_SynchronizedDevice_init_default;
     mCurrentDeviceData.has_id          = true;
@@ -272,6 +277,7 @@ void DeviceSynchronizer::GetUniqueId()
 void DeviceSynchronizer::SynchronizationCompleteAddDevice()
 {
     VerifyOrDie(mState == State::ReceivedResponse || mState == State::GettingUid);
+    ChipLogProgress(NotSpecified, "Synchronization complete and add device");
 
 #if defined(PW_RPC_ENABLED)
     AddSynchronizedDevice(mCurrentDeviceData);

examples/fabric-admin/device_manager/PairingManager.cpp

@@ -285,6 +285,12 @@ void PairingManager::OnPairingDeleted(CHIP_ERROR err)
 
 void PairingManager::OnCommissioningComplete(NodeId nodeId, CHIP_ERROR err)
 {
+    if (mPairingDelegate)
+    {
+        mPairingDelegate->OnCommissioningComplete(nodeId, err);
+        SetPairingDelegate(nullptr);
+    }
+
     if (err == CHIP_NO_ERROR)
     {
         // print to console
@@ -308,12 +314,6 @@ void PairingManager::OnCommissioningComplete(NodeId nodeId, CHIP_ERROR err)
         }
         ChipLogProgress(NotSpecified, "Device commissioning Failure: %s", ErrorStr(err));
     }
-
-    if (mPairingDelegate)
-    {
-        mPairingDelegate->OnCommissioningComplete(nodeId, err);
-        SetPairingDelegate(nullptr);
-    }
 }
 
 void PairingManager::OnReadCommissioningInfo(const Controller::ReadCommissioningInfo & info)

examples/fabric-admin/rpc/RpcServer.cpp

@@ -55,7 +55,7 @@ struct ScopedNodeIdHasher
     }
 };
 
-class FabricAdmin final : public rpc::FabricAdmin, public IcdManager::Delegate
+class FabricAdmin final : public rpc::FabricAdmin, public admin::PairingDelegate, public IcdManager::Delegate
 {
 public:
     void OnCheckInCompleted(const app::ICDClientInfo & clientInfo) override
@@ -96,6 +96,33 @@ class FabricAdmin final : public rpc::FabricAdmin, public IcdManager::Delegate
         }
     }
 
+    void OnCommissioningComplete(NodeId deviceId, CHIP_ERROR err) override
+    {
+        if (mNodeId != deviceId)
+        {
+            ChipLogError(NotSpecified,
+                         "Tried to pair a non-bridge device (0x:" ChipLogFormatX64 ") with result: %" CHIP_ERROR_FORMAT,
+                         ChipLogValueX64(deviceId), err.Format());
+            return;
+        }
+        else
+        {
+            ChipLogProgress(NotSpecified, "Reverse commission succeeded for NodeId:" ChipLogFormatX64, ChipLogValueX64(mNodeId));
+        }
+
+        if (err == CHIP_NO_ERROR)
+        {
+            DeviceManager::Instance().SetRemoteBridgeNodeId(deviceId);
+        }
+        else
+        {
+            ChipLogError(NotSpecified, "Failed to pair bridge device (0x:" ChipLogFormatX64 ") with error: %" CHIP_ERROR_FORMAT,
+                         ChipLogValueX64(deviceId), err.Format());
+        }
+
+        mNodeId = kUndefinedNodeId;
+    }
+
     pw::Status OpenCommissioningWindow(const chip_rpc_DeviceCommissioningWindowInfo & request,
                                        chip_rpc_OperationStatus & response) override
     {
@@ -146,13 +173,14 @@ class FabricAdmin final : public rpc::FabricAdmin, public IcdManager::Delegate
 
         if (error == CHIP_NO_ERROR)
         {
-            NodeId nodeId = DeviceManager::Instance().GetNextAvailableNodeId();
+            mNodeId = DeviceManager::Instance().GetNextAvailableNodeId();
+            ;
 
             // After responding with RequestCommissioningApproval to the node where the client initiated the
             // RequestCommissioningApproval, you need to wait for it to open a commissioning window on its bridge.
             usleep(kCommissionPrepareTimeMs * 1000);
-
-            DeviceManager::Instance().PairRemoteDevice(nodeId, code.c_str());
+            PairingManager::Instance().SetPairingDelegate(this);
+            DeviceManager::Instance().PairRemoteDevice(mNodeId, code.c_str());
         }
         else
         {
@@ -224,6 +252,8 @@ class FabricAdmin final : public rpc::FabricAdmin, public IcdManager::Delegate
         Platform::Delete(data);
     }
 
+    NodeId mNodeId = chip::kUndefinedNodeId;
+
     // 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.