From 43669135c2ecda7e4e0c0e941d7dca250429bdc9 Mon Sep 17 00:00:00 2001 From: lpbeliveau-silabs <112982107+lpbeliveau-silabs@users.noreply.github.com> Date: Mon, 14 Aug 2023 18:06:34 -0400 Subject: [PATCH] [ReadHandler] Unification of serial "now" timestamp (#28657) * Removed multiple calls to GetCurrentMonotonicTimestamp in series to ensure that the NOW value used remains the same along each checks * removed mNow, using stack now timestamps instead * Added missing @param description * Removed un-necessary comments --- src/app/reporting/ReportScheduler.h | 28 ++++++++-------- src/app/reporting/ReportSchedulerImpl.cpp | 30 ++++++++++------- src/app/reporting/ReportSchedulerImpl.h | 4 +-- .../SynchronizedReportSchedulerImpl.cpp | 32 +++++++++---------- .../SynchronizedReportSchedulerImpl.h | 8 ++--- 5 files changed, 56 insertions(+), 46 deletions(-) diff --git a/src/app/reporting/ReportScheduler.h b/src/app/reporting/ReportScheduler.h index 5b7b62709985bd..b6abbdb478e506 100644 --- a/src/app/reporting/ReportScheduler.h +++ b/src/app/reporting/ReportScheduler.h @@ -30,8 +30,6 @@ namespace reporting { // Forward declaration of TestReportScheduler to allow it to be friend with ReportScheduler class TestReportScheduler; -using Timestamp = System::Clock::Timestamp; - class TimerContext { public: @@ -42,6 +40,8 @@ class TimerContext class ReportScheduler : public ReadHandler::Observer, public ICDStateObserver { public: + using Timestamp = System::Clock::Timestamp; + /// @brief This class acts as an interface between the report scheduler and the system timer to reduce dependencies on the /// system layer. class TimerDelegate @@ -63,25 +63,22 @@ class ReportScheduler : public ReadHandler::Observer, public ICDStateObserver class ReadHandlerNode : public TimerContext { public: - ReadHandlerNode(ReadHandler * aReadHandler, TimerDelegate * aTimerDelegate, ReportScheduler * aScheduler) : - mTimerDelegate(aTimerDelegate), mScheduler(aScheduler) + ReadHandlerNode(ReadHandler * aReadHandler, ReportScheduler * aScheduler, const Timestamp & now) : mScheduler(aScheduler) { VerifyOrDie(aReadHandler != nullptr); - VerifyOrDie(aTimerDelegate != nullptr); VerifyOrDie(aScheduler != nullptr); mReadHandler = aReadHandler; - SetIntervalTimeStamps(aReadHandler); + SetIntervalTimeStamps(aReadHandler, now); } ReadHandler * GetReadHandler() const { return mReadHandler; } /// @brief Check if the Node is reportable now, meaning its readhandler was made reportable by attribute dirtying and /// handler state, and minimal time interval since last report has elapsed, or the maximal time interval since last /// report has elapsed - bool IsReportableNow() const + /// @param now current time to use for the check, user must ensure to provide a valid time for this to be reliable + bool IsReportableNow(const Timestamp & now) const { - Timestamp now = mTimerDelegate->GetCurrentMonotonicTimestamp(); - return (mReadHandler->CanStartReporting() && (now >= mMinTimestamp && (mReadHandler->IsDirty() || now >= mMaxTimestamp || now >= mSyncTimestamp))); } @@ -89,11 +86,14 @@ class ReportScheduler : public ReadHandler::Observer, public ICDStateObserver bool IsEngineRunScheduled() const { return mEngineRunScheduled; } void SetEngineRunScheduled(bool aEngineRunScheduled) { mEngineRunScheduled = aEngineRunScheduled; } - void SetIntervalTimeStamps(ReadHandler * aReadHandler) + /// @brief Set the interval timestamps for the node based on the read handler reporting intervals + /// @param aReadHandler read handler to get the intervals from + /// @param now current time to calculate the mMin and mMax timestamps, user must ensure to provide a valid time for this to + /// be reliable + void SetIntervalTimeStamps(ReadHandler * aReadHandler, const Timestamp & now) { uint16_t minInterval, maxInterval; aReadHandler->GetReportingIntervals(minInterval, maxInterval); - Timestamp now = mTimerDelegate->GetCurrentMonotonicTimestamp(); mMinTimestamp = now + System::Clock::Seconds16(minInterval); mMaxTimestamp = now + System::Clock::Seconds16(maxInterval); mSyncTimestamp = mMaxTimestamp; @@ -118,7 +118,6 @@ class ReportScheduler : public ReadHandler::Observer, public ICDStateObserver System::Clock::Timestamp GetSyncTimestamp() const { return mSyncTimestamp; } private: - TimerDelegate * mTimerDelegate; ReadHandler * mReadHandler; ReportScheduler * mScheduler; Timestamp mMinTimestamp; @@ -141,9 +140,12 @@ class ReportScheduler : public ReadHandler::Observer, public ICDStateObserver /// @param aReadHandler read handler to check bool IsReportableNow(ReadHandler * aReadHandler) { + // Update the now timestamp to ensure external calls to IsReportableNow are always comparing to the current time + Timestamp now = mTimerDelegate->GetCurrentMonotonicTimestamp(); ReadHandlerNode * node = FindReadHandlerNode(aReadHandler); - return (nullptr != node) ? node->IsReportableNow() : false; + return (nullptr != node) ? node->IsReportableNow(now) : false; } + /// @brief Check if a ReadHandler is reportable without considering the timing bool IsReadHandlerReportable(ReadHandler * aReadHandler) const { return aReadHandler->ShouldStartReporting(); } /// @brief Sets the ForceDirty flag of a ReadHandler diff --git a/src/app/reporting/ReportSchedulerImpl.cpp b/src/app/reporting/ReportSchedulerImpl.cpp index 44be647b921d9d..632d2f456990cd 100644 --- a/src/app/reporting/ReportSchedulerImpl.cpp +++ b/src/app/reporting/ReportSchedulerImpl.cpp @@ -43,7 +43,7 @@ void ReportSchedulerImpl::OnEnterActiveMode() { #if ICD_REPORT_ON_ENTER_ACTIVE_MODE Timestamp now = mTimerDelegate->GetCurrentMonotonicTimestamp(); - mNodesPool.ForEachActiveObject([now, this](ReadHandlerNode * node) { + mNodesPool.ForEachActiveObject([now](ReadHandlerNode * node) { if (now >= node->GetMinTimestamp()) { this->HandlerForceDirtyState(node->GetReadHandler()); @@ -62,9 +62,12 @@ void ReportSchedulerImpl::OnSubscriptionEstablished(ReadHandler * aReadHandler) ReadHandlerNode * newNode = FindReadHandlerNode(aReadHandler); // Handler must not be registered yet; it's just being constructed. VerifyOrDie(nullptr == newNode); + + Timestamp now = mTimerDelegate->GetCurrentMonotonicTimestamp(); + // 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); + newNode = mNodesPool.CreateObject(aReadHandler, this, now); ChipLogProgress(DataManagement, "Registered a ReadHandler that will schedule a report between system Timestamp: %" PRIu64 @@ -77,19 +80,25 @@ void ReportSchedulerImpl::OnBecameReportable(ReadHandler * aReadHandler) { ReadHandlerNode * node = FindReadHandlerNode(aReadHandler); VerifyOrReturn(nullptr != node); + + Timestamp now = mTimerDelegate->GetCurrentMonotonicTimestamp(); + Milliseconds32 newTimeout; - CalculateNextReportTimeout(newTimeout, node); - ScheduleReport(newTimeout, node); + CalculateNextReportTimeout(newTimeout, node, now); + ScheduleReport(newTimeout, node, now); } void ReportSchedulerImpl::OnSubscriptionReportSent(ReadHandler * aReadHandler) { ReadHandlerNode * node = FindReadHandlerNode(aReadHandler); VerifyOrReturn(nullptr != node); - node->SetIntervalTimeStamps(aReadHandler); + + Timestamp now = mTimerDelegate->GetCurrentMonotonicTimestamp(); + + node->SetIntervalTimeStamps(aReadHandler, now); Milliseconds32 newTimeout; - CalculateNextReportTimeout(newTimeout, node); - ScheduleReport(newTimeout, node); + CalculateNextReportTimeout(newTimeout, node, now); + ScheduleReport(newTimeout, node, now); node->SetEngineRunScheduled(false); } @@ -105,7 +114,7 @@ void ReportSchedulerImpl::OnReadHandlerDestroyed(ReadHandler * aReadHandler) mNodesPool.ReleaseObject(removeNode); } -CHIP_ERROR ReportSchedulerImpl::ScheduleReport(Timeout timeout, ReadHandlerNode * node) +CHIP_ERROR ReportSchedulerImpl::ScheduleReport(Timeout timeout, ReadHandlerNode * node, const Timestamp & now) { // Cancel Report if it is currently scheduled mTimerDelegate->CancelTimer(node); @@ -141,13 +150,12 @@ bool ReportSchedulerImpl::IsReportScheduled(ReadHandler * aReadHandler) return mTimerDelegate->IsTimerActive(node); } -CHIP_ERROR ReportSchedulerImpl::CalculateNextReportTimeout(Timeout & timeout, ReadHandlerNode * aNode) +CHIP_ERROR ReportSchedulerImpl::CalculateNextReportTimeout(Timeout & timeout, ReadHandlerNode * aNode, const Timestamp & now) { VerifyOrReturnError(nullptr != FindReadHandlerNode(aNode->GetReadHandler()), CHIP_ERROR_INVALID_ARGUMENT); - Timestamp now = mTimerDelegate->GetCurrentMonotonicTimestamp(); // If the handler is reportable now, just schedule a report immediately - if (aNode->IsReportableNow()) + if (aNode->IsReportableNow(now)) { // If the handler is reportable now, just schedule a report immediately timeout = Milliseconds32(0); diff --git a/src/app/reporting/ReportSchedulerImpl.h b/src/app/reporting/ReportSchedulerImpl.h index 003be7cb4637eb..f3870192c4b1f6 100644 --- a/src/app/reporting/ReportSchedulerImpl.h +++ b/src/app/reporting/ReportSchedulerImpl.h @@ -46,14 +46,14 @@ class ReportSchedulerImpl : public ReportScheduler void ReportTimerCallback() override; protected: - virtual CHIP_ERROR ScheduleReport(Timeout timeout, ReadHandlerNode * node); + virtual CHIP_ERROR ScheduleReport(Timeout timeout, ReadHandlerNode * node, const Timestamp & now); void CancelReport(ReadHandler * aReadHandler); virtual void UnregisterAllHandlers(); private: friend class chip::app::reporting::TestReportScheduler; - virtual CHIP_ERROR CalculateNextReportTimeout(Timeout & timeout, ReadHandlerNode * aNode); + virtual CHIP_ERROR CalculateNextReportTimeout(Timeout & timeout, ReadHandlerNode * aNode, const Timestamp & now); }; } // namespace reporting diff --git a/src/app/reporting/SynchronizedReportSchedulerImpl.cpp b/src/app/reporting/SynchronizedReportSchedulerImpl.cpp index b3f700b8cbcc0f..d9a410f555e506 100644 --- a/src/app/reporting/SynchronizedReportSchedulerImpl.cpp +++ b/src/app/reporting/SynchronizedReportSchedulerImpl.cpp @@ -44,7 +44,7 @@ void SynchronizedReportSchedulerImpl::OnReadHandlerDestroyed(ReadHandler * aRead } } -CHIP_ERROR SynchronizedReportSchedulerImpl::ScheduleReport(Timeout timeout, ReadHandlerNode * node) +CHIP_ERROR SynchronizedReportSchedulerImpl::ScheduleReport(Timeout timeout, ReadHandlerNode * node, const Timestamp & now) { // Cancel Report if it is currently scheduled mTimerDelegate->CancelTimer(this); @@ -54,7 +54,7 @@ CHIP_ERROR SynchronizedReportSchedulerImpl::ScheduleReport(Timeout timeout, Read return CHIP_NO_ERROR; } ReturnErrorOnFailure(mTimerDelegate->StartTimer(this, timeout)); - mTestNextReportTimestamp = mTimerDelegate->GetCurrentMonotonicTimestamp() + timeout; + mTestNextReportTimestamp = now + timeout; return CHIP_NO_ERROR; } @@ -73,10 +73,9 @@ bool SynchronizedReportSchedulerImpl::IsReportScheduled() /// @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() +CHIP_ERROR SynchronizedReportSchedulerImpl::FindNextMaxInterval(const Timestamp & now) { VerifyOrReturnError(mNodesPool.Allocated(), CHIP_ERROR_INVALID_LIST_LENGTH); - System::Clock::Timestamp now = mTimerDelegate->GetCurrentMonotonicTimestamp(); System::Clock::Timestamp earliest = now + Seconds16::max(); mNodesPool.ForEachActiveObject([&earliest, now](ReadHandlerNode * node) { @@ -97,10 +96,10 @@ CHIP_ERROR SynchronizedReportSchedulerImpl::FindNextMaxInterval() /// 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() +CHIP_ERROR SynchronizedReportSchedulerImpl::FindNextMinInterval(const Timestamp & now) { VerifyOrReturnError(mNodesPool.Allocated(), CHIP_ERROR_INVALID_LIST_LENGTH); - System::Clock::Timestamp latest = mTimerDelegate->GetCurrentMonotonicTimestamp(); + System::Clock::Timestamp latest = now; mNodesPool.ForEachActiveObject([&latest, this](ReadHandlerNode * node) { if (node->GetMinTimestamp() > latest && this->IsReadHandlerReportable(node->GetReadHandler()) && @@ -118,20 +117,19 @@ CHIP_ERROR SynchronizedReportSchedulerImpl::FindNextMinInterval() return CHIP_NO_ERROR; } -CHIP_ERROR SynchronizedReportSchedulerImpl::CalculateNextReportTimeout(Timeout & timeout, ReadHandlerNode * aNode) +CHIP_ERROR SynchronizedReportSchedulerImpl::CalculateNextReportTimeout(Timeout & timeout, ReadHandlerNode * aNode, + const Timestamp & now) { VerifyOrReturnError(nullptr != FindReadHandlerNode(aNode->GetReadHandler()), CHIP_ERROR_INVALID_ARGUMENT); - ReturnErrorOnFailure(FindNextMaxInterval()); - ReturnErrorOnFailure(FindNextMinInterval()); + ReturnErrorOnFailure(FindNextMaxInterval(now)); + ReturnErrorOnFailure(FindNextMinInterval(now)); bool reportableNow = false; bool reportableAtMin = false; - Timestamp now = mTimerDelegate->GetCurrentMonotonicTimestamp(); - - mNodesPool.ForEachActiveObject([&reportableNow, &reportableAtMin, this](ReadHandlerNode * node) { + mNodesPool.ForEachActiveObject([&reportableNow, &reportableAtMin, this, now](ReadHandlerNode * node) { if (!node->IsEngineRunScheduled()) { - if (node->IsReportableNow()) + if (node->IsReportableNow(now)) { reportableNow = true; return Loop::Break; @@ -166,7 +164,7 @@ CHIP_ERROR SynchronizedReportSchedulerImpl::CalculateNextReportTimeout(Timeout & mNodesPool.ForEachActiveObject([now, timeout](ReadHandlerNode * node) { // 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 (!node->IsReportableNow()) + if (!node->IsReportableNow(now)) { node->SetSyncTimestamp(Milliseconds64(now + timeout)); } @@ -181,10 +179,12 @@ CHIP_ERROR SynchronizedReportSchedulerImpl::CalculateNextReportTimeout(Timeout & /// the engine already verifies that read handlers are reportable before sending a report void SynchronizedReportSchedulerImpl::TimerFired() { + Timestamp now = mTimerDelegate->GetCurrentMonotonicTimestamp(); + InteractionModelEngine::GetInstance()->GetReportingEngine().ScheduleRun(); - mNodesPool.ForEachActiveObject([](ReadHandlerNode * node) { - if (node->IsReportableNow()) + mNodesPool.ForEachActiveObject([now](ReadHandlerNode * node) { + if (node->IsReportableNow(now)) { node->SetEngineRunScheduled(true); ChipLogProgress(DataManagement, "Handler: %p with min: %" PRIu64 " and max: %" PRIu64 " and sync: %" PRIu64, (node), diff --git a/src/app/reporting/SynchronizedReportSchedulerImpl.h b/src/app/reporting/SynchronizedReportSchedulerImpl.h index 9fd4a8ca0aa684..9e3689d144194a 100644 --- a/src/app/reporting/SynchronizedReportSchedulerImpl.h +++ b/src/app/reporting/SynchronizedReportSchedulerImpl.h @@ -43,15 +43,15 @@ class SynchronizedReportSchedulerImpl : public ReportSchedulerImpl, public Timer void TimerFired() override; protected: - CHIP_ERROR ScheduleReport(System::Clock::Timeout timeout, ReadHandlerNode * node) override; + CHIP_ERROR ScheduleReport(System::Clock::Timeout timeout, ReadHandlerNode * node, const Timestamp & now) override; void CancelReport(); private: friend class chip::app::reporting::TestReportScheduler; - CHIP_ERROR FindNextMinInterval(); - CHIP_ERROR FindNextMaxInterval(); - CHIP_ERROR CalculateNextReportTimeout(Timeout & timeout, ReadHandlerNode * aReadHandlerNode) override; + CHIP_ERROR FindNextMinInterval(const Timestamp & now); + CHIP_ERROR FindNextMaxInterval(const Timestamp & now); + CHIP_ERROR CalculateNextReportTimeout(Timeout & timeout, ReadHandlerNode * aReadHandlerNode, const Timestamp & now) override; Timestamp mNextMaxTimestamp = Milliseconds64(0); Timestamp mNextMinTimestamp = Milliseconds64(0);