Review feedback

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;

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

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 ) *

src/controller/python/test/test_scripts/cluster_objects.py

@@ -189,15 +189,15 @@ def subUpdate(path: TypedAttributePath, transaction: SubscriptionTransaction):
 
     @classmethod
     @base.test_case
-    ''' 
+    async def TestSubscribeZeroMinInterval(cls, devCtrl):
+        '''
         This validates receiving subscription reports for two attributes at a time in quick succession after issuing a command that results in attribute side-effects.
         Specifically, it relies on the fact that the second attribute is changed in a different execution context than the first. This ensures that we pick-up the first
         attribute change and generate a notification, and validating that shortly after that, we generate a second report for the second change.
 
         This is done using subscriptions with a min reporting interval of 0 to ensure timely notification of the above. An On() command is sent to the OnOff cluster
         which should simultaneously set the state to On as well as set the level to 254.
-    '''
-    async def TestSubscribeZeroMinInterval(cls, devCtrl):
+        '''
         logger.info("Test Subscription With MinInterval of 0")
         sub = await devCtrl.ReadAttribute(nodeid=NODE_ID, attributes=[Clusters.OnOff, Clusters.LevelControl], reportInterval=(0, 60))
         data = sub.GetAttributes()