From 782c7d8f4ddb6bf70b17d526626d7d0e911ad569 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Tue, 20 Aug 2024 19:51:07 +0000 Subject: [PATCH 1/9] Add expiry to KeepActive as per spec into fabric-admin example --- examples/fabric-admin/rpc/RpcServer.cpp | 33 +++++++++++++++++++------ 1 file changed, 25 insertions(+), 8 deletions(-) diff --git a/examples/fabric-admin/rpc/RpcServer.cpp b/examples/fabric-admin/rpc/RpcServer.cpp index f240feda22a2d4..9a2c7418e34800 100644 --- a/examples/fabric-admin/rpc/RpcServer.cpp +++ b/examples/fabric-admin/rpc/RpcServer.cpp @@ -50,17 +50,22 @@ class FabricAdmin final : public rpc::FabricAdmin, public IcdManager::Delegate chip::NodeId nodeId = clientInfo.peer_node.GetNodeId(); auto it = mPendingKeepActiveTimesMs.find(nodeId); VerifyOrReturn(it != mPendingKeepActiveTimesMs.end()); - // TODO(#33221): We also need a mechanism here to drop KeepActive - // request if they were recieved over 60 mins ago. - uint32_t stayActiveDurationMs = it->second; + + KeepActiveDataForCheckIn checkInData = it->second; + mPendingKeepActiveTimesMs.erase(nodeId); + System::Clock::Milliseconds64 timeNowMs = + std::chrono::duration_cast(System::SystemClock().GetMonotonicMilliseconds64()); + + if (timeNowMs > checkInData.mRequestExpiresAtMs) { + ChipLogError(NotSpecified, "ICD check-in for device we have been waiting, came after KeepActive expiry. Reqeust dropped"); + return; + } // TODO(#33221): If there is a failure in sending the message this request just gets dropped. // Work to see if there should be update to spec on whether some sort of failure later on // Should be indicated in some manner, or identify a better recovery mechanism here. - mPendingKeepActiveTimesMs.erase(nodeId); - auto onDone = [=](uint32_t promisedActiveDuration) { ActiveChanged(nodeId, promisedActiveDuration); }; - CHIP_ERROR err = StayActiveSender::SendStayActiveCommand(stayActiveDurationMs, clientInfo.peer_node, + CHIP_ERROR err = StayActiveSender::SendStayActiveCommand(checkInData.mStayActiveDurationMs, clientInfo.peer_node, chip::app::InteractionModelEngine::GetInstance(), onDone); if (err != CHIP_NO_ERROR) { @@ -145,10 +150,22 @@ class FabricAdmin final : public rpc::FabricAdmin, public IcdManager::Delegate void ScheduleSendingKeepActiveOnCheckIn(chip::NodeId nodeId, uint32_t stayActiveDurationMs) { - mPendingKeepActiveTimesMs[nodeId] = stayActiveDurationMs; + + System::Clock::Milliseconds64 timeNowMs = + std::chrono::duration_cast(System::SystemClock().GetMonotonicMilliseconds64()); + // Spec says we should expire the request 60 mins after we get it + System::Clock::Milliseconds64 expireTimeMs = timeNowMs + System::Clock::Milliseconds64(60*60*1000); + KeepActiveDataForCheckIn checkInData = {.mStayActiveDurationMs = stayActiveDurationMs, .mRequestExpiresAtMs = expireTimeMs}; + mPendingKeepActiveTimesMs[nodeId] = checkInData; } private: + struct KeepActiveDataForCheckIn + { + uint32_t mStayActiveDurationMs = 0; + System::Clock::Milliseconds64 mRequestExpiresAtMs; + }; + struct KeepActiveWorkData { KeepActiveWorkData(FabricAdmin * fabricAdmin, chip::NodeId nodeId, uint32_t stayActiveDurationMs) : @@ -168,7 +185,7 @@ class FabricAdmin final : public rpc::FabricAdmin, public IcdManager::Delegate } // Modifications to mPendingKeepActiveTimesMs should be done on the MatterEventLoop thread - std::map mPendingKeepActiveTimesMs; + std::map mPendingKeepActiveTimesMs; }; FabricAdmin fabric_admin_service; From b102d760a8b46d9b6195f2866c9c0815da7dac00 Mon Sep 17 00:00:00 2001 From: "Restyled.io" Date: Tue, 20 Aug 2024 19:52:03 +0000 Subject: [PATCH 2/9] Restyled by clang-format --- examples/fabric-admin/rpc/RpcServer.cpp | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/examples/fabric-admin/rpc/RpcServer.cpp b/examples/fabric-admin/rpc/RpcServer.cpp index 9a2c7418e34800..2f35765175e59b 100644 --- a/examples/fabric-admin/rpc/RpcServer.cpp +++ b/examples/fabric-admin/rpc/RpcServer.cpp @@ -56,8 +56,10 @@ class FabricAdmin final : public rpc::FabricAdmin, public IcdManager::Delegate System::Clock::Milliseconds64 timeNowMs = std::chrono::duration_cast(System::SystemClock().GetMonotonicMilliseconds64()); - if (timeNowMs > checkInData.mRequestExpiresAtMs) { - ChipLogError(NotSpecified, "ICD check-in for device we have been waiting, came after KeepActive expiry. Reqeust dropped"); + if (timeNowMs > checkInData.mRequestExpiresAtMs) + { + ChipLogError(NotSpecified, + "ICD check-in for device we have been waiting, came after KeepActive expiry. Reqeust dropped"); return; } @@ -154,9 +156,10 @@ class FabricAdmin final : public rpc::FabricAdmin, public IcdManager::Delegate System::Clock::Milliseconds64 timeNowMs = std::chrono::duration_cast(System::SystemClock().GetMonotonicMilliseconds64()); // Spec says we should expire the request 60 mins after we get it - System::Clock::Milliseconds64 expireTimeMs = timeNowMs + System::Clock::Milliseconds64(60*60*1000); - KeepActiveDataForCheckIn checkInData = {.mStayActiveDurationMs = stayActiveDurationMs, .mRequestExpiresAtMs = expireTimeMs}; - mPendingKeepActiveTimesMs[nodeId] = checkInData; + System::Clock::Milliseconds64 expireTimeMs = timeNowMs + System::Clock::Milliseconds64(60 * 60 * 1000); + KeepActiveDataForCheckIn checkInData = { .mStayActiveDurationMs = stayActiveDurationMs, + .mRequestExpiresAtMs = expireTimeMs }; + mPendingKeepActiveTimesMs[nodeId] = checkInData; } private: From a61b9f064e3efc3ae220d91e2fbe081d025d45cf Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Tue, 20 Aug 2024 20:53:55 +0000 Subject: [PATCH 3/9] Self Review fix --- examples/fabric-admin/rpc/RpcServer.cpp | 30 ++++++++++++------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/examples/fabric-admin/rpc/RpcServer.cpp b/examples/fabric-admin/rpc/RpcServer.cpp index 2f35765175e59b..609dbe98687376 100644 --- a/examples/fabric-admin/rpc/RpcServer.cpp +++ b/examples/fabric-admin/rpc/RpcServer.cpp @@ -48,15 +48,16 @@ class FabricAdmin final : public rpc::FabricAdmin, public IcdManager::Delegate void OnCheckInCompleted(const chip::app::ICDClientInfo & clientInfo) override { chip::NodeId nodeId = clientInfo.peer_node.GetNodeId(); - auto it = mPendingKeepActiveTimesMs.find(nodeId); - VerifyOrReturn(it != mPendingKeepActiveTimesMs.end()); + auto it = mPendingCheckIn.find(nodeId); + VerifyOrReturn(it != mPendingCheckIn.end()); KeepActiveDataForCheckIn checkInData = it->second; - mPendingKeepActiveTimesMs.erase(nodeId); - System::Clock::Milliseconds64 timeNowMs = - std::chrono::duration_cast(System::SystemClock().GetMonotonicMilliseconds64()); + // Removed from pending map as check-in from this node has occured and we will handle the pending KeepActive + // request. + mPendingCheckIn.erase(nodeId); - if (timeNowMs > checkInData.mRequestExpiresAtMs) + auto timeNowMs = System::SystemClock().GetMonotonicTimestamp(); + if (timeNowMs > checkInData.mRequestExpiryTimestamp) { ChipLogError(NotSpecified, "ICD check-in for device we have been waiting, came after KeepActive expiry. Reqeust dropped"); @@ -153,20 +154,19 @@ class FabricAdmin final : public rpc::FabricAdmin, public IcdManager::Delegate void ScheduleSendingKeepActiveOnCheckIn(chip::NodeId nodeId, uint32_t stayActiveDurationMs) { - System::Clock::Milliseconds64 timeNowMs = - std::chrono::duration_cast(System::SystemClock().GetMonotonicMilliseconds64()); + auto timeNowMs = System::SystemClock().GetMonotonicTimestamp(); // Spec says we should expire the request 60 mins after we get it - System::Clock::Milliseconds64 expireTimeMs = timeNowMs + System::Clock::Milliseconds64(60 * 60 * 1000); - KeepActiveDataForCheckIn checkInData = { .mStayActiveDurationMs = stayActiveDurationMs, - .mRequestExpiresAtMs = expireTimeMs }; - mPendingKeepActiveTimesMs[nodeId] = checkInData; + System::Clock::Timestamp expiryTimestamp = timeNowMs + System::Clock::Seconds64(60 * 60); + KeepActiveDataForCheckIn checkInData = { .mStayActiveDurationMs = stayActiveDurationMs, + .mRequestExpiryTimestamp = expiryTimestamp }; + mPendingCheckIn[nodeId] = checkInData; } private: struct KeepActiveDataForCheckIn { uint32_t mStayActiveDurationMs = 0; - System::Clock::Milliseconds64 mRequestExpiresAtMs; + System::Clock::Timestamp mRequestExpiryTimestamp; }; struct KeepActiveWorkData @@ -187,8 +187,8 @@ class FabricAdmin final : public rpc::FabricAdmin, public IcdManager::Delegate chip::Platform::Delete(data); } - // Modifications to mPendingKeepActiveTimesMs should be done on the MatterEventLoop thread - std::map mPendingKeepActiveTimesMs; + // Modifications to mPendingCheckIn should be done on the MatterEventLoop thread + std::map mPendingCheckIn; }; FabricAdmin fabric_admin_service; From 9f554a6d7fd5349ebaecd60b10703a87a4939add Mon Sep 17 00:00:00 2001 From: "Restyled.io" Date: Tue, 20 Aug 2024 20:54:17 +0000 Subject: [PATCH 4/9] Restyled by clang-format --- examples/fabric-admin/rpc/RpcServer.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/examples/fabric-admin/rpc/RpcServer.cpp b/examples/fabric-admin/rpc/RpcServer.cpp index 609dbe98687376..2ebc0938bbd11f 100644 --- a/examples/fabric-admin/rpc/RpcServer.cpp +++ b/examples/fabric-admin/rpc/RpcServer.cpp @@ -157,9 +157,9 @@ class FabricAdmin final : public rpc::FabricAdmin, public IcdManager::Delegate auto timeNowMs = System::SystemClock().GetMonotonicTimestamp(); // Spec says we should expire the request 60 mins after we get it System::Clock::Timestamp expiryTimestamp = timeNowMs + System::Clock::Seconds64(60 * 60); - KeepActiveDataForCheckIn checkInData = { .mStayActiveDurationMs = stayActiveDurationMs, - .mRequestExpiryTimestamp = expiryTimestamp }; - mPendingCheckIn[nodeId] = checkInData; + KeepActiveDataForCheckIn checkInData = { .mStayActiveDurationMs = stayActiveDurationMs, + .mRequestExpiryTimestamp = expiryTimestamp }; + mPendingCheckIn[nodeId] = checkInData; } private: From d2a9c4b6deb50034903bd2df3543eafdfb13f308 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Tue, 20 Aug 2024 21:19:59 +0000 Subject: [PATCH 5/9] Name fix --- examples/fabric-admin/rpc/RpcServer.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/examples/fabric-admin/rpc/RpcServer.cpp b/examples/fabric-admin/rpc/RpcServer.cpp index 2ebc0938bbd11f..447579648c08ff 100644 --- a/examples/fabric-admin/rpc/RpcServer.cpp +++ b/examples/fabric-admin/rpc/RpcServer.cpp @@ -56,8 +56,8 @@ class FabricAdmin final : public rpc::FabricAdmin, public IcdManager::Delegate // request. mPendingCheckIn.erase(nodeId); - auto timeNowMs = System::SystemClock().GetMonotonicTimestamp(); - if (timeNowMs > checkInData.mRequestExpiryTimestamp) + auto timeNow = System::SystemClock().GetMonotonicTimestamp(); + if (timeNow > checkInData.mRequestExpiryTimestamp) { ChipLogError(NotSpecified, "ICD check-in for device we have been waiting, came after KeepActive expiry. Reqeust dropped"); @@ -154,9 +154,9 @@ class FabricAdmin final : public rpc::FabricAdmin, public IcdManager::Delegate void ScheduleSendingKeepActiveOnCheckIn(chip::NodeId nodeId, uint32_t stayActiveDurationMs) { - auto timeNowMs = System::SystemClock().GetMonotonicTimestamp(); + auto timeNow = System::SystemClock().GetMonotonicTimestamp(); // Spec says we should expire the request 60 mins after we get it - System::Clock::Timestamp expiryTimestamp = timeNowMs + System::Clock::Seconds64(60 * 60); + System::Clock::Timestamp expiryTimestamp = timeNow + System::Clock::Seconds64(60 * 60); KeepActiveDataForCheckIn checkInData = { .mStayActiveDurationMs = stayActiveDurationMs, .mRequestExpiryTimestamp = expiryTimestamp }; mPendingCheckIn[nodeId] = checkInData; From 5557b0135c49c73a945f0d500b880250e6671563 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Wed, 21 Aug 2024 14:59:00 +0000 Subject: [PATCH 6/9] Address PR comments --- examples/fabric-admin/rpc/RpcServer.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/examples/fabric-admin/rpc/RpcServer.cpp b/examples/fabric-admin/rpc/RpcServer.cpp index 447579648c08ff..ba0e8eb3842c08 100644 --- a/examples/fabric-admin/rpc/RpcServer.cpp +++ b/examples/fabric-admin/rpc/RpcServer.cpp @@ -60,7 +60,7 @@ class FabricAdmin final : public rpc::FabricAdmin, public IcdManager::Delegate if (timeNow > checkInData.mRequestExpiryTimestamp) { ChipLogError(NotSpecified, - "ICD check-in for device we have been waiting, came after KeepActive expiry. Reqeust dropped"); + "ICD check-in for device we have been waiting, came after KeepActive expiry. Reqeust dropped for Node ID: 0x%lx", nodeId); return; } @@ -188,7 +188,9 @@ class FabricAdmin final : public rpc::FabricAdmin, public IcdManager::Delegate } // Modifications to mPendingCheckIn should be done on the MatterEventLoop thread - std::map mPendingCheckIn; + // otherwise we would need a mutex protecting this data to prevent race as this + // data is accessible by both RPC thread and Matter eventloop. + std::unordered_map mPendingCheckIn; }; FabricAdmin fabric_admin_service; From 1cf8ef8536259c5619c612f58bcc842dac14b14d Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Wed, 21 Aug 2024 14:59:47 +0000 Subject: [PATCH 7/9] Address PR comments --- examples/fabric-admin/rpc/RpcServer.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/examples/fabric-admin/rpc/RpcServer.cpp b/examples/fabric-admin/rpc/RpcServer.cpp index ba0e8eb3842c08..4f852dbaec84c0 100644 --- a/examples/fabric-admin/rpc/RpcServer.cpp +++ b/examples/fabric-admin/rpc/RpcServer.cpp @@ -47,7 +47,7 @@ class FabricAdmin final : public rpc::FabricAdmin, public IcdManager::Delegate public: void OnCheckInCompleted(const chip::app::ICDClientInfo & clientInfo) override { - chip::NodeId nodeId = clientInfo.peer_node.GetNodeId(); + NodeId nodeId = clientInfo.peer_node.GetNodeId(); auto it = mPendingCheckIn.find(nodeId); VerifyOrReturn(it != mPendingCheckIn.end()); @@ -190,7 +190,7 @@ class FabricAdmin final : public rpc::FabricAdmin, public IcdManager::Delegate // 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. - std::unordered_map mPendingCheckIn; + std::unordered_map mPendingCheckIn; }; FabricAdmin fabric_admin_service; From 4593e5b003f2133c7747d18a1a5ce09ab65a8e33 Mon Sep 17 00:00:00 2001 From: "Restyled.io" Date: Wed, 21 Aug 2024 15:00:05 +0000 Subject: [PATCH 8/9] Restyled by clang-format --- examples/fabric-admin/rpc/RpcServer.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/examples/fabric-admin/rpc/RpcServer.cpp b/examples/fabric-admin/rpc/RpcServer.cpp index 4f852dbaec84c0..ee77877cf9f6c2 100644 --- a/examples/fabric-admin/rpc/RpcServer.cpp +++ b/examples/fabric-admin/rpc/RpcServer.cpp @@ -48,7 +48,7 @@ class FabricAdmin final : public rpc::FabricAdmin, public IcdManager::Delegate void OnCheckInCompleted(const chip::app::ICDClientInfo & clientInfo) override { NodeId nodeId = clientInfo.peer_node.GetNodeId(); - auto it = mPendingCheckIn.find(nodeId); + auto it = mPendingCheckIn.find(nodeId); VerifyOrReturn(it != mPendingCheckIn.end()); KeepActiveDataForCheckIn checkInData = it->second; @@ -59,8 +59,10 @@ class FabricAdmin final : public rpc::FabricAdmin, public IcdManager::Delegate auto timeNow = System::SystemClock().GetMonotonicTimestamp(); if (timeNow > checkInData.mRequestExpiryTimestamp) { - ChipLogError(NotSpecified, - "ICD check-in for device we have been waiting, came after KeepActive expiry. Reqeust dropped for Node ID: 0x%lx", nodeId); + ChipLogError( + NotSpecified, + "ICD check-in for device we have been waiting, came after KeepActive expiry. Reqeust dropped for Node ID: 0x%lx", + nodeId); return; } From 3992acb6797644b600ff2fe867abbf1ad7f8f7c4 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Wed, 21 Aug 2024 16:20:53 +0000 Subject: [PATCH 9/9] Address PR comments --- examples/fabric-admin/rpc/RpcServer.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/examples/fabric-admin/rpc/RpcServer.cpp b/examples/fabric-admin/rpc/RpcServer.cpp index ee77877cf9f6c2..eb2d9883c881a7 100644 --- a/examples/fabric-admin/rpc/RpcServer.cpp +++ b/examples/fabric-admin/rpc/RpcServer.cpp @@ -47,6 +47,8 @@ class FabricAdmin final : public rpc::FabricAdmin, public IcdManager::Delegate public: void OnCheckInCompleted(const chip::app::ICDClientInfo & clientInfo) override { + // Needs for accessing mPendingCheckIn + assertChipStackLockedByCurrentThread(); NodeId nodeId = clientInfo.peer_node.GetNodeId(); auto it = mPendingCheckIn.find(nodeId); VerifyOrReturn(it != mPendingCheckIn.end()); @@ -155,6 +157,8 @@ class FabricAdmin final : public rpc::FabricAdmin, public IcdManager::Delegate void ScheduleSendingKeepActiveOnCheckIn(chip::NodeId nodeId, uint32_t stayActiveDurationMs) { + // Needs for accessing mPendingCheckIn + assertChipStackLockedByCurrentThread(); auto timeNow = System::SystemClock().GetMonotonicTimestamp(); // Spec says we should expire the request 60 mins after we get it