Put back rescheduling in the OnReadHandlerSubscribed, added a CanEmit…

…ReadReport() method for readhandler for read request. Fixed call order in TestReportScheduler tests
project-chip · Aug 8, 2023 · ad530a1 · ad530a1
1 parent 61100df
commit ad530a1
Show file tree

Hide file tree

Showing 5 changed files with 21 additions and 18 deletions.
diff --git a/src/app/ReadHandler.cpp b/src/app/ReadHandler.cpp
@@ -604,7 +604,7 @@ void ReadHandler::MoveToState(const HandlerState aTargetState)
     //
     if (aTargetState == HandlerState::CanStartReporting)
     {
-        if (IsType(ReadHandler::InteractionType::Read) || IsPriming())
+        if (CanEmitReadReport())
         {
             InteractionModelEngine::GetInstance()->GetReportingEngine().ScheduleRun();
         }
@@ -884,7 +884,7 @@ void ReadHandler::SetStateFlag(ReadHandlerFlags aFlag, bool aValue)
     // If we became reportable, schedule a reporting run.
     if (!oldReportable && ShouldStartReporting())
     {
-        if (IsType(ReadHandler::InteractionType::Read) || IsPriming())
+        if (CanEmitReadReport())
         {
             InteractionModelEngine::GetInstance()->GetReportingEngine().ScheduleRun();
         }

diff --git a/src/app/ReadHandler.h b/src/app/ReadHandler.h
@@ -166,8 +166,8 @@ class ReadHandler : public Messaging::ExchangeDelegate
     public:
         virtual ~Observer() = default;
 
-        /// @brief Callback invoked to notify a ReadHandler was created and can be registered
-        /// @param[in] apReadHandler  ReadHandler getting added
+        /// @brief Callback invoked to notify a subscription was successfully established for a read handler
+        /// @param[in] apReadHandler  ReadHandler that completed its subscription
         virtual void OnReadHandlerSubscribed(ReadHandler * apReadHandler) = 0;
 
         /// @brief Callback invoked when a ReadHandler went from a non reportable state to a reportable state. Indicates to the
@@ -335,10 +335,11 @@ class ReadHandler : public Messaging::ExchangeDelegate
     bool ShouldStartReporting() const
     {
         // Important: Anything that changes ShouldStartReporting() from false to true
-        // (which can only happen for subscriptions) must call 
+        // (which can only happen for subscriptions) must call
         // mObserver->OnBecameReportable(this).
-        return mState == HandlerState::CanStartReporting && IsDirty();
+        return mState == HandlerState::CanStartReporting && (CanEmitReadReport() || IsDirty());
     }
+    bool CanEmitReadReport() const { return IsType(ReadHandler::InteractionType::Read) || IsPriming(); }
     bool CanStartReporting() const { return mState == HandlerState::CanStartReporting; }
     bool IsAwaitingReportResponse() const { return mState == HandlerState::AwaitingReportResponse; }
 

diff --git a/src/app/reporting/Engine.cpp b/src/app/reporting/Engine.cpp
@@ -636,8 +636,7 @@ void Engine::Run()
         ReadHandler * readHandler = imEngine->ActiveHandlerAt(mCurReadHandlerIdx % (uint32_t) imEngine->mReadHandlers.Allocated());
         VerifyOrDie(readHandler != nullptr);
 
-        if (readHandler->IsType(ReadHandler::InteractionType::Read) || readHandler->IsPriming() ||
-            imEngine->GetReportScheduler()->IsReportableNow(readHandler))
+        if (readHandler->CanEmitReadReport() || imEngine->GetReportScheduler()->IsReportableNow(readHandler))
         {
             mRunningReadHandler = readHandler;
             CHIP_ERROR err      = BuildAndSendSingleReportData(readHandler);

diff --git a/src/app/reporting/ReportSchedulerImpl.cpp b/src/app/reporting/ReportSchedulerImpl.cpp
@@ -68,6 +68,10 @@ void ReportSchedulerImpl::OnReadHandlerSubscribed(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;
+    CalculateNextReportTimeout(newTimeout, newNode);
+    ScheduleReport(newTimeout, newNode);
 }
 
 /// @brief When a ReadHandler becomes reportable, schedule, recalculate and reschedule the report.

diff --git a/src/app/tests/TestReportScheduler.cpp b/src/app/tests/TestReportScheduler.cpp
@@ -351,8 +351,9 @@ class TestReportScheduler
         NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == readHandler1->SetMaxReportingInterval(2));
         NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == readHandler1->SetMinReportingIntervalForTests(1));
         sScheduler.OnReadHandlerSubscribed(readHandler1);
-        readHandler1->ClearStateFlag(ReadHandler::ReadHandlerFlags::PrimingReports);
         readHandler1->MoveToState(ReadHandler::HandlerState::CanStartReporting);
+        readHandler1->ClearStateFlag(ReadHandler::ReadHandlerFlags::PrimingReports);
+
         readHandler1->ForceDirtyState();
 
         // Clean read handler, will be triggered at max interval
@@ -362,17 +363,17 @@ class TestReportScheduler
         NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == readHandler2->SetMaxReportingInterval(3));
         NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == readHandler2->SetMinReportingIntervalForTests(0));
         sScheduler.OnReadHandlerSubscribed(readHandler2);
-        readHandler2->ClearStateFlag(ReadHandler::ReadHandlerFlags::PrimingReports);
         readHandler2->MoveToState(ReadHandler::HandlerState::CanStartReporting);
+        readHandler2->ClearStateFlag(ReadHandler::ReadHandlerFlags::PrimingReports);
 
         // Clean read handler, will be triggered at max interval, but will be cancelled before
         ReadHandler * readHandler3 =
             readHandlerPool.CreateObject(nullCallback, exchangeCtx, ReadHandler::InteractionType::Subscribe, &sScheduler);
         NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == readHandler3->SetMaxReportingInterval(3));
         NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == readHandler3->SetMinReportingIntervalForTests(0));
         sScheduler.OnReadHandlerSubscribed(readHandler3);
-        readHandler3->ClearStateFlag(ReadHandler::ReadHandlerFlags::PrimingReports);
         readHandler3->MoveToState(ReadHandler::HandlerState::CanStartReporting);
+        readHandler3->ClearStateFlag(ReadHandler::ReadHandlerFlags::PrimingReports);
 
         // Confirms that none of the ReadHandlers are currently reportable
         NL_TEST_ASSERT(aSuite, !sScheduler.IsReportableNow(readHandler1));
@@ -428,8 +429,8 @@ class TestReportScheduler
             readHandlerPool.CreateObject(nullCallback, exchangeCtx, ReadHandler::InteractionType::Subscribe, &sScheduler);
         NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == readHandler->SetMaxReportingInterval(2));
         NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == readHandler->SetMinReportingIntervalForTests(1));
-
         sScheduler.OnReadHandlerSubscribed(readHandler);
+        readHandler->MoveToState(ReadHandler::HandlerState::CanStartReporting);
         readHandler->ClearStateFlag(ReadHandler::ReadHandlerFlags::PrimingReports);
 
         // Verifies OnReadHandlerSubscribed registered the ReadHandler in the scheduler
@@ -444,8 +445,6 @@ class TestReportScheduler
         NL_TEST_ASSERT(aSuite, node->GetMinTimestamp().count() == 1000);
         NL_TEST_ASSERT(aSuite, node->GetMaxTimestamp().count() == 2000);
 
-        // Do those manually to avoid scheduling an engine run
-        readHandler->MoveToState(ReadHandler::HandlerState::CanStartReporting);
         NL_TEST_ASSERT(aSuite, sScheduler.IsReportScheduled(readHandler));
 
         NL_TEST_ASSERT(aSuite, nullptr != node);
@@ -511,18 +510,18 @@ class TestReportScheduler
         NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == readHandler1->SetMaxReportingInterval(2));
         NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == readHandler1->SetMinReportingIntervalForTests(0));
         syncScheduler.OnReadHandlerSubscribed(readHandler1);
+        readHandler1->MoveToState(ReadHandler::HandlerState::CanStartReporting);
         readHandler1->ClearStateFlag(ReadHandler::ReadHandlerFlags::PrimingReports);
         ReadHandlerNode * node1 = syncScheduler.FindReadHandlerNode(readHandler1);
-        readHandler1->MoveToState(ReadHandler::HandlerState::CanStartReporting);
 
         ReadHandler * readHandler2 =
             readHandlerPool.CreateObject(nullCallback, exchangeCtx, ReadHandler::InteractionType::Subscribe, &syncScheduler);
         NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == readHandler2->SetMaxReportingInterval(3));
         NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == readHandler2->SetMinReportingIntervalForTests(1));
         syncScheduler.OnReadHandlerSubscribed(readHandler2);
+        readHandler2->MoveToState(ReadHandler::HandlerState::CanStartReporting);
         readHandler2->ClearStateFlag(ReadHandler::ReadHandlerFlags::PrimingReports);
         ReadHandlerNode * node2 = syncScheduler.FindReadHandlerNode(readHandler2);
-        readHandler2->MoveToState(ReadHandler::HandlerState::CanStartReporting);
 
         // Confirm all handler are currently registered in the scheduler
         NL_TEST_ASSERT(aSuite, syncScheduler.GetNumReadHandlers() == 2);
@@ -626,9 +625,9 @@ class TestReportScheduler
         NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == readHandler3->SetMaxReportingInterval(3));
         NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == readHandler3->SetMinReportingIntervalForTests(2));
         syncScheduler.OnReadHandlerSubscribed(readHandler3);
+        readHandler3->MoveToState(ReadHandler::HandlerState::CanStartReporting);
         readHandler3->ClearStateFlag(ReadHandler::ReadHandlerFlags::PrimingReports);
         ReadHandlerNode * node3 = syncScheduler.FindReadHandlerNode(readHandler3);
-        readHandler3->MoveToState(ReadHandler::HandlerState::CanStartReporting);
 
         // Confirm all handler are currently registered in the scheduler
         NL_TEST_ASSERT(aSuite, syncScheduler.GetNumReadHandlers() == 3);
@@ -684,9 +683,9 @@ class TestReportScheduler
         NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == readHandler4->SetMaxReportingInterval(1));
         NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == readHandler4->SetMinReportingIntervalForTests(0));
         syncScheduler.OnReadHandlerSubscribed(readHandler4);
+        readHandler4->MoveToState(ReadHandler::HandlerState::CanStartReporting);
         readHandler4->ClearStateFlag(ReadHandler::ReadHandlerFlags::PrimingReports);
         ReadHandlerNode * node4 = syncScheduler.FindReadHandlerNode(readHandler4);
-        readHandler4->MoveToState(ReadHandler::HandlerState::CanStartReporting);
 
         // Confirm all handler are currently registered in the scheduler
         NL_TEST_ASSERT(aSuite, syncScheduler.GetNumReadHandlers() == 4);