[ReadHandler] Removed Scheduling of report from OnReadHandlerCreated (#…

…28536)

* Removed Scheduling of report from OnReadHandlerCreated since it caused immediate scheduling before the intervals are negotiated

* Separated Read reports from Subscription reports and renamed flags and accessors for clarity

* Apply suggestions from code review

Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>

* Put back rescheduling in the OnReadHandlerSubscribed, added a CanEmitReadReport() method for readhandler for read request. Fixed call order in TestReportScheduler tests

* Removed redundancy by removing un-necessary OnSubscriptionAction(), added comment for OnReadHandlerSubscribed and modified reporting condition for Read reports to be sent independently from the report scheduler

* Apply suggestions from code review

Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>

* Appplied change to method name and fixe condition in SetStateFlag

* Update src/app/ReadHandler.h

Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>

* Removed un-necessary check in SetStateFlag

---------

Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>

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->OnSubscriptionEstablished(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())
  {
@@ -335,7 +334,7 @@ CHIP_ERROR ReadHandler::SendReportData(System::PacketBufferHandle && aPayload, b
  // Priming reports are handled when we send a SubscribeResponse.
  if (IsType(InteractionType::Subscribe) && !IsPriming() && !IsChunkedReport())
  {
- mObserver->OnSubscriptionAction(this);
+ mObserver->OnSubscriptionReportSent(this);
  }
  }
  if (!aMoreChunks)
@@ -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 (ShouldReportUnscheduled())
+ {
+ InteractionModelEngine::GetInstance()->GetReportingEngine().ScheduleRun();
+ }
+ else
+ {
+ // If we became reportable, the scheduler will schedule a run as soon as allowed
+ mObserver->OnBecameReportable(this);
+ }
  }
 }
 
@@ -647,8 +653,6 @@ CHIP_ERROR ReadHandler::SendSubscribeResponse()
  ReturnErrorOnFailure(writer.Finalize(&packet));
  VerifyOrReturnLogError(mExchangeCtx, CHIP_ERROR_INCORRECT_STATE);
 
- mObserver->OnSubscriptionAction(this);
-
  ClearStateFlag(ReadHandlerFlags::PrimingReports);
  return mExchangeCtx->SendMessage(Protocols::InteractionModel::MsgType::SubscribeResponse, std::move(packet));
 }
@@ -785,7 +789,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,11 +876,11 @@ 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);
@@ -895,7 +899,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

@@ -166,20 +166,23 @@ 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
- virtual void OnReadHandlerCreated(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 to notify a subscription was successfully established for the ReadHandler
+ /// @param[in] apReadHandler ReadHandler that completed its subscription
+ virtual void OnSubscriptionEstablished(ReadHandler * apReadHandler) = 0;
+
+ /// @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 when the min interval allows it.
+ ///
+ /// This will only be invoked for subscribe-type ReadHandler objects, and only after
+ /// OnSubscriptionEstablished has been called.
+ ///
+ /// @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
  /// maxInterval time period.
  /// @param[in] apReadHandler ReadHandler that has generated a report
- virtual void OnSubscriptionAction(ReadHandler * apReadHandler) = 0;
+ virtual void OnSubscriptionReportSent(ReadHandler * apReadHandler) = 0;
 
  /// @brief Callback invoked when a ReadHandler is getting removed so it can be unregistered
  /// @param[in] apReadHandler ReadHandler getting destroyed
@@ -333,13 +336,22 @@ 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 ShouldStartReporting() from false to true
+ // (which can only happen for subscriptions) must call
+ // mObserver->OnBecameReportable(this).
+ return CanStartReporting() && (ShouldReportUnscheduled() || IsDirty());
+ }
+ /// @brief CanStartReporting() is true if the ReadHandler is in a state where it could generate
+ /// a (possibly empty) report if someone asked it to.
+ bool CanStartReporting() const { return mState == HandlerState::CanStartReporting; }
+ /// @brief ShouldReportUnscheduled() is true if the ReadHandler should be asked to generate reports
+ /// without consulting the report scheduler.
+ bool ShouldReportUnscheduled() const
+ {
+ return CanStartReporting() && (IsType(ReadHandler::InteractionType::Read) || IsPriming());
  }
- bool IsGeneratingReports() const { return mState == HandlerState::GeneratingReports; }
  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 +430,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

@@ -638,7 +638,7 @@ void Engine::Run()
  ReadHandler * readHandler = imEngine->ActiveHandlerAt(mCurReadHandlerIdx % (uint32_t) imEngine->mReadHandlers.Allocated());
  VerifyOrDie(readHandler != nullptr);
 
- if (imEngine->GetReportScheduler()->IsReportableNow(readHandler))
+ if (readHandler->ShouldReportUnscheduled() || imEngine->GetReportScheduler()->IsReportableNow(readHandler))
  {
  mRunningReadHandler = readHandler;
  CHIP_ERROR err = BuildAndSendSingleReportData(readHandler);
@@ -831,7 +831,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

@@ -54,8 +54,10 @@ void ReportSchedulerImpl::OnEnterActiveMode()
 #endif
 }
 
-/// @brief When a ReadHandler is added, register it, which will schedule an engine run
-void ReportSchedulerImpl::OnReadHandlerCreated(ReadHandler * aReadHandler)
+/// @brief When a ReadHandler is added, 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.
+void ReportSchedulerImpl::OnSubscriptionEstablished(ReadHandler * aReadHandler)
 {
  ReadHandlerNode * newNode = FindReadHandlerNode(aReadHandler);
  // Handler must not be registered yet; it's just being constructed.
@@ -68,11 +70,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.
@@ -85,7 +82,7 @@ void ReportSchedulerImpl::OnBecameReportable(ReadHandler * aReadHandler)
  ScheduleReport(newTimeout, node);
 }
 
-void ReportSchedulerImpl::OnSubscriptionAction(ReadHandler * aReadHandler)
+void ReportSchedulerImpl::OnSubscriptionReportSent(ReadHandler * aReadHandler)
 {
  ReadHandlerNode * node = FindReadHandlerNode(aReadHandler);
  VerifyOrReturn(nullptr != node);

src/app/reporting/ReportSchedulerImpl.h

@@ -36,9 +36,9 @@ class ReportSchedulerImpl : public ReportScheduler
  void OnEnterActiveMode() override;
 
  // ReadHandlerObserver
- void OnReadHandlerCreated(ReadHandler * aReadHandler) final;
+ void OnSubscriptionEstablished(ReadHandler * aReadHandler) final;
  void OnBecameReportable(ReadHandler * aReadHandler) final;
- void OnSubscriptionAction(ReadHandler * aReadHandler) final;
+ void OnSubscriptionReportSent(ReadHandler * aReadHandler) final;
  void OnReadHandlerDestroyed(ReadHandler * aReadHandler) override;
 
  bool IsReportScheduled(ReadHandler * aReadHandler);

src/app/tests/TestReadInteraction.cpp

@@ -2234,7 +2234,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.
@@ -2245,7 +2245,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
@@ -2257,12 +2257,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);