Merge branch 'master' into YAML-Updates-2-23-2024

Author: raul-marquez-csa

src/app/reporting/ReportScheduler.h

@@ -51,7 +51,7 @@ class TimerContext
  *
  * It inherits the ReadHandler::Observer class to be notified of reportability changes in the ReadHandlers.
  * It inherits the ICDStateObserver class to allow the implementation to generate reports based on the changes in ICD devices state,
- * such as going from idle to active and vice-versa.
+ * such as going from idle to active mode and vice-versa.
  *
  * @note The logic for how and when to schedule reports is implemented in the subclasses of ReportScheduler, such as
  * ReportSchedulerImpl and SyncronizedReportSchedulerImpl.
@@ -70,7 +70,7 @@ class ReportScheduler : public ReadHandler::Observer, public ICDStateObserver
         /// @brief Start a timer for a given context. The report scheduler must always cancel an existing timer for a context (using
         /// CancelTimer) before starting a new one for that context.
         /// @param context context to pass to the timer callback.
-        /// @param aTimeout time in miliseconds before the timer expires
+        /// @param aTimeout time in milliseconds before the timer expires
         virtual CHIP_ERROR StartTimer(TimerContext * context, System::Clock::Timeout aTimeout) = 0;
         /// @brief Cancel a timer for a given context
         /// @param context used to identify the timer to cancel
@@ -82,25 +82,34 @@ class ReportScheduler : public ReadHandler::Observer, public ICDStateObserver
     /**
      * @class ReadHandlerNode
      *
-     * @brief This class is in charge of determining when a ReadHandler is reportable depending on the monotonic timestamp of the
-     * system and the intervals of the ReadHandler. It inherits the TimerContext class to allow it to be used as a context for a
-     * TimerDelegate so the TimerDelegate can call the TimerFired method when the timer expires.
+     * @brief This class is responsible for determining when a ReadHandler is reportable depending on the monotonic timestamp of
+     *  the system and the intervals of the ReadHandler. It inherits the TimerContext class to allow it to be used as a context for
+     *  a TimerDelegate so that the TimerDelegate can call the TimerFired method when the timer expires.
      *
-     *  The Logic to determine if a ReadHandler is reportable at a precise timestamp is as follows:
-     *  1: The ReadHandler is in the CanStartReporting state
-     *  2: The minimal interval since last report has elapsed
-     *  3: The maximal interval since last report has elapsed or the ReadHandler is dirty
-     *  If the three conditions are met, the ReadHandler is reportable.
+     *  Three conditions that can prevent the ReadHandler from being reportable:
+     *  1: The ReadHandler is not in the CanStartReporting state:
+     *      This condition can be resolved by setting the CanStartReporting flag on the ReadHandler
      *
-     *  Additionnal flags have been provided for specific use cases:
+     *  2: The minimal interval since the last report has not elapsed
+     *      This condition can be resolved after enough time has passed since the last report or by setting the EngineRunScheduled
+     *      flag
      *
-     *  CanbeSynced: Mechanism to allow the ReadHandler to emit a report if another readHandler is ReportableNow.
-     *  This flag can substitute the maximal interval condition or the dirty condition. It is currently only used by the
-     *  SynchronizedReportScheduler.
+     *  3: The maximal interval since the last report has not elapsed and the ReadHandler is not dirty:
+     *      This condition can be resolved after enough time has passed since the last report to reach the max interval, by the
+     *      ReadHandler becoming dirty or by setting the CanBeSynced flag and having another ReadHandler needing to report.
+     *
+     *  Once the 3 conditions are met, the ReadHandler is considered reportable.
+     *
+     *  Flags:
+     *
+     *  CanBeSynced: Mechanism to allow the ReadHandler to emit a report if another readHandler is ReportableNow.
+     *  This flag is currently only used by the SynchronizedReportScheduler to allow firing reports of ReadHandlers at the same
+     *  time.
      *
      *  EngineRunScheduled: Mechanism to ensure that the reporting engine will see the ReadHandler as reportable if a timer fires.
-     *  This flag can substitute the minimal interval condition or the maximal interval condition. The goal is to allow for
-     *  reporting when timers fire earlier than the minimal timestamp du to mechanism such as NTP clock adjustments.
+     *  This flag is used to confirm that the next report timer has fired for a ReadHandler, thus allowing reporting when timers
+     *  fire earlier than the minimal timestamp due to mechanisms such as NTP clock adjustments.
+     *
      */
     class ReadHandlerNode : public TimerContext
     {
@@ -125,12 +134,12 @@ class ReportScheduler : public ReadHandler::Observer, public ICDStateObserver
         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
+        /// handler state, and minimal time interval since the last report has elapsed, or the maximal time interval since the last
         /// report has elapsed.
-        /// @note If a handler has been flaged as scheduled for engine run, it will be reported regardless of the timestamps. This
-        /// is done to guarantee that the reporting engine will see the handler as reportable if a timer fires, even if it fires
-        /// early.
-        /// @param now current time to use for the check, user must ensure to provide a valid time for this to be reliable
+        /// @note If a handler has been flagged as scheduled for an engine run, it will be reported regardless of the timestamps.
+        /// This is done to guarantee that the reporting engine will see the handler as reportable if a timer fires, even if it
+        /// fires early.
+        /// @param now current time to use for the check, the user must ensure to provide a valid time for this to be reliable
         bool IsReportableNow(const Timestamp & now) const
         {
             return (mReadHandler->CanStartReporting() &&
@@ -149,8 +158,8 @@ class ReportScheduler : public ReadHandler::Observer, public ICDStateObserver
 
         /// @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
+        /// @param now current time to calculate the mMin and mMax timestamps, the user must ensure to provide a valid time for this
+        /// to be reliable
         void SetIntervalTimeStamps(ReadHandler * aReadHandler, const Timestamp & now)
         {
             uint16_t minInterval, maxInterval;
@@ -178,9 +187,7 @@ class ReportScheduler : public ReadHandler::Observer, public ICDStateObserver
     };
 
     ReportScheduler(TimerDelegate * aTimerDelegate) : mTimerDelegate(aTimerDelegate) {}
-    /**
-     *  Interface to act on changes in the ReadHandler reportability
-     */
+
     virtual ~ReportScheduler() = default;
 
     virtual void ReportTimerCallback() = 0;
@@ -225,7 +232,7 @@ class ReportScheduler : public ReadHandler::Observer, public ICDStateObserver
 
     /// @brief Find the ReadHandlerNode for a given ReadHandler pointer
     /// @param [in] aReadHandler ReadHandler pointer to look for in the ReadHandler nodes list
-    /// @return Node Address if node was found, nullptr otherwise
+    /// @return Node Address if the node was found, nullptr otherwise
     ReadHandlerNode * FindReadHandlerNode(const ReadHandler * aReadHandler)
     {
         ReadHandlerNode * foundNode = nullptr;

src/app/reporting/ReportSchedulerImpl.cpp

@@ -92,6 +92,8 @@ void ReportSchedulerImpl::OnSubscriptionReportSent(ReadHandler * aReadHandler)
 
     Timestamp now = mTimerDelegate->GetCurrentMonotonicTimestamp();
 
+    // This method is called after the report is sent, so the ReadHandler is no longer reportable, and thus CanBeSynced and
+    // EngineRunScheduled of the node associated with the ReadHandler are set to false here.
     node->SetCanBeSynced(false);
     node->SetIntervalTimeStamps(aReadHandler, now);
     Milliseconds32 newTimeout;

src/app/reporting/ReportSchedulerImpl.h

@@ -29,20 +29,33 @@ namespace reporting {
  *
  * @brief This class extends ReportScheduler and provides a scheduling logic for the CHIP Interaction Model Reporting Engine.
  *
- * It is reponsible for implementing the ReadHandler and ICD observers callbacks to the Scheduler can take actions whenever a
- * ReadHandler event occurs or the ICD changes modes.
+ * It is responsible for implementing the ReadHandler and ICD observers callbacks so the Scheduler can take action whenever a
+ * ReadHandler event occurs or the ICD mode change occurs.
  *
- * All ReadHandlers Observers callbacks rely on the node pool to create or find the node associated to the ReadHandler that
+ * All ReadHandlers Observers callbacks rely on the node pool to create or find the node associated with the ReadHandler that
  * triggered the callback and will use the FindReadHandlerNode() method to do so.
  *
  * ## Scheduling Logic
  *
  * This class implements a scheduling logic that calculates the next report timeout based on the current system timestamp, the state
  * of the ReadHandlers associated with the scheduler nodes and the min and max intervals of the ReadHandlers.
  *
- * @note This class mimics the original scheduling in which the ReadHandlers would  schedule themselves. The key difference is that
- * this implementation only relies on a single timer from the scheduling moment rather than having a timer expiring on the min
- * interval that would trigger the start of a second timer expiring on the max interval.
+ * The logic is as follows:
+ *
+ * - When a ReadHandler is created for a subscription, the scheduler adds a node and registers it in the scheduler node pool.
+ *
+ * - Each node can schedule a report independently from the other nodes, and thus each node has its timer.
+ *
+ * - The timeout of each node timer is calculated when its associated ReadHandler becomes reportable, when a report is sent for
+ *   the ReadHandler.
+ *
+ * - The scheduler calculates the next report timeout of each node timer based on the current system timestamp and the state of the
+ *  ReadHandlers. If the ReadHandler is not reportable, the timeout is the difference between the next max interval and now. If the
+ *  ReadHandler is reportable, the timeout is the difference between the next min interval and now. If that min interval is in the
+ *  past, the scheduler directly calls the TimerFired() method instead of starting a timer.
+ *
+ *
+
  */
 class ReportSchedulerImpl : public ReportScheduler
 {
@@ -55,32 +68,35 @@ class ReportSchedulerImpl : public ReportScheduler
     // ICDStateObserver
 
     /**
-     * @brief When the ICD changes to Idle, no action is taken in this implementation.
+     * @brief This implementation is not attempting any synchronization on external events as each Node is scheduled independently
+     *        solely based on its ReadHandler's state. Therefore, no synchronization action on the ICDState is needed in this
+     *        implementation.
      */
     void OnTransitionToIdle() override{};
 
     /**
-     * @brief When the ICD changes to Active, this implementation will trigger a report emission on each ReadHandler that is not
-     * blocked on its min interval.
+     * @brief When the ICD transitions to Active mode, this implementation will trigger a report emission on each ReadHandler that
+     * is not blocked by its min interval.
      *
-     * @note Most action triggering a change to the Active mode already trigger a report emission, so this method is optionnal as it
-     * might be redundant.
+     * @note Most of the actions that trigger a change to the Active mode already trigger a report emission (e.g. Event or Attribute
+     * change), so this method is optional as it might be redundant.
      */
     void OnEnterActiveMode() override;
 
     /**
-     * @brief When the ICD changes operation mode, no action is taken in this implementation.
+     * @brief Similar to the OnTransitionToIdle() method, this implementation does not attempt any synchronization on ICD events,
+     *        therefore no action is needed on the ICDModeChange() method.
      */
     void OnICDModeChange() override{};
 
     // ReadHandlerObserver
 
     /**
-     * @brief When a ReadHandler is added, adds a node and register it in the scheduler node pool. Scheduling the report here is
-     * un-necessary since the ReadHandler will call MoveToState(HandlerState::CanStartReporting);, which will call
-     * OnBecameReportable() and schedule the report.
+     * @brief When a ReadHandler is created for a subscription, the scheduler adds a node and registers it in the scheduler node
+     * pool. Scheduling the report here is unnecessary since the ReadHandler will call
+     * MoveToState(HandlerState::CanStartReporting);, which will call OnBecameReportable() and schedule a report.
      *
-     * @note This method sets a now Timestamp that is used to calculate the next report timeout.
+     * @note This method sets a timestamp to the call time that is used as an input parameter by the ScheduleReport method.
      */
     void OnSubscriptionEstablished(ReadHandler * aReadHandler) final;
 
@@ -94,9 +110,6 @@ class ReportSchedulerImpl : public ReportScheduler
     /**
      * @brief When a ReadHandler report is sent, recalculate and reschedule the report.
      *
-     * @note This method is called after the report is sent, so the ReadHandler is no longer reportable, and thus CanBeSynced and
-     * EngineRunScheduled of the node associated to the ReadHandler are set to false in this method.
-     *
      * @note This method sets a now Timestamp that is used to calculate the next report timeout.
      */
     void OnSubscriptionReportSent(ReadHandler * aReadHandler) final;
@@ -106,22 +119,27 @@ class ReportSchedulerImpl : public ReportScheduler
      */
     void OnReadHandlerDestroyed(ReadHandler * aReadHandler) override;
 
+    /**
+     * @brief Checks if a report is scheduled for the ReadHandler by checking if the timer is active.
+     *
+     *      @note If the CalculateNextReportTimeout outputs 0, the TimerFired() will be called directly instead of starting a timer,
+     *      so this method will return false.
+     */
     virtual bool IsReportScheduled(ReadHandler * aReadHandler);
 
     void ReportTimerCallback() override;
 
 protected:
     /**
-     * @brief Schedule a report for the ReadHandler associated to the node.
+     * @brief Schedule a report for the ReadHandler associated with a ReadHandlerNode.
      *
-     *  If a report is already scheduled for the ReadHandler, cancel it and schedule a new one.
-     *  If the timeout is 0, directly calls the TimerFired() method of the node instead of scheduling a report.
+     * @note If a report is already scheduled for the ReadHandler, this method will cancel it and schedule a new one.
      *
      * @param[in] timeout The timeout to schedule the report.
-     * @param[in] node The node associated to the ReadHandler.
+     * @param[in] node The node associated with the ReadHandler.
      * @param[in] now The current system timestamp.
      *
-     * @return CHIP_ERROR CHIP_NO_ERROR on success, timer related error code otherwise (This can only fail on starting the timer)
+     * @return CHIP_ERROR CHIP_NO_ERROR on success, timer-related error code otherwise (This can only fail on starting the timer)
      */
     virtual CHIP_ERROR ScheduleReport(Timeout timeout, ReadHandlerNode * node, const Timestamp & now);
     void CancelReport(ReadHandler * aReadHandler);
@@ -131,19 +149,14 @@ class ReportSchedulerImpl : public ReportScheduler
     friend class chip::app::reporting::TestReportScheduler;
 
     /**
-     * @brief Find the next timer when a report should be scheduled for a ReadHandler.
+     * @brief Find the next timestamp when a report should be scheduled for a ReadHandler.
      *
-     * @param[out] timeout The timeout to calculate.
+     * @param[out] timeout The timeout calculated from the "now" timestamp provided as an input parameter.
      * @param[in] aNode The node associated to the ReadHandler.
      * @param[in] now The current system timestamp.
      *
      * @return CHIP_ERROR CHIP_NO_ERROR on success or CHIP_ERROR_INVALID_ARGUMENT if aNode is not in the pool.
      *
-     * The logic is as follows:
-     * - If the ReadHandler is reportable now, the timeout is 0.
-     * - If the ReadHandler is reportable, but the current timestamp is earlier thant the next min interval's timestamp, the timeout
-     * is the delta between the next min interval and now.
-     * - If the ReadHandler is not reportable, the timeout is the difference between the next max interval and now.
      */
     virtual CHIP_ERROR CalculateNextReportTimeout(Timeout & timeout, ReadHandlerNode * aNode, const Timestamp & now);
 };

src/app/reporting/SynchronizedReportSchedulerImpl.cpp

@@ -109,7 +109,9 @@ CHIP_ERROR SynchronizedReportSchedulerImpl::FindNextMinInterval(const Timestamp
     System::Clock::Timestamp latest = now;
 
     mNodesPool.ForEachActiveObject([&latest, this](ReadHandlerNode * node) {
-        // We only consider the min interval if the handler is reportable to prevent holding the reports
+        // We only consider the min interval if the handler is reportable. This is done to have only reportable handlers
+        // contribute to setting the next min interval and avoid delaying a report for a handler that would not generate
+        // a one on its min interval anyway.
         if (node->GetMinTimestamp() > latest && this->IsReadHandlerReportable(node->GetReadHandler()) &&
             node->GetMinTimestamp() <= this->mNextMaxTimestamp)
         {
@@ -135,8 +137,9 @@ CHIP_ERROR SynchronizedReportSchedulerImpl::CalculateNextReportTimeout(Timeout &
 
     // Find out if any handler is reportable now or at the next min interval
     mNodesPool.ForEachActiveObject([&reportableNow, &reportableAtMin, this, now](ReadHandlerNode * node) {
-        // If a node is already scheduled, we don't need to check if it is reportable now, unless a chunked report is in progress
-        // in which case we need to keep scheduling engine runs until the report is complete
+        // If a node is already scheduled, we don't need to check if it is reportable now unless a chunked report is in progress.
+        // In this case, the node will be Reportable, as it is impossible to have node->IsChunkedReport() == true without being
+        // reportable, therefore we need to keep scheduling engine runs until the report is complete
         if (!node->IsEngineRunScheduled() || node->IsChunkedReport())
         {
             if (node->IsReportableNow(now))
@@ -182,7 +185,8 @@ void SynchronizedReportSchedulerImpl::TimerFired()
     mNodesPool.ForEachActiveObject([now, &firedEarly](ReadHandlerNode * node) {
         if (node->GetMinTimestamp() <= now)
         {
-            // Mark the handler as CanBeSynced if the min interval has elapsed so it will emit a report on the next engine run
+            // Since this handler can now report whenever it wants to, mark it as allowed to report if any other handler is
+            // reporting using the CanBeSynced flag.
             node->SetCanBeSynced(true);
         }
 
@@ -201,14 +205,17 @@ void SynchronizedReportSchedulerImpl::TimerFired()
 
     if (firedEarly)
     {
-        // If we fired the timer early, we need to recalculate the next report timeout and reschedule the report
+        // If we fired the timer early, we need to recalculate the next report timeout and reschedule the report so it can run when
+        // at least one read handler is reportable. Here we can't set the SetEngineRunScheduled flag to true, because this flag
+        // allows handlers to generate reports before their min (assuming their min has elapsed from the timer's perspective but not
+        // from the monotonic timer), and we don't know which handler was the one that should be reportable.
         Timeout timeout = Milliseconds32(0);
         ReturnOnFailure(CalculateNextReportTimeout(timeout, nullptr, now));
         ScheduleReport(timeout, nullptr, now);
     }
     else
     {
-        // If we did not fire the timer early, we can schedule an engine run
+        // If we have a reportable handler, we can schedule an engine run
         InteractionModelEngine::GetInstance()->GetReportingEngine().ScheduleRun();
     }
 }

src/app/reporting/SynchronizedReportSchedulerImpl.h

@@ -33,41 +33,43 @@ using TimerDelegate   = ReportScheduler::TimerDelegate;
 /**
  * @class Synchronized ReportSchedulerImpl
  *
- * @brief This class extends ReportSchedulerImpl and overrides it's scheduling logic.
+ * @brief This class extends ReportSchedulerImpl and overrides its scheduling logic.
  *
- * It only overrides Observers method where the scheduling logic make it necessary, the others are kept as is.
+ * It overrides the OnTransitionToIdle methods from ReadHandler::Observer.
  *
- * It inherits from TimerContext so that it can be used as a TimerDelegate instead on relying on the nodes to schedule themselves.
+ * It inherits from TimerContext so that it can be used as a TimerDelegate instead of relying on the nodes to schedule themselves.
  *
  * ## Scheduling Logic
  *
  * This class implements a scheduling logic that aims to make all ReadHandlers report at the same time when possible.
- * The goal is to minimize the different times a device wakes up to report, and thus this aims to schedule all reports at the latest
+ * The goal is to minimize the number of times a device wakes up to report, and thus this aims to schedule all reports at the latest
  * possible time while ensuring that all reports get sent before their max interval.
  *
- * The logic also aims to minimize the impact on the responsivity of the device.
+ * The logic also aims to minimize the impact on the responsiveness of the device.
  *
  * The scheduling logic is as follows:
- * - The CalculateNextReportTimeout is called by the same ReadHandler Observer callbacks than the non-synchronized implementation:
+ * - The CalculateNextReportTimeout is called by any ReadHandler methods that affect when/whether a report should be sent. These
+ *   are:
  *      * OnSubscriptionEstablished,
  *      * OnBecameReportable,
  *      * OnSubscriptionReportSent
  *
- * - The Synchronized Scheduler keeps track of the next min and max interval timestamps. It updates in CalculateNextReportTimeout
+ * - The Synchronized Scheduler keeps track of the next min and max interval timestamps and updates them in
+ *   CalculateNextReportTimeout
  *
  * - The next max interval is calculated as the earliest max interval of all the registered ReadHandlersNodes.
  *
  * - The next min interval is calculated as the latest min interval of the registered ReadHandlersNodes that:
  *     * Have a min timestamp greater than the current time
- *     * Are Reportable (this prevents a ReadHandler that is not reportable to hold the report of all the others)
+ *     * Are Reportable (this prevents a ReadHandler that is not reportable from blocking the reporting of other ReadHandlers)
  *       TODO: Assess if we want to keep this behavior or simply let the min interval be the earliest min interval to prevent cases
  *             where a ReadHandler with a dirty path but a very high min interval blocks all reports
- * - If no ReadHandlerNode matches min interval  the criteria, the next min interval is set to current timestamp.
+ * - If no ReadHandlerNode matches the min interval criteria, the next min interval is set to the current timestamp.
  *
  * - The next report timeout is calculated in CalculatedNextReportTimeout based on the next min and max interval timestamps, as well
  * as the status of each ReadHandlerNode in the pool.
  *
- * @note Unlike the non-synchronized implementation, the Synchronized Scheduler will reschedule itself in the event where a timer
+ * @note Unlike the non-synchronized implementation, the Synchronized Scheduler will reschedule itself in the event that a timer
  * fires before a reportable timestamp is reached.
  *
  * @note In this implementation, nodes still keep track of their own min and max interval timestamps.
@@ -86,15 +88,15 @@ class SynchronizedReportSchedulerImpl : public ReportSchedulerImpl, public Timer
 
     /** @brief Callback called when the report timer expires to schedule an engine run regardless of the state of the ReadHandlers,
      *
-     * It loops through all handlers and sets their CanBeSynced flag to true if the current timstamp is greater than
+     * It loops through all handlers and sets their CanBeSynced flag to true if the current timestamp is greater than
      * their respective minimal timestamps.
      *
      * While looping, it checks if any handler is reportable now. If not, we recalculate the next report timeout and reschedule the
      * report.
      *
-     * If a Readhangler is reportable now, an engine run is scheduled.
+     * If a Readhandler is reportable now, an engine run is scheduled.
      *
-     * If the timer expires after all nodes were unregistered, no action is taken.
+     * If the timer expires after all nodes are unregistered, no action is taken.
      */
     void TimerFired() override;
 
@@ -104,11 +106,11 @@ class SynchronizedReportSchedulerImpl : public ReportSchedulerImpl, public Timer
      *
      * If a report is already scheduled, cancel it and schedule a new one.
      *
-     * @param[in] timeout The timeout to schedule the report.
-     * @param[in] node The node associated to the ReadHandler.
+     * @param[in] timeout The delay before the report will happen.
+     * @param[in] node The node associated with the ReadHandler.
      * @param[in] now The current system timestamp.
      *
-     * @return CHIP_ERROR CHIP_NO_ERROR on success, timer related error code otherwise (This can only fail on starting the timer)
+     * @return CHIP_ERROR CHIP_NO_ERROR on success, timer-related error code otherwise (This can only fail on starting the timer)
      */
     CHIP_ERROR ScheduleReport(System::Clock::Timeout timeout, ReadHandlerNode * node, const Timestamp & now) override;
     void CancelReport();
@@ -119,7 +121,8 @@ class SynchronizedReportSchedulerImpl : public ReportSchedulerImpl, public Timer
     /**
      * @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.
+     * timestamp, this will set the "now" parameter as the common minimum timestamp, thus allowing the report to be sent
+     * immediately.
      *
      * @param[in] now The current system timestamp, set by the event that triggered the call of this method.
      *
@@ -150,13 +153,23 @@ class SynchronizedReportSchedulerImpl : public ReportSchedulerImpl, public Timer
      *      and the current time.
      *   * If no ReadHandlerNode is reportable, the timeout is set to the difference between the Scheduler's max timestamp and the
      *      current time.
+      @note When looping through the ReadHandlerNodes, the IsEngineRunScheduled flag is used to prevent calling ScheduleRun on a
+            ReadHandler that already has an engine run scheduled, which would cause an endless report loop in some cases. The only
+            reason why we would want to call ScheduleRun on a node that already has an engine run scheduled is if the ongoing report
+            is chunked, which means that the report is not fully sent yet and that the EngineRun should be scheduled again until
+     there are no chunks left.
+
+          The Endless Reporting Loop Scenario would be:
+          1. At least two ReadHandlers are registered to the Scheduler
+          2. ScheduleRun() is called with 2 reportable ReadHandlers (meaning they both return true to IsReportableNow())
+          3. The Scheduler sends the first report and calls OnSubscriptionReportSent on the ReadHandler
+          4. OnSubscriptionReportSent calls CalculateNextReportTimeout, which loops through all ReadHandlers and finds that a least
+           one ReadHandler is reportable now, and thus sets the timeout to 0.
+          5. OnSubscriptionReportSent then calls ScheduleReport with a timeout of 0, which calls TimerFired on the Scheduler
+          6. If the MinInterval of the ReadHandler is 0, the Scheduler will set the CanBeSynced flag to true, and the
+             IsReportableNow Will return true since (now >= MinTimestamp || CanBeSynced()) will be true.
+          7. ScheduleRun() will be called on the ReadHandler with 2 reportable ReadHandlers, and the loop will start again.
      *
-     * @note Since this method is called after the OnSubscriptionReportSent callback, to avoid an endless reporting loop, Nodes with
-     * the IsEngineRunScheduled flag set are ignored when finding if the Scheduler should report at min, max or now.
-     *
-     * @note If a ReadHandler's report is Chunked, the IsEngineRunScheduled is ignored since we do want to keep rescheduling the
-     *       report to the now timestamp until it is fully sent. IsChunkedReport is used to prevent starting a chunked report and
-     * then waiting on the max interval after the first chunk is sent.
      */
     CHIP_ERROR CalculateNextReportTimeout(Timeout & timeout, ReadHandlerNode * aReadHandlerNode, const Timestamp & now) override;