Review feedback

project-chip · Apr 19, 2022 · 13013f7 · 13013f7
1 parent 6065077
commit 13013f7
Show file tree

Hide file tree

Showing 3 changed files with 28 additions and 19 deletions.
diff --git a/src/app/ReadHandler.cpp b/src/app/ReadHandler.cpp
@@ -156,11 +156,7 @@ CHIP_ERROR ReadHandler::OnStatusResponse(Messaging::ExchangeContext * apExchange
     case HandlerState::AwaitingReportResponse:
         if (IsChunkedReport())
         {
-            MoveToState(HandlerState::GeneratingReports);
             mpExchangeCtx->WillSendMessage();
-
-            // Trigger ReportingEngine run for sending next chunk of data.
-            SuccessOrExit(err = InteractionModelEngine::GetInstance()->GetReportingEngine().ScheduleRun());
         }
         else if (IsType(InteractionType::Subscribe))
         {
@@ -181,21 +177,19 @@ CHIP_ERROR ReadHandler::OnStatusResponse(Messaging::ExchangeContext * apExchange
             }
             else
             {
-                MoveToState(HandlerState::GeneratingReports);
                 mpExchangeCtx = nullptr;
             }
-
-            //
-            // Schedule execution of the ReportingEngine to drive further report generation activity if needed
-            // on this and other subscription that have already full-filled their minimum reporting hold-off requirements
-            // (i.e OnUnblockHoldReportCallback was already called before we got this status response).
-            //
-            SuccessOrExit(err = InteractionModelEngine::GetInstance()->GetReportingEngine().ScheduleRun());
         }
         else
         {
+            //
+            // We're done processing a read, so let's close out and return.
+            //
             Close();
+            return CHIP_NO_ERROR;
         }
+
+        MoveToState(HandlerState::GeneratingReports);
         break;
 
     case HandlerState::GeneratingReports:
@@ -384,8 +378,6 @@ CHIP_ERROR ReadHandler::ProcessReadRequest(System::PacketBufferHandle && aPayloa
     ReturnErrorOnFailure(readRequestParser.ExitContainer());
     MoveToState(HandlerState::GeneratingReports);
 
-    ReturnErrorOnFailure(InteractionModelEngine::GetInstance()->GetReportingEngine().ScheduleRun());
-
     mpExchangeCtx->WillSendMessage();
 
     // There must be no code after the WillSendMessage() call that can cause
@@ -604,11 +596,25 @@ const char * ReadHandler::GetStateStr() const
 
 void ReadHandler::MoveToState(const HandlerState aTargetState)
 {
+    if (aTargetState == mState)
+    {
+        return;
+    }
+
     if (IsAwaitingReportResponse() && aTargetState != HandlerState::AwaitingReportResponse)
     {
         InteractionModelEngine::GetInstance()->GetReportingEngine().OnReportConfirm();
     }
 
+    //
+    // If we just unblocked sending reports, let's go ahead and schedule the reporting
+    // engine to run to run to kick that off.
+    //
+    if (aTargetState == HandlerState::GeneratingReports)
+    {
+        InteractionModelEngine::GetInstance()->GetReportingEngine().ScheduleRun();
+    }
+
     mState = aTargetState;
     ChipLogDetail(DataManagement, "IM RH moving to [%s]", GetStateStr());
 }
@@ -657,7 +663,6 @@ CHIP_ERROR ReadHandler::SendSubscribeResponse()
     ReturnErrorOnFailure(RefreshSubscribeSyncTimer());
 
     mIsPrimingReports = false;
-    MoveToState(HandlerState::GeneratingReports);
     return mpExchangeCtx->SendMessage(Protocols::InteractionModel::MsgType::SubscribeResponse, std::move(packet));
 }
 
@@ -741,8 +746,6 @@ CHIP_ERROR ReadHandler::ProcessSubscribeRequest(System::PacketBufferHandle && aP
     ReturnErrorOnFailure(subscribeRequestParser.ExitContainer());
     MoveToState(HandlerState::GeneratingReports);
 
-    InteractionModelEngine::GetInstance()->GetReportingEngine().ScheduleRun();
-
     mpExchangeCtx->WillSendMessage();
 
     return CHIP_NO_ERROR;

diff --git a/src/app/ReadHandler.h b/src/app/ReadHandler.h
@@ -280,8 +280,8 @@ class ReadHandler : public Messaging::ExchangeDelegate
     enum class HandlerState
     {
         Idle,                   ///< The handler has been initialized and is ready
-        GeneratingReports,      ///< The handler has received either a Read or Subscribe request and is the process of generating a
-                                ///< report.
+        GeneratingReports,      ///< The handler has is now capable of generating reports and may generate one immediately
+                                ///< or later when other criteria is satifified (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.
     };

diff --git a/src/app/tests/TestReadInteraction.cpp b/src/app/tests/TestReadInteraction.cpp
@@ -1825,6 +1825,12 @@ void TestReadInteraction::TestSubscribeWildcard(nlTestSuite * apSuite, void * ap
 
             ctx.DrainAndServiceIO();
 
+            //
+            // Not sure why I had to add this, and didn't have cycles to figure out why.
+            // Tracked in Issue #17528.
+            //
+            ctx.DrainAndServiceIO();
+
             NL_TEST_ASSERT(apSuite, delegate.mGotReport);
             // Mock endpoint3 has 13 attributes in total, and we subscribed twice.
             // And attribute 3/2/4 is a list with 6 elements and list chunking is applied to it, thus we should receive ( 13 + 6 ) *