[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

@@ -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,37 +63,37 @@ 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)));
  }
 
  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

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);

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

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),

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);