Correctly call OnReportBegin/End. (#12071)

With basic chunking support already in the tree, This fixes up the logic bug where OnReportBegin/End were not being called at the right times on incoming report processing. This also fixes up some terminology confusion in ReadClient/Handler where the use of `mIsInitialReport` meant two different things in those two contexts: In ReadClient, it literally meant the first report in a train of priming reports, while on the handler, it referred to the entire priming phase of reporting.
project-chip · Jan 4, 2022 · 2087193 · 2087193
1 parent 7d9d113
commit 2087193
Show file tree

Hide file tree

Showing 4 changed files with 30 additions and 16 deletions.
diff --git a/src/app/ReadClient.cpp b/src/app/ReadClient.cpp
@@ -391,12 +391,18 @@ CHIP_ERROR ReadClient::ProcessReportData(System::PacketBufferHandle && aPayload)
         TLV::TLVReader attributeReportIBsReader;
         attributeReportIBs.GetReader(&attributeReportIBsReader);
 
-        mpCallback->OnReportBegin(this);
+        if (IsInitialReport())
+        {
+            mpCallback->OnReportBegin(this);
+        }
 
         err = ProcessAttributeReportIBs(attributeReportIBsReader);
         SuccessOrExit(err);
 
-        mpCallback->OnReportEnd(this);
+        if (!mPendingMoreChunks)
+        {
+            mpCallback->OnReportEnd(this);
+        }
     }
 
 exit:
@@ -500,6 +506,10 @@ CHIP_ERROR ReadClient::ProcessAttributeReportIBs(TLV::TLVReader & aAttributeRepo
 
             ConcreteDataAttributePath attributePath(clusterInfo.mEndpointId, clusterInfo.mClusterId, clusterInfo.mAttributeId);
 
+            //
+            // TODO: Add support for correctly handling appends/updates whenever list chunking support
+            // on the server side is added.
+            //
             if (dataReader.GetType() == TLV::kTLVType_Array)
             {
                 attributePath.mListOp = ConcreteDataAttributePath::ListOperation::ReplaceAll;

diff --git a/src/app/ReadHandler.cpp b/src/app/ReadHandler.cpp
@@ -47,7 +47,7 @@ CHIP_ERROR ReadHandler::Init(Messaging::ExchangeManager * apExchangeMgr, Interac
     mpAttributeClusterInfoList = nullptr;
     mpEventClusterInfoList     = nullptr;
     mCurrentPriority           = PriorityLevel::Invalid;
-    mInitialReport             = true;
+    mIsPrimingReports          = true;
     MoveToState(HandlerState::Initialized);
     mpDelegate          = apDelegate;
     mSubscriptionId     = 0;
@@ -103,7 +103,7 @@ void ReadHandler::Shutdown(ShutdownOptions aOptions)
     mpAttributeClusterInfoList = nullptr;
     mpEventClusterInfoList     = nullptr;
     mCurrentPriority           = PriorityLevel::Invalid;
-    mInitialReport             = false;
+    mIsPrimingReports          = false;
     mpDelegate                 = nullptr;
     mHoldReport                = false;
     mDirty                     = false;
@@ -156,7 +156,7 @@ CHIP_ERROR ReadHandler::OnStatusResponse(Messaging::ExchangeContext * apExchange
         else if (IsSubscriptionType())
         {
             InteractionModelEngine::GetInstance()->GetReportingEngine().OnReportConfirm();
-            if (IsInitialReport())
+            if (IsPriming())
             {
                 err           = SendSubscribeResponse();
                 mpExchangeCtx = nullptr;
@@ -192,7 +192,7 @@ CHIP_ERROR ReadHandler::OnStatusResponse(Messaging::ExchangeContext * apExchange
 CHIP_ERROR ReadHandler::SendReportData(System::PacketBufferHandle && aPayload, bool aMoreChunks)
 {
     VerifyOrReturnLogError(IsReportable(), CHIP_ERROR_INCORRECT_STATE);
-    if (IsInitialReport() || IsChunkedReport())
+    if (IsPriming() || IsChunkedReport())
     {
         mSessionHandle.SetValue(mpExchangeCtx->GetSessionHandle());
     }
@@ -221,7 +221,7 @@ CHIP_ERROR ReadHandler::SendReportData(System::PacketBufferHandle && aPayload, b
 
     if (err == CHIP_NO_ERROR)
     {
-        if (IsSubscriptionType() && !IsInitialReport())
+        if (IsSubscriptionType() && !IsPriming())
         {
             err = RefreshSubscribeSyncTimer();
         }
@@ -396,7 +396,7 @@ CHIP_ERROR ReadHandler::ProcessAttributePathList(AttributePathIBs::Parser & aAtt
         SuccessOrExit(err);
         err = InteractionModelEngine::GetInstance()->PushFront(mpAttributeClusterInfoList, clusterInfo);
         SuccessOrExit(err);
-        mInitialReport = true;
+        mIsPrimingReports = true;
     }
     // if we have exhausted this container
     if (CHIP_END_OF_TLV == err)
@@ -539,7 +539,7 @@ CHIP_ERROR ReadHandler::SendSubscribeResponse()
     VerifyOrReturnLogError(mpExchangeCtx != nullptr, CHIP_ERROR_INCORRECT_STATE);
 
     ReturnErrorOnFailure(RefreshSubscribeSyncTimer());
-    mInitialReport = false;
+    mIsPrimingReports = false;
     MoveToState(HandlerState::GeneratingReports);
     if (mpDelegate != nullptr)
     {

diff --git a/src/app/ReadHandler.h b/src/app/ReadHandler.h
@@ -129,7 +129,7 @@ class ReadHandler : public Messaging::ExchangeDelegate
     bool IsReadType() { return mInteractionType == InteractionType::Read; }
     bool IsSubscriptionType() { return mInteractionType == InteractionType::Subscribe; }
     bool IsChunkedReport() { return mIsChunkedReport; }
-    bool IsInitialReport() { return mInitialReport; }
+    bool IsPriming() { return mIsPrimingReports; }
     bool IsActiveSubscription() const { return mActiveSubscription; }
     CHIP_ERROR OnSubscribeRequest(Messaging::ExchangeContext * apExchangeContext, System::PacketBufferHandle && aPayload);
     void GetSubscriptionId(uint64_t & aSubscriptionId) { aSubscriptionId = mSubscriptionId; }
@@ -193,11 +193,15 @@ class ReadHandler : public Messaging::ExchangeDelegate
     EventNumber mLastScheduledEventNumber[kNumPriorityLevel];
     Messaging::ExchangeManager * mpExchangeMgr = nullptr;
     InteractionModelDelegate * mpDelegate      = nullptr;
-    bool mInitialReport                        = false;
-    InteractionType mInteractionType           = InteractionType::Read;
-    uint64_t mSubscriptionId                   = 0;
-    uint16_t mMinIntervalFloorSeconds          = 0;
-    uint16_t mMaxIntervalCeilingSeconds        = 0;
+
+    // Tracks whether we're in the initial phase of receiving priming
+    // reports, which is always true for reads and true for subscriptions
+    // prior to receiving a subscribe response.
+    bool mIsPrimingReports              = false;
+    InteractionType mInteractionType    = InteractionType::Read;
+    uint64_t mSubscriptionId            = 0;
+    uint16_t mMinIntervalFloorSeconds   = 0;
+    uint16_t mMaxIntervalCeilingSeconds = 0;
     Optional<SessionHandle> mSessionHandle;
     bool mHoldReport         = false;
     bool mDirty              = false;

diff --git a/src/app/reporting/Engine.cpp b/src/app/reporting/Engine.cpp
@@ -106,7 +106,7 @@ CHIP_ERROR Engine::BuildSingleReportDataAttributeReportIBs(ReportDataMessage::Bu
         for (; apReadHandler->GetAttributePathExpandIterator()->Get(readPath);
              apReadHandler->GetAttributePathExpandIterator()->Next())
         {
-            if (!apReadHandler->IsInitialReport())
+            if (!apReadHandler->IsPriming())
             {
                 bool concretePathDirty = false;
                 // TODO: Optimize this implementation by making the iterator only emit intersected paths.