Merge 1c92916 into 0b4caea

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