From e22fb9e34bb8a7281ea6cb1620ce96f9f73c996c Mon Sep 17 00:00:00 2001 From: lpbeliveau-silabs Date: Wed, 5 Jul 2023 13:47:50 -0400 Subject: [PATCH 01/12] Added a Synchronized ReportScheduler along with test to confirm the behavior on the scheduler with up to 4 ReadHandlers as well logging mechanism to find out what handler fires at what time. --- src/app/BUILD.gn | 2 + src/app/reporting/ReportScheduler.h | 31 +- src/app/reporting/ReportSchedulerImpl.cpp | 6 +- src/app/reporting/ReportSchedulerImpl.h | 6 +- .../SynchronizedReportSchedulerImpl.cpp | 249 +++++++++++++ .../SynchronizedReportSchedulerImpl.h | 71 ++++ src/app/tests/TestReportScheduler.cpp | 332 +++++++++++++++++- 7 files changed, 680 insertions(+), 17 deletions(-) create mode 100644 src/app/reporting/SynchronizedReportSchedulerImpl.cpp create mode 100644 src/app/reporting/SynchronizedReportSchedulerImpl.h diff --git a/src/app/BUILD.gn b/src/app/BUILD.gn index 7f2c4d82280392..bd31b04841e9f3 100644 --- a/src/app/BUILD.gn +++ b/src/app/BUILD.gn @@ -196,6 +196,8 @@ static_library("app") { "reporting/ReportScheduler.h", "reporting/ReportSchedulerImpl.cpp", "reporting/ReportSchedulerImpl.h", + "reporting/SynchronizedReportSchedulerImpl.cpp", + "reporting/SynchronizedReportSchedulerImpl.h", "reporting/reporting.h", ] diff --git a/src/app/reporting/ReportScheduler.h b/src/app/reporting/ReportScheduler.h index 80d391c171c7e5..371e6bc7d8aa5e 100644 --- a/src/app/reporting/ReportScheduler.h +++ b/src/app/reporting/ReportScheduler.h @@ -56,7 +56,7 @@ class ReportScheduler : public ReadHandler::Observer class ReadHandlerNode : public IntrusiveListNodeBase<> { public: - using TimerCompleteCallback = void (*)(); + using TimerCompleteCallback = std::function; ReadHandlerNode(ReadHandler * aReadHandler, TimerDelegate * aTimerDelegate, TimerCompleteCallback aCallback) : mTimerDelegate(aTimerDelegate), mCallback(aCallback) @@ -78,22 +78,31 @@ class ReportScheduler : public ReadHandler::Observer // the scheduler in the ReadHandler Timestamp now = mTimerDelegate->GetCurrentMonotonicTimestamp(); return (mReadHandler->IsGeneratingReports() && - ((now >= mMinTimestamp && mReadHandler->IsDirty()) || now >= mMaxTimestamp)); + (now >= mMinTimestamp && (mReadHandler->IsDirty() || now >= mMaxTimestamp || now >= mSyncTimestamp))); } void SetIntervalTimeStamps(ReadHandler * aReadHandler) { uint16_t minInterval, maxInterval; aReadHandler->GetReportingIntervals(minInterval, maxInterval); - Timestamp now = mTimerDelegate->GetCurrentMonotonicTimestamp(); - mMinTimestamp = now + System::Clock::Seconds16(minInterval); - mMaxTimestamp = now + System::Clock::Seconds16(maxInterval); + Timestamp now = mTimerDelegate->GetCurrentMonotonicTimestamp(); + mMinTimestamp = now + System::Clock::Seconds16(minInterval); + mMaxTimestamp = now + System::Clock::Seconds16(maxInterval); + mSyncTimestamp = mMaxTimestamp; } void RunCallback() { mCallback(); } - Timestamp GetMinTimestamp() const { return mMinTimestamp; } - Timestamp GetMaxTimestamp() const { return mMaxTimestamp; } + void SetSyncTimestamp(System::Clock::Timestamp aSyncTimestamp) + { + // Prevents the sync timestamp to be set to a value lower than the min timestamp + VerifyOrReturn(aSyncTimestamp >= mMinTimestamp); + mSyncTimestamp = aSyncTimestamp; + } + + System::Clock::Timestamp GetMinTimestamp() const { return mMinTimestamp; } + System::Clock::Timestamp GetMaxTimestamp() const { return mMaxTimestamp; } + System::Clock::Timestamp GetSyncTimestamp() const { return mSyncTimestamp; } private: TimerDelegate * mTimerDelegate; @@ -101,6 +110,7 @@ class ReportScheduler : public ReadHandler::Observer ReadHandler * mReadHandler; Timestamp mMinTimestamp; Timestamp mMaxTimestamp; + Timestamp mSyncTimestamp; }; ReportScheduler(TimerDelegate * aTimerDelegate) : mTimerDelegate(aTimerDelegate) {} @@ -113,7 +123,10 @@ class ReportScheduler : public ReadHandler::Observer virtual bool IsReportScheduled(ReadHandler * aReadHandler) = 0; /// @brief Check whether a ReadHandler is reportable right now, taking into account its minimum and maximum intervals. /// @param aReadHandler read handler to check - bool IsReportableNow(ReadHandler * aReadHandler) { return FindReadHandlerNode(aReadHandler)->IsReportableNow(); }; + bool IsReportableNow(ReadHandler * aReadHandler) + { + return FindReadHandlerNode(aReadHandler)->IsReportableNow(); + }; // TODO: Change the IsReportableNow to IsReportable() for readHandlers /// @brief Check if a ReadHandler is reportable without considering the timing bool IsReadHandlerReportable(ReadHandler * aReadHandler) const { @@ -123,6 +136,8 @@ class ReportScheduler : public ReadHandler::Observer /// @brief Get the number of ReadHandlers registered in the scheduler's node pool size_t GetNumReadHandlers() const { return mNodesPool.Allocated(); } + virtual void ReportTimerCallback() = 0; + protected: friend class chip::app::reporting::TestReportScheduler; diff --git a/src/app/reporting/ReportSchedulerImpl.cpp b/src/app/reporting/ReportSchedulerImpl.cpp index 4b45ab9d1b6deb..4bb6ffa8fa13c2 100644 --- a/src/app/reporting/ReportSchedulerImpl.cpp +++ b/src/app/reporting/ReportSchedulerImpl.cpp @@ -30,9 +30,11 @@ using ReadHandlerNode = ReportScheduler::ReadHandlerNode; /// @brief Callback called when the report timer expires to schedule an engine run regardless of the state of the ReadHandlers, as /// the engine already verifies that read handlers are reportable before sending a report -static void ReportTimerCallback() +void ReportSchedulerImpl::ReportTimerCallback() { +#ifndef CONFIG_BUILD_FOR_HOST_UNIT_TEST InteractionModelEngine::GetInstance()->GetReportingEngine().ScheduleRun(); +#endif } ReportSchedulerImpl::ReportSchedulerImpl(TimerDelegate * aTimerDelegate) : ReportScheduler(aTimerDelegate) @@ -91,7 +93,7 @@ CHIP_ERROR ReportSchedulerImpl::RegisterReadHandler(ReadHandler * aReadHandler) VerifyOrDie(nullptr == newNode); // The NodePool is the same size as the ReadHandler pool from the IM Engine, so we don't need a check for size here since if a // ReadHandler was created, space should be available. - newNode = mNodesPool.CreateObject(aReadHandler, mTimerDelegate, ReportTimerCallback); + newNode = mNodesPool.CreateObject(aReadHandler, mTimerDelegate, [this]() { this->ReportTimerCallback(); }); mReadHandlerList.PushBack(newNode); ChipLogProgress(DataManagement, diff --git a/src/app/reporting/ReportSchedulerImpl.h b/src/app/reporting/ReportSchedulerImpl.h index 849f9b797b5f93..f87d9a47c65806 100644 --- a/src/app/reporting/ReportSchedulerImpl.h +++ b/src/app/reporting/ReportSchedulerImpl.h @@ -36,6 +36,10 @@ class ReportSchedulerImpl : public ReportScheduler void OnSubscriptionAction(ReadHandler * aReadHandler) override; void OnReadHandlerDestroyed(ReadHandler * aReadHandler) override; + bool IsReportScheduled(ReadHandler * aReadHandler) override; + + void ReportTimerCallback() override; + protected: virtual CHIP_ERROR RegisterReadHandler(ReadHandler * aReadHandler); virtual CHIP_ERROR ScheduleReport(System::Clock::Timeout timeout, ReadHandlerNode * node); @@ -46,8 +50,6 @@ class ReportSchedulerImpl : public ReportScheduler private: friend class chip::app::reporting::TestReportScheduler; - bool IsReportScheduled(ReadHandler * aReadHandler) override; - /// @brief Start a timer for a given ReadHandlerNode, ensures that if a timer is already running for this node, it is cancelled /// @param node Node of the ReadHandler list to start a timer for /// @param aTimeout Delay before the timer expires diff --git a/src/app/reporting/SynchronizedReportSchedulerImpl.cpp b/src/app/reporting/SynchronizedReportSchedulerImpl.cpp new file mode 100644 index 00000000000000..ab33ede57dd909 --- /dev/null +++ b/src/app/reporting/SynchronizedReportSchedulerImpl.cpp @@ -0,0 +1,249 @@ +/* + * + * Copyright (c) 2023 Project CHIP Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include +#include +#include + +namespace chip { +namespace app { +namespace reporting { + +using Seconds16 = System::Clock::Seconds16; +using Milliseconds32 = System::Clock::Milliseconds32; +using Timeout = System::Clock::Timeout; +using Timestamp = System::Clock::Timestamp; +using ReadHandlerNode = ReportScheduler::ReadHandlerNode; + +/// @brief When a ReadHandler becomes reportable, recaculate the earliest possible interval before scheduling an engine run and +/// reschedule the report +void SynchronizedReportSchedulerImpl::OnBecameReportable(ReadHandler * aReadHandler) +{ + ReadHandlerNode * node = FindReadHandlerNode(aReadHandler); + VerifyOrReturn(nullptr != node); + Milliseconds32 newTimeout; + ReturnOnFailure(CalculateNextReportTimeout(newTimeout, node)); + ScheduleReport(newTimeout, node); +} + +void SynchronizedReportSchedulerImpl::OnSubscriptionAction(ReadHandler * aReadHandler) +{ + ReadHandlerNode * node = FindReadHandlerNode(aReadHandler); + VerifyOrReturn(nullptr != node); + + // Update the node's timestamps + node->SetIntervalTimeStamps(aReadHandler); + + Milliseconds32 newTimeout; + ReturnOnFailure(CalculateNextReportTimeout(newTimeout, node)); + ScheduleReport(newTimeout, node); +} + +CHIP_ERROR SynchronizedReportSchedulerImpl::RegisterReadHandler(ReadHandler * aReadHandler) +{ + ReadHandlerNode * newNode = FindReadHandlerNode(aReadHandler); + // If the handler is already registered, no need to register it again + VerifyOrReturnValue(nullptr == newNode, CHIP_NO_ERROR); + newNode = mNodesPool.CreateObject(aReadHandler, mTimerDelegate, [this]() { this->ReportTimerCallback(); }); + mReadHandlerList.PushBack(newNode); + + ChipLogProgress(DataManagement, + "Registered ReadHandler that will schedule a report between system Timestamp: %" PRIu64 + " and system Timestamp %" PRIu64 ".", + newNode->GetMinTimestamp().count(), newNode->GetMaxTimestamp().count()); + + Milliseconds32 newTimeout; + CHIP_ERROR err = CalculateNextReportTimeout(newTimeout, newNode); + if (CHIP_NO_ERROR != err) + { + mReadHandlerList.Remove(newNode); + mNodesPool.ReleaseObject(newNode); + return err; + } + + ReturnErrorOnFailure(ScheduleReport(newTimeout, newNode)); + return CHIP_NO_ERROR; +} + +CHIP_ERROR SynchronizedReportSchedulerImpl::ScheduleReport(Timeout timeout, ReadHandlerNode * node) +{ + // Cancel Report if it is currently scheduled + CancelSchedulerTimer(nullptr); + + ReturnErrorOnFailure(StartSchedulerTimer(nullptr, timeout)); + mNextReportTimestamp = mTimerDelegate->GetCurrentMonotonicTimestamp() + timeout; + + return CHIP_NO_ERROR; +} + +void SynchronizedReportSchedulerImpl::CancelReport(ReadHandler * aReadHandler) +{ + // We don't need to take action on the handler, since the timer is common here + CancelSchedulerTimer(nullptr); +} + +void SynchronizedReportSchedulerImpl::UnregisterReadHandler(ReadHandler * aReadHandler) +{ + // Verify list is populated and handler is not null + VerifyOrReturn((!mReadHandlerList.Empty() || (nullptr == aReadHandler))); + + ReadHandlerNode * removeNode = FindReadHandlerNode(aReadHandler); + // Nothing to remove if the handler is not found in the list + VerifyOrReturn(nullptr != removeNode); + + mReadHandlerList.Remove(removeNode); + mNodesPool.ReleaseObject(removeNode); + + if (mReadHandlerList.Empty()) + { + // Only cancel the timer if there are no more handlers registered + CancelReport(aReadHandler); + } +} + +/// @brief Checks if the timer is active for the given ReadHandler. Since all read handlers are scheduled on the same timer, we +/// check if the node is in the list and if the timer is active for the ReportScheduler +bool SynchronizedReportSchedulerImpl::IsReportScheduled(ReadHandler * aReadHandler) +{ + ReadHandlerNode * node = FindReadHandlerNode(aReadHandler); + VerifyOrReturnValue(nullptr != node, false); + return CheckSchedulerTimerActive(nullptr); +} + +CHIP_ERROR SynchronizedReportSchedulerImpl::StartSchedulerTimer(ReadHandlerNode * node, System::Clock::Timeout aTimeout) +{ + return mTimerDelegate->StartTimer(this, aTimeout); +} +void SynchronizedReportSchedulerImpl::CancelSchedulerTimer(ReadHandlerNode * node) +{ + mTimerDelegate->CancelTimer(this); +} +bool SynchronizedReportSchedulerImpl::CheckSchedulerTimerActive(ReadHandlerNode * node) +{ + return mTimerDelegate->IsTimerActive(this); +} + +/// @brief Find the smallest maximum interval possible and sets it as the common maximum +/// @return NO_ERROR if the smallest maximum interval was found, error otherwise, INVALID LIST LENGTH if the list is empty +CHIP_ERROR SynchronizedReportSchedulerImpl::FindNextMaxInterval() +{ + System::Clock::Timestamp now = mTimerDelegate->GetCurrentMonotonicTimestamp(); + System::Clock::Timestamp earliest = now + Milliseconds64(kSubscriptionMaxIntervalPublisherLimit); + VerifyOrReturnError(!mReadHandlerList.Empty(), CHIP_ERROR_INVALID_LIST_LENGTH); + for (auto & iter : mReadHandlerList) + { + if (iter.GetMaxTimestamp() < earliest && iter.GetMaxTimestamp() > now) + { + earliest = iter.GetMaxTimestamp(); + } + } + + mNextMaxTimestamp = earliest; + + return CHIP_NO_ERROR; +} + +/// @brief Find the highest minimum timestamp possible that still respects the lowest max timestamp and sets it as the common +/// minimum. If the max timestamp has not been updated and is in the past, or if no min timestamp is lower than the current max +/// timestamp, this will set now as the common minimum timestamp, thus allowing the report to be sent immediately. +/// @return NO_ERROR if the highest minimum timestamp was found, error otherwise, INVALID LIST LENGTH if the list is empty +CHIP_ERROR SynchronizedReportSchedulerImpl::FindNextMinInterval() +{ + System::Clock::Timestamp now = mTimerDelegate->GetCurrentMonotonicTimestamp(); + System::Clock::Timestamp latest = now; + + VerifyOrReturnError(!mReadHandlerList.Empty(), CHIP_ERROR_INVALID_LIST_LENGTH); + for (auto & iter : mReadHandlerList) + { + if (iter.GetMinTimestamp() > latest) + { + // We do not want the new min to be set above the max for any handler + if (iter.GetMinTimestamp() <= mNextMaxTimestamp) + { + latest = iter.GetMinTimestamp(); + } + } + } + + mNextMinTimestamp = latest; + + return CHIP_NO_ERROR; +} + +CHIP_ERROR SynchronizedReportSchedulerImpl::CalculateNextReportTimeout(System::Clock::Timeout & timeout, ReadHandlerNode * aNode) +{ + VerifyOrReturnError(mReadHandlerList.Contains(aNode), CHIP_ERROR_INVALID_ARGUMENT); + ReturnErrorOnFailure(FindNextMaxInterval()); + ReturnErrorOnFailure(FindNextMinInterval()); + + Timestamp now = mTimerDelegate->GetCurrentMonotonicTimestamp(); + // Find out if any handler is reportable now + if (IsReadHandlerReportable(aNode->GetReadHandler())) + { + if (now > mNextMinTimestamp) + { + timeout = Milliseconds32(0); + } + else + { + timeout = Milliseconds32(mNextMinTimestamp.count() - now.count()); + } + } + else + { + timeout = Milliseconds32(mNextMaxTimestamp.count() - now.count()); + } + + // Updates the synching time of each handler + for (auto & iter : mReadHandlerList) + { + // Prevent modifying the sync if the handler is currently reportable, sync's purpose is to allow handler to become + // reportable earlier than their max interval + if (!iter.IsReportableNow()) + { + iter.SetSyncTimestamp(Milliseconds64(now + timeout)); + } + } + + return CHIP_NO_ERROR; +} + +/// @brief Callback called when the report timer expires to schedule an engine run regardless of the state of the ReadHandlers, as +/// the engine already verifies that read handlers are reportable before sending a report +void SynchronizedReportSchedulerImpl::ReportTimerCallback() +{ +#ifndef CONFIG_BUILD_FOR_HOST_UNIT_TEST + InteractionModelEngine::GetInstance()->GetReportingEngine().ScheduleRun(); +#else + Timestamp now = mTimerDelegate->GetCurrentMonotonicTimestamp(); + ChipLogProgress(DataManagement, "Engine run at time: %" PRIu64 " for Handlers:", now.count()); + + for (auto & iter : mReadHandlerList) + { + if (iter.IsReportableNow()) + { + ChipLogProgress(DataManagement, "Handler: %p" PRIu64 " with min: %" PRIu64 " and max: %" PRIu64 " and sync: %" PRIu64, + (&iter), iter.GetMinTimestamp().count(), iter.GetMaxTimestamp().count(), + iter.GetSyncTimestamp().count()); + } + } +#endif +} + +} // namespace reporting +} // namespace app +} // namespace chip diff --git a/src/app/reporting/SynchronizedReportSchedulerImpl.h b/src/app/reporting/SynchronizedReportSchedulerImpl.h new file mode 100644 index 00000000000000..3d62133b7a2527 --- /dev/null +++ b/src/app/reporting/SynchronizedReportSchedulerImpl.h @@ -0,0 +1,71 @@ +/* + * + * Copyright (c) 2023 Project CHIP Authors + * All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#pragma once + +#include + +namespace chip { +namespace app { +namespace reporting { + +using Timeout = System::Clock::Timeout; +using Timestamp = System::Clock::Timestamp; +using Milliseconds64 = System::Clock::Milliseconds64; +using ReadHandlerNode = ReportScheduler::ReadHandlerNode; +using TimerDelegate = ReportScheduler::TimerDelegate; + +class SynchronizedReportSchedulerImpl : public ReportSchedulerImpl +{ +public: + SynchronizedReportSchedulerImpl(TimerDelegate * aTimerDelegate) : ReportSchedulerImpl(aTimerDelegate) {} + ~SynchronizedReportSchedulerImpl() {} + + // ReadHandlerObserver + void OnBecameReportable(ReadHandler * aReadHandler) override; + void OnSubscriptionAction(ReadHandler * aReadHandler) override; + + bool IsReportScheduled(ReadHandler * aReadHandler) override; + + void ReportTimerCallback() override; + +protected: + CHIP_ERROR RegisterReadHandler(ReadHandler * aReadHandler) override; + virtual CHIP_ERROR ScheduleReport(System::Clock::Timeout timeout, ReadHandlerNode * node) override; + void CancelReport(ReadHandler * aReadHandler) override; + void UnregisterReadHandler(ReadHandler * aReadHandler) override; + +private: + friend class chip::app::reporting::TestReportScheduler; + + CHIP_ERROR StartSchedulerTimer(ReadHandlerNode * node, Timeout aTimeout) override; + void CancelSchedulerTimer(ReadHandlerNode * node) override; + bool CheckSchedulerTimerActive(ReadHandlerNode * node) override; + + CHIP_ERROR FindNextMinInterval(); + CHIP_ERROR FindNextMaxInterval(); + CHIP_ERROR CalculateNextReportTimeout(Timeout & timeout, ReadHandlerNode * aReadHandlerNode); + + Timestamp mNextMaxTimestamp = Milliseconds64(0); + Timestamp mNextMinTimestamp = Milliseconds64(0); + Timestamp mNextReportTimestamp = Milliseconds64(0); +}; + +} // namespace reporting +} // namespace app +} // namespace chip diff --git a/src/app/tests/TestReportScheduler.cpp b/src/app/tests/TestReportScheduler.cpp index a24422010c47c9..7d9a7139be3595 100644 --- a/src/app/tests/TestReportScheduler.cpp +++ b/src/app/tests/TestReportScheduler.cpp @@ -18,6 +18,7 @@ #include #include +#include #include #include #include @@ -71,11 +72,10 @@ namespace chip { namespace app { namespace reporting { -using InteractionModelEngine = InteractionModelEngine; -using ReportScheduler = reporting::ReportScheduler; -using ReportSchedulerImpl = reporting::ReportSchedulerImpl; -using ReadHandlerNode = reporting::ReportScheduler::ReadHandlerNode; -using Milliseconds64 = System::Clock::Milliseconds64; +using ReportScheduler = reporting::ReportScheduler; +using ReportSchedulerImpl = reporting::ReportSchedulerImpl; +using ReadHandlerNode = reporting::ReportScheduler::ReadHandlerNode; +using Milliseconds64 = System::Clock::Milliseconds64; static const size_t kNumMaxReadHandlers = 16; @@ -174,9 +174,69 @@ class TestTimerDelegate : public ReportScheduler::TimerDelegate } }; +/// @brief TestTimerSynchronizedDelegate is a mock of the TimerDelegate interface that allows to control the time without dependency +/// on the system layer. This also simulates the system timer by verifying if the timeout expired when incrementing the mock +/// timestamp. only one timer can be active at a time, which is the one has the earliest timeout. +/// It is used to test the SynchronizedReportSchedulerImpl. +class TestTimerSynchronizedDelegate : public ReportScheduler::TimerDelegate +{ +public: + static void TimerCallbackInterface(System::Layer * aLayer, void * aAppState) + { + SynchronizedReportSchedulerImpl * scheduler = static_cast(aAppState); + scheduler->ReportTimerCallback(); + } + virtual CHIP_ERROR StartTimer(void * context, System::Clock::Timeout aTimeout) override + { + SynchronizedReportSchedulerImpl * scheduler = static_cast(context); + if (nullptr == scheduler) + { + return CHIP_ERROR_INCORRECT_STATE; + } + + mSyncScheduler = scheduler; + mTimerTimeout = mMockSystemTimestamp + aTimeout; + return CHIP_NO_ERROR; + } + virtual void CancelTimer(void * context) override + { + VerifyOrReturn(nullptr != mSyncScheduler); + mSyncScheduler = nullptr; + mTimerTimeout = System::Clock::Milliseconds64(0x7FFFFFFFFFFFFFFF); + } + virtual bool IsTimerActive(void * context) override + { + return (nullptr != mSyncScheduler) && (mTimerTimeout > mMockSystemTimestamp); + } + + virtual System::Clock::Timestamp GetCurrentMonotonicTimestamp() override { return mMockSystemTimestamp; } + + void SetMockSystemTimestamp(System::Clock::Timestamp aMockTimestamp) { mMockSystemTimestamp = aMockTimestamp; } + + // Increment the mock timestamp one milisecond at a time for a total of aTime miliseconds. Checks if + void IncrementMockTimestamp(System::Clock::Milliseconds64 aTime) + { + for (System::Clock::Milliseconds64 i = System::Clock::Milliseconds64(0); i < aTime; i++) + { + mMockSystemTimestamp++; + if (mMockSystemTimestamp == mTimerTimeout) + { + TimerCallbackInterface(nullptr, mSyncScheduler); + } + } + } + + SynchronizedReportSchedulerImpl * mSyncScheduler = nullptr; + System::Clock::Timeout mTimerTimeout = System::Clock::Milliseconds64(0x7FFFFFFFFFFFFFFF); + System::Clock::Timestamp mMockSystemTimestamp = System::Clock::Milliseconds64(0); +}; + TestTimerDelegate sTestTimerDelegate; ReportSchedulerImpl sScheduler(&sTestTimerDelegate); +TestTimerSynchronizedDelegate sTestTimerSynchronizedDelegate; +SynchronizedReportSchedulerImpl syncScheduler(&sTestTimerSynchronizedDelegate); + class TestReportScheduler { public: @@ -392,6 +452,267 @@ class TestReportScheduler exchangeCtx->Close(); NL_TEST_ASSERT(aSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 0); } + + static void TestSynchronizedScheduler(nlTestSuite * aSuite, void * aContext) + { + TestContext & ctx = *static_cast(aContext); + NullReadHandlerCallback nullCallback; + // exchange context + Messaging::ExchangeContext * exchangeCtx = ctx.NewExchangeToAlice(nullptr, false); + + // First test: ReadHandler 2 merge on ReadHandler 1 max interval + // Read handler pool + ObjectPool readHandlerPool; + + // Initilaize the mock system time + sTestTimerSynchronizedDelegate.SetMockSystemTimestamp(System::Clock::Milliseconds64(0)); + + ReadHandler * readHandler1 = + readHandlerPool.CreateObject(nullCallback, exchangeCtx, ReadHandler::InteractionType::Subscribe); + NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == readHandler1->SetMaxReportingInterval(2)); + NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == readHandler1->SetMinReportingIntervalForTests(0)); + readHandler1->MoveToState(ReadHandler::HandlerState::GeneratingReports); + readHandler1->SetObserver(&syncScheduler); + readHandler1->mObserver->OnReadHandlerCreated(readHandler1); + + ReadHandler * readHandler2 = + readHandlerPool.CreateObject(nullCallback, exchangeCtx, ReadHandler::InteractionType::Subscribe); + NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == readHandler2->SetMaxReportingInterval(3)); + NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == readHandler2->SetMinReportingIntervalForTests(1)); + readHandler2->MoveToState(ReadHandler::HandlerState::GeneratingReports); + readHandler2->SetObserver(&syncScheduler); + readHandler2->mObserver->OnReadHandlerCreated(readHandler2); + + // Confirm all handler are currently registered in the scheduler + NL_TEST_ASSERT(aSuite, syncScheduler.GetNumReadHandlers() == 2); + + ReadHandlerNode * node1 = syncScheduler.FindReadHandlerNode(readHandler1); + ReadHandlerNode * node2 = syncScheduler.FindReadHandlerNode(readHandler2); + + // Confirm that a report emission is scheduled and that it will happen on readHandler1 max timestamp + NL_TEST_ASSERT(aSuite, syncScheduler.IsReportScheduled(readHandler1)); + NL_TEST_ASSERT(aSuite, syncScheduler.IsReportScheduled(readHandler2)); + + // Validates that the lowest max is selected as the common max timestamp + NL_TEST_ASSERT(aSuite, syncScheduler.mNextMaxTimestamp == node1->GetMaxTimestamp()); + // Validates that the highest min is selected as the common min interval + NL_TEST_ASSERT(aSuite, syncScheduler.mNextMinTimestamp == node2->GetMinTimestamp()); + // Validates that the next report emission is scheduled on the common max timestamp + NL_TEST_ASSERT(aSuite, syncScheduler.mNextReportTimestamp == syncScheduler.mNextMaxTimestamp); + + // Simulate waiting for the max interval to expire (2s) + // Increment the mock system time to the max to the time waited + sTestTimerSynchronizedDelegate.IncrementMockTimestamp(System::Clock::Milliseconds64(2000)); + + // Confirm that both handlers are now reportable since the timer has expired + NL_TEST_ASSERT(aSuite, syncScheduler.IsReportableNow(readHandler1)); + // Confirm timeout has expired and no report is scheduled, an engine run would typically happen here + NL_TEST_ASSERT(aSuite, !sScheduler.IsReportScheduled(readHandler1)); + NL_TEST_ASSERT(aSuite, !sScheduler.IsReportScheduled(readHandler1)); + + // Simulate that the OnBecameReportable callback was called on readHandler1 + readHandler1->mObserver->OnBecameReportable(readHandler1); + // Confirm that the read handler is now reportable due to the sync mechanism that forced a dirty flag + NL_TEST_ASSERT(aSuite, syncScheduler.IsReportableNow(readHandler2)); + + // Simulate a report emission for readHandler1 + readHandler1->mObserver->OnSubscriptionAction(readHandler1); + // Simulate a report emission for readHandler2 + readHandler2->mObserver->OnSubscriptionAction(readHandler2); + + // Validate that the max timestamp for both readhandlers got updated and that the next report emission is scheduled on + // the new max timestamp for readhandler1 + NL_TEST_ASSERT(aSuite, node1->GetMaxTimestamp() > sTestTimerSynchronizedDelegate.GetCurrentMonotonicTimestamp()); + NL_TEST_ASSERT(aSuite, syncScheduler.mNextReportTimestamp == node1->GetMaxTimestamp()); + + // Confirm behavior when a read handler becomes dirty + readHandler1->ForceDirtyState(); + // since the min interval on readHandler1 is 0, it should be reportable now + NL_TEST_ASSERT(aSuite, syncScheduler.IsReportableNow(readHandler1)); + readHandler1->mObserver->OnBecameReportable(readHandler1); + // Confirm that the next report emission is scheduled on the min timestamp of readHandler2 as it is the highest + NL_TEST_ASSERT(aSuite, syncScheduler.mNextReportTimestamp == node2->GetMinTimestamp()); + // Simulate wait enough for min timestamp of readHandler2 to be reached (1s) + // Increment the mock system time to the max to the time waited + sTestTimerSynchronizedDelegate.IncrementMockTimestamp(System::Clock::Milliseconds64(1000)); + + // Confirm that readHandler2 is now reportable + NL_TEST_ASSERT(aSuite, syncScheduler.IsReportableNow(readHandler2)); + readHandler2->mObserver->OnBecameReportable(readHandler2); + + readHandler1->ClearForceDirtyFlag(); + // Simulate a report emission for readHandler1 + readHandler1->mObserver->OnSubscriptionAction(readHandler1); + NL_TEST_ASSERT(aSuite, !syncScheduler.IsReportableNow(readHandler1)); + + // ReadHandler 2 should still be reportable from the sync mechanism + NL_TEST_ASSERT(aSuite, syncScheduler.IsReportableNow(readHandler2)); + readHandler1->mObserver->OnSubscriptionAction(readHandler2); + NL_TEST_ASSERT(aSuite, !syncScheduler.IsReportableNow(readHandler2)); + + // Validate next report scheduled on the max timestamp of readHandler1 + NL_TEST_ASSERT(aSuite, node1->GetMaxTimestamp() > sTestTimerSynchronizedDelegate.GetCurrentMonotonicTimestamp()); + NL_TEST_ASSERT(aSuite, syncScheduler.mNextReportTimestamp == node1->GetMaxTimestamp()); + + // Simulate a new ReadHandler being added with a min forcing a conflict + + // Wait for 1 second, nothing should happen here + ctx.GetIOContext().DriveIOUntil(System::Clock::Milliseconds32(1000), + [&]() -> bool { return syncScheduler.IsReportableNow(readHandler1); }); + // Increment the mock system time to the max to the time waited + sTestTimerSynchronizedDelegate.IncrementMockTimestamp(System::Clock::Milliseconds64(1000)); + + ReadHandler * readHandler3 = + readHandlerPool.CreateObject(nullCallback, exchangeCtx, ReadHandler::InteractionType::Subscribe); + NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == readHandler3->SetMaxReportingInterval(3)); + NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == readHandler3->SetMinReportingIntervalForTests(2)); + readHandler3->MoveToState(ReadHandler::HandlerState::GeneratingReports); + readHandler3->SetObserver(&syncScheduler); + readHandler3->mObserver->OnReadHandlerCreated(readHandler3); + + // Confirm all handler are currently registered in the scheduler + NL_TEST_ASSERT(aSuite, syncScheduler.GetNumReadHandlers() == 3); + ReadHandlerNode * node3 = syncScheduler.FindReadHandlerNode(readHandler3); + + // Since the min interval on readHandler3 is 2, it should be above the current max timestamp, therefore the next report + // should still happen on the max timestamp of readHandler1 and the sync should be done on future reports + NL_TEST_ASSERT(aSuite, syncScheduler.mNextReportTimestamp == node1->GetMaxTimestamp()); + // The min timestamp should also not have changed since the min of readhandler3 is higher than the current max + NL_TEST_ASSERT(aSuite, syncScheduler.mNextMinTimestamp == node2->GetMinTimestamp()); + + ctx.GetIOContext().DriveIOUntil(System::Clock::Milliseconds32(1100), + [&]() -> bool { return syncScheduler.IsReportableNow(readHandler1); }); + // Increment the mock system time to the max to the time waited + sTestTimerSynchronizedDelegate.IncrementMockTimestamp(System::Clock::Milliseconds64(1000)); + + // Confirm that readHandler1 and readHandler 2 are now reportable, whilst readHandler3 is not + NL_TEST_ASSERT(aSuite, syncScheduler.IsReportableNow(readHandler1)); + NL_TEST_ASSERT(aSuite, syncScheduler.IsReportableNow(readHandler2)); + NL_TEST_ASSERT(aSuite, !syncScheduler.IsReportableNow(readHandler3)); + readHandler1->mObserver->OnBecameReportable(readHandler1); + readHandler2->mObserver->OnBecameReportable(readHandler2); + + // Simulate a report emission for readHandler1 and readHandler2 + readHandler1->mObserver->OnSubscriptionAction(readHandler1); + readHandler1->mObserver->OnSubscriptionAction(readHandler2); + NL_TEST_ASSERT(aSuite, !syncScheduler.IsReportableNow(readHandler1)); + NL_TEST_ASSERT(aSuite, !syncScheduler.IsReportableNow(readHandler2)); + + // Confirm that next report is scheduled on the max timestamp of readHandler3 and other 2 readHandlers are synced + NL_TEST_ASSERT(aSuite, syncScheduler.mNextReportTimestamp == node3->GetMaxTimestamp()); + NL_TEST_ASSERT(aSuite, syncScheduler.mNextMinTimestamp == node3->GetMinTimestamp()); + NL_TEST_ASSERT(aSuite, node1->GetSyncTimestamp() == node3->GetMaxTimestamp()); + NL_TEST_ASSERT(aSuite, node2->GetSyncTimestamp() == node3->GetMaxTimestamp()); + + // Simulate a report emission for all readHandlers on the max timestamp of readHandler3 on confirm everything is synced + ctx.GetIOContext().DriveIOUntil(System::Clock::Milliseconds32(2000), + [&]() -> bool { return syncScheduler.IsReportableNow(readHandler3); }); + // Increment the mock system time to the max to the time waited + sTestTimerSynchronizedDelegate.IncrementMockTimestamp(System::Clock::Milliseconds64(2000)); + + NL_TEST_ASSERT(aSuite, syncScheduler.IsReportableNow(readHandler1)); + NL_TEST_ASSERT(aSuite, syncScheduler.IsReportableNow(readHandler2)); + NL_TEST_ASSERT(aSuite, syncScheduler.IsReportableNow(readHandler3)); + readHandler1->mObserver->OnBecameReportable(readHandler1); + readHandler2->mObserver->OnBecameReportable(readHandler2); + readHandler3->mObserver->OnBecameReportable(readHandler3); + // Engine run should happen here and send all reports + readHandler1->mObserver->OnSubscriptionAction(readHandler1); + readHandler2->mObserver->OnSubscriptionAction(readHandler2); + readHandler3->mObserver->OnSubscriptionAction(readHandler3); + NL_TEST_ASSERT(aSuite, !syncScheduler.IsReportableNow(readHandler1)); + NL_TEST_ASSERT(aSuite, !syncScheduler.IsReportableNow(readHandler2)); + NL_TEST_ASSERT(aSuite, !syncScheduler.IsReportableNow(readHandler3)); + NL_TEST_ASSERT(aSuite, syncScheduler.mNextReportTimestamp == node1->GetMaxTimestamp()); + NL_TEST_ASSERT(aSuite, syncScheduler.mNextMinTimestamp == node3->GetMinTimestamp()); + NL_TEST_ASSERT(aSuite, node1->GetSyncTimestamp() == node1->GetMaxTimestamp()); + NL_TEST_ASSERT(aSuite, node2->GetSyncTimestamp() == node1->GetMaxTimestamp()); + + // Now simulate a new readHandler being added with a max forcing a conflict + ReadHandler * readHandler4 = + readHandlerPool.CreateObject(nullCallback, exchangeCtx, ReadHandler::InteractionType::Subscribe); + NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == readHandler4->SetMaxReportingInterval(1)); + NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == readHandler4->SetMinReportingIntervalForTests(0)); + readHandler4->MoveToState(ReadHandler::HandlerState::GeneratingReports); + readHandler4->SetObserver(&syncScheduler); + readHandler4->mObserver->OnReadHandlerCreated(readHandler4); + + // Confirm all handler are currently registered in the scheduler + NL_TEST_ASSERT(aSuite, syncScheduler.GetNumReadHandlers() == 4); + ReadHandlerNode * node4 = syncScheduler.FindReadHandlerNode(readHandler4); + + // Confirm next report is scheduled on the max timestamp of readHandler4 and other handlers be synced + NL_TEST_ASSERT(aSuite, syncScheduler.mNextReportTimestamp == node4->GetMaxTimestamp()); + NL_TEST_ASSERT(aSuite, node1->GetSyncTimestamp() == node4->GetMaxTimestamp()); + NL_TEST_ASSERT(aSuite, node2->GetSyncTimestamp() == node4->GetMaxTimestamp()); + NL_TEST_ASSERT(aSuite, node3->GetSyncTimestamp() == node1->GetMaxTimestamp()); + + ctx.GetIOContext().DriveIOUntil(System::Clock::Milliseconds32(1100), + [&]() -> bool { return syncScheduler.IsReportableNow(readHandler4); }); + // Increment the mock system time to the max to the time waited + sTestTimerSynchronizedDelegate.IncrementMockTimestamp(System::Clock::Milliseconds64(1100)); + + // Confirm readHandler4, 1 and 1 are reportable + NL_TEST_ASSERT(aSuite, syncScheduler.IsReportableNow(readHandler1)); + NL_TEST_ASSERT(aSuite, syncScheduler.IsReportableNow(readHandler2)); + NL_TEST_ASSERT(aSuite, !syncScheduler.IsReportableNow(readHandler3)); + NL_TEST_ASSERT(aSuite, syncScheduler.IsReportableNow(readHandler4)); + readHandler4->mObserver->OnBecameReportable(readHandler1); + readHandler4->mObserver->OnBecameReportable(readHandler2); + readHandler4->mObserver->OnBecameReportable(readHandler4); + readHandler4->mObserver->OnSubscriptionAction(readHandler1); + readHandler4->mObserver->OnSubscriptionAction(readHandler2); + readHandler4->mObserver->OnSubscriptionAction(readHandler4); + NL_TEST_ASSERT(aSuite, !syncScheduler.IsReportableNow(readHandler1)); + NL_TEST_ASSERT(aSuite, !syncScheduler.IsReportableNow(readHandler2)); + NL_TEST_ASSERT(aSuite, !syncScheduler.IsReportableNow(readHandler4)); + + ctx.GetIOContext().DriveIOUntil(System::Clock::Milliseconds32(1000), + [&]() -> bool { return syncScheduler.IsReportableNow(readHandler3); }); + // Increment the mock system time to the max to the time waited + sTestTimerSynchronizedDelegate.IncrementMockTimestamp(System::Clock::Milliseconds64(1000)); + + // Confirm readHandler3 is reportable and other handlers are synced + NL_TEST_ASSERT(aSuite, syncScheduler.IsReportableNow(readHandler1)); + NL_TEST_ASSERT(aSuite, syncScheduler.IsReportableNow(readHandler2)); + NL_TEST_ASSERT(aSuite, syncScheduler.IsReportableNow(readHandler3)); + NL_TEST_ASSERT(aSuite, syncScheduler.IsReportableNow(readHandler4)); + readHandler4->mObserver->OnBecameReportable(readHandler1); + readHandler4->mObserver->OnBecameReportable(readHandler2); + readHandler4->mObserver->OnBecameReportable(readHandler3); + readHandler4->mObserver->OnBecameReportable(readHandler4); + readHandler4->mObserver->OnSubscriptionAction(readHandler1); + readHandler4->mObserver->OnSubscriptionAction(readHandler2); + readHandler4->mObserver->OnSubscriptionAction(readHandler3); + readHandler4->mObserver->OnSubscriptionAction(readHandler4); + NL_TEST_ASSERT(aSuite, !syncScheduler.IsReportableNow(readHandler1)); + NL_TEST_ASSERT(aSuite, !syncScheduler.IsReportableNow(readHandler2)); + NL_TEST_ASSERT(aSuite, !syncScheduler.IsReportableNow(readHandler3)); + NL_TEST_ASSERT(aSuite, !syncScheduler.IsReportableNow(readHandler4)); + + // Next emission should be scheduled on the max timestamp of readHandler4 as it is the most restrictive, and other handlers + // should get synced except handler 3 as its min is higher + NL_TEST_ASSERT(aSuite, syncScheduler.mNextReportTimestamp == node4->GetMaxTimestamp()); + NL_TEST_ASSERT(aSuite, node1->GetSyncTimestamp() == node4->GetMaxTimestamp()); + NL_TEST_ASSERT(aSuite, node2->GetSyncTimestamp() == node4->GetMaxTimestamp()); + NL_TEST_ASSERT(aSuite, node3->GetSyncTimestamp() == node1->GetMaxTimestamp()); + + ctx.GetIOContext().DriveIOUntil(System::Clock::Milliseconds32(1000), + [&]() -> bool { return syncScheduler.IsReportableNow(readHandler4); }); + // Increment the mock system time to the max to the time waited + sTestTimerSynchronizedDelegate.IncrementMockTimestamp(System::Clock::Milliseconds64(1000)); + + // Confirm readHandler 1-2-4 are reportable and handler 3 is not because of it min interval + NL_TEST_ASSERT(aSuite, syncScheduler.IsReportableNow(readHandler1)); + NL_TEST_ASSERT(aSuite, syncScheduler.IsReportableNow(readHandler2)); + NL_TEST_ASSERT(aSuite, !syncScheduler.IsReportableNow(readHandler3)); + NL_TEST_ASSERT(aSuite, syncScheduler.IsReportableNow(readHandler4)); + + syncScheduler.UnregisterAllHandlers(); + readHandlerPool.ReleaseAll(); + exchangeCtx->Close(); + NL_TEST_ASSERT(aSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 0); + } }; } // namespace reporting @@ -408,6 +729,7 @@ static nlTest sTests[] = { NL_TEST_DEF("TestReadHandlerList", chip::app::reporting::TestReportScheduler::TestReadHandlerList), NL_TEST_DEF("TestReportTiming", chip::app::reporting::TestReportScheduler::TestReportTiming), NL_TEST_DEF("TestObserverCallbacks", chip::app::reporting::TestReportScheduler::TestObserverCallbacks), + NL_TEST_DEF("TestSynchronizedScheduler", chip::app::reporting::TestReportScheduler::TestSynchronizedScheduler), NL_TEST_SENTINEL(), }; From 47c8094382d618ef065bb50ee69f0ce6aa48da87 Mon Sep 17 00:00:00 2001 From: lpbeliveau-silabs Date: Thu, 13 Jul 2023 13:29:21 -0400 Subject: [PATCH 02/12] Added a quick fix because TestDecoding won't compile otherwise, this doesn't belong here --- src/lib/format/tests/TestDecoding.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lib/format/tests/TestDecoding.cpp b/src/lib/format/tests/TestDecoding.cpp index f9ef31719f5b6b..83c4424364579b 100644 --- a/src/lib/format/tests/TestDecoding.cpp +++ b/src/lib/format/tests/TestDecoding.cpp @@ -34,7 +34,7 @@ using namespace chip::TLV; using namespace chip::TLVMeta; using namespace chip::TestData; -const Entry _empty_item[0] = {}; +const Entry _empty_item[] = { { ItemInfo(), 0 } }; const std::array, 1> empty_meta = { { { 0, _empty_item } } }; const Entry _FakeProtocolData[] = { From bec57748f5bec22b9b69fb270f1aa18e40ad937f Mon Sep 17 00:00:00 2001 From: lpbeliveau-silabs <112982107+lpbeliveau-silabs@users.noreply.github.com> Date: Fri, 14 Jul 2023 10:23:18 -0400 Subject: [PATCH 03/12] Apply suggestions from code review Co-authored-by: Boris Zbarsky --- src/app/reporting/ReportScheduler.h | 2 +- src/app/reporting/SynchronizedReportSchedulerImpl.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/app/reporting/ReportScheduler.h b/src/app/reporting/ReportScheduler.h index 371e6bc7d8aa5e..43becc4a651cb6 100644 --- a/src/app/reporting/ReportScheduler.h +++ b/src/app/reporting/ReportScheduler.h @@ -95,7 +95,7 @@ class ReportScheduler : public ReadHandler::Observer void SetSyncTimestamp(System::Clock::Timestamp aSyncTimestamp) { - // Prevents the sync timestamp to be set to a value lower than the min timestamp + // Prevents the sync timestamp being set to a value lower than the min timestamp VerifyOrReturn(aSyncTimestamp >= mMinTimestamp); mSyncTimestamp = aSyncTimestamp; } diff --git a/src/app/reporting/SynchronizedReportSchedulerImpl.cpp b/src/app/reporting/SynchronizedReportSchedulerImpl.cpp index ab33ede57dd909..0d0d9512651a28 100644 --- a/src/app/reporting/SynchronizedReportSchedulerImpl.cpp +++ b/src/app/reporting/SynchronizedReportSchedulerImpl.cpp @@ -137,7 +137,7 @@ bool SynchronizedReportSchedulerImpl::CheckSchedulerTimerActive(ReadHandlerNode return mTimerDelegate->IsTimerActive(this); } -/// @brief Find the smallest maximum interval possible and sets it as the common maximum +/// @brief Find the smallest maximum interval possible and set it as the common maximum /// @return NO_ERROR if the smallest maximum interval was found, error otherwise, INVALID LIST LENGTH if the list is empty CHIP_ERROR SynchronizedReportSchedulerImpl::FindNextMaxInterval() { From 7a7166925514ef6def80d28ed29056c2c95ee44b Mon Sep 17 00:00:00 2001 From: lpbeliveau-silabs Date: Fri, 14 Jul 2023 16:37:33 -0400 Subject: [PATCH 04/12] Refactored ReportScheduler Impls to better take advantage of inheritance, removed bloat, excluded test for platform in which problems are caused due to unprocessed engine runs --- src/app/ReadHandler.h | 8 +- src/app/reporting/ReportScheduler.h | 5 +- src/app/reporting/ReportSchedulerImpl.cpp | 84 ++++++-------- src/app/reporting/ReportSchedulerImpl.h | 15 +-- .../SynchronizedReportSchedulerImpl.cpp | 105 +++--------------- .../SynchronizedReportSchedulerImpl.h | 23 ++-- src/app/tests/BUILD.gn | 11 +- src/app/tests/TestReportScheduler.cpp | 18 +-- src/lib/format/tests/TestDecoding.cpp | 2 +- 9 files changed, 90 insertions(+), 181 deletions(-) diff --git a/src/app/ReadHandler.h b/src/app/ReadHandler.h index 77c88c12cd1ff9..000a57ea65ae0f 100644 --- a/src/app/ReadHandler.h +++ b/src/app/ReadHandler.h @@ -51,8 +51,14 @@ #include #include +#if CHIP_DEVICE_CONFIG_ENABLE_SED +#define SUBSCRIPTION_MAX_INTERVAL_PUBLISHER_LIMIT CHIP_DEVICE_CONFIG_SED_IDLE_INTERVAL +#else +#define SUBSCRIPTION_MAX_INTERVAL_PUBLISHER_LIMIT 3600 // 3600 seconds +#endif + // https://github.com/CHIP-Specifications/connectedhomeip-spec/blob/61a9d19e6af12fdfb0872bcff26d19de6c680a1a/src/Ch02_Architecture.adoc#1122-subscribe-interaction-limits -constexpr uint16_t kSubscriptionMaxIntervalPublisherLimit = 3600; // 3600 seconds +constexpr uint16_t kSubscriptionMaxIntervalPublisherLimit = SUBSCRIPTION_MAX_INTERVAL_PUBLISHER_LIMIT; namespace chip { namespace app { diff --git a/src/app/reporting/ReportScheduler.h b/src/app/reporting/ReportScheduler.h index 43becc4a651cb6..14685301a09c71 100644 --- a/src/app/reporting/ReportScheduler.h +++ b/src/app/reporting/ReportScheduler.h @@ -110,7 +110,8 @@ class ReportScheduler : public ReadHandler::Observer ReadHandler * mReadHandler; Timestamp mMinTimestamp; Timestamp mMaxTimestamp; - Timestamp mSyncTimestamp; + Timestamp mSyncTimestamp; // Timestamp at which the read handler will be allowed to emit a report so it can be synced with + // other handlers that have an earlier max timestamp }; ReportScheduler(TimerDelegate * aTimerDelegate) : mTimerDelegate(aTimerDelegate) {} @@ -136,8 +137,6 @@ class ReportScheduler : public ReadHandler::Observer /// @brief Get the number of ReadHandlers registered in the scheduler's node pool size_t GetNumReadHandlers() const { return mNodesPool.Allocated(); } - virtual void ReportTimerCallback() = 0; - protected: friend class chip::app::reporting::TestReportScheduler; diff --git a/src/app/reporting/ReportSchedulerImpl.cpp b/src/app/reporting/ReportSchedulerImpl.cpp index 4bb6ffa8fa13c2..be280323d5fa03 100644 --- a/src/app/reporting/ReportSchedulerImpl.cpp +++ b/src/app/reporting/ReportSchedulerImpl.cpp @@ -22,19 +22,14 @@ namespace chip { namespace app { namespace reporting { -using Seconds16 = System::Clock::Seconds16; -using Milliseconds32 = System::Clock::Milliseconds32; -using Timeout = System::Clock::Timeout; -using Timestamp = System::Clock::Timestamp; +using namespace System::Clock; using ReadHandlerNode = ReportScheduler::ReadHandlerNode; /// @brief Callback called when the report timer expires to schedule an engine run regardless of the state of the ReadHandlers, as /// the engine already verifies that read handlers are reportable before sending a report void ReportSchedulerImpl::ReportTimerCallback() { -#ifndef CONFIG_BUILD_FOR_HOST_UNIT_TEST InteractionModelEngine::GetInstance()->GetReportingEngine().ScheduleRun(); -#endif } ReportSchedulerImpl::ReportSchedulerImpl(TimerDelegate * aTimerDelegate) : ReportScheduler(aTimerDelegate) @@ -56,17 +51,7 @@ void ReportSchedulerImpl::OnBecameReportable(ReadHandler * aReadHandler) VerifyOrReturn(nullptr != node); Milliseconds32 newTimeout; - if (node->IsReportableNow()) - { - // If the handler is reportable now, just schedule a report immediately - newTimeout = Milliseconds32(0); - } - else - { - // If the handler is not reportable now, schedule a report for the min interval - newTimeout = node->GetMinTimestamp() - mTimerDelegate->GetCurrentMonotonicTimestamp(); - } - + ReturnOnFailure(CalculateNextReportTimeout(newTimeout, node)); ScheduleReport(newTimeout, node); } @@ -76,7 +61,8 @@ void ReportSchedulerImpl::OnSubscriptionAction(ReadHandler * apReadHandler) VerifyOrReturn(nullptr != node); // Schedule callback for max interval by computing the difference between the max timestamp and the current timestamp node->SetIntervalTimeStamps(apReadHandler); - Milliseconds32 newTimeout = node->GetMaxTimestamp() - mTimerDelegate->GetCurrentMonotonicTimestamp(); + Milliseconds32 newTimeout; + ReturnOnFailure(CalculateNextReportTimeout(newTimeout, node)); ScheduleReport(newTimeout, node); } @@ -101,26 +87,9 @@ CHIP_ERROR ReportSchedulerImpl::RegisterReadHandler(ReadHandler * aReadHandler) " and system Timestamp %" PRIu64 ".", newNode->GetMinTimestamp().count(), newNode->GetMaxTimestamp().count()); - Timestamp now = mTimerDelegate->GetCurrentMonotonicTimestamp(); Milliseconds32 newTimeout; - // If the handler is reportable, schedule a report for the min interval, otherwise schedule a report for the max interval - if (newNode->IsReportableNow()) - { - // If the handler is reportable now, just schedule a report immediately - newTimeout = Milliseconds32(0); - } - else if (IsReadHandlerReportable(aReadHandler) && (newNode->GetMinTimestamp() > now)) - { - // If the handler is reportable now, but the min interval is not elapsed, schedule a report for the moment the min interval - // has elapsed - newTimeout = newNode->GetMinTimestamp() - now; - } - else - { - // If the handler is not reportable now, schedule a report for the max interval - newTimeout = newNode->GetMaxTimestamp() - now; - } - + // No need to check for error here, since the node is already in the list otherwise we would have Died + CalculateNextReportTimeout(newTimeout, newNode); ReturnErrorOnFailure(ScheduleReport(newTimeout, newNode)); return CHIP_NO_ERROR; } @@ -128,8 +97,8 @@ CHIP_ERROR ReportSchedulerImpl::RegisterReadHandler(ReadHandler * aReadHandler) CHIP_ERROR ReportSchedulerImpl::ScheduleReport(Timeout timeout, ReadHandlerNode * node) { // Cancel Report if it is currently scheduled - CancelSchedulerTimer(node); - StartSchedulerTimer(node, timeout); + mTimerDelegate->CancelTimer(node); + ReturnErrorOnFailure(mTimerDelegate->StartTimer(node, timeout)); return CHIP_NO_ERROR; } @@ -138,7 +107,7 @@ void ReportSchedulerImpl::CancelReport(ReadHandler * aReadHandler) { ReadHandlerNode * node = FindReadHandlerNode(aReadHandler); VerifyOrReturn(nullptr != node); - CancelSchedulerTimer(node); + mTimerDelegate->CancelTimer(node); } void ReportSchedulerImpl::UnregisterReadHandler(ReadHandler * aReadHandler) @@ -166,23 +135,32 @@ bool ReportSchedulerImpl::IsReportScheduled(ReadHandler * aReadHandler) { ReadHandlerNode * node = FindReadHandlerNode(aReadHandler); VerifyOrReturnValue(nullptr != node, false); - return CheckSchedulerTimerActive(node); -} - -CHIP_ERROR ReportSchedulerImpl::StartSchedulerTimer(ReadHandlerNode * node, System::Clock::Timeout aTimeout) -{ - // Schedule Report - return mTimerDelegate->StartTimer(node, aTimeout); + return mTimerDelegate->IsTimerActive(node); } -void ReportSchedulerImpl::CancelSchedulerTimer(ReadHandlerNode * node) +CHIP_ERROR ReportSchedulerImpl::CalculateNextReportTimeout(Timeout & timeout, ReadHandlerNode * aNode) { - mTimerDelegate->CancelTimer(node); -} + VerifyOrReturnError(mReadHandlerList.Contains(aNode), CHIP_ERROR_INVALID_ARGUMENT); + Timestamp now = mTimerDelegate->GetCurrentMonotonicTimestamp(); -bool ReportSchedulerImpl::CheckSchedulerTimerActive(ReadHandlerNode * node) -{ - return mTimerDelegate->IsTimerActive(node); + // If the handler is reportable now, just schedule a report immediately + if (aNode->IsReportableNow()) + { + // If the handler is reportable now, just schedule a report immediately + timeout = Milliseconds32(0); + } + else if (IsReadHandlerReportable(aNode->GetReadHandler()) && (aNode->GetMinTimestamp() > now)) + { + // If the handler is reportable now, but the min interval is not elapsed, schedule a report for the moment the min interval + // has elapsed + timeout = aNode->GetMinTimestamp() - now; + } + else + { + // If the handler is not reportable now, schedule a report for the max interval + timeout = aNode->GetMaxTimestamp() - now; + } + return CHIP_NO_ERROR; } } // namespace reporting diff --git a/src/app/reporting/ReportSchedulerImpl.h b/src/app/reporting/ReportSchedulerImpl.h index f87d9a47c65806..079164ce9c77aa 100644 --- a/src/app/reporting/ReportSchedulerImpl.h +++ b/src/app/reporting/ReportSchedulerImpl.h @@ -24,6 +24,8 @@ namespace chip { namespace app { namespace reporting { +using Timeout = System::Clock::Timeout; + class ReportSchedulerImpl : public ReportScheduler { public: @@ -38,11 +40,11 @@ class ReportSchedulerImpl : public ReportScheduler bool IsReportScheduled(ReadHandler * aReadHandler) override; - void ReportTimerCallback() override; + void ReportTimerCallback(); protected: virtual CHIP_ERROR RegisterReadHandler(ReadHandler * aReadHandler); - virtual CHIP_ERROR ScheduleReport(System::Clock::Timeout timeout, ReadHandlerNode * node); + virtual CHIP_ERROR ScheduleReport(Timeout timeout, ReadHandlerNode * node); virtual void CancelReport(ReadHandler * aReadHandler); virtual void UnregisterReadHandler(ReadHandler * aReadHandler); virtual void UnregisterAllHandlers(); @@ -50,14 +52,7 @@ class ReportSchedulerImpl : public ReportScheduler private: friend class chip::app::reporting::TestReportScheduler; - /// @brief Start a timer for a given ReadHandlerNode, ensures that if a timer is already running for this node, it is cancelled - /// @param node Node of the ReadHandler list to start a timer for - /// @param aTimeout Delay before the timer expires - virtual CHIP_ERROR StartSchedulerTimer(ReadHandlerNode * node, System::Clock::Timeout aTimeout); - /// @brief Cancel the timer for a given ReadHandlerNode - virtual void CancelSchedulerTimer(ReadHandlerNode * node); - /// @brief Check if the timer for a given ReadHandlerNode is active - virtual bool CheckSchedulerTimerActive(ReadHandlerNode * node); + virtual CHIP_ERROR CalculateNextReportTimeout(Timeout & timeout, ReadHandlerNode * aNode); }; } // namespace reporting diff --git a/src/app/reporting/SynchronizedReportSchedulerImpl.cpp b/src/app/reporting/SynchronizedReportSchedulerImpl.cpp index 0d0d9512651a28..3333428f080f0f 100644 --- a/src/app/reporting/SynchronizedReportSchedulerImpl.cpp +++ b/src/app/reporting/SynchronizedReportSchedulerImpl.cpp @@ -23,69 +23,15 @@ namespace chip { namespace app { namespace reporting { -using Seconds16 = System::Clock::Seconds16; -using Milliseconds32 = System::Clock::Milliseconds32; -using Timeout = System::Clock::Timeout; -using Timestamp = System::Clock::Timestamp; +using namespace System::Clock; using ReadHandlerNode = ReportScheduler::ReadHandlerNode; -/// @brief When a ReadHandler becomes reportable, recaculate the earliest possible interval before scheduling an engine run and -/// reschedule the report -void SynchronizedReportSchedulerImpl::OnBecameReportable(ReadHandler * aReadHandler) -{ - ReadHandlerNode * node = FindReadHandlerNode(aReadHandler); - VerifyOrReturn(nullptr != node); - Milliseconds32 newTimeout; - ReturnOnFailure(CalculateNextReportTimeout(newTimeout, node)); - ScheduleReport(newTimeout, node); -} - -void SynchronizedReportSchedulerImpl::OnSubscriptionAction(ReadHandler * aReadHandler) -{ - ReadHandlerNode * node = FindReadHandlerNode(aReadHandler); - VerifyOrReturn(nullptr != node); - - // Update the node's timestamps - node->SetIntervalTimeStamps(aReadHandler); - - Milliseconds32 newTimeout; - ReturnOnFailure(CalculateNextReportTimeout(newTimeout, node)); - ScheduleReport(newTimeout, node); -} - -CHIP_ERROR SynchronizedReportSchedulerImpl::RegisterReadHandler(ReadHandler * aReadHandler) -{ - ReadHandlerNode * newNode = FindReadHandlerNode(aReadHandler); - // If the handler is already registered, no need to register it again - VerifyOrReturnValue(nullptr == newNode, CHIP_NO_ERROR); - newNode = mNodesPool.CreateObject(aReadHandler, mTimerDelegate, [this]() { this->ReportTimerCallback(); }); - mReadHandlerList.PushBack(newNode); - - ChipLogProgress(DataManagement, - "Registered ReadHandler that will schedule a report between system Timestamp: %" PRIu64 - " and system Timestamp %" PRIu64 ".", - newNode->GetMinTimestamp().count(), newNode->GetMaxTimestamp().count()); - - Milliseconds32 newTimeout; - CHIP_ERROR err = CalculateNextReportTimeout(newTimeout, newNode); - if (CHIP_NO_ERROR != err) - { - mReadHandlerList.Remove(newNode); - mNodesPool.ReleaseObject(newNode); - return err; - } - - ReturnErrorOnFailure(ScheduleReport(newTimeout, newNode)); - return CHIP_NO_ERROR; -} - CHIP_ERROR SynchronizedReportSchedulerImpl::ScheduleReport(Timeout timeout, ReadHandlerNode * node) { // Cancel Report if it is currently scheduled - CancelSchedulerTimer(nullptr); - - ReturnErrorOnFailure(StartSchedulerTimer(nullptr, timeout)); - mNextReportTimestamp = mTimerDelegate->GetCurrentMonotonicTimestamp() + timeout; + mTimerDelegate->CancelTimer(this); + ReturnErrorOnFailure(mTimerDelegate->StartTimer(this, timeout)); + mTestNextReportTimestamp = mTimerDelegate->GetCurrentMonotonicTimestamp() + timeout; return CHIP_NO_ERROR; } @@ -93,7 +39,7 @@ CHIP_ERROR SynchronizedReportSchedulerImpl::ScheduleReport(Timeout timeout, Read void SynchronizedReportSchedulerImpl::CancelReport(ReadHandler * aReadHandler) { // We don't need to take action on the handler, since the timer is common here - CancelSchedulerTimer(nullptr); + mTimerDelegate->CancelTimer(this); } void SynchronizedReportSchedulerImpl::UnregisterReadHandler(ReadHandler * aReadHandler) @@ -121,19 +67,6 @@ bool SynchronizedReportSchedulerImpl::IsReportScheduled(ReadHandler * aReadHandl { ReadHandlerNode * node = FindReadHandlerNode(aReadHandler); VerifyOrReturnValue(nullptr != node, false); - return CheckSchedulerTimerActive(nullptr); -} - -CHIP_ERROR SynchronizedReportSchedulerImpl::StartSchedulerTimer(ReadHandlerNode * node, System::Clock::Timeout aTimeout) -{ - return mTimerDelegate->StartTimer(this, aTimeout); -} -void SynchronizedReportSchedulerImpl::CancelSchedulerTimer(ReadHandlerNode * node) -{ - mTimerDelegate->CancelTimer(this); -} -bool SynchronizedReportSchedulerImpl::CheckSchedulerTimerActive(ReadHandlerNode * node) -{ return mTimerDelegate->IsTimerActive(this); } @@ -141,9 +74,10 @@ bool SynchronizedReportSchedulerImpl::CheckSchedulerTimerActive(ReadHandlerNode /// @return NO_ERROR if the smallest maximum interval was found, error otherwise, INVALID LIST LENGTH if the list is empty CHIP_ERROR SynchronizedReportSchedulerImpl::FindNextMaxInterval() { - System::Clock::Timestamp now = mTimerDelegate->GetCurrentMonotonicTimestamp(); - System::Clock::Timestamp earliest = now + Milliseconds64(kSubscriptionMaxIntervalPublisherLimit); VerifyOrReturnError(!mReadHandlerList.Empty(), CHIP_ERROR_INVALID_LIST_LENGTH); + System::Clock::Timestamp now = mTimerDelegate->GetCurrentMonotonicTimestamp(); + System::Clock::Timestamp earliest = now + Seconds16(kSubscriptionMaxIntervalPublisherLimit); + for (auto & iter : mReadHandlerList) { if (iter.GetMaxTimestamp() < earliest && iter.GetMaxTimestamp() > now) @@ -163,10 +97,9 @@ CHIP_ERROR SynchronizedReportSchedulerImpl::FindNextMaxInterval() /// @return NO_ERROR if the highest minimum timestamp was found, error otherwise, INVALID LIST LENGTH if the list is empty CHIP_ERROR SynchronizedReportSchedulerImpl::FindNextMinInterval() { - System::Clock::Timestamp now = mTimerDelegate->GetCurrentMonotonicTimestamp(); - System::Clock::Timestamp latest = now; - VerifyOrReturnError(!mReadHandlerList.Empty(), CHIP_ERROR_INVALID_LIST_LENGTH); + System::Clock::Timestamp latest = mTimerDelegate->GetCurrentMonotonicTimestamp(); + for (auto & iter : mReadHandlerList) { if (iter.GetMinTimestamp() > latest) @@ -184,7 +117,7 @@ CHIP_ERROR SynchronizedReportSchedulerImpl::FindNextMinInterval() return CHIP_NO_ERROR; } -CHIP_ERROR SynchronizedReportSchedulerImpl::CalculateNextReportTimeout(System::Clock::Timeout & timeout, ReadHandlerNode * aNode) +CHIP_ERROR SynchronizedReportSchedulerImpl::CalculateNextReportTimeout(Timeout & timeout, ReadHandlerNode * aNode) { VerifyOrReturnError(mReadHandlerList.Contains(aNode), CHIP_ERROR_INVALID_ARGUMENT); ReturnErrorOnFailure(FindNextMaxInterval()); @@ -200,12 +133,12 @@ CHIP_ERROR SynchronizedReportSchedulerImpl::CalculateNextReportTimeout(System::C } else { - timeout = Milliseconds32(mNextMinTimestamp.count() - now.count()); + timeout = mNextMinTimestamp - now; } } else { - timeout = Milliseconds32(mNextMaxTimestamp.count() - now.count()); + timeout = mNextMaxTimestamp - now; } // Updates the synching time of each handler @@ -226,22 +159,18 @@ CHIP_ERROR SynchronizedReportSchedulerImpl::CalculateNextReportTimeout(System::C /// the engine already verifies that read handlers are reportable before sending a report void SynchronizedReportSchedulerImpl::ReportTimerCallback() { -#ifndef CONFIG_BUILD_FOR_HOST_UNIT_TEST - InteractionModelEngine::GetInstance()->GetReportingEngine().ScheduleRun(); -#else + ReportSchedulerImpl::ReportTimerCallback(); + Timestamp now = mTimerDelegate->GetCurrentMonotonicTimestamp(); ChipLogProgress(DataManagement, "Engine run at time: %" PRIu64 " for Handlers:", now.count()); - for (auto & iter : mReadHandlerList) { if (iter.IsReportableNow()) { - ChipLogProgress(DataManagement, "Handler: %p" PRIu64 " with min: %" PRIu64 " and max: %" PRIu64 " and sync: %" PRIu64, - (&iter), iter.GetMinTimestamp().count(), iter.GetMaxTimestamp().count(), - iter.GetSyncTimestamp().count()); + ChipLogProgress(DataManagement, "Handler: %p with min: %" PRIu64 " and max: %" PRIu64 " and sync: %" PRIu64, (&iter), + iter.GetMinTimestamp().count(), iter.GetMaxTimestamp().count(), iter.GetSyncTimestamp().count()); } } -#endif } } // namespace reporting diff --git a/src/app/reporting/SynchronizedReportSchedulerImpl.h b/src/app/reporting/SynchronizedReportSchedulerImpl.h index 3d62133b7a2527..8d9b659a92b889 100644 --- a/src/app/reporting/SynchronizedReportSchedulerImpl.h +++ b/src/app/reporting/SynchronizedReportSchedulerImpl.h @@ -36,34 +36,27 @@ class SynchronizedReportSchedulerImpl : public ReportSchedulerImpl SynchronizedReportSchedulerImpl(TimerDelegate * aTimerDelegate) : ReportSchedulerImpl(aTimerDelegate) {} ~SynchronizedReportSchedulerImpl() {} - // ReadHandlerObserver - void OnBecameReportable(ReadHandler * aReadHandler) override; - void OnSubscriptionAction(ReadHandler * aReadHandler) override; - bool IsReportScheduled(ReadHandler * aReadHandler) override; - void ReportTimerCallback() override; + void ReportTimerCallback(); protected: - CHIP_ERROR RegisterReadHandler(ReadHandler * aReadHandler) override; - virtual CHIP_ERROR ScheduleReport(System::Clock::Timeout timeout, ReadHandlerNode * node) override; + CHIP_ERROR ScheduleReport(System::Clock::Timeout timeout, ReadHandlerNode * node) override; void CancelReport(ReadHandler * aReadHandler) override; void UnregisterReadHandler(ReadHandler * aReadHandler) override; private: friend class chip::app::reporting::TestReportScheduler; - CHIP_ERROR StartSchedulerTimer(ReadHandlerNode * node, Timeout aTimeout) override; - void CancelSchedulerTimer(ReadHandlerNode * node) override; - bool CheckSchedulerTimerActive(ReadHandlerNode * node) override; - CHIP_ERROR FindNextMinInterval(); CHIP_ERROR FindNextMaxInterval(); - CHIP_ERROR CalculateNextReportTimeout(Timeout & timeout, ReadHandlerNode * aReadHandlerNode); + CHIP_ERROR CalculateNextReportTimeout(Timeout & timeout, ReadHandlerNode * aReadHandlerNode) override; + + Timestamp mNextMaxTimestamp = Milliseconds64(0); + Timestamp mNextMinTimestamp = Milliseconds64(0); - Timestamp mNextMaxTimestamp = Milliseconds64(0); - Timestamp mNextMinTimestamp = Milliseconds64(0); - Timestamp mNextReportTimestamp = Milliseconds64(0); + // Timestamp of the next report to be scheduled, only used for testing + Timestamp mTestNextReportTimestamp = Milliseconds64(0); }; } // namespace reporting diff --git a/src/app/tests/BUILD.gn b/src/app/tests/BUILD.gn index 365596da641d03..a05edd6181dd16 100644 --- a/src/app/tests/BUILD.gn +++ b/src/app/tests/BUILD.gn @@ -144,7 +144,6 @@ chip_test_suite("tests") { "TestOperationalStateDelegate.cpp", "TestPendingNotificationMap.cpp", "TestReadInteraction.cpp", - "TestReportScheduler.cpp", "TestReportingEngine.cpp", "TestSceneTable.cpp", "TestStatusIB.cpp", @@ -178,6 +177,16 @@ chip_test_suite("tests") { test_sources += [ "TestEventLogging.cpp" ] } + # The platform manager is not properly clearing queues in test teardown, which results in + # DrainIO calls to not being able to run in expected time (5seconds) if unprocesses engine + # run are remaining, causing tests to crash in Open IoT SDK and Zephyr tests since they are + # running all tests in one file. We need to figure out how to properly clean the event queues + # before enabling this test for these platforms. + if (chip_device_platform != "nrfconnect" && + chip_device_platform != "openiotsdk") { + test_sources += [ "TestReportScheduler.cpp" ] + } + cflags = [ "-Wconversion" ] public_deps = [ diff --git a/src/app/tests/TestReportScheduler.cpp b/src/app/tests/TestReportScheduler.cpp index 7d9a7139be3595..9cd386e001df44 100644 --- a/src/app/tests/TestReportScheduler.cpp +++ b/src/app/tests/TestReportScheduler.cpp @@ -498,7 +498,7 @@ class TestReportScheduler // Validates that the highest min is selected as the common min interval NL_TEST_ASSERT(aSuite, syncScheduler.mNextMinTimestamp == node2->GetMinTimestamp()); // Validates that the next report emission is scheduled on the common max timestamp - NL_TEST_ASSERT(aSuite, syncScheduler.mNextReportTimestamp == syncScheduler.mNextMaxTimestamp); + NL_TEST_ASSERT(aSuite, syncScheduler.mTestNextReportTimestamp == syncScheduler.mNextMaxTimestamp); // Simulate waiting for the max interval to expire (2s) // Increment the mock system time to the max to the time waited @@ -523,7 +523,7 @@ class TestReportScheduler // Validate that the max timestamp for both readhandlers got updated and that the next report emission is scheduled on // the new max timestamp for readhandler1 NL_TEST_ASSERT(aSuite, node1->GetMaxTimestamp() > sTestTimerSynchronizedDelegate.GetCurrentMonotonicTimestamp()); - NL_TEST_ASSERT(aSuite, syncScheduler.mNextReportTimestamp == node1->GetMaxTimestamp()); + NL_TEST_ASSERT(aSuite, syncScheduler.mTestNextReportTimestamp == node1->GetMaxTimestamp()); // Confirm behavior when a read handler becomes dirty readHandler1->ForceDirtyState(); @@ -531,7 +531,7 @@ class TestReportScheduler NL_TEST_ASSERT(aSuite, syncScheduler.IsReportableNow(readHandler1)); readHandler1->mObserver->OnBecameReportable(readHandler1); // Confirm that the next report emission is scheduled on the min timestamp of readHandler2 as it is the highest - NL_TEST_ASSERT(aSuite, syncScheduler.mNextReportTimestamp == node2->GetMinTimestamp()); + NL_TEST_ASSERT(aSuite, syncScheduler.mTestNextReportTimestamp == node2->GetMinTimestamp()); // Simulate wait enough for min timestamp of readHandler2 to be reached (1s) // Increment the mock system time to the max to the time waited sTestTimerSynchronizedDelegate.IncrementMockTimestamp(System::Clock::Milliseconds64(1000)); @@ -552,7 +552,7 @@ class TestReportScheduler // Validate next report scheduled on the max timestamp of readHandler1 NL_TEST_ASSERT(aSuite, node1->GetMaxTimestamp() > sTestTimerSynchronizedDelegate.GetCurrentMonotonicTimestamp()); - NL_TEST_ASSERT(aSuite, syncScheduler.mNextReportTimestamp == node1->GetMaxTimestamp()); + NL_TEST_ASSERT(aSuite, syncScheduler.mTestNextReportTimestamp == node1->GetMaxTimestamp()); // Simulate a new ReadHandler being added with a min forcing a conflict @@ -576,7 +576,7 @@ class TestReportScheduler // Since the min interval on readHandler3 is 2, it should be above the current max timestamp, therefore the next report // should still happen on the max timestamp of readHandler1 and the sync should be done on future reports - NL_TEST_ASSERT(aSuite, syncScheduler.mNextReportTimestamp == node1->GetMaxTimestamp()); + NL_TEST_ASSERT(aSuite, syncScheduler.mTestNextReportTimestamp == node1->GetMaxTimestamp()); // The min timestamp should also not have changed since the min of readhandler3 is higher than the current max NL_TEST_ASSERT(aSuite, syncScheduler.mNextMinTimestamp == node2->GetMinTimestamp()); @@ -599,7 +599,7 @@ class TestReportScheduler NL_TEST_ASSERT(aSuite, !syncScheduler.IsReportableNow(readHandler2)); // Confirm that next report is scheduled on the max timestamp of readHandler3 and other 2 readHandlers are synced - NL_TEST_ASSERT(aSuite, syncScheduler.mNextReportTimestamp == node3->GetMaxTimestamp()); + NL_TEST_ASSERT(aSuite, syncScheduler.mTestNextReportTimestamp == node3->GetMaxTimestamp()); NL_TEST_ASSERT(aSuite, syncScheduler.mNextMinTimestamp == node3->GetMinTimestamp()); NL_TEST_ASSERT(aSuite, node1->GetSyncTimestamp() == node3->GetMaxTimestamp()); NL_TEST_ASSERT(aSuite, node2->GetSyncTimestamp() == node3->GetMaxTimestamp()); @@ -623,7 +623,7 @@ class TestReportScheduler NL_TEST_ASSERT(aSuite, !syncScheduler.IsReportableNow(readHandler1)); NL_TEST_ASSERT(aSuite, !syncScheduler.IsReportableNow(readHandler2)); NL_TEST_ASSERT(aSuite, !syncScheduler.IsReportableNow(readHandler3)); - NL_TEST_ASSERT(aSuite, syncScheduler.mNextReportTimestamp == node1->GetMaxTimestamp()); + NL_TEST_ASSERT(aSuite, syncScheduler.mTestNextReportTimestamp == node1->GetMaxTimestamp()); NL_TEST_ASSERT(aSuite, syncScheduler.mNextMinTimestamp == node3->GetMinTimestamp()); NL_TEST_ASSERT(aSuite, node1->GetSyncTimestamp() == node1->GetMaxTimestamp()); NL_TEST_ASSERT(aSuite, node2->GetSyncTimestamp() == node1->GetMaxTimestamp()); @@ -642,7 +642,7 @@ class TestReportScheduler ReadHandlerNode * node4 = syncScheduler.FindReadHandlerNode(readHandler4); // Confirm next report is scheduled on the max timestamp of readHandler4 and other handlers be synced - NL_TEST_ASSERT(aSuite, syncScheduler.mNextReportTimestamp == node4->GetMaxTimestamp()); + NL_TEST_ASSERT(aSuite, syncScheduler.mTestNextReportTimestamp == node4->GetMaxTimestamp()); NL_TEST_ASSERT(aSuite, node1->GetSyncTimestamp() == node4->GetMaxTimestamp()); NL_TEST_ASSERT(aSuite, node2->GetSyncTimestamp() == node4->GetMaxTimestamp()); NL_TEST_ASSERT(aSuite, node3->GetSyncTimestamp() == node1->GetMaxTimestamp()); @@ -692,7 +692,7 @@ class TestReportScheduler // Next emission should be scheduled on the max timestamp of readHandler4 as it is the most restrictive, and other handlers // should get synced except handler 3 as its min is higher - NL_TEST_ASSERT(aSuite, syncScheduler.mNextReportTimestamp == node4->GetMaxTimestamp()); + NL_TEST_ASSERT(aSuite, syncScheduler.mTestNextReportTimestamp == node4->GetMaxTimestamp()); NL_TEST_ASSERT(aSuite, node1->GetSyncTimestamp() == node4->GetMaxTimestamp()); NL_TEST_ASSERT(aSuite, node2->GetSyncTimestamp() == node4->GetMaxTimestamp()); NL_TEST_ASSERT(aSuite, node3->GetSyncTimestamp() == node1->GetMaxTimestamp()); diff --git a/src/lib/format/tests/TestDecoding.cpp b/src/lib/format/tests/TestDecoding.cpp index 83c4424364579b..ecf2b51db3adeb 100644 --- a/src/lib/format/tests/TestDecoding.cpp +++ b/src/lib/format/tests/TestDecoding.cpp @@ -34,7 +34,7 @@ using namespace chip::TLV; using namespace chip::TLVMeta; using namespace chip::TestData; -const Entry _empty_item[] = { { ItemInfo(), 0 } }; +const Entry _empty_item[] = {}; const std::array, 1> empty_meta = { { { 0, _empty_item } } }; const Entry _FakeProtocolData[] = { From 855885ec0cea450ff6be9322ca857261f32658b7 Mon Sep 17 00:00:00 2001 From: lpbeliveau-silabs <112982107+lpbeliveau-silabs@users.noreply.github.com> Date: Sat, 15 Jul 2023 13:28:38 -0400 Subject: [PATCH 05/12] Apply suggestions from code review Co-authored-by: Boris Zbarsky --- src/app/reporting/ReportScheduler.h | 2 +- src/app/tests/BUILD.gn | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/app/reporting/ReportScheduler.h b/src/app/reporting/ReportScheduler.h index 14685301a09c71..2b768a0ffea6ed 100644 --- a/src/app/reporting/ReportScheduler.h +++ b/src/app/reporting/ReportScheduler.h @@ -127,7 +127,7 @@ class ReportScheduler : public ReadHandler::Observer bool IsReportableNow(ReadHandler * aReadHandler) { return FindReadHandlerNode(aReadHandler)->IsReportableNow(); - }; // TODO: Change the IsReportableNow to IsReportable() for readHandlers + } // TODO: Change the IsReportableNow to IsReportable() for readHandlers /// @brief Check if a ReadHandler is reportable without considering the timing bool IsReadHandlerReportable(ReadHandler * aReadHandler) const { diff --git a/src/app/tests/BUILD.gn b/src/app/tests/BUILD.gn index a05edd6181dd16..0b49e0a657cf1d 100644 --- a/src/app/tests/BUILD.gn +++ b/src/app/tests/BUILD.gn @@ -178,8 +178,8 @@ chip_test_suite("tests") { } # The platform manager is not properly clearing queues in test teardown, which results in - # DrainIO calls to not being able to run in expected time (5seconds) if unprocesses engine - # run are remaining, causing tests to crash in Open IoT SDK and Zephyr tests since they are + # DrainIO calls not being able to run in expected time (5seconds) if unprocessed reported engine + # runs are remaining, causing tests to crash in Open IoT SDK and Zephyr tests since they are # running all tests in one file. We need to figure out how to properly clean the event queues # before enabling this test for these platforms. if (chip_device_platform != "nrfconnect" && From aa9611ac395fe9343a2b069dbafc48832c2b76c1 Mon Sep 17 00:00:00 2001 From: lpbeliveau-silabs Date: Mon, 17 Jul 2023 12:54:45 -0400 Subject: [PATCH 06/12] Applied comment review and refactoed next timeout calculation logic --- src/app/ReadHandler.h | 8 +- src/app/reporting/ReportScheduler.h | 13 +- src/app/reporting/ReportSchedulerImpl.cpp | 65 ++++---- src/app/reporting/ReportSchedulerImpl.h | 16 +- .../SynchronizedReportSchedulerImpl.cpp | 40 ++--- .../SynchronizedReportSchedulerImpl.h | 7 +- src/app/tests/TestReportScheduler.cpp | 143 ++++++++++++------ src/lib/format/tests/TestDecoding.cpp | 2 +- 8 files changed, 165 insertions(+), 129 deletions(-) diff --git a/src/app/ReadHandler.h b/src/app/ReadHandler.h index 000a57ea65ae0f..2e87451434f294 100644 --- a/src/app/ReadHandler.h +++ b/src/app/ReadHandler.h @@ -51,14 +51,8 @@ #include #include -#if CHIP_DEVICE_CONFIG_ENABLE_SED -#define SUBSCRIPTION_MAX_INTERVAL_PUBLISHER_LIMIT CHIP_DEVICE_CONFIG_SED_IDLE_INTERVAL -#else -#define SUBSCRIPTION_MAX_INTERVAL_PUBLISHER_LIMIT 3600 // 3600 seconds -#endif - // https://github.com/CHIP-Specifications/connectedhomeip-spec/blob/61a9d19e6af12fdfb0872bcff26d19de6c680a1a/src/Ch02_Architecture.adoc#1122-subscribe-interaction-limits -constexpr uint16_t kSubscriptionMaxIntervalPublisherLimit = SUBSCRIPTION_MAX_INTERVAL_PUBLISHER_LIMIT; +constexpr uint16_t kSubscriptionMaxIntervalPublisherLimit = 3600; namespace chip { namespace app { diff --git a/src/app/reporting/ReportScheduler.h b/src/app/reporting/ReportScheduler.h index 2b768a0ffea6ed..f51d78dae5e39e 100644 --- a/src/app/reporting/ReportScheduler.h +++ b/src/app/reporting/ReportScheduler.h @@ -81,6 +81,9 @@ class ReportScheduler : public ReadHandler::Observer (now >= mMinTimestamp && (mReadHandler->IsDirty() || now >= mMaxTimestamp || now >= mSyncTimestamp))); } + bool IsEnginRunScheduled() const { return mEnginRunScheduled; } + void SetEngineRunScheduled(bool aEnginRunScheduled) { mEnginRunScheduled = aEnginRunScheduled; } + void SetIntervalTimeStamps(ReadHandler * aReadHandler) { uint16_t minInterval, maxInterval; @@ -91,7 +94,11 @@ class ReportScheduler : public ReadHandler::Observer mSyncTimestamp = mMaxTimestamp; } - void RunCallback() { mCallback(); } + void RunCallback() + { + mCallback(); + SetEngineRunScheduled(true); + } void SetSyncTimestamp(System::Clock::Timestamp aSyncTimestamp) { @@ -112,6 +119,8 @@ class ReportScheduler : public ReadHandler::Observer Timestamp mMaxTimestamp; Timestamp mSyncTimestamp; // Timestamp at which the read handler will be allowed to emit a report so it can be synced with // other handlers that have an earlier max timestamp + bool mEnginRunScheduled = false; // Flag to indicate if the engine run is already scheduled so the scheduler can ignore + // it when calculating the next run time }; ReportScheduler(TimerDelegate * aTimerDelegate) : mTimerDelegate(aTimerDelegate) {} @@ -120,8 +129,6 @@ class ReportScheduler : public ReadHandler::Observer */ virtual ~ReportScheduler() = default; - /// @brief Check if a ReadHandler is scheduled for reporting - virtual bool IsReportScheduled(ReadHandler * aReadHandler) = 0; /// @brief Check whether a ReadHandler is reportable right now, taking into account its minimum and maximum intervals. /// @param aReadHandler read handler to check bool IsReportableNow(ReadHandler * aReadHandler) diff --git a/src/app/reporting/ReportSchedulerImpl.cpp b/src/app/reporting/ReportSchedulerImpl.cpp index be280323d5fa03..07875068d12cf1 100644 --- a/src/app/reporting/ReportSchedulerImpl.cpp +++ b/src/app/reporting/ReportSchedulerImpl.cpp @@ -40,7 +40,23 @@ ReportSchedulerImpl::ReportSchedulerImpl(TimerDelegate * aTimerDelegate) : Repor /// @brief When a ReadHandler is added, register it, which will schedule an engine run void ReportSchedulerImpl::OnReadHandlerCreated(ReadHandler * aReadHandler) { - RegisterReadHandler(aReadHandler); + ReadHandlerNode * newNode = FindReadHandlerNode(aReadHandler); + // Handler must not be registered yet; it's just being constructed. + VerifyOrDie(nullptr == newNode); + // The NodePool is the same size as the ReadHandler pool from the IM Engine, so we don't need a check for size here since if a + // ReadHandler was created, space should be available. + newNode = mNodesPool.CreateObject(aReadHandler, mTimerDelegate, [this]() { this->ReportTimerCallback(); }); + mReadHandlerList.PushBack(newNode); + + ChipLogProgress(DataManagement, + "Registered a ReadHandler that will schedule a report between system Timestamp: %" PRIu64 + " and system Timestamp %" PRIu64 ".", + newNode->GetMinTimestamp().count(), newNode->GetMaxTimestamp().count()); + + Milliseconds32 newTimeout; + // No need to check for error here, since the node is already in the list otherwise we would have Died + CalculateNextReportTimeout(newTimeout, newNode); + ScheduleReport(newTimeout, newNode); } /// @brief When a ReadHandler becomes reportable, schedule, verifies if the min interval of a handleris elapsed. If not, @@ -51,7 +67,7 @@ void ReportSchedulerImpl::OnBecameReportable(ReadHandler * aReadHandler) VerifyOrReturn(nullptr != node); Milliseconds32 newTimeout; - ReturnOnFailure(CalculateNextReportTimeout(newTimeout, node)); + CalculateNextReportTimeout(newTimeout, node); ScheduleReport(newTimeout, node); } @@ -59,39 +75,24 @@ void ReportSchedulerImpl::OnSubscriptionAction(ReadHandler * apReadHandler) { ReadHandlerNode * node = FindReadHandlerNode(apReadHandler); VerifyOrReturn(nullptr != node); - // Schedule callback for max interval by computing the difference between the max timestamp and the current timestamp node->SetIntervalTimeStamps(apReadHandler); Milliseconds32 newTimeout; - ReturnOnFailure(CalculateNextReportTimeout(newTimeout, node)); + CalculateNextReportTimeout(newTimeout, node); ScheduleReport(newTimeout, node); + node->SetEngineRunScheduled(false); } /// @brief When a ReadHandler is removed, unregister it, which will cancel any scheduled report void ReportSchedulerImpl::OnReadHandlerDestroyed(ReadHandler * aReadHandler) { - UnregisterReadHandler(aReadHandler); -} - -CHIP_ERROR ReportSchedulerImpl::RegisterReadHandler(ReadHandler * aReadHandler) -{ - ReadHandlerNode * newNode = FindReadHandlerNode(aReadHandler); - // Handler must not be registered yet; it's just being constructed. - VerifyOrDie(nullptr == newNode); - // The NodePool is the same size as the ReadHandler pool from the IM Engine, so we don't need a check for size here since if a - // ReadHandler was created, space should be available. - newNode = mNodesPool.CreateObject(aReadHandler, mTimerDelegate, [this]() { this->ReportTimerCallback(); }); - mReadHandlerList.PushBack(newNode); + CancelReport(aReadHandler); - ChipLogProgress(DataManagement, - "Registered a ReadHandler that will schedule a report between system Timestamp: %" PRIu64 - " and system Timestamp %" PRIu64 ".", - newNode->GetMinTimestamp().count(), newNode->GetMaxTimestamp().count()); + ReadHandlerNode * removeNode = FindReadHandlerNode(aReadHandler); + // Nothing to remove if the handler is not found in the list + VerifyOrReturn(nullptr != removeNode); - Milliseconds32 newTimeout; - // No need to check for error here, since the node is already in the list otherwise we would have Died - CalculateNextReportTimeout(newTimeout, newNode); - ReturnErrorOnFailure(ScheduleReport(newTimeout, newNode)); - return CHIP_NO_ERROR; + mReadHandlerList.Remove(removeNode); + mNodesPool.ReleaseObject(removeNode); } CHIP_ERROR ReportSchedulerImpl::ScheduleReport(Timeout timeout, ReadHandlerNode * node) @@ -110,24 +111,12 @@ void ReportSchedulerImpl::CancelReport(ReadHandler * aReadHandler) mTimerDelegate->CancelTimer(node); } -void ReportSchedulerImpl::UnregisterReadHandler(ReadHandler * aReadHandler) -{ - CancelReport(aReadHandler); - - ReadHandlerNode * removeNode = FindReadHandlerNode(aReadHandler); - // Nothing to remove if the handler is not found in the list - VerifyOrReturn(nullptr != removeNode); - - mReadHandlerList.Remove(removeNode); - mNodesPool.ReleaseObject(removeNode); -} - void ReportSchedulerImpl::UnregisterAllHandlers() { while (!mReadHandlerList.Empty()) { ReadHandler * firstReadHandler = mReadHandlerList.begin()->GetReadHandler(); - UnregisterReadHandler(firstReadHandler); + OnReadHandlerDestroyed(firstReadHandler); } } diff --git a/src/app/reporting/ReportSchedulerImpl.h b/src/app/reporting/ReportSchedulerImpl.h index 079164ce9c77aa..fd86580cf84ef7 100644 --- a/src/app/reporting/ReportSchedulerImpl.h +++ b/src/app/reporting/ReportSchedulerImpl.h @@ -24,29 +24,27 @@ namespace chip { namespace app { namespace reporting { -using Timeout = System::Clock::Timeout; - class ReportSchedulerImpl : public ReportScheduler { public: + using Timeout = System::Clock::Timeout; + ReportSchedulerImpl(TimerDelegate * aTimerDelegate); ~ReportSchedulerImpl() override { UnregisterAllHandlers(); } // ReadHandlerObserver - void OnReadHandlerCreated(ReadHandler * aReadHandler) override; - void OnBecameReportable(ReadHandler * aReadHandler) override; - void OnSubscriptionAction(ReadHandler * aReadHandler) override; + void OnReadHandlerCreated(ReadHandler * aReadHandler) final; + void OnBecameReportable(ReadHandler * aReadHandler) final; + void OnSubscriptionAction(ReadHandler * aReadHandler) final; void OnReadHandlerDestroyed(ReadHandler * aReadHandler) override; - bool IsReportScheduled(ReadHandler * aReadHandler) override; + bool IsReportScheduled(ReadHandler * aReadHandler); void ReportTimerCallback(); protected: - virtual CHIP_ERROR RegisterReadHandler(ReadHandler * aReadHandler); virtual CHIP_ERROR ScheduleReport(Timeout timeout, ReadHandlerNode * node); - virtual void CancelReport(ReadHandler * aReadHandler); - virtual void UnregisterReadHandler(ReadHandler * aReadHandler); + void CancelReport(ReadHandler * aReadHandler); virtual void UnregisterAllHandlers(); private: diff --git a/src/app/reporting/SynchronizedReportSchedulerImpl.cpp b/src/app/reporting/SynchronizedReportSchedulerImpl.cpp index 3333428f080f0f..920775cfe6cc94 100644 --- a/src/app/reporting/SynchronizedReportSchedulerImpl.cpp +++ b/src/app/reporting/SynchronizedReportSchedulerImpl.cpp @@ -26,23 +26,7 @@ namespace reporting { using namespace System::Clock; using ReadHandlerNode = ReportScheduler::ReadHandlerNode; -CHIP_ERROR SynchronizedReportSchedulerImpl::ScheduleReport(Timeout timeout, ReadHandlerNode * node) -{ - // Cancel Report if it is currently scheduled - mTimerDelegate->CancelTimer(this); - ReturnErrorOnFailure(mTimerDelegate->StartTimer(this, timeout)); - mTestNextReportTimestamp = mTimerDelegate->GetCurrentMonotonicTimestamp() + timeout; - - return CHIP_NO_ERROR; -} - -void SynchronizedReportSchedulerImpl::CancelReport(ReadHandler * aReadHandler) -{ - // We don't need to take action on the handler, since the timer is common here - mTimerDelegate->CancelTimer(this); -} - -void SynchronizedReportSchedulerImpl::UnregisterReadHandler(ReadHandler * aReadHandler) +void SynchronizedReportSchedulerImpl::OnReadHandlerDestroyed(ReadHandler * aReadHandler) { // Verify list is populated and handler is not null VerifyOrReturn((!mReadHandlerList.Empty() || (nullptr == aReadHandler))); @@ -57,10 +41,26 @@ void SynchronizedReportSchedulerImpl::UnregisterReadHandler(ReadHandler * aReadH if (mReadHandlerList.Empty()) { // Only cancel the timer if there are no more handlers registered - CancelReport(aReadHandler); + CancelReport(); } } +CHIP_ERROR SynchronizedReportSchedulerImpl::ScheduleReport(Timeout timeout, ReadHandlerNode * node) +{ + // Cancel Report if it is currently scheduled + mTimerDelegate->CancelTimer(this); + ReturnErrorOnFailure(mTimerDelegate->StartTimer(this, timeout)); + mTestNextReportTimestamp = mTimerDelegate->GetCurrentMonotonicTimestamp() + timeout; + + return CHIP_NO_ERROR; +} + +void SynchronizedReportSchedulerImpl::CancelReport() +{ + // We don't need to take action on the handler, since the timer is common here + mTimerDelegate->CancelTimer(this); +} + /// @brief Checks if the timer is active for the given ReadHandler. Since all read handlers are scheduled on the same timer, we /// check if the node is in the list and if the timer is active for the ReportScheduler bool SynchronizedReportSchedulerImpl::IsReportScheduled(ReadHandler * aReadHandler) @@ -76,7 +76,7 @@ CHIP_ERROR SynchronizedReportSchedulerImpl::FindNextMaxInterval() { VerifyOrReturnError(!mReadHandlerList.Empty(), CHIP_ERROR_INVALID_LIST_LENGTH); System::Clock::Timestamp now = mTimerDelegate->GetCurrentMonotonicTimestamp(); - System::Clock::Timestamp earliest = now + Seconds16(kSubscriptionMaxIntervalPublisherLimit); + System::Clock::Timestamp earliest = now + Seconds16::max(); for (auto & iter : mReadHandlerList) { @@ -102,7 +102,7 @@ CHIP_ERROR SynchronizedReportSchedulerImpl::FindNextMinInterval() for (auto & iter : mReadHandlerList) { - if (iter.GetMinTimestamp() > latest) + if (iter.GetMinTimestamp() > latest && IsReadHandlerReportable(iter.GetReadHandler())) { // We do not want the new min to be set above the max for any handler if (iter.GetMinTimestamp() <= mNextMaxTimestamp) diff --git a/src/app/reporting/SynchronizedReportSchedulerImpl.h b/src/app/reporting/SynchronizedReportSchedulerImpl.h index 8d9b659a92b889..3e4d9e1cd0fb14 100644 --- a/src/app/reporting/SynchronizedReportSchedulerImpl.h +++ b/src/app/reporting/SynchronizedReportSchedulerImpl.h @@ -33,17 +33,18 @@ using TimerDelegate = ReportScheduler::TimerDelegate; class SynchronizedReportSchedulerImpl : public ReportSchedulerImpl { public: + void OnReadHandlerDestroyed(ReadHandler * aReadHandler) override; + SynchronizedReportSchedulerImpl(TimerDelegate * aTimerDelegate) : ReportSchedulerImpl(aTimerDelegate) {} ~SynchronizedReportSchedulerImpl() {} - bool IsReportScheduled(ReadHandler * aReadHandler) override; + bool IsReportScheduled(ReadHandler * aReadHandler); void ReportTimerCallback(); protected: CHIP_ERROR ScheduleReport(System::Clock::Timeout timeout, ReadHandlerNode * node) override; - void CancelReport(ReadHandler * aReadHandler) override; - void UnregisterReadHandler(ReadHandler * aReadHandler) override; + void CancelReport(); private: friend class chip::app::reporting::TestReportScheduler; diff --git a/src/app/tests/TestReportScheduler.cpp b/src/app/tests/TestReportScheduler.cpp index 9cd386e001df44..31a431e6f07731 100644 --- a/src/app/tests/TestReportScheduler.cpp +++ b/src/app/tests/TestReportScheduler.cpp @@ -259,7 +259,8 @@ class TestReportScheduler readHandlerPool.CreateObject(nullCallback, exchangeCtx, ReadHandler::InteractionType::Subscribe); NL_TEST_ASSERT(aSuite, nullptr != readHandler); VerifyOrReturn(nullptr != readHandler); - NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == sScheduler.RegisterReadHandler(readHandler)); + // Register ReadHandler using callback method + sScheduler.OnReadHandlerCreated(readHandler); NL_TEST_ASSERT(aSuite, nullptr != sScheduler.FindReadHandlerNode(readHandler)); } @@ -269,7 +270,7 @@ class TestReportScheduler // Test unregister first ReadHandler ReadHandler * firstReadHandler = sScheduler.mReadHandlerList.begin()->GetReadHandler(); - sScheduler.UnregisterReadHandler(firstReadHandler); + sScheduler.OnReadHandlerDestroyed(firstReadHandler); NL_TEST_ASSERT(aSuite, sScheduler.GetNumReadHandlers() == kNumMaxReadHandlers - 1); NL_TEST_ASSERT(aSuite, nullptr == sScheduler.FindReadHandlerNode(firstReadHandler)); @@ -280,7 +281,7 @@ class TestReportScheduler iter++; } ReadHandler * middleReadHandler = iter->GetReadHandler(); - sScheduler.UnregisterReadHandler(middleReadHandler); + sScheduler.OnReadHandlerDestroyed(middleReadHandler); NL_TEST_ASSERT(aSuite, sScheduler.GetNumReadHandlers() == kNumMaxReadHandlers - 2); NL_TEST_ASSERT(aSuite, nullptr == sScheduler.FindReadHandlerNode(middleReadHandler)); @@ -288,7 +289,7 @@ class TestReportScheduler iter = sScheduler.mReadHandlerList.end(); iter--; ReadHandler * lastReadHandler = iter->GetReadHandler(); - sScheduler.UnregisterReadHandler(lastReadHandler); + sScheduler.OnReadHandlerDestroyed(lastReadHandler); NL_TEST_ASSERT(aSuite, sScheduler.GetNumReadHandlers() == kNumMaxReadHandlers - 3); NL_TEST_ASSERT(aSuite, nullptr == sScheduler.FindReadHandlerNode(lastReadHandler)); @@ -324,8 +325,9 @@ class TestReportScheduler NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == readHandler1->SetMaxReportingInterval(2)); NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == readHandler1->SetMinReportingIntervalForTests(1)); // Do those manually to avoid scheduling an engine run - readHandler1->mFlags.Set(ReadHandler::ReadHandlerFlags::ForceDirty, true); + readHandler1->ForceDirtyState(); readHandler1->mState = ReadHandler::HandlerState::GeneratingReports; + sScheduler.OnReadHandlerCreated(readHandler1); // Clean read handler, will be triggered at max interval ReadHandler * readHandler2 = @@ -334,6 +336,7 @@ class TestReportScheduler NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == readHandler2->SetMinReportingIntervalForTests(0)); // Do those manually to avoid scheduling an engine run readHandler2->mState = ReadHandler::HandlerState::GeneratingReports; + sScheduler.OnReadHandlerCreated(readHandler2); // Clean read handler, will be triggered at max interval, but will be cancelled before ReadHandler * readHandler3 = @@ -342,10 +345,7 @@ class TestReportScheduler NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == readHandler3->SetMinReportingIntervalForTests(0)); // Do those manually to avoid scheduling an engine run readHandler3->mState = ReadHandler::HandlerState::GeneratingReports; - - NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == sScheduler.RegisterReadHandler(readHandler1)); - NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == sScheduler.RegisterReadHandler(readHandler2)); - NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == sScheduler.RegisterReadHandler(readHandler3)); + sScheduler.OnReadHandlerCreated(readHandler3); // Confirms that none of the ReadHandlers are currently reportable NL_TEST_ASSERT(aSuite, !sScheduler.IsReportableNow(readHandler1)); @@ -412,7 +412,7 @@ class TestReportScheduler NL_TEST_ASSERT(aSuite, node->GetReadHandler() == readHandler); // Test OnBecameReportable - readHandler->mFlags.Set(ReadHandler::ReadHandlerFlags::ForceDirty, true); + readHandler->ForceDirtyState(); readHandler->mObserver->OnBecameReportable(readHandler); // Should have changed the scheduled timeout to the handler's min interval, to check, we wait for the min interval to // expire @@ -423,7 +423,7 @@ class TestReportScheduler NL_TEST_ASSERT(aSuite, !sScheduler.IsReportScheduled(readHandler)); // Test OnSubscriptionAction - readHandler->mFlags.Set(ReadHandler::ReadHandlerFlags::ForceDirty, false); + readHandler->ClearForceDirtyFlag(); readHandler->mObserver->OnSubscriptionAction(readHandler); // Should have changed the scheduled timeout to the handlers max interval, to check, we wait for the min interval to // confirm it is not expired yet so the report should still be scheduled @@ -447,7 +447,7 @@ class TestReportScheduler NL_TEST_ASSERT(aSuite, sScheduler.GetNumReadHandlers() == 0); NL_TEST_ASSERT(aSuite, nullptr == sScheduler.FindReadHandlerNode(readHandler)); - sScheduler.UnregisterReadHandler(readHandler); + sScheduler.OnReadHandlerDestroyed(readHandler); readHandlerPool.ReleaseAll(); exchangeCtx->Close(); NL_TEST_ASSERT(aSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 0); @@ -495,8 +495,8 @@ class TestReportScheduler // Validates that the lowest max is selected as the common max timestamp NL_TEST_ASSERT(aSuite, syncScheduler.mNextMaxTimestamp == node1->GetMaxTimestamp()); - // Validates that the highest min is selected as the common min interval - NL_TEST_ASSERT(aSuite, syncScheduler.mNextMinTimestamp == node2->GetMinTimestamp()); + // Validates that the highest reportable min is selected as the common min interval (0 here) + NL_TEST_ASSERT(aSuite, syncScheduler.mNextMinTimestamp == node1->GetMinTimestamp()); // Validates that the next report emission is scheduled on the common max timestamp NL_TEST_ASSERT(aSuite, syncScheduler.mTestNextReportTimestamp == syncScheduler.mNextMaxTimestamp); @@ -508,7 +508,7 @@ class TestReportScheduler NL_TEST_ASSERT(aSuite, syncScheduler.IsReportableNow(readHandler1)); // Confirm timeout has expired and no report is scheduled, an engine run would typically happen here NL_TEST_ASSERT(aSuite, !sScheduler.IsReportScheduled(readHandler1)); - NL_TEST_ASSERT(aSuite, !sScheduler.IsReportScheduled(readHandler1)); + NL_TEST_ASSERT(aSuite, !sScheduler.IsReportScheduled(readHandler2)); // Simulate that the OnBecameReportable callback was called on readHandler1 readHandler1->mObserver->OnBecameReportable(readHandler1); @@ -526,39 +526,69 @@ class TestReportScheduler NL_TEST_ASSERT(aSuite, syncScheduler.mTestNextReportTimestamp == node1->GetMaxTimestamp()); // Confirm behavior when a read handler becomes dirty - readHandler1->ForceDirtyState(); - // since the min interval on readHandler1 is 0, it should be reportable now - NL_TEST_ASSERT(aSuite, syncScheduler.IsReportableNow(readHandler1)); - readHandler1->mObserver->OnBecameReportable(readHandler1); - // Confirm that the next report emission is scheduled on the min timestamp of readHandler2 as it is the highest - NL_TEST_ASSERT(aSuite, syncScheduler.mTestNextReportTimestamp == node2->GetMinTimestamp()); + readHandler2->ForceDirtyState(); + NL_TEST_ASSERT(aSuite, !syncScheduler.IsReportableNow(readHandler2)); + NL_TEST_ASSERT(aSuite, !syncScheduler.IsReportableNow(readHandler1)); // Simulate wait enough for min timestamp of readHandler2 to be reached (1s) // Increment the mock system time to the max to the time waited sTestTimerSynchronizedDelegate.IncrementMockTimestamp(System::Clock::Milliseconds64(1000)); - // Confirm that readHandler2 is now reportable NL_TEST_ASSERT(aSuite, syncScheduler.IsReportableNow(readHandler2)); readHandler2->mObserver->OnBecameReportable(readHandler2); - readHandler1->ClearForceDirtyFlag(); + // since the min interval on readHandler1 is 0, it should also be reportable now by sync mechanism + NL_TEST_ASSERT(aSuite, syncScheduler.IsReportableNow(readHandler1)); + NL_TEST_ASSERT(aSuite, node1->GetSyncTimestamp() == node2->GetMinTimestamp()); + + // Confirm that the next report emission is scheduled on the min timestamp of readHandler2 as it is the highest reportable + NL_TEST_ASSERT(aSuite, syncScheduler.mTestNextReportTimestamp == node2->GetMinTimestamp()); + // Simulate a report emission for readHandler1 readHandler1->mObserver->OnSubscriptionAction(readHandler1); NL_TEST_ASSERT(aSuite, !syncScheduler.IsReportableNow(readHandler1)); - // ReadHandler 2 should still be reportable from the sync mechanism + // ReadHandler 2 should still be reportable since it hasn't emitted a report yet NL_TEST_ASSERT(aSuite, syncScheduler.IsReportableNow(readHandler2)); - readHandler1->mObserver->OnSubscriptionAction(readHandler2); + readHandler2->ClearForceDirtyFlag(); + readHandler2->mObserver->OnSubscriptionAction(readHandler2); NL_TEST_ASSERT(aSuite, !syncScheduler.IsReportableNow(readHandler2)); // Validate next report scheduled on the max timestamp of readHandler1 NL_TEST_ASSERT(aSuite, node1->GetMaxTimestamp() > sTestTimerSynchronizedDelegate.GetCurrentMonotonicTimestamp()); NL_TEST_ASSERT(aSuite, syncScheduler.mTestNextReportTimestamp == node1->GetMaxTimestamp()); + // Simulate readHandler1 becoming dirty after less than 1 seconds + sTestTimerSynchronizedDelegate.IncrementMockTimestamp(System::Clock::Milliseconds64(900)); + readHandler1->ForceDirtyState(); + NL_TEST_ASSERT(aSuite, syncScheduler.IsReportableNow(readHandler1)); + NL_TEST_ASSERT(aSuite, !syncScheduler.IsReportableNow(readHandler2)); + readHandler1->mObserver->OnBecameReportable(readHandler1); + + // Validate next report scheduled on the min timestamp of readHandler1 (readHandler 2 is not currently reportable) + NL_TEST_ASSERT(aSuite, + syncScheduler.mTestNextReportTimestamp == sTestTimerSynchronizedDelegate.GetCurrentMonotonicTimestamp()); + // Simulate a report emission for readHandler1 + readHandler1->ClearForceDirtyFlag(); + readHandler1->mObserver->OnSubscriptionAction(readHandler1); + NL_TEST_ASSERT(aSuite, !syncScheduler.IsReportableNow(readHandler1)); + NL_TEST_ASSERT(aSuite, !syncScheduler.IsReportableNow(readHandler2)); + + // The next report should be scheduler on the max timestamp of readHandler1 and not readHandler2 should be synced + NL_TEST_ASSERT(aSuite, syncScheduler.mTestNextReportTimestamp == node1->GetMaxTimestamp()); + NL_TEST_ASSERT(aSuite, node2->GetSyncTimestamp() == node1->GetMaxTimestamp()); + + sTestTimerSynchronizedDelegate.IncrementMockTimestamp(System::Clock::Milliseconds64(2000)); + NL_TEST_ASSERT(aSuite, syncScheduler.IsReportableNow(readHandler1)); + NL_TEST_ASSERT(aSuite, syncScheduler.IsReportableNow(readHandler2)); + readHandler1->mObserver->OnSubscriptionAction(readHandler1); + readHandler2->mObserver->OnSubscriptionAction(readHandler2); + NL_TEST_ASSERT(aSuite, !syncScheduler.IsReportableNow(readHandler1)); + NL_TEST_ASSERT(aSuite, !syncScheduler.IsReportableNow(readHandler2)); + NL_TEST_ASSERT(aSuite, syncScheduler.mTestNextReportTimestamp == node1->GetMaxTimestamp()); + // Simulate a new ReadHandler being added with a min forcing a conflict // Wait for 1 second, nothing should happen here - ctx.GetIOContext().DriveIOUntil(System::Clock::Milliseconds32(1000), - [&]() -> bool { return syncScheduler.IsReportableNow(readHandler1); }); // Increment the mock system time to the max to the time waited sTestTimerSynchronizedDelegate.IncrementMockTimestamp(System::Clock::Milliseconds64(1000)); @@ -580,8 +610,6 @@ class TestReportScheduler // The min timestamp should also not have changed since the min of readhandler3 is higher than the current max NL_TEST_ASSERT(aSuite, syncScheduler.mNextMinTimestamp == node2->GetMinTimestamp()); - ctx.GetIOContext().DriveIOUntil(System::Clock::Milliseconds32(1100), - [&]() -> bool { return syncScheduler.IsReportableNow(readHandler1); }); // Increment the mock system time to the max to the time waited sTestTimerSynchronizedDelegate.IncrementMockTimestamp(System::Clock::Milliseconds64(1000)); @@ -600,13 +628,10 @@ class TestReportScheduler // Confirm that next report is scheduled on the max timestamp of readHandler3 and other 2 readHandlers are synced NL_TEST_ASSERT(aSuite, syncScheduler.mTestNextReportTimestamp == node3->GetMaxTimestamp()); - NL_TEST_ASSERT(aSuite, syncScheduler.mNextMinTimestamp == node3->GetMinTimestamp()); NL_TEST_ASSERT(aSuite, node1->GetSyncTimestamp() == node3->GetMaxTimestamp()); NL_TEST_ASSERT(aSuite, node2->GetSyncTimestamp() == node3->GetMaxTimestamp()); // Simulate a report emission for all readHandlers on the max timestamp of readHandler3 on confirm everything is synced - ctx.GetIOContext().DriveIOUntil(System::Clock::Milliseconds32(2000), - [&]() -> bool { return syncScheduler.IsReportableNow(readHandler3); }); // Increment the mock system time to the max to the time waited sTestTimerSynchronizedDelegate.IncrementMockTimestamp(System::Clock::Milliseconds64(2000)); @@ -624,7 +649,6 @@ class TestReportScheduler NL_TEST_ASSERT(aSuite, !syncScheduler.IsReportableNow(readHandler2)); NL_TEST_ASSERT(aSuite, !syncScheduler.IsReportableNow(readHandler3)); NL_TEST_ASSERT(aSuite, syncScheduler.mTestNextReportTimestamp == node1->GetMaxTimestamp()); - NL_TEST_ASSERT(aSuite, syncScheduler.mNextMinTimestamp == node3->GetMinTimestamp()); NL_TEST_ASSERT(aSuite, node1->GetSyncTimestamp() == node1->GetMaxTimestamp()); NL_TEST_ASSERT(aSuite, node2->GetSyncTimestamp() == node1->GetMaxTimestamp()); @@ -647,8 +671,6 @@ class TestReportScheduler NL_TEST_ASSERT(aSuite, node2->GetSyncTimestamp() == node4->GetMaxTimestamp()); NL_TEST_ASSERT(aSuite, node3->GetSyncTimestamp() == node1->GetMaxTimestamp()); - ctx.GetIOContext().DriveIOUntil(System::Clock::Milliseconds32(1100), - [&]() -> bool { return syncScheduler.IsReportableNow(readHandler4); }); // Increment the mock system time to the max to the time waited sTestTimerSynchronizedDelegate.IncrementMockTimestamp(System::Clock::Milliseconds64(1100)); @@ -667,8 +689,6 @@ class TestReportScheduler NL_TEST_ASSERT(aSuite, !syncScheduler.IsReportableNow(readHandler2)); NL_TEST_ASSERT(aSuite, !syncScheduler.IsReportableNow(readHandler4)); - ctx.GetIOContext().DriveIOUntil(System::Clock::Milliseconds32(1000), - [&]() -> bool { return syncScheduler.IsReportableNow(readHandler3); }); // Increment the mock system time to the max to the time waited sTestTimerSynchronizedDelegate.IncrementMockTimestamp(System::Clock::Milliseconds64(1000)); @@ -677,14 +697,14 @@ class TestReportScheduler NL_TEST_ASSERT(aSuite, syncScheduler.IsReportableNow(readHandler2)); NL_TEST_ASSERT(aSuite, syncScheduler.IsReportableNow(readHandler3)); NL_TEST_ASSERT(aSuite, syncScheduler.IsReportableNow(readHandler4)); - readHandler4->mObserver->OnBecameReportable(readHandler1); - readHandler4->mObserver->OnBecameReportable(readHandler2); - readHandler4->mObserver->OnBecameReportable(readHandler3); - readHandler4->mObserver->OnBecameReportable(readHandler4); - readHandler4->mObserver->OnSubscriptionAction(readHandler1); - readHandler4->mObserver->OnSubscriptionAction(readHandler2); - readHandler4->mObserver->OnSubscriptionAction(readHandler3); - readHandler4->mObserver->OnSubscriptionAction(readHandler4); + syncScheduler.OnBecameReportable(readHandler1); + syncScheduler.OnBecameReportable(readHandler2); + syncScheduler.OnBecameReportable(readHandler3); + syncScheduler.OnBecameReportable(readHandler4); + syncScheduler.OnSubscriptionAction(readHandler1); + syncScheduler.OnSubscriptionAction(readHandler2); + syncScheduler.OnSubscriptionAction(readHandler3); + syncScheduler.OnSubscriptionAction(readHandler4); NL_TEST_ASSERT(aSuite, !syncScheduler.IsReportableNow(readHandler1)); NL_TEST_ASSERT(aSuite, !syncScheduler.IsReportableNow(readHandler2)); NL_TEST_ASSERT(aSuite, !syncScheduler.IsReportableNow(readHandler3)); @@ -697,17 +717,44 @@ class TestReportScheduler NL_TEST_ASSERT(aSuite, node2->GetSyncTimestamp() == node4->GetMaxTimestamp()); NL_TEST_ASSERT(aSuite, node3->GetSyncTimestamp() == node1->GetMaxTimestamp()); - ctx.GetIOContext().DriveIOUntil(System::Clock::Milliseconds32(1000), - [&]() -> bool { return syncScheduler.IsReportableNow(readHandler4); }); // Increment the mock system time to the max to the time waited sTestTimerSynchronizedDelegate.IncrementMockTimestamp(System::Clock::Milliseconds64(1000)); - // Confirm readHandler 1-2-4 are reportable and handler 3 is not because of it min interval + // Confirm readHandler 1-2-4 are reportable and handler 3 is not because of its min interval NL_TEST_ASSERT(aSuite, syncScheduler.IsReportableNow(readHandler1)); NL_TEST_ASSERT(aSuite, syncScheduler.IsReportableNow(readHandler2)); NL_TEST_ASSERT(aSuite, !syncScheduler.IsReportableNow(readHandler3)); NL_TEST_ASSERT(aSuite, syncScheduler.IsReportableNow(readHandler4)); + syncScheduler.OnReadHandlerDestroyed(readHandler1); + syncScheduler.OnReadHandlerDestroyed(readHandler2); + syncScheduler.OnReadHandlerDestroyed(readHandler3); + syncScheduler.OnReadHandlerDestroyed(readHandler4); + + // Reset all handlers + // Test case: Scheduler 1 and 2 are reportable but min2 > max1, they should sync only when possible (min2 = 3, max1 = 2) + NL_TEST_ASSERT(aSuite, syncScheduler.GetNumReadHandlers() == 0); + + NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == readHandler1->SetMaxReportingInterval(2)); + NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == readHandler1->SetMinReportingIntervalForTests(0)); + syncScheduler.OnReadHandlerCreated(readHandler1); + readHandler1->ForceDirtyState(); + syncScheduler.OnBecameReportable(readHandler1); + NL_TEST_ASSERT(aSuite, syncScheduler.IsReportableNow(readHandler1)); + + NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == readHandler2->SetMaxReportingInterval(4)); + NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == readHandler2->SetMinReportingIntervalForTests(3)); + syncScheduler.OnReadHandlerCreated(readHandler2); + readHandler2->ForceDirtyState(); + syncScheduler.OnBecameReportable(readHandler2); + NL_TEST_ASSERT(aSuite, !syncScheduler.IsReportableNow(readHandler2)); + + NL_TEST_ASSERT(aSuite, syncScheduler.mTestNextReportTimestamp == node1->GetMaxTimestamp()); + NL_TEST_ASSERT(aSuite, node2->GetSyncTimestamp() == node2->GetMinTimestamp()); + + // Increment the mock system time to the max to the time waited + sTestTimerSynchronizedDelegate.IncrementMockTimestamp(System::Clock::Milliseconds64(2000)); + syncScheduler.UnregisterAllHandlers(); readHandlerPool.ReleaseAll(); exchangeCtx->Close(); diff --git a/src/lib/format/tests/TestDecoding.cpp b/src/lib/format/tests/TestDecoding.cpp index ecf2b51db3adeb..f9ef31719f5b6b 100644 --- a/src/lib/format/tests/TestDecoding.cpp +++ b/src/lib/format/tests/TestDecoding.cpp @@ -34,7 +34,7 @@ using namespace chip::TLV; using namespace chip::TLVMeta; using namespace chip::TestData; -const Entry _empty_item[] = {}; +const Entry _empty_item[0] = {}; const std::array, 1> empty_meta = { { { 0, _empty_item } } }; const Entry _FakeProtocolData[] = { From 35f7d41be0ab0299e10263147ddddf9843e649b9 Mon Sep 17 00:00:00 2001 From: lpbeliveau-silabs Date: Mon, 17 Jul 2023 21:49:47 -0400 Subject: [PATCH 07/12] Completed unit test and logic --- src/app/reporting/ReportScheduler.h | 2 +- .../SynchronizedReportSchedulerImpl.cpp | 39 ++++++--- .../SynchronizedReportSchedulerImpl.h | 2 +- src/app/tests/TestReportScheduler.cpp | 84 ++++++++++++++----- 4 files changed, 95 insertions(+), 32 deletions(-) diff --git a/src/app/reporting/ReportScheduler.h b/src/app/reporting/ReportScheduler.h index f51d78dae5e39e..20e5fd63be9747 100644 --- a/src/app/reporting/ReportScheduler.h +++ b/src/app/reporting/ReportScheduler.h @@ -81,7 +81,7 @@ class ReportScheduler : public ReadHandler::Observer (now >= mMinTimestamp && (mReadHandler->IsDirty() || now >= mMaxTimestamp || now >= mSyncTimestamp))); } - bool IsEnginRunScheduled() const { return mEnginRunScheduled; } + bool IsEngineRunScheduled() const { return mEnginRunScheduled; } void SetEngineRunScheduled(bool aEnginRunScheduled) { mEnginRunScheduled = aEnginRunScheduled; } void SetIntervalTimeStamps(ReadHandler * aReadHandler) diff --git a/src/app/reporting/SynchronizedReportSchedulerImpl.cpp b/src/app/reporting/SynchronizedReportSchedulerImpl.cpp index 920775cfe6cc94..b56f9ff032fa1a 100644 --- a/src/app/reporting/SynchronizedReportSchedulerImpl.cpp +++ b/src/app/reporting/SynchronizedReportSchedulerImpl.cpp @@ -63,10 +63,8 @@ void SynchronizedReportSchedulerImpl::CancelReport() /// @brief Checks if the timer is active for the given ReadHandler. Since all read handlers are scheduled on the same timer, we /// check if the node is in the list and if the timer is active for the ReportScheduler -bool SynchronizedReportSchedulerImpl::IsReportScheduled(ReadHandler * aReadHandler) +bool SynchronizedReportSchedulerImpl::IsReportScheduled() { - ReadHandlerNode * node = FindReadHandlerNode(aReadHandler); - VerifyOrReturnValue(nullptr != node, false); return mTimerDelegate->IsTimerActive(this); } @@ -122,20 +120,38 @@ CHIP_ERROR SynchronizedReportSchedulerImpl::CalculateNextReportTimeout(Timeout & VerifyOrReturnError(mReadHandlerList.Contains(aNode), CHIP_ERROR_INVALID_ARGUMENT); ReturnErrorOnFailure(FindNextMaxInterval()); ReturnErrorOnFailure(FindNextMinInterval()); + bool reportableNow = false; + bool reportableAtMin = false; Timestamp now = mTimerDelegate->GetCurrentMonotonicTimestamp(); - // Find out if any handler is reportable now - if (IsReadHandlerReportable(aNode->GetReadHandler())) + + for (auto & iter : mReadHandlerList) { - if (now > mNextMinTimestamp) - { - timeout = Milliseconds32(0); - } - else + if (!iter.IsEngineRunScheduled()) { - timeout = mNextMinTimestamp - now; + if (iter.IsReportableNow()) + { + reportableNow = true; + break; + } + + if (IsReadHandlerReportable(iter.GetReadHandler()) && iter.GetMinTimestamp() <= mNextMaxTimestamp) + { + reportableAtMin = true; + } } } + + // Find out if any handler is reportable now + + if (reportableNow) + { + timeout = Milliseconds32(0); + } + else if (reportableAtMin) + { + timeout = mNextMinTimestamp - now; + } else { timeout = mNextMaxTimestamp - now; @@ -167,6 +183,7 @@ void SynchronizedReportSchedulerImpl::ReportTimerCallback() { if (iter.IsReportableNow()) { + iter.SetEngineRunScheduled(true); ChipLogProgress(DataManagement, "Handler: %p with min: %" PRIu64 " and max: %" PRIu64 " and sync: %" PRIu64, (&iter), iter.GetMinTimestamp().count(), iter.GetMaxTimestamp().count(), iter.GetSyncTimestamp().count()); } diff --git a/src/app/reporting/SynchronizedReportSchedulerImpl.h b/src/app/reporting/SynchronizedReportSchedulerImpl.h index 3e4d9e1cd0fb14..cce4525f59a9bc 100644 --- a/src/app/reporting/SynchronizedReportSchedulerImpl.h +++ b/src/app/reporting/SynchronizedReportSchedulerImpl.h @@ -38,7 +38,7 @@ class SynchronizedReportSchedulerImpl : public ReportSchedulerImpl SynchronizedReportSchedulerImpl(TimerDelegate * aTimerDelegate) : ReportSchedulerImpl(aTimerDelegate) {} ~SynchronizedReportSchedulerImpl() {} - bool IsReportScheduled(ReadHandler * aReadHandler); + bool IsReportScheduled(); void ReportTimerCallback(); diff --git a/src/app/tests/TestReportScheduler.cpp b/src/app/tests/TestReportScheduler.cpp index 31a431e6f07731..5c9e5a84d50772 100644 --- a/src/app/tests/TestReportScheduler.cpp +++ b/src/app/tests/TestReportScheduler.cpp @@ -160,7 +160,8 @@ class TestTimerDelegate : public ReportScheduler::TimerDelegate void SetMockSystemTimestamp(System::Clock::Timestamp aMockTimestamp) { mMockSystemTimestamp = aMockTimestamp; } - // Increment the mock timestamp one milisecond at a time for a total of aTime miliseconds. Checks if + // Increment the mock timestamp by aTime and call callbacks for timers that have expired. Checks if the timeout expired after + // incrementing void IncrementMockTimestamp(System::Clock::Milliseconds64 aTime) { mMockSystemTimestamp = mMockSystemTimestamp + aTime; @@ -213,7 +214,8 @@ class TestTimerSynchronizedDelegate : public ReportScheduler::TimerDelegate void SetMockSystemTimestamp(System::Clock::Timestamp aMockTimestamp) { mMockSystemTimestamp = aMockTimestamp; } - // Increment the mock timestamp one milisecond at a time for a total of aTime miliseconds. Checks if + // Increment the mock timestamp one milisecond at a time for a total of aTime miliseconds. Checks if the timeout expired when + // incrementing void IncrementMockTimestamp(System::Clock::Milliseconds64 aTime) { for (System::Clock::Milliseconds64 i = System::Clock::Milliseconds64(0); i < aTime; i++) @@ -224,6 +226,14 @@ class TestTimerSynchronizedDelegate : public ReportScheduler::TimerDelegate TimerCallbackInterface(nullptr, mSyncScheduler); } } + + if (aTime == System::Clock::Milliseconds64(0)) + { + if (mMockSystemTimestamp == mTimerTimeout) + { + TimerCallbackInterface(nullptr, mSyncScheduler); + } + } } SynchronizedReportSchedulerImpl * mSyncScheduler = nullptr; @@ -325,9 +335,9 @@ class TestReportScheduler NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == readHandler1->SetMaxReportingInterval(2)); NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == readHandler1->SetMinReportingIntervalForTests(1)); // Do those manually to avoid scheduling an engine run - readHandler1->ForceDirtyState(); readHandler1->mState = ReadHandler::HandlerState::GeneratingReports; sScheduler.OnReadHandlerCreated(readHandler1); + readHandler1->ForceDirtyState(); // Clean read handler, will be triggered at max interval ReadHandler * readHandler2 = @@ -489,9 +499,8 @@ class TestReportScheduler ReadHandlerNode * node1 = syncScheduler.FindReadHandlerNode(readHandler1); ReadHandlerNode * node2 = syncScheduler.FindReadHandlerNode(readHandler2); - // Confirm that a report emission is scheduled and that it will happen on readHandler1 max timestamp - NL_TEST_ASSERT(aSuite, syncScheduler.IsReportScheduled(readHandler1)); - NL_TEST_ASSERT(aSuite, syncScheduler.IsReportScheduled(readHandler2)); + // Confirm that a report emission is scheduled + NL_TEST_ASSERT(aSuite, syncScheduler.IsReportScheduled()); // Validates that the lowest max is selected as the common max timestamp NL_TEST_ASSERT(aSuite, syncScheduler.mNextMaxTimestamp == node1->GetMaxTimestamp()); @@ -501,11 +510,11 @@ class TestReportScheduler NL_TEST_ASSERT(aSuite, syncScheduler.mTestNextReportTimestamp == syncScheduler.mNextMaxTimestamp); // Simulate waiting for the max interval to expire (2s) - // Increment the mock system time to the max to the time waited sTestTimerSynchronizedDelegate.IncrementMockTimestamp(System::Clock::Milliseconds64(2000)); // Confirm that both handlers are now reportable since the timer has expired NL_TEST_ASSERT(aSuite, syncScheduler.IsReportableNow(readHandler1)); + NL_TEST_ASSERT(aSuite, syncScheduler.IsReportableNow(readHandler2)); // Confirm timeout has expired and no report is scheduled, an engine run would typically happen here NL_TEST_ASSERT(aSuite, !sScheduler.IsReportScheduled(readHandler1)); NL_TEST_ASSERT(aSuite, !sScheduler.IsReportScheduled(readHandler2)); @@ -530,12 +539,19 @@ class TestReportScheduler NL_TEST_ASSERT(aSuite, !syncScheduler.IsReportableNow(readHandler2)); NL_TEST_ASSERT(aSuite, !syncScheduler.IsReportableNow(readHandler1)); // Simulate wait enough for min timestamp of readHandler2 to be reached (1s) - // Increment the mock system time to the max to the time waited sTestTimerSynchronizedDelegate.IncrementMockTimestamp(System::Clock::Milliseconds64(1000)); NL_TEST_ASSERT(aSuite, syncScheduler.IsReportableNow(readHandler2)); + NL_TEST_ASSERT(aSuite, !syncScheduler.IsReportableNow(readHandler1)); readHandler2->mObserver->OnBecameReportable(readHandler2); + // confirm report scheduled now + NL_TEST_ASSERT(aSuite, + syncScheduler.mTestNextReportTimestamp == sTestTimerSynchronizedDelegate.GetCurrentMonotonicTimestamp()); + // Increment the timestamp by 0 here to trigger an engine run as the mock timer is only calling the timeout callback if we + // increment the mock timestamp + sTestTimerSynchronizedDelegate.IncrementMockTimestamp(System::Clock::Milliseconds64(0)); + // since the min interval on readHandler1 is 0, it should also be reportable now by sync mechanism NL_TEST_ASSERT(aSuite, syncScheduler.IsReportableNow(readHandler1)); NL_TEST_ASSERT(aSuite, node1->GetSyncTimestamp() == node2->GetMinTimestamp()); @@ -573,7 +589,7 @@ class TestReportScheduler NL_TEST_ASSERT(aSuite, !syncScheduler.IsReportableNow(readHandler1)); NL_TEST_ASSERT(aSuite, !syncScheduler.IsReportableNow(readHandler2)); - // The next report should be scheduler on the max timestamp of readHandler1 and not readHandler2 should be synced + // The next report should be scheduler on the max timestamp of readHandler1 and readHandler2 should be synced NL_TEST_ASSERT(aSuite, syncScheduler.mTestNextReportTimestamp == node1->GetMaxTimestamp()); NL_TEST_ASSERT(aSuite, node2->GetSyncTimestamp() == node1->GetMaxTimestamp()); @@ -586,10 +602,9 @@ class TestReportScheduler NL_TEST_ASSERT(aSuite, !syncScheduler.IsReportableNow(readHandler2)); NL_TEST_ASSERT(aSuite, syncScheduler.mTestNextReportTimestamp == node1->GetMaxTimestamp()); - // Simulate a new ReadHandler being added with a min forcing a conflict + // Simulate a new ReadHandler being added with a min timestamp that will force a conflict // Wait for 1 second, nothing should happen here - // Increment the mock system time to the max to the time waited sTestTimerSynchronizedDelegate.IncrementMockTimestamp(System::Clock::Milliseconds64(1000)); ReadHandler * readHandler3 = @@ -610,7 +625,6 @@ class TestReportScheduler // The min timestamp should also not have changed since the min of readhandler3 is higher than the current max NL_TEST_ASSERT(aSuite, syncScheduler.mNextMinTimestamp == node2->GetMinTimestamp()); - // Increment the mock system time to the max to the time waited sTestTimerSynchronizedDelegate.IncrementMockTimestamp(System::Clock::Milliseconds64(1000)); // Confirm that readHandler1 and readHandler 2 are now reportable, whilst readHandler3 is not @@ -631,8 +645,6 @@ class TestReportScheduler NL_TEST_ASSERT(aSuite, node1->GetSyncTimestamp() == node3->GetMaxTimestamp()); NL_TEST_ASSERT(aSuite, node2->GetSyncTimestamp() == node3->GetMaxTimestamp()); - // Simulate a report emission for all readHandlers on the max timestamp of readHandler3 on confirm everything is synced - // Increment the mock system time to the max to the time waited sTestTimerSynchronizedDelegate.IncrementMockTimestamp(System::Clock::Milliseconds64(2000)); NL_TEST_ASSERT(aSuite, syncScheduler.IsReportableNow(readHandler1)); @@ -671,7 +683,6 @@ class TestReportScheduler NL_TEST_ASSERT(aSuite, node2->GetSyncTimestamp() == node4->GetMaxTimestamp()); NL_TEST_ASSERT(aSuite, node3->GetSyncTimestamp() == node1->GetMaxTimestamp()); - // Increment the mock system time to the max to the time waited sTestTimerSynchronizedDelegate.IncrementMockTimestamp(System::Clock::Milliseconds64(1100)); // Confirm readHandler4, 1 and 1 are reportable @@ -689,7 +700,6 @@ class TestReportScheduler NL_TEST_ASSERT(aSuite, !syncScheduler.IsReportableNow(readHandler2)); NL_TEST_ASSERT(aSuite, !syncScheduler.IsReportableNow(readHandler4)); - // Increment the mock system time to the max to the time waited sTestTimerSynchronizedDelegate.IncrementMockTimestamp(System::Clock::Milliseconds64(1000)); // Confirm readHandler3 is reportable and other handlers are synced @@ -717,7 +727,6 @@ class TestReportScheduler NL_TEST_ASSERT(aSuite, node2->GetSyncTimestamp() == node4->GetMaxTimestamp()); NL_TEST_ASSERT(aSuite, node3->GetSyncTimestamp() == node1->GetMaxTimestamp()); - // Increment the mock system time to the max to the time waited sTestTimerSynchronizedDelegate.IncrementMockTimestamp(System::Clock::Milliseconds64(1000)); // Confirm readHandler 1-2-4 are reportable and handler 3 is not because of its min interval @@ -735,25 +744,62 @@ class TestReportScheduler // Test case: Scheduler 1 and 2 are reportable but min2 > max1, they should sync only when possible (min2 = 3, max1 = 2) NL_TEST_ASSERT(aSuite, syncScheduler.GetNumReadHandlers() == 0); + readHandler1->MoveToState(ReadHandler::HandlerState::Idle); NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == readHandler1->SetMaxReportingInterval(2)); NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == readHandler1->SetMinReportingIntervalForTests(0)); + readHandler1->MoveToState(ReadHandler::HandlerState::GeneratingReports); syncScheduler.OnReadHandlerCreated(readHandler1); readHandler1->ForceDirtyState(); syncScheduler.OnBecameReportable(readHandler1); NL_TEST_ASSERT(aSuite, syncScheduler.IsReportableNow(readHandler1)); + readHandler2->MoveToState(ReadHandler::HandlerState::Idle); NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == readHandler2->SetMaxReportingInterval(4)); NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == readHandler2->SetMinReportingIntervalForTests(3)); + readHandler2->MoveToState(ReadHandler::HandlerState::GeneratingReports); syncScheduler.OnReadHandlerCreated(readHandler2); readHandler2->ForceDirtyState(); syncScheduler.OnBecameReportable(readHandler2); NL_TEST_ASSERT(aSuite, !syncScheduler.IsReportableNow(readHandler2)); + node1 = syncScheduler.FindReadHandlerNode(readHandler1); + node2 = syncScheduler.FindReadHandlerNode(readHandler2); + + // Verify report is scheduled immediately as readHandler1 is dirty and its min == 0 + NL_TEST_ASSERT(aSuite, syncScheduler.mTestNextReportTimestamp == node1->GetMinTimestamp()); + readHandler1->ClearForceDirtyFlag(); // report got emited so clear dirty flag + syncScheduler.OnSubscriptionAction(readHandler1); + NL_TEST_ASSERT(aSuite, !syncScheduler.IsReportableNow(readHandler1)); + NL_TEST_ASSERT(aSuite, !syncScheduler.IsReportableNow(readHandler2)); + + // Confirm next report is scheduled on the max timestamp of readHandler1 and readhandler2 is not synced NL_TEST_ASSERT(aSuite, syncScheduler.mTestNextReportTimestamp == node1->GetMaxTimestamp()); - NL_TEST_ASSERT(aSuite, node2->GetSyncTimestamp() == node2->GetMinTimestamp()); + // Node 2's sync timestamp should have remained unaffected since its min is higher + NL_TEST_ASSERT(aSuite, node2->GetSyncTimestamp() == node2->GetMaxTimestamp()); - // Increment the mock system time to the max to the time waited sTestTimerSynchronizedDelegate.IncrementMockTimestamp(System::Clock::Milliseconds64(2000)); + // Verify handler 1 became reportable + NL_TEST_ASSERT(aSuite, syncScheduler.IsReportableNow(readHandler1)); + NL_TEST_ASSERT(aSuite, !syncScheduler.IsReportableNow(readHandler2)); + syncScheduler.OnBecameReportable(readHandler1); + + // simulate run with only readhandler1 reportable + syncScheduler.OnSubscriptionAction(readHandler1); + NL_TEST_ASSERT(aSuite, !syncScheduler.IsReportableNow(readHandler1)); + NL_TEST_ASSERT(aSuite, !syncScheduler.IsReportableNow(readHandler2)); + NL_TEST_ASSERT(aSuite, syncScheduler.mTestNextReportTimestamp == node2->GetMinTimestamp()); + NL_TEST_ASSERT(aSuite, node1->GetSyncTimestamp() == node2->GetMinTimestamp()); + + sTestTimerSynchronizedDelegate.IncrementMockTimestamp(System::Clock::Milliseconds64(1000)); + NL_TEST_ASSERT(aSuite, syncScheduler.IsReportableNow(readHandler1)); + NL_TEST_ASSERT(aSuite, syncScheduler.IsReportableNow(readHandler2)); + + readHandler2->ClearForceDirtyFlag(); + syncScheduler.OnSubscriptionAction(readHandler1); + syncScheduler.OnSubscriptionAction(readHandler2); + + NL_TEST_ASSERT(aSuite, syncScheduler.mTestNextReportTimestamp == node1->GetMaxTimestamp()); + NL_TEST_ASSERT(aSuite, node2->GetSyncTimestamp() == node2->GetMaxTimestamp()); syncScheduler.UnregisterAllHandlers(); readHandlerPool.ReleaseAll(); From 64d869bc3d478dbf2d2394aa10c2313fd467cfb4 Mon Sep 17 00:00:00 2001 From: lpbeliveau-silabs Date: Tue, 18 Jul 2023 10:17:37 -0400 Subject: [PATCH 08/12] Passing a ReportSchedulerPointer instead of an std::function to avoid dynamical memory allocation --- src/app/reporting/ReportScheduler.h | 14 +++++++------- src/app/reporting/ReportSchedulerImpl.cpp | 2 +- src/app/reporting/ReportSchedulerImpl.h | 2 +- .../reporting/SynchronizedReportSchedulerImpl.h | 2 +- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/app/reporting/ReportScheduler.h b/src/app/reporting/ReportScheduler.h index 20e5fd63be9747..f074a81daf52f2 100644 --- a/src/app/reporting/ReportScheduler.h +++ b/src/app/reporting/ReportScheduler.h @@ -56,14 +56,12 @@ class ReportScheduler : public ReadHandler::Observer class ReadHandlerNode : public IntrusiveListNodeBase<> { public: - using TimerCompleteCallback = std::function; - - ReadHandlerNode(ReadHandler * aReadHandler, TimerDelegate * aTimerDelegate, TimerCompleteCallback aCallback) : - mTimerDelegate(aTimerDelegate), mCallback(aCallback) + ReadHandlerNode(ReadHandler * aReadHandler, TimerDelegate * aTimerDelegate, ReportScheduler * aScheduler) : + mTimerDelegate(aTimerDelegate), mScheduler(aScheduler) { VerifyOrDie(aReadHandler != nullptr); VerifyOrDie(aTimerDelegate != nullptr); - VerifyOrDie(aCallback != nullptr); + VerifyOrDie(aScheduler != nullptr); mReadHandler = aReadHandler; SetIntervalTimeStamps(aReadHandler); @@ -96,7 +94,7 @@ class ReportScheduler : public ReadHandler::Observer void RunCallback() { - mCallback(); + mScheduler->ReportTimerCallback(); SetEngineRunScheduled(true); } @@ -113,8 +111,8 @@ class ReportScheduler : public ReadHandler::Observer private: TimerDelegate * mTimerDelegate; - TimerCompleteCallback mCallback; ReadHandler * mReadHandler; + ReportScheduler * mScheduler; Timestamp mMinTimestamp; Timestamp mMaxTimestamp; Timestamp mSyncTimestamp; // Timestamp at which the read handler will be allowed to emit a report so it can be synced with @@ -129,6 +127,8 @@ class ReportScheduler : public ReadHandler::Observer */ virtual ~ReportScheduler() = default; + virtual void ReportTimerCallback() = 0; + /// @brief Check whether a ReadHandler is reportable right now, taking into account its minimum and maximum intervals. /// @param aReadHandler read handler to check bool IsReportableNow(ReadHandler * aReadHandler) diff --git a/src/app/reporting/ReportSchedulerImpl.cpp b/src/app/reporting/ReportSchedulerImpl.cpp index 07875068d12cf1..b49884aa3f699f 100644 --- a/src/app/reporting/ReportSchedulerImpl.cpp +++ b/src/app/reporting/ReportSchedulerImpl.cpp @@ -45,7 +45,7 @@ void ReportSchedulerImpl::OnReadHandlerCreated(ReadHandler * aReadHandler) VerifyOrDie(nullptr == newNode); // The NodePool is the same size as the ReadHandler pool from the IM Engine, so we don't need a check for size here since if a // ReadHandler was created, space should be available. - newNode = mNodesPool.CreateObject(aReadHandler, mTimerDelegate, [this]() { this->ReportTimerCallback(); }); + newNode = mNodesPool.CreateObject(aReadHandler, mTimerDelegate, this); mReadHandlerList.PushBack(newNode); ChipLogProgress(DataManagement, diff --git a/src/app/reporting/ReportSchedulerImpl.h b/src/app/reporting/ReportSchedulerImpl.h index fd86580cf84ef7..3ac47449f834bc 100644 --- a/src/app/reporting/ReportSchedulerImpl.h +++ b/src/app/reporting/ReportSchedulerImpl.h @@ -40,7 +40,7 @@ class ReportSchedulerImpl : public ReportScheduler bool IsReportScheduled(ReadHandler * aReadHandler); - void ReportTimerCallback(); + void ReportTimerCallback() override; protected: virtual CHIP_ERROR ScheduleReport(Timeout timeout, ReadHandlerNode * node); diff --git a/src/app/reporting/SynchronizedReportSchedulerImpl.h b/src/app/reporting/SynchronizedReportSchedulerImpl.h index cce4525f59a9bc..18ee69520e5651 100644 --- a/src/app/reporting/SynchronizedReportSchedulerImpl.h +++ b/src/app/reporting/SynchronizedReportSchedulerImpl.h @@ -40,7 +40,7 @@ class SynchronizedReportSchedulerImpl : public ReportSchedulerImpl bool IsReportScheduled(); - void ReportTimerCallback(); + void ReportTimerCallback() override; protected: CHIP_ERROR ScheduleReport(System::Clock::Timeout timeout, ReadHandlerNode * node) override; From 67adf5358440bed6f53d15ff8f3950ad24cc5306 Mon Sep 17 00:00:00 2001 From: lpbeliveau-silabs Date: Tue, 18 Jul 2023 13:52:56 -0400 Subject: [PATCH 09/12] undid ReadHandler changes --- src/app/ReadHandler.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/ReadHandler.h b/src/app/ReadHandler.h index 2e87451434f294..77c88c12cd1ff9 100644 --- a/src/app/ReadHandler.h +++ b/src/app/ReadHandler.h @@ -52,7 +52,7 @@ #include // https://github.com/CHIP-Specifications/connectedhomeip-spec/blob/61a9d19e6af12fdfb0872bcff26d19de6c680a1a/src/Ch02_Architecture.adoc#1122-subscribe-interaction-limits -constexpr uint16_t kSubscriptionMaxIntervalPublisherLimit = 3600; +constexpr uint16_t kSubscriptionMaxIntervalPublisherLimit = 3600; // 3600 seconds namespace chip { namespace app { From fe00af4a92469ddfe6039efe2618583acb6e84de Mon Sep 17 00:00:00 2001 From: lpbeliveau-silabs <112982107+lpbeliveau-silabs@users.noreply.github.com> Date: Wed, 19 Jul 2023 09:03:56 -0400 Subject: [PATCH 10/12] Update src/app/reporting/ReportScheduler.h Co-authored-by: Boris Zbarsky --- src/app/reporting/ReportScheduler.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/reporting/ReportScheduler.h b/src/app/reporting/ReportScheduler.h index f074a81daf52f2..04765378858508 100644 --- a/src/app/reporting/ReportScheduler.h +++ b/src/app/reporting/ReportScheduler.h @@ -79,7 +79,7 @@ class ReportScheduler : public ReadHandler::Observer (now >= mMinTimestamp && (mReadHandler->IsDirty() || now >= mMaxTimestamp || now >= mSyncTimestamp))); } - bool IsEngineRunScheduled() const { return mEnginRunScheduled; } + bool IsEngineRunScheduled() const { return mEngineRunScheduled; } void SetEngineRunScheduled(bool aEnginRunScheduled) { mEnginRunScheduled = aEnginRunScheduled; } void SetIntervalTimeStamps(ReadHandler * aReadHandler) From a5785236bd1e3c3cbf6f91e3af3e47f587cb42c7 Mon Sep 17 00:00:00 2001 From: lpbeliveau-silabs Date: Wed, 19 Jul 2023 10:52:13 -0400 Subject: [PATCH 11/12] Removed un-necessary nullptr check, addressed comments regarding tests and added doc on unclear behavior --- src/app/reporting/ReportScheduler.h | 9 ++++---- .../SynchronizedReportSchedulerImpl.cpp | 5 +++-- src/app/tests/TestReportScheduler.cpp | 21 ++++++++++++------- 3 files changed, 22 insertions(+), 13 deletions(-) diff --git a/src/app/reporting/ReportScheduler.h b/src/app/reporting/ReportScheduler.h index 04765378858508..0449ba3114f857 100644 --- a/src/app/reporting/ReportScheduler.h +++ b/src/app/reporting/ReportScheduler.h @@ -80,7 +80,7 @@ class ReportScheduler : public ReadHandler::Observer } bool IsEngineRunScheduled() const { return mEngineRunScheduled; } - void SetEngineRunScheduled(bool aEnginRunScheduled) { mEnginRunScheduled = aEnginRunScheduled; } + void SetEngineRunScheduled(bool aEnginRunScheduled) { mEngineRunScheduled = aEnginRunScheduled; } void SetIntervalTimeStamps(ReadHandler * aReadHandler) { @@ -100,7 +100,8 @@ class ReportScheduler : public ReadHandler::Observer void SetSyncTimestamp(System::Clock::Timestamp aSyncTimestamp) { - // Prevents the sync timestamp being set to a value lower than the min timestamp + // Prevents the sync timestamp being set to a value lower than the min timestamp to prevent it to appear as reportable + // on the next timeout calculation and cause the scheduler to run the engine too early VerifyOrReturn(aSyncTimestamp >= mMinTimestamp); mSyncTimestamp = aSyncTimestamp; } @@ -117,8 +118,8 @@ class ReportScheduler : public ReadHandler::Observer Timestamp mMaxTimestamp; Timestamp mSyncTimestamp; // Timestamp at which the read handler will be allowed to emit a report so it can be synced with // other handlers that have an earlier max timestamp - bool mEnginRunScheduled = false; // Flag to indicate if the engine run is already scheduled so the scheduler can ignore - // it when calculating the next run time + bool mEngineRunScheduled = false; // Flag to indicate if the engine run is already scheduled so the scheduler can ignore + // it when calculating the next run time }; ReportScheduler(TimerDelegate * aTimerDelegate) : mTimerDelegate(aTimerDelegate) {} diff --git a/src/app/reporting/SynchronizedReportSchedulerImpl.cpp b/src/app/reporting/SynchronizedReportSchedulerImpl.cpp index b56f9ff032fa1a..16713e37e05741 100644 --- a/src/app/reporting/SynchronizedReportSchedulerImpl.cpp +++ b/src/app/reporting/SynchronizedReportSchedulerImpl.cpp @@ -28,8 +28,8 @@ using ReadHandlerNode = ReportScheduler::ReadHandlerNode; void SynchronizedReportSchedulerImpl::OnReadHandlerDestroyed(ReadHandler * aReadHandler) { - // Verify list is populated and handler is not null - VerifyOrReturn((!mReadHandlerList.Empty() || (nullptr == aReadHandler))); + // Verify list is populated + VerifyOrReturn((!mReadHandlerList.Empty())); ReadHandlerNode * removeNode = FindReadHandlerNode(aReadHandler); // Nothing to remove if the handler is not found in the list @@ -154,6 +154,7 @@ CHIP_ERROR SynchronizedReportSchedulerImpl::CalculateNextReportTimeout(Timeout & } else { + // Schedule report at next max otherwise timeout = mNextMaxTimestamp - now; } diff --git a/src/app/tests/TestReportScheduler.cpp b/src/app/tests/TestReportScheduler.cpp index 5c9e5a84d50772..a0eb6ec77c74db 100644 --- a/src/app/tests/TestReportScheduler.cpp +++ b/src/app/tests/TestReportScheduler.cpp @@ -677,19 +677,23 @@ class TestReportScheduler NL_TEST_ASSERT(aSuite, syncScheduler.GetNumReadHandlers() == 4); ReadHandlerNode * node4 = syncScheduler.FindReadHandlerNode(readHandler4); - // Confirm next report is scheduled on the max timestamp of readHandler4 and other handlers be synced + // Confirm next report is scheduled on the max timestamp of readHandler4 and other handlers 1 and 2 are synced NL_TEST_ASSERT(aSuite, syncScheduler.mTestNextReportTimestamp == node4->GetMaxTimestamp()); NL_TEST_ASSERT(aSuite, node1->GetSyncTimestamp() == node4->GetMaxTimestamp()); NL_TEST_ASSERT(aSuite, node2->GetSyncTimestamp() == node4->GetMaxTimestamp()); + + // Confirm handler 3 is synched on a later timestamp since its min is higher than the max of readHandler4 NL_TEST_ASSERT(aSuite, node3->GetSyncTimestamp() == node1->GetMaxTimestamp()); sTestTimerSynchronizedDelegate.IncrementMockTimestamp(System::Clock::Milliseconds64(1100)); - // Confirm readHandler4, 1 and 1 are reportable + // Confirm readHandler1, 2 and 4 are reportable NL_TEST_ASSERT(aSuite, syncScheduler.IsReportableNow(readHandler1)); NL_TEST_ASSERT(aSuite, syncScheduler.IsReportableNow(readHandler2)); - NL_TEST_ASSERT(aSuite, !syncScheduler.IsReportableNow(readHandler3)); NL_TEST_ASSERT(aSuite, syncScheduler.IsReportableNow(readHandler4)); + + // Confirm readHandler3 is not reportable + NL_TEST_ASSERT(aSuite, !syncScheduler.IsReportableNow(readHandler3)); readHandler4->mObserver->OnBecameReportable(readHandler1); readHandler4->mObserver->OnBecameReportable(readHandler2); readHandler4->mObserver->OnBecameReportable(readHandler4); @@ -720,21 +724,24 @@ class TestReportScheduler NL_TEST_ASSERT(aSuite, !syncScheduler.IsReportableNow(readHandler3)); NL_TEST_ASSERT(aSuite, !syncScheduler.IsReportableNow(readHandler4)); - // Next emission should be scheduled on the max timestamp of readHandler4 as it is the most restrictive, and other handlers - // should get synced except handler 3 as its min is higher + // Next emission should be scheduled on the max timestamp of readHandler4 as it is the most restrictive, and handlers 1 and + // 2 should be synced to handler 4 NL_TEST_ASSERT(aSuite, syncScheduler.mTestNextReportTimestamp == node4->GetMaxTimestamp()); NL_TEST_ASSERT(aSuite, node1->GetSyncTimestamp() == node4->GetMaxTimestamp()); NL_TEST_ASSERT(aSuite, node2->GetSyncTimestamp() == node4->GetMaxTimestamp()); + // handler 3 should have a sync on a different point as its min is higher, in this case it is the max timestamp of handler 1 NL_TEST_ASSERT(aSuite, node3->GetSyncTimestamp() == node1->GetMaxTimestamp()); sTestTimerSynchronizedDelegate.IncrementMockTimestamp(System::Clock::Milliseconds64(1000)); - // Confirm readHandler 1-2-4 are reportable and handler 3 is not because of its min interval + // Confirm readHandler 1-2-4 are reportable NL_TEST_ASSERT(aSuite, syncScheduler.IsReportableNow(readHandler1)); NL_TEST_ASSERT(aSuite, syncScheduler.IsReportableNow(readHandler2)); - NL_TEST_ASSERT(aSuite, !syncScheduler.IsReportableNow(readHandler3)); NL_TEST_ASSERT(aSuite, syncScheduler.IsReportableNow(readHandler4)); + // Confirm readHandler3 is not reportable because of its min interval + NL_TEST_ASSERT(aSuite, !syncScheduler.IsReportableNow(readHandler3)); + syncScheduler.OnReadHandlerDestroyed(readHandler1); syncScheduler.OnReadHandlerDestroyed(readHandler2); syncScheduler.OnReadHandlerDestroyed(readHandler3); From 92d1bcd5ef747a242911f3daee17218e6f9f7ad4 Mon Sep 17 00:00:00 2001 From: lpbeliveau-silabs Date: Wed, 19 Jul 2023 13:43:22 -0400 Subject: [PATCH 12/12] Addressed redundant test --- src/app/tests/TestReportScheduler.cpp | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/app/tests/TestReportScheduler.cpp b/src/app/tests/TestReportScheduler.cpp index a0eb6ec77c74db..c789865bcc50b2 100644 --- a/src/app/tests/TestReportScheduler.cpp +++ b/src/app/tests/TestReportScheduler.cpp @@ -512,18 +512,14 @@ class TestReportScheduler // Simulate waiting for the max interval to expire (2s) sTestTimerSynchronizedDelegate.IncrementMockTimestamp(System::Clock::Milliseconds64(2000)); - // Confirm that both handlers are now reportable since the timer has expired + // Confirm that both handlers are now reportable since the timer has expired (readHandler1 from its max and readHandler2 + // from its sync) NL_TEST_ASSERT(aSuite, syncScheduler.IsReportableNow(readHandler1)); NL_TEST_ASSERT(aSuite, syncScheduler.IsReportableNow(readHandler2)); // Confirm timeout has expired and no report is scheduled, an engine run would typically happen here NL_TEST_ASSERT(aSuite, !sScheduler.IsReportScheduled(readHandler1)); NL_TEST_ASSERT(aSuite, !sScheduler.IsReportScheduled(readHandler2)); - // Simulate that the OnBecameReportable callback was called on readHandler1 - readHandler1->mObserver->OnBecameReportable(readHandler1); - // Confirm that the read handler is now reportable due to the sync mechanism that forced a dirty flag - NL_TEST_ASSERT(aSuite, syncScheduler.IsReportableNow(readHandler2)); - // Simulate a report emission for readHandler1 readHandler1->mObserver->OnSubscriptionAction(readHandler1); // Simulate a report emission for readHandler2