From 21ac31a0872b7d4c7280549093134148214bc549 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Mon, 21 Oct 2024 15:10:34 +0000 Subject: [PATCH 1/2] Call DeviceReachableChanged when subscription fails --- .../device_manager/DeviceSubscription.cpp | 22 ++++++++++++++++++- .../device_manager/DeviceSynchronization.cpp | 6 ++++- examples/fabric-admin/rpc/RpcClient.cpp | 22 +++++++++++++++++++ examples/fabric-admin/rpc/RpcClient.h | 11 ++++++++++ 4 files changed, 59 insertions(+), 2 deletions(-) diff --git a/examples/fabric-admin/device_manager/DeviceSubscription.cpp b/examples/fabric-admin/device_manager/DeviceSubscription.cpp index 0da2bdddd4fbf5..38d67968b2fce6 100644 --- a/examples/fabric-admin/device_manager/DeviceSubscription.cpp +++ b/examples/fabric-admin/device_manager/DeviceSubscription.cpp @@ -125,6 +125,16 @@ void DeviceSubscription::OnDone(ReadClient * apReadClient) void DeviceSubscription::OnError(CHIP_ERROR error) { +#if defined(PW_RPC_ENABLED) + if (error == CHIP_ERROR_TIMEOUT && mState == State::SubscriptionStarted) + { + chip_rpc_ReachabilityChanged reachabilityChanged; + reachabilityChanged.has_id = true; + reachabilityChanged.id = mCurrentAdministratorCommissioningAttributes.id; + reachabilityChanged.reachability = false; + DeviceReachableChanged(reachabilityChanged); + } +#endif ChipLogProgress(NotSpecified, "Error subscribing: %" CHIP_ERROR_FORMAT, error.Format()); } @@ -198,7 +208,17 @@ void DeviceSubscription::OnDeviceConnectionFailure(const ScopedNodeId & peerId, { VerifyOrDie(mState == State::Connecting || mState == State::Stopping); ChipLogError(NotSpecified, "DeviceSubscription failed to connect to " ChipLogFormatX64, ChipLogValueX64(peerId.GetNodeId())); - // TODO(#35333) Figure out how we should recover if we fail to connect and mState == State::Connecting. +#if defined(PW_RPC_ENABLED) + if (mState == State::Connecting) + { + chip_rpc_ReachabilityChanged reachabilityChanged; + reachabilityChanged.has_id = true; + reachabilityChanged.id = mCurrentAdministratorCommissioningAttributes.id; + reachabilityChanged.reachability = false; + DeviceReachableChanged(reachabilityChanged); + + } +#endif // After calling mOnDoneCallback we are indicating that `this` is deleted and we shouldn't do anything else with // DeviceSubscription. diff --git a/examples/fabric-admin/device_manager/DeviceSynchronization.cpp b/examples/fabric-admin/device_manager/DeviceSynchronization.cpp index 6c2284018e0d19..497c6c305e435f 100644 --- a/examples/fabric-admin/device_manager/DeviceSynchronization.cpp +++ b/examples/fabric-admin/device_manager/DeviceSynchronization.cpp @@ -277,12 +277,16 @@ void DeviceSynchronizer::SynchronizationCompleteAddDevice() if (!mCurrentDeviceData.is_icd) { VerifyOrDie(mController); - // TODO(#35333) Figure out how we should recover in this circumstance. 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)); + chip_rpc_ReachabilityChanged reachabilityChanged; + reachabilityChanged.has_id = true; + reachabilityChanged.id = mCurrentDeviceData.id; + reachabilityChanged.reachability = false; + DeviceReachableChanged(reachabilityChanged); } } #endif diff --git a/examples/fabric-admin/rpc/RpcClient.cpp b/examples/fabric-admin/rpc/RpcClient.cpp index e33dd0472e6fe2..9964dc6767b67e 100644 --- a/examples/fabric-admin/rpc/RpcClient.cpp +++ b/examples/fabric-admin/rpc/RpcClient.cpp @@ -205,3 +205,25 @@ CHIP_ERROR AdminCommissioningAttributeChanged(const chip_rpc_AdministratorCommis return WaitForResponse(call); } + +CHIP_ERROR DeviceReachableChanged(const chip_rpc_ReachabilityChanged & data) +{ + ChipLogProgress(NotSpecified, "DeviceReachableChanged"); + // TODO(#35333): When there is some sort of device manager in fabric-admin that handles all the devices we + // are currently connected to (and not just device on the remote bridge), we should notify that manager + // so that it can properly handle any sort of reconnection logic. This can either be done here when + // `data.reachability == false`, or more control can be given wherever DeviceReachableChanged is currently + // called + + // 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. + auto call = fabricBridgeClient.DeviceReachableChanged(data, RpcCompletedWithEmptyResponse); + + if (!call.active()) + { + // The RPC call was not sent. This could occur due to, for example, an invalid channel ID. Handle if necessary. + return CHIP_ERROR_INTERNAL; + } + + return WaitForResponse(call); +} diff --git a/examples/fabric-admin/rpc/RpcClient.h b/examples/fabric-admin/rpc/RpcClient.h index da6a82115a3cd2..a4cefe8bd8e076 100644 --- a/examples/fabric-admin/rpc/RpcClient.h +++ b/examples/fabric-admin/rpc/RpcClient.h @@ -88,3 +88,14 @@ CHIP_ERROR ActiveChanged(chip::ScopedNodeId scopedNodeId, uint32_t promisedActiv * - CHIP_ERROR_INTERNAL: An internal error occurred while activating the RPC call. */ CHIP_ERROR AdminCommissioningAttributeChanged(const chip_rpc_AdministratorCommissioningChanged & data); + +/** + * @brief Notify that reachachability of the bridged device has changed + * + * @param data information regarding change in reachability of the bridged device. + * @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 DeviceReachableChanged(const chip_rpc_ReachabilityChanged & data); From 4a305ff4dab84831025e60f05967462dd35d4495 Mon Sep 17 00:00:00 2001 From: "Restyled.io" Date: Mon, 21 Oct 2024 15:21:31 +0000 Subject: [PATCH 2/2] Restyled by clang-format --- .../fabric-admin/device_manager/DeviceSubscription.cpp | 9 ++++----- .../device_manager/DeviceSynchronization.cpp | 4 ++-- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/examples/fabric-admin/device_manager/DeviceSubscription.cpp b/examples/fabric-admin/device_manager/DeviceSubscription.cpp index 38d67968b2fce6..434d349e8c5113 100644 --- a/examples/fabric-admin/device_manager/DeviceSubscription.cpp +++ b/examples/fabric-admin/device_manager/DeviceSubscription.cpp @@ -129,8 +129,8 @@ void DeviceSubscription::OnError(CHIP_ERROR error) if (error == CHIP_ERROR_TIMEOUT && mState == State::SubscriptionStarted) { chip_rpc_ReachabilityChanged reachabilityChanged; - reachabilityChanged.has_id = true; - reachabilityChanged.id = mCurrentAdministratorCommissioningAttributes.id; + reachabilityChanged.has_id = true; + reachabilityChanged.id = mCurrentAdministratorCommissioningAttributes.id; reachabilityChanged.reachability = false; DeviceReachableChanged(reachabilityChanged); } @@ -212,11 +212,10 @@ void DeviceSubscription::OnDeviceConnectionFailure(const ScopedNodeId & peerId, if (mState == State::Connecting) { chip_rpc_ReachabilityChanged reachabilityChanged; - reachabilityChanged.has_id = true; - reachabilityChanged.id = mCurrentAdministratorCommissioningAttributes.id; + reachabilityChanged.has_id = true; + reachabilityChanged.id = mCurrentAdministratorCommissioningAttributes.id; reachabilityChanged.reachability = false; DeviceReachableChanged(reachabilityChanged); - } #endif diff --git a/examples/fabric-admin/device_manager/DeviceSynchronization.cpp b/examples/fabric-admin/device_manager/DeviceSynchronization.cpp index 497c6c305e435f..1e8728ee59b567 100644 --- a/examples/fabric-admin/device_manager/DeviceSynchronization.cpp +++ b/examples/fabric-admin/device_manager/DeviceSynchronization.cpp @@ -283,8 +283,8 @@ void DeviceSynchronizer::SynchronizationCompleteAddDevice() { ChipLogError(NotSpecified, "Failed start subscription to NodeId:" ChipLogFormatX64, ChipLogValueX64(mNodeId)); chip_rpc_ReachabilityChanged reachabilityChanged; - reachabilityChanged.has_id = true; - reachabilityChanged.id = mCurrentDeviceData.id; + reachabilityChanged.has_id = true; + reachabilityChanged.id = mCurrentDeviceData.id; reachabilityChanged.reachability = false; DeviceReachableChanged(reachabilityChanged); }