[ReadHandler] Removed Scheduling of report from OnReadHandlerCreated

src/app/ReadHandler.cpp

@@ -79,7 +79,6 @@ ReadHandler::ReadHandler(ManagementCallback & apCallback, Messaging::ExchangeCon
 
     VerifyOrDie(observer != nullptr);
     mObserver = observer;
-    mObserver->OnReadHandlerCreated(this);
 }
 
 #if CHIP_CONFIG_PERSIST_SUBSCRIPTIONS
@@ -92,7 +91,6 @@ ReadHandler::ReadHandler(ManagementCallback & apCallback, Observer * observer) :
 
     VerifyOrDie(observer != nullptr);
     mObserver = observer;
-    mObserver->OnReadHandlerCreated(this);
 }
 
 void ReadHandler::ResumeSubscription(CASESessionManager & caseSessionManager,
@@ -235,6 +233,7 @@ CHIP_ERROR ReadHandler::OnStatusResponse(Messaging::ExchangeContext * apExchange
                 {
                     appCallback->OnSubscriptionEstablished(*this);
                 }
+                mObserver->OnReadHandlerSubscribed(this);
             }
         }
         else
@@ -246,10 +245,10 @@ CHIP_ERROR ReadHandler::OnStatusResponse(Messaging::ExchangeContext * apExchange
             return CHIP_NO_ERROR;
         }
 
-        MoveToState(HandlerState::GeneratingReports);
+        MoveToState(HandlerState::CanStartReporting);
         break;
 
-    case HandlerState::GeneratingReports:
+    case HandlerState::CanStartReporting:
     case HandlerState::Idle:
     default:
         err = CHIP_ERROR_INCORRECT_STATE;
@@ -262,7 +261,7 @@ CHIP_ERROR ReadHandler::OnStatusResponse(Messaging::ExchangeContext * apExchange
 
 CHIP_ERROR ReadHandler::SendStatusReport(Protocols::InteractionModel::Status aStatus)
 {
-    VerifyOrReturnLogError(mState == HandlerState::GeneratingReports, CHIP_ERROR_INCORRECT_STATE);
+    VerifyOrReturnLogError(mState == HandlerState::CanStartReporting, CHIP_ERROR_INCORRECT_STATE);
     if (IsPriming() || IsChunkedReport())
     {
         mSessionHandle.Grab(mExchangeCtx->GetSessionHandle());
@@ -286,7 +285,7 @@ CHIP_ERROR ReadHandler::SendStatusReport(Protocols::InteractionModel::Status aSt
 
 CHIP_ERROR ReadHandler::SendReportData(System::PacketBufferHandle && aPayload, bool aMoreChunks)
 {
-    VerifyOrReturnLogError(mState == HandlerState::GeneratingReports, CHIP_ERROR_INCORRECT_STATE);
+    VerifyOrReturnLogError(mState == HandlerState::CanStartReporting, CHIP_ERROR_INCORRECT_STATE);
     VerifyOrDie(!IsAwaitingReportResponse()); // Should not be reportable!
     if (IsPriming() || IsChunkedReport())
     {
@@ -456,7 +455,7 @@ CHIP_ERROR ReadHandler::ProcessReadRequest(System::PacketBufferHandle && aPayloa
     ReturnErrorOnFailure(readRequestParser.GetIsFabricFiltered(&isFabricFiltered));
     SetStateFlag(ReadHandlerFlags::FabricFiltered, isFabricFiltered);
     ReturnErrorOnFailure(readRequestParser.ExitContainer());
-    MoveToState(HandlerState::GeneratingReports);
+    MoveToState(HandlerState::CanStartReporting);
 
     mExchangeCtx->WillSendMessage();
 
@@ -574,8 +573,8 @@ const char * ReadHandler::GetStateStr() const
         return "Idle";
     case HandlerState::AwaitingDestruction:
         return "AwaitingDestruction";
-    case HandlerState::GeneratingReports:
-        return "GeneratingReports";
+    case HandlerState::CanStartReporting:
+        return "CanStartReporting";
 
     case HandlerState::AwaitingReportResponse:
         return "AwaitingReportResponse";
@@ -603,10 +602,17 @@ void ReadHandler::MoveToState(const HandlerState aTargetState)
     // If we just unblocked sending reports, let's go ahead and schedule the reporting
     // engine to run to kick that off.
     //
-    if (aTargetState == HandlerState::GeneratingReports)
+    if (aTargetState == HandlerState::CanStartReporting)
     {
-        // mObserver will take care of scheduling the report as soon as allowed
-        mObserver->OnBecameReportable(this);
+        if (IsType(ReadHandler::InteractionType::Read) || IsPriming())
+        {
+            InteractionModelEngine::GetInstance()->GetReportingEngine().ScheduleRun();
+        }
+        else
+        {
+            // If we became reportable, the scheduler will schedule a run as soon as allowed
+            mObserver->OnBecameReportable(this);
+        }
     }
 }
 
@@ -785,7 +791,7 @@ CHIP_ERROR ReadHandler::ProcessSubscribeRequest(System::PacketBufferHandle && aP
     SetStateFlag(ReadHandlerFlags::FabricFiltered, isFabricFiltered);
     ReturnErrorOnFailure(Crypto::DRBG_get_bytes(reinterpret_cast<uint8_t *>(&mSubscriptionId), sizeof(mSubscriptionId)));
     ReturnErrorOnFailure(subscribeRequestParser.ExitContainer());
-    MoveToState(HandlerState::GeneratingReports);
+    MoveToState(HandlerState::CanStartReporting);
 
     mExchangeCtx->WillSendMessage();
 
@@ -872,14 +878,21 @@ void ReadHandler::ForceDirtyState()
 
 void ReadHandler::SetStateFlag(ReadHandlerFlags aFlag, bool aValue)
 {
-    bool oldReportable = IsReportable();
+    bool oldReportable = ShouldStartReporting();
     mFlags.Set(aFlag, aValue);
 
     // If we became reportable, schedule a reporting run.
-    if (!oldReportable && IsReportable())
+    if (!oldReportable && ShouldStartReporting())
     {
-        // If we became reportable, the scheduler will schedule a run as soon as allowed
-        mObserver->OnBecameReportable(this);
+        if (IsType(ReadHandler::InteractionType::Read) || IsPriming())
+        {
+            InteractionModelEngine::GetInstance()->GetReportingEngine().ScheduleRun();
+        }
+        else
+        {
+            // If we became reportable, the scheduler will schedule a run as soon as allowed
+            mObserver->OnBecameReportable(this);
+        }
     }
 }
 
@@ -895,7 +908,7 @@ void ReadHandler::HandleDeviceConnected(void * context, Messaging::ExchangeManag
 
     _this->mSessionHandle.Grab(sessionHandle);
 
-    _this->MoveToState(HandlerState::GeneratingReports);
+    _this->MoveToState(HandlerState::CanStartReporting);
 
     ObjectList<AttributePathParams> * attributePath = _this->mpAttributePathList;
     while (attributePath)

src/app/ReadHandler.h

@@ -168,12 +168,11 @@ class ReadHandler : public Messaging::ExchangeDelegate
 
         /// @brief Callback invoked to notify a ReadHandler was created and can be registered
         /// @param[in] apReadHandler  ReadHandler getting added
-        virtual void OnReadHandlerCreated(ReadHandler * apReadHandler) = 0;
+        virtual void OnReadHandlerSubscribed(ReadHandler * apReadHandler) = 0;
 
-        /// @brief Callback invoked when a ReadHandler went from a non reportable state to a reportable state so a report can be
-        /// sent immediately if the minimal interval allows it. Otherwise the report should be rescheduled to the earliest time
-        /// allowed.
-        /// @param[in] apReadHandler  ReadHandler that became dirty
+        /// @brief Callback invoked when a ReadHandler went from a non reportable state to a reportable state. Indicates to the
+        /// observer that a report should be emitted if the min interval allows it.
+        /// @param[in] apReadHandler  ReadHandler that became dirty and in HandlerState::CanStartReporting state
         virtual void OnBecameReportable(ReadHandler * apReadHandler) = 0;
 
         /// @brief Callback invoked when the read handler needs to make sure to send a message to the subscriber within the next
@@ -333,13 +332,14 @@ class ReadHandler : public Messaging::ExchangeDelegate
     bool IsIdle() const { return mState == HandlerState::Idle; }
 
     /// @brief Returns whether the ReadHandler is in a state where it can send a report and there is data to report.
-    bool IsReportable() const
+    bool ShouldStartReporting() const
     {
-        // Important: Anything that changes the state IsReportable must call mObserver->OnBecameReportable(this) for the scheduler
-        // to plan the next run accordingly.
-        return mState == HandlerState::GeneratingReports && IsDirty();
+        // Important: Anything that changes the state ShouldStartReporting() must either call mObserver->OnBecameReportable(this)
+        // for active subscription or InteractionModelEngine::GetInstance()->GetReportingEngine().ScheduleRun() for read request and
+        // priming subscription.
+        return mState == HandlerState::CanStartReporting && IsDirty();
     }
-    bool IsGeneratingReports() const { return mState == HandlerState::GeneratingReports; }
+    bool CanStartReporting() const { return mState == HandlerState::CanStartReporting; }
     bool IsAwaitingReportResponse() const { return mState == HandlerState::AwaitingReportResponse; }
 
     // Resets the path iterator to the beginning of the whole report for generating a series of new reports.
@@ -418,14 +418,14 @@ class ReadHandler : public Messaging::ExchangeDelegate
     friend class chip::app::reporting::Engine;
     friend class chip::app::InteractionModelEngine;
 
-    // The report scheduler needs to be able to access StateFlag private functions IsReportable(), IsGeneratingReports(),
+    // The report scheduler needs to be able to access StateFlag private functions ShouldStartReporting(), CanStartReporting(),
     // ForceDirtyState() and IsDirty() to know when to schedule a run so it is declared as a friend class.
     friend class chip::app::reporting::ReportScheduler;
 
     enum class HandlerState : uint8_t
     {
         Idle,                   ///< The handler has been initialized and is ready
-        GeneratingReports,      ///< The handler has is now capable of generating reports and may generate one immediately
+        CanStartReporting,      ///< The handler has is now capable of generating reports and may generate one immediately
                                 ///< or later when other criteria are satisfied (e.g hold-off for min reporting interval).
         AwaitingReportResponse, ///< The handler has sent the report to the client and is awaiting a status response.
         AwaitingDestruction,    ///< The object has completed its work and is awaiting destruction by the application.

src/app/reporting/Engine.cpp

@@ -636,7 +636,8 @@ void Engine::Run()
         ReadHandler * readHandler = imEngine->ActiveHandlerAt(mCurReadHandlerIdx % (uint32_t) imEngine->mReadHandlers.Allocated());
         VerifyOrDie(readHandler != nullptr);
 
-        if (imEngine->GetReportScheduler()->IsReportableNow(readHandler))
+        if (readHandler->IsType(ReadHandler::InteractionType::Read) || readHandler->IsPriming() ||
+            imEngine->GetReportScheduler()->IsReportableNow(readHandler))
         {
             mRunningReadHandler = readHandler;
             CHIP_ERROR err      = BuildAndSendSingleReportData(readHandler);
@@ -829,7 +830,7 @@ CHIP_ERROR Engine::SetDirty(AttributePathParams & aAttributePath)
             // We call AttributePathIsDirty for both read interactions and subscribe interactions, since we may send inconsistent
             // attribute data between two chunks. AttributePathIsDirty will not schedule a new run for read handlers which are
             // waiting for a response to the last message chunk for read interactions.
-            if (handler->IsGeneratingReports() || handler->IsAwaitingReportResponse())
+            if (handler->CanStartReporting() || handler->IsAwaitingReportResponse())
             {
                 for (auto object = handler->GetAttributePathList(); object != nullptr; object = object->mpNext)
                 {

src/app/reporting/ReportScheduler.h

@@ -82,7 +82,7 @@ class ReportScheduler : public ReadHandler::Observer, public ICDStateObserver
         {
             Timestamp now = mTimerDelegate->GetCurrentMonotonicTimestamp();
 
-            return (mReadHandler->IsGeneratingReports() &&
+            return (mReadHandler->CanStartReporting() &&
                     (now >= mMinTimestamp && (mReadHandler->IsDirty() || now >= mMaxTimestamp || now >= mSyncTimestamp)));
         }
 
@@ -139,9 +139,13 @@ class ReportScheduler : public ReadHandler::Observer, public ICDStateObserver
 
     /// @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)
+    {
+        ReadHandlerNode * node = FindReadHandlerNode(aReadHandler);
+        return (nullptr != node) ? node->IsReportableNow() : false;
+    }
     /// @brief Check if a ReadHandler is reportable without considering the timing
-    bool IsReadHandlerReportable(ReadHandler * aReadHandler) const { return aReadHandler->IsReportable(); }
+    bool IsReadHandlerReportable(ReadHandler * aReadHandler) const { return aReadHandler->ShouldStartReporting(); }
     /// @brief Sets the ForceDirty flag of a ReadHandler
     void HandlerForceDirtyState(ReadHandler * aReadHandler) { aReadHandler->ForceDirtyState(); }
 

src/app/reporting/ReportSchedulerImpl.cpp

@@ -55,7 +55,7 @@ void ReportSchedulerImpl::OnEnterActiveMode()
 }
 
 /// @brief When a ReadHandler is added, register it, which will schedule an engine run
-void ReportSchedulerImpl::OnReadHandlerCreated(ReadHandler * aReadHandler)
+void ReportSchedulerImpl::OnReadHandlerSubscribed(ReadHandler * aReadHandler)
 {
     ReadHandlerNode * newNode = FindReadHandlerNode(aReadHandler);
     // Handler must not be registered yet; it's just being constructed.
@@ -68,11 +68,6 @@ void ReportSchedulerImpl::OnReadHandlerCreated(ReadHandler * aReadHandler)
                     "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, recalculate and reschedule the report.

src/app/reporting/ReportSchedulerImpl.h

@@ -36,7 +36,7 @@ class ReportSchedulerImpl : public ReportScheduler
     void OnEnterActiveMode() override;
 
     // ReadHandlerObserver
-    void OnReadHandlerCreated(ReadHandler * aReadHandler) final;
+    void OnReadHandlerSubscribed(ReadHandler * aReadHandler) final;
     void OnBecameReportable(ReadHandler * aReadHandler) final;
     void OnSubscriptionAction(ReadHandler * aReadHandler) final;
     void OnReadHandlerDestroyed(ReadHandler * aReadHandler) override;

src/app/tests/TestReadInteraction.cpp

@@ -2225,7 +2225,7 @@ void TestReadInteraction::TestSubscribeUrgentWildcardEvent(nlTestSuite * apSuite
                        reportScheduler->GetMinTimestampForHandler(delegate.mpReadHandler) <
                            System::SystemClock().GetMonotonicTimestamp());
         NL_TEST_ASSERT(apSuite, !delegate.mpReadHandler->IsDirty());
-        NL_TEST_ASSERT(apSuite, !delegate.mpReadHandler->IsReportable());
+        NL_TEST_ASSERT(apSuite, !delegate.mpReadHandler->ShouldStartReporting());
 
         // And the non-urgent one should not have changed state either, since
         // it's waiting for the max-interval.
@@ -2236,7 +2236,7 @@ void TestReadInteraction::TestSubscribeUrgentWildcardEvent(nlTestSuite * apSuite
                        reportScheduler->GetMaxTimestampForHandler(nonUrgentDelegate.mpReadHandler) >
                            System::SystemClock().GetMonotonicTimestamp());
         NL_TEST_ASSERT(apSuite, !nonUrgentDelegate.mpReadHandler->IsDirty());
-        NL_TEST_ASSERT(apSuite, !nonUrgentDelegate.mpReadHandler->IsReportable());
+        NL_TEST_ASSERT(apSuite, !nonUrgentDelegate.mpReadHandler->ShouldStartReporting());
 
         // There should be no reporting run scheduled.  This is very important;
         // otherwise we can get a false-positive pass below because the run was
@@ -2248,12 +2248,12 @@ void TestReadInteraction::TestSubscribeUrgentWildcardEvent(nlTestSuite * apSuite
 
         // Urgent read handler should now be dirty, and reportable.
         NL_TEST_ASSERT(apSuite, delegate.mpReadHandler->IsDirty());
-        NL_TEST_ASSERT(apSuite, delegate.mpReadHandler->IsReportable());
+        NL_TEST_ASSERT(apSuite, delegate.mpReadHandler->ShouldStartReporting());
         NL_TEST_ASSERT(apSuite, reportScheduler->IsReadHandlerReportable(delegate.mpReadHandler));
 
         // Non-urgent read handler should not be reportable.
         NL_TEST_ASSERT(apSuite, !nonUrgentDelegate.mpReadHandler->IsDirty());
-        NL_TEST_ASSERT(apSuite, !nonUrgentDelegate.mpReadHandler->IsReportable());
+        NL_TEST_ASSERT(apSuite, !nonUrgentDelegate.mpReadHandler->ShouldStartReporting());
 
         // Still no reporting should have happened.
         NL_TEST_ASSERT(apSuite, !delegate.mGotEventResponse);