From ee7e12c09e60c193754bbf1a7ba21fb45ce868db Mon Sep 17 00:00:00 2001 From: Yufeng Wang Date: Mon, 15 Jul 2024 11:37:29 -0700 Subject: [PATCH 1/3] Remove global RPC call handler for each RPC call --- examples/fabric-admin/rpc/RpcClient.cpp | 37 ++++++------------- .../fabric-bridge-app/linux/RpcClient.cpp | 17 +++------ 2 files changed, 17 insertions(+), 37 deletions(-) diff --git a/examples/fabric-admin/rpc/RpcClient.cpp b/examples/fabric-admin/rpc/RpcClient.cpp index 40d2e8ba09aa2e..10e14dfe4cfab3 100644 --- a/examples/fabric-admin/rpc/RpcClient.cpp +++ b/examples/fabric-admin/rpc/RpcClient.cpp @@ -43,14 +43,13 @@ constexpr uint32_t kDefaultChannelId = 1; // Fabric Bridge Client rpc::pw_rpc::nanopb::FabricBridge::Client fabricBridgeClient(rpc::client::GetDefaultRpcClient(), kDefaultChannelId); -pw::rpc::NanopbUnaryReceiver<::pw_protobuf_Empty> addSynchronizedDeviceCall; -pw::rpc::NanopbUnaryReceiver<::pw_protobuf_Empty> removeSynchronizedDeviceCall; std::mutex responseMutex; std::condition_variable responseCv; bool responseReceived = false; CHIP_ERROR responseError = CHIP_NO_ERROR; +// By passing the call object to WaitForResponse to keep RPC call alive until it completes or timeout. template CHIP_ERROR WaitForResponse(CallType & call) { @@ -117,52 +116,38 @@ CHIP_ERROR AddSynchronizedDevice(chip::NodeId nodeId) { ChipLogProgress(NotSpecified, "AddSynchronizedDevice"); - if (addSynchronizedDeviceCall.active()) - { - ChipLogError(NotSpecified, "Add Synchronized Device operation is in progress\n"); - return CHIP_ERROR_BUSY; - } - chip_rpc_SynchronizedDevice device; device.node_id = nodeId; - // By assigning the returned call to the global 'addSynchronizedDeviceCall', 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. - addSynchronizedDeviceCall = fabricBridgeClient.AddSynchronizedDevice(device, OnAddDeviceResponseCompleted); + // 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.AddSynchronizedDevice(device, OnAddDeviceResponseCompleted); - if (!addSynchronizedDeviceCall.active()) + 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(addSynchronizedDeviceCall); + return WaitForResponse(call); } CHIP_ERROR RemoveSynchronizedDevice(chip::NodeId nodeId) { ChipLogProgress(NotSpecified, "RemoveSynchronizedDevice"); - if (removeSynchronizedDeviceCall.active()) - { - ChipLogError(NotSpecified, "Remove Synchronized Device operation is in progress\n"); - return CHIP_ERROR_BUSY; - } - chip_rpc_SynchronizedDevice device; device.node_id = nodeId; - // By assigning the returned call to the global 'removeSynchronizedDeviceCall', 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. - removeSynchronizedDeviceCall = fabricBridgeClient.RemoveSynchronizedDevice(device, OnRemoveDeviceResponseCompleted); + // 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.RemoveSynchronizedDevice(device, OnRemoveDeviceResponseCompleted); - if (!removeSynchronizedDeviceCall.active()) + 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(removeSynchronizedDeviceCall); + return WaitForResponse(call); } diff --git a/examples/fabric-bridge-app/linux/RpcClient.cpp b/examples/fabric-bridge-app/linux/RpcClient.cpp index c26d2b8f0aad31..d31bbbfa9156e3 100644 --- a/examples/fabric-bridge-app/linux/RpcClient.cpp +++ b/examples/fabric-bridge-app/linux/RpcClient.cpp @@ -43,13 +43,13 @@ constexpr uint32_t kDefaultChannelId = 1; // Fabric Admin Client rpc::pw_rpc::nanopb::FabricAdmin::Client fabricAdminClient(rpc::client::GetDefaultRpcClient(), kDefaultChannelId); -pw::rpc::NanopbUnaryReceiver<::chip_rpc_OperationStatus> openCommissioningWindowCall; std::mutex responseMutex; std::condition_variable responseCv; bool responseReceived = false; CHIP_ERROR responseError = CHIP_NO_ERROR; +// By passing the call object to WaitForResponse to keep RPC call alive until it completes or timeout. template CHIP_ERROR WaitForResponse(CallType & call) { @@ -98,23 +98,18 @@ CHIP_ERROR OpenCommissioningWindow(NodeId nodeId) { ChipLogProgress(NotSpecified, "OpenCommissioningWindow with Node Id 0x:" ChipLogFormatX64, ChipLogValueX64(nodeId)); - if (openCommissioningWindowCall.active()) - { - ChipLogError(NotSpecified, "OpenCommissioningWindow is in progress\n"); - return CHIP_ERROR_BUSY; - } - chip_rpc_DeviceInfo device; device.node_id = nodeId; - // The RPC will remain active as long as `openCommissioningWindowCall` is alive. - openCommissioningWindowCall = fabricAdminClient.OpenCommissioningWindow(device, OnOpenCommissioningWindowCompleted); + // 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 = fabricAdminClient.OpenCommissioningWindow(device, OnOpenCommissioningWindowCompleted); - if (!openCommissioningWindowCall.active()) + 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(openCommissioningWindowCall); + return WaitForResponse(call); } From 692c1dc0ebdb583c580c02a875c33db4e65450d2 Mon Sep 17 00:00:00 2001 From: Yufeng Wang Date: Mon, 15 Jul 2024 13:26:54 -0700 Subject: [PATCH 2/3] Update examples/fabric-admin/rpc/RpcClient.cpp Co-authored-by: Terence Hampson --- examples/fabric-admin/rpc/RpcClient.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/examples/fabric-admin/rpc/RpcClient.cpp b/examples/fabric-admin/rpc/RpcClient.cpp index 10e14dfe4cfab3..be9ba23df57b3f 100644 --- a/examples/fabric-admin/rpc/RpcClient.cpp +++ b/examples/fabric-admin/rpc/RpcClient.cpp @@ -49,7 +49,7 @@ std::condition_variable responseCv; bool responseReceived = false; CHIP_ERROR responseError = CHIP_NO_ERROR; -// By passing the call object to WaitForResponse to keep RPC call alive until it completes or timeout. +// By passing the `call` parameter into WaitForResponse we are explicitly trying to insure the caller takes into consideration that the lifetime of the `call` object when calling WaitForResponse template CHIP_ERROR WaitForResponse(CallType & call) { From 42e77d705f2759cca4c0cc392ce1567f4f6ad55f Mon Sep 17 00:00:00 2001 From: Yufeng Wang Date: Mon, 15 Jul 2024 13:32:20 -0700 Subject: [PATCH 3/3] Address review comments --- examples/fabric-admin/rpc/RpcClient.cpp | 3 ++- examples/fabric-bridge-app/linux/RpcClient.cpp | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/examples/fabric-admin/rpc/RpcClient.cpp b/examples/fabric-admin/rpc/RpcClient.cpp index be9ba23df57b3f..00ca1204cc1f19 100644 --- a/examples/fabric-admin/rpc/RpcClient.cpp +++ b/examples/fabric-admin/rpc/RpcClient.cpp @@ -49,7 +49,8 @@ std::condition_variable responseCv; bool responseReceived = false; CHIP_ERROR responseError = CHIP_NO_ERROR; -// By passing the `call` parameter into WaitForResponse we are explicitly trying to insure the caller takes into consideration that the lifetime of the `call` object when calling WaitForResponse +// By passing the `call` parameter into WaitForResponse we are explicitly trying to insure the caller takes into consideration that +// the lifetime of the `call` object when calling WaitForResponse template CHIP_ERROR WaitForResponse(CallType & call) { diff --git a/examples/fabric-bridge-app/linux/RpcClient.cpp b/examples/fabric-bridge-app/linux/RpcClient.cpp index d31bbbfa9156e3..02916b87aaa06a 100644 --- a/examples/fabric-bridge-app/linux/RpcClient.cpp +++ b/examples/fabric-bridge-app/linux/RpcClient.cpp @@ -49,7 +49,8 @@ std::condition_variable responseCv; bool responseReceived = false; CHIP_ERROR responseError = CHIP_NO_ERROR; -// By passing the call object to WaitForResponse to keep RPC call alive until it completes or timeout. +// By passing the `call` parameter into WaitForResponse we are explicitly trying to insure the caller takes into consideration that +// the lifetime of the `call` object when calling WaitForResponse template CHIP_ERROR WaitForResponse(CallType & call) {