From cd0928f8b3b6547bd6616bbc2bcd49fe0714f050 Mon Sep 17 00:00:00 2001 From: lpbeliveau-silabs Date: Tue, 15 Aug 2023 09:18:59 -0400 Subject: [PATCH 1/5] Added a check before going to idle mode to emit a report if it would avoid the device waking up earlier --- src/app/icd/ICDManager.cpp | 15 +++++++++++++++ src/app/icd/ICDManager.h | 1 + src/app/icd/ICDStateObserver.h | 11 ++++++++++- src/app/reporting/ReportSchedulerImpl.cpp | 2 ++ src/app/reporting/ReportSchedulerImpl.h | 1 + .../reporting/SynchronizedReportSchedulerImpl.cpp | 13 +++++++++++++ .../reporting/SynchronizedReportSchedulerImpl.h | 2 ++ 7 files changed, 44 insertions(+), 1 deletion(-) diff --git a/src/app/icd/ICDManager.cpp b/src/app/icd/ICDManager.cpp index 287d0dd11a385a..42b0b150a16176 100644 --- a/src/app/icd/ICDManager.cpp +++ b/src/app/icd/ICDManager.cpp @@ -67,6 +67,7 @@ void ICDManager::Shutdown() // cancel any running timer of the icd DeviceLayer::SystemLayer().CancelTimer(OnIdleModeDone, this); DeviceLayer::SystemLayer().CancelTimer(OnActiveModeDone, this); + DeviceLayer::SystemLayer().CancelTimer(OnActiveModeAlmostDone, this); mICDMode = ICDMode::SIT; mOperationalState = OperationalState::IdleMode; mStorage = nullptr; @@ -157,6 +158,8 @@ void ICDManager::UpdateOperationState(OperationalState state) mOperationalState = OperationalState::ActiveMode; uint32_t activeModeInterval = IcdManagementServer::GetInstance().GetActiveModeInterval(); DeviceLayer::SystemLayer().StartTimer(System::Clock::Timeout(activeModeInterval), OnActiveModeDone, this); + DeviceLayer::SystemLayer().StartTimer(System::Clock::Timeout(activeModeInterval - ICD_ACTIVE_TIME_JITTER_MS), + OnActiveModeAlmostDone, this); CHIP_ERROR err = DeviceLayer::ConnectivityMgr().SetPollingInterval(GetFastPollingInterval()); if (err != CHIP_NO_ERROR) @@ -170,6 +173,8 @@ void ICDManager::UpdateOperationState(OperationalState state) { uint16_t activeModeThreshold = IcdManagementServer::GetInstance().GetActiveModeThreshold(); DeviceLayer::SystemLayer().ExtendTimerTo(System::Clock::Timeout(activeModeThreshold), OnActiveModeDone, this); + DeviceLayer::SystemLayer().ExtendTimerTo(System::Clock::Timeout(activeModeThreshold - ICD_ACTIVE_TIME_JITTER_MS), + OnActiveModeAlmostDone, this); } } } @@ -207,5 +212,15 @@ void ICDManager::OnActiveModeDone(System::Layer * aLayer, void * appState) pIcdManager->UpdateOperationState(OperationalState::IdleMode); } } + +void ICDManager::OnActiveModeAlmostDone(System::Layer * aLayer, void * appState) +{ + ICDManager * pIcdManager = reinterpret_cast(appState); + + // OnActiveModeAlmostDone will trigger a report message if reporting is needed, which should extend the active mode until the + // ack for the report is received. + pIcdManager->mStateObserver->OnActiveModeAlmostDone(); +} + } // namespace app } // namespace chip diff --git a/src/app/icd/ICDManager.h b/src/app/icd/ICDManager.h index 6e9890240c5eee..f817dfbacab422 100644 --- a/src/app/icd/ICDManager.h +++ b/src/app/icd/ICDManager.h @@ -75,6 +75,7 @@ class ICDManager static void OnIdleModeDone(System::Layer * aLayer, void * appState); static void OnActiveModeDone(System::Layer * aLayer, void * appState); + static void OnActiveModeAlmostDone(System::Layer * aLayer, void * appState); private: // SIT ICDs should have a SlowPollingThreshold shorter than or equal to 15s (spec 9.16.1.5) diff --git a/src/app/icd/ICDStateObserver.h b/src/app/icd/ICDStateObserver.h index aa088a7b87f81a..2ce377017e07d3 100644 --- a/src/app/icd/ICDStateObserver.h +++ b/src/app/icd/ICDStateObserver.h @@ -16,6 +16,14 @@ */ #pragma once +#ifndef ICD_SLEEP_TIME_JITTER_MS +#define ICD_SLEEP_TIME_JITTER_MS (CHIP_CONFIG_ICD_IDLE_MODE_INTERVAL * 0.75) +#endif + +#ifndef ICD_ACTIVE_TIME_JITTER_MS +#define ICD_ACTIVE_TIME_JITTER_MS 300 +#endif + namespace chip { namespace app { @@ -23,7 +31,8 @@ class ICDStateObserver { public: virtual ~ICDStateObserver() {} - virtual void OnEnterActiveMode() = 0; + virtual void OnEnterActiveMode() = 0; + virtual void OnActiveModeAlmostDone() = 0; }; } // namespace app diff --git a/src/app/reporting/ReportSchedulerImpl.cpp b/src/app/reporting/ReportSchedulerImpl.cpp index 41d80cc7114ae6..a7cec13ffe5fc1 100644 --- a/src/app/reporting/ReportSchedulerImpl.cpp +++ b/src/app/reporting/ReportSchedulerImpl.cpp @@ -37,6 +37,8 @@ ReportSchedulerImpl::ReportSchedulerImpl(TimerDelegate * aTimerDelegate) : Repor VerifyOrDie(nullptr != mTimerDelegate); } +void ReportSchedulerImpl::OnActiveModeAlmostDone() {} + /// @brief Method that triggers a report emission on each ReadHandler that is not blocked on its min interval. /// Each read handler that is not blocked is immediately marked dirty so that it will report as soon as possible. void ReportSchedulerImpl::OnEnterActiveMode() diff --git a/src/app/reporting/ReportSchedulerImpl.h b/src/app/reporting/ReportSchedulerImpl.h index f3870192c4b1f6..0de9ced719df01 100644 --- a/src/app/reporting/ReportSchedulerImpl.h +++ b/src/app/reporting/ReportSchedulerImpl.h @@ -34,6 +34,7 @@ class ReportSchedulerImpl : public ReportScheduler // ICDStateObserver void OnEnterActiveMode() override; + void OnActiveModeAlmostDone() override; // ReadHandlerObserver void OnSubscriptionEstablished(ReadHandler * aReadHandler) final; diff --git a/src/app/reporting/SynchronizedReportSchedulerImpl.cpp b/src/app/reporting/SynchronizedReportSchedulerImpl.cpp index ddb8bb7b1c4149..d0a4c73b041304 100644 --- a/src/app/reporting/SynchronizedReportSchedulerImpl.cpp +++ b/src/app/reporting/SynchronizedReportSchedulerImpl.cpp @@ -44,6 +44,19 @@ void SynchronizedReportSchedulerImpl::OnReadHandlerDestroyed(ReadHandler * aRead } } +void SynchronizedReportSchedulerImpl::OnActiveModeAlmostDone() +{ + Timestamp now = mTimerDelegate->GetCurrentMonotonicTimestamp(); + uint32_t targetIdleInterval = static_cast(ICD_SLEEP_TIME_JITTER_MS); + VerifyOrReturn(now >= mTestNextReportTimestamp); + if (((mTestNextReportTimestamp - now) < Seconds16(targetIdleInterval)) && (now > mNextMinTimestamp)) + { + // If the next report is due in less than the idle mode interval and we are past the min interval, we can just send it now + CancelReport(); + TimerFired(); + } +} + CHIP_ERROR SynchronizedReportSchedulerImpl::ScheduleReport(Timeout timeout, ReadHandlerNode * node, const Timestamp & now) { // Cancel Report if it is currently scheduled diff --git a/src/app/reporting/SynchronizedReportSchedulerImpl.h b/src/app/reporting/SynchronizedReportSchedulerImpl.h index 9e3689d144194a..34498b9af30b5a 100644 --- a/src/app/reporting/SynchronizedReportSchedulerImpl.h +++ b/src/app/reporting/SynchronizedReportSchedulerImpl.h @@ -38,6 +38,8 @@ class SynchronizedReportSchedulerImpl : public ReportSchedulerImpl, public Timer SynchronizedReportSchedulerImpl(TimerDelegate * aTimerDelegate) : ReportSchedulerImpl(aTimerDelegate) {} ~SynchronizedReportSchedulerImpl() override { UnregisterAllHandlers(); } + void OnActiveModeAlmostDone() override; + bool IsReportScheduled(); void TimerFired() override; From d8bca7191fb9f9c2b2c66abf40b5ab1f4d35be68 Mon Sep 17 00:00:00 2001 From: lpbeliveau-silabs Date: Mon, 21 Aug 2023 10:55:27 -0400 Subject: [PATCH 2/5] Added check to prevent looping when emiting a report before leaving active mode, Changed OnActiveModeAlmostDone to OnTransitionToIdle, added check to prevent wrap around when computing OnTransitionToIdle timeout --- src/app/icd/ICDManager.cpp | 27 +++++++++++++------ src/app/icd/ICDManager.h | 9 ++++++- src/app/icd/ICDStateObserver.h | 4 +-- src/app/reporting/ReportSchedulerImpl.cpp | 2 +- src/app/reporting/ReportSchedulerImpl.h | 2 +- .../SynchronizedReportSchedulerImpl.cpp | 2 +- .../SynchronizedReportSchedulerImpl.h | 2 +- 7 files changed, 33 insertions(+), 15 deletions(-) diff --git a/src/app/icd/ICDManager.cpp b/src/app/icd/ICDManager.cpp index 42b0b150a16176..84e333c2f45e97 100644 --- a/src/app/icd/ICDManager.cpp +++ b/src/app/icd/ICDManager.cpp @@ -67,7 +67,7 @@ void ICDManager::Shutdown() // cancel any running timer of the icd DeviceLayer::SystemLayer().CancelTimer(OnIdleModeDone, this); DeviceLayer::SystemLayer().CancelTimer(OnActiveModeDone, this); - DeviceLayer::SystemLayer().CancelTimer(OnActiveModeAlmostDone, this); + DeviceLayer::SystemLayer().CancelTimer(OnTransitionToIdle, this); mICDMode = ICDMode::SIT; mOperationalState = OperationalState::IdleMode; mStorage = nullptr; @@ -158,8 +158,9 @@ void ICDManager::UpdateOperationState(OperationalState state) mOperationalState = OperationalState::ActiveMode; uint32_t activeModeInterval = IcdManagementServer::GetInstance().GetActiveModeInterval(); DeviceLayer::SystemLayer().StartTimer(System::Clock::Timeout(activeModeInterval), OnActiveModeDone, this); - DeviceLayer::SystemLayer().StartTimer(System::Clock::Timeout(activeModeInterval - ICD_ACTIVE_TIME_JITTER_MS), - OnActiveModeAlmostDone, this); + uint32_t activeModeJitterInterval = + (activeModeInterval >= ICD_ACTIVE_TIME_JITTER_MS) ? activeModeInterval - ICD_ACTIVE_TIME_JITTER_MS : 0; + DeviceLayer::SystemLayer().StartTimer(System::Clock::Timeout(activeModeJitterInterval), OnTransitionToIdle, this); CHIP_ERROR err = DeviceLayer::ConnectivityMgr().SetPollingInterval(GetFastPollingInterval()); if (err != CHIP_NO_ERROR) @@ -173,8 +174,14 @@ void ICDManager::UpdateOperationState(OperationalState state) { uint16_t activeModeThreshold = IcdManagementServer::GetInstance().GetActiveModeThreshold(); DeviceLayer::SystemLayer().ExtendTimerTo(System::Clock::Timeout(activeModeThreshold), OnActiveModeDone, this); - DeviceLayer::SystemLayer().ExtendTimerTo(System::Clock::Timeout(activeModeThreshold - ICD_ACTIVE_TIME_JITTER_MS), - OnActiveModeAlmostDone, this); + uint16_t activeModeJitterThreshold = + (activeModeThreshold >= ICD_ACTIVE_TIME_JITTER_MS) ? activeModeThreshold - ICD_ACTIVE_TIME_JITTER_MS : 0; + if (!mTransitionToIdleCalled) + { + mTransitionToIdleCalled = true; + DeviceLayer::SystemLayer().ExtendTimerTo(System::Clock::Timeout(activeModeJitterThreshold), OnTransitionToIdle, + this); + } } } } @@ -200,6 +207,10 @@ void ICDManager::OnIdleModeDone(System::Layer * aLayer, void * appState) { ICDManager * pIcdManager = reinterpret_cast(appState); pIcdManager->UpdateOperationState(OperationalState::ActiveMode); + + // We only reset this flag when idle mode is complete to avoid re-triggering the check when an event brings us back to active, + // which could cause a loop. + pIcdManager->mTransitionToIdleCalled = false; } void ICDManager::OnActiveModeDone(System::Layer * aLayer, void * appState) @@ -213,13 +224,13 @@ void ICDManager::OnActiveModeDone(System::Layer * aLayer, void * appState) } } -void ICDManager::OnActiveModeAlmostDone(System::Layer * aLayer, void * appState) +void ICDManager::OnTransitionToIdle(System::Layer * aLayer, void * appState) { ICDManager * pIcdManager = reinterpret_cast(appState); - // OnActiveModeAlmostDone will trigger a report message if reporting is needed, which should extend the active mode until the + // OnTransitionToIdle will trigger a report message if reporting is needed, which should extend the active mode until the // ack for the report is received. - pIcdManager->mStateObserver->OnActiveModeAlmostDone(); + pIcdManager->mStateObserver->OnTransitionToIdle(); } } // namespace app diff --git a/src/app/icd/ICDManager.h b/src/app/icd/ICDManager.h index f817dfbacab422..7a84a1a3c4dfee 100644 --- a/src/app/icd/ICDManager.h +++ b/src/app/icd/ICDManager.h @@ -75,7 +75,13 @@ class ICDManager static void OnIdleModeDone(System::Layer * aLayer, void * appState); static void OnActiveModeDone(System::Layer * aLayer, void * appState); - static void OnActiveModeAlmostDone(System::Layer * aLayer, void * appState); + /** + * @brief Callback function called shortly before the device enters idle mode to allow checks to be made. This is currently only + * called once to prevent entering in a loop if some events re-trigger this check (for instance if a check for subscription + * before entering idle mode leads to emiting a report, we will re-enter UpdateOperationState and check again for subscription, + * etc.) + */ + static void OnTransitionToIdle(System::Layer * aLayer, void * appState); private: // SIT ICDs should have a SlowPollingThreshold shorter than or equal to 15s (spec 9.16.1.5) @@ -96,6 +102,7 @@ class ICDManager PersistentStorageDelegate * mStorage = nullptr; FabricTable * mFabricTable = nullptr; ICDStateObserver * mStateObserver = nullptr; + bool mTransitionToIdleCalled = false; }; } // namespace app diff --git a/src/app/icd/ICDStateObserver.h b/src/app/icd/ICDStateObserver.h index 2ce377017e07d3..7c71d4b3c5b463 100644 --- a/src/app/icd/ICDStateObserver.h +++ b/src/app/icd/ICDStateObserver.h @@ -31,8 +31,8 @@ class ICDStateObserver { public: virtual ~ICDStateObserver() {} - virtual void OnEnterActiveMode() = 0; - virtual void OnActiveModeAlmostDone() = 0; + virtual void OnEnterActiveMode() = 0; + virtual void OnTransitionToIdle() = 0; }; } // namespace app diff --git a/src/app/reporting/ReportSchedulerImpl.cpp b/src/app/reporting/ReportSchedulerImpl.cpp index a7cec13ffe5fc1..6c24dd0f5d7dc7 100644 --- a/src/app/reporting/ReportSchedulerImpl.cpp +++ b/src/app/reporting/ReportSchedulerImpl.cpp @@ -37,7 +37,7 @@ ReportSchedulerImpl::ReportSchedulerImpl(TimerDelegate * aTimerDelegate) : Repor VerifyOrDie(nullptr != mTimerDelegate); } -void ReportSchedulerImpl::OnActiveModeAlmostDone() {} +void ReportSchedulerImpl::OnTransitionToIdle() {} /// @brief Method that triggers a report emission on each ReadHandler that is not blocked on its min interval. /// Each read handler that is not blocked is immediately marked dirty so that it will report as soon as possible. diff --git a/src/app/reporting/ReportSchedulerImpl.h b/src/app/reporting/ReportSchedulerImpl.h index 0de9ced719df01..33c70d39f53f21 100644 --- a/src/app/reporting/ReportSchedulerImpl.h +++ b/src/app/reporting/ReportSchedulerImpl.h @@ -34,7 +34,7 @@ class ReportSchedulerImpl : public ReportScheduler // ICDStateObserver void OnEnterActiveMode() override; - void OnActiveModeAlmostDone() override; + void OnTransitionToIdle() override; // ReadHandlerObserver void OnSubscriptionEstablished(ReadHandler * aReadHandler) final; diff --git a/src/app/reporting/SynchronizedReportSchedulerImpl.cpp b/src/app/reporting/SynchronizedReportSchedulerImpl.cpp index d0a4c73b041304..55992e35febd24 100644 --- a/src/app/reporting/SynchronizedReportSchedulerImpl.cpp +++ b/src/app/reporting/SynchronizedReportSchedulerImpl.cpp @@ -44,7 +44,7 @@ void SynchronizedReportSchedulerImpl::OnReadHandlerDestroyed(ReadHandler * aRead } } -void SynchronizedReportSchedulerImpl::OnActiveModeAlmostDone() +void SynchronizedReportSchedulerImpl::OnTransitionToIdle() { Timestamp now = mTimerDelegate->GetCurrentMonotonicTimestamp(); uint32_t targetIdleInterval = static_cast(ICD_SLEEP_TIME_JITTER_MS); diff --git a/src/app/reporting/SynchronizedReportSchedulerImpl.h b/src/app/reporting/SynchronizedReportSchedulerImpl.h index 34498b9af30b5a..69f9649b3a575f 100644 --- a/src/app/reporting/SynchronizedReportSchedulerImpl.h +++ b/src/app/reporting/SynchronizedReportSchedulerImpl.h @@ -38,7 +38,7 @@ class SynchronizedReportSchedulerImpl : public ReportSchedulerImpl, public Timer SynchronizedReportSchedulerImpl(TimerDelegate * aTimerDelegate) : ReportSchedulerImpl(aTimerDelegate) {} ~SynchronizedReportSchedulerImpl() override { UnregisterAllHandlers(); } - void OnActiveModeAlmostDone() override; + void OnTransitionToIdle() override; bool IsReportScheduled(); From 8dfed19a1566db55486e17b7fab044f2a489f832 Mon Sep 17 00:00:00 2001 From: lpbeliveau-silabs Date: Mon, 21 Aug 2023 14:05:14 -0400 Subject: [PATCH 3/5] Moved boolean set to true to proper place --- src/app/icd/ICDManager.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/icd/ICDManager.cpp b/src/app/icd/ICDManager.cpp index 84e333c2f45e97..a68aeb724132f0 100644 --- a/src/app/icd/ICDManager.cpp +++ b/src/app/icd/ICDManager.cpp @@ -178,7 +178,6 @@ void ICDManager::UpdateOperationState(OperationalState state) (activeModeThreshold >= ICD_ACTIVE_TIME_JITTER_MS) ? activeModeThreshold - ICD_ACTIVE_TIME_JITTER_MS : 0; if (!mTransitionToIdleCalled) { - mTransitionToIdleCalled = true; DeviceLayer::SystemLayer().ExtendTimerTo(System::Clock::Timeout(activeModeJitterThreshold), OnTransitionToIdle, this); } @@ -231,6 +230,7 @@ void ICDManager::OnTransitionToIdle(System::Layer * aLayer, void * appState) // OnTransitionToIdle will trigger a report message if reporting is needed, which should extend the active mode until the // ack for the report is received. pIcdManager->mStateObserver->OnTransitionToIdle(); + pIcdManager->mTransitionToIdleCalled = true; } } // namespace app From fdcb713c201c33aa8685f8cdfbe6830ff0c66b1b Mon Sep 17 00:00:00 2001 From: lpbeliveau-silabs Date: Tue, 22 Aug 2023 09:43:01 -0400 Subject: [PATCH 4/5] Set flag before calling observer method --- src/app/icd/ICDManager.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/icd/ICDManager.cpp b/src/app/icd/ICDManager.cpp index a68aeb724132f0..6601877793f8e0 100644 --- a/src/app/icd/ICDManager.cpp +++ b/src/app/icd/ICDManager.cpp @@ -229,8 +229,8 @@ void ICDManager::OnTransitionToIdle(System::Layer * aLayer, void * appState) // OnTransitionToIdle will trigger a report message if reporting is needed, which should extend the active mode until the // ack for the report is received. - pIcdManager->mStateObserver->OnTransitionToIdle(); pIcdManager->mTransitionToIdleCalled = true; + pIcdManager->mStateObserver->OnTransitionToIdle(); } } // namespace app From 2b3e177866905785dc2e3187aa42e728690450b1 Mon Sep 17 00:00:00 2001 From: lpbeliveau-silabs Date: Tue, 22 Aug 2023 10:07:35 -0400 Subject: [PATCH 5/5] Added stub in observer test function --- src/app/tests/TestICDManager.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/app/tests/TestICDManager.cpp b/src/app/tests/TestICDManager.cpp index 81739aea43ceb4..2f5546f067c2e4 100644 --- a/src/app/tests/TestICDManager.cpp +++ b/src/app/tests/TestICDManager.cpp @@ -37,6 +37,7 @@ class TestICDStateObserver : public app::ICDStateObserver { public: void OnEnterActiveMode() {} + void OnTransitionToIdle() {} }; TestICDStateObserver mICDStateObserver;