From 260224115542776bf287f15bbf697c824d037673 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Wed, 4 Sep 2024 18:04:37 -0400 Subject: [PATCH] Address follow-up PR comments for fabric-admin UniqueIdGetter (#35405) --------- Co-authored-by: Restyled.io Co-authored-by: saurabhst --- examples/fabric-admin/BUILD.gn | 4 +- .../device_manager/DeviceSynchronization.cpp | 63 +++++++++++-------- .../device_manager/DeviceSynchronization.h | 6 +- .../{UidGetter.cpp => UniqueIdGetter.cpp} | 24 +++---- .../{UidGetter.h => UniqueIdGetter.h} | 20 ++++-- 5 files changed, 71 insertions(+), 46 deletions(-) rename examples/fabric-admin/device_manager/{UidGetter.cpp => UniqueIdGetter.cpp} (80%) rename examples/fabric-admin/device_manager/{UidGetter.h => UniqueIdGetter.h} (73%) diff --git a/examples/fabric-admin/BUILD.gn b/examples/fabric-admin/BUILD.gn index 36ba7ec4e15d51..d1add205f8826c 100644 --- 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 index bc3e9b31fe6b1b..6cc1cea690df2f 100644 --- 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 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 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 index 495fecd60bd4b5..11b640236268dd 100644 --- 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 #include @@ -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/examples/fabric-admin/device_manager/UidGetter.cpp b/examples/fabric-admin/device_manager/UniqueIdGetter.cpp similarity index 80% rename from examples/fabric-admin/device_manager/UidGetter.cpp rename to examples/fabric-admin/device_manager/UniqueIdGetter.cpp index baeaba6a02498c..3aba21752d9c67 100644 --- a/examples/fabric-admin/device_manager/UidGetter.cpp +++ b/examples/fabric-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(context)->OnDeviceConnected(exchangeMgr, sessionHandle); + reinterpret_cast(context)->OnDeviceConnected(exchangeMgr, sessionHandle); } void OnDeviceConnectionFailureWrapper(void * context, const ScopedNodeId & peerId, CHIP_ERROR error) { - reinterpret_cast(context)->OnDeviceConnectionFailure(peerId, error); + reinterpret_cast(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(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(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/examples/fabric-admin/device_manager/UidGetter.h b/examples/fabric-admin/device_manager/UniqueIdGetter.h similarity index 73% rename from examples/fabric-admin/device_manager/UidGetter.h rename to examples/fabric-admin/device_manager/UniqueIdGetter.h index 5f99c75198c5dc..6680adf5bdfc97 100644 --- a/examples/fabric-admin/device_manager/UidGetter.h +++ b/examples/fabric-admin/device_manager/UniqueIdGetter.h @@ -24,15 +24,27 @@ #include #include -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)>; - 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