Call OnReportBegin in ReadClient for events, and report in OnError ca…

…llback in Darwin framework (#19337) * Call OnReportBegin before ReadClient calls OnEventData, and report attribute/event in OnError in Darwin framework callback Issue 18783 - Sort out interaction of OnReportBegin/OnReportEnd with subscriptions in the Darwin framework Update src/darwin/Framework/CHIP/CHIPDevice.mm Co-authored-by: Boris Zbarsky <[email protected]> * fix style Co-authored-by: Boris Zbarsky <[email protected]>
project-chip · Jun 27, 2022 · 4499380 · 4499380
1 parent d3107fb
commit 4499380
Show file tree

Hide file tree

Showing 4 changed files with 40 additions and 19 deletions.
diff --git a/src/app/BufferedReadCallback.h b/src/app/BufferedReadCallback.h
@@ -69,7 +69,12 @@ class BufferedReadCallback : public ReadClient::Callback
     void OnReportBegin() override;
     void OnReportEnd() override;
     void OnAttributeData(const ConcreteDataAttributePath & aPath, TLV::TLVReader * apData, const StatusIB & aStatus) override;
-    void OnError(CHIP_ERROR aError) override { return mCallback.OnError(aError); }
+    void OnError(CHIP_ERROR aError) override
+    {
+        mBufferedList.clear();
+        return mCallback.OnError(aError);
+    }
+
     void OnEventData(const EventHeader & aEventHeader, TLV::TLVReader * apData, const StatusIB * apStatus) override
     {
         return mCallback.OnEventData(aEventHeader, apData, apStatus);

diff --git a/src/app/ReadClient.cpp b/src/app/ReadClient.cpp
@@ -91,7 +91,7 @@ ReadClient::ReadClient(InteractionModelEngine * apImEngine, Messaging::ExchangeM
 
 void ReadClient::ClearActiveSubscriptionState()
 {
-    mIsInitialReport         = true;
+    mIsReporting             = false;
     mIsPrimingReports        = true;
     mPendingMoreChunks       = false;
     mMinIntervalFloorSeconds = 0;
@@ -558,24 +558,15 @@ CHIP_ERROR ReadClient::ProcessReportData(System::PacketBufferHandle && aPayload)
     else if (err == CHIP_NO_ERROR)
     {
         TLV::TLVReader attributeReportIBsReader;
-        mSawAttributeReportsInCurrentReport = true;
         attributeReportIBs.GetReader(&attributeReportIBsReader);
-
-        if (mIsInitialReport)
-        {
-            mpCallback.OnReportBegin();
-            mIsInitialReport = false;
-        }
-
         err = ProcessAttributeReportIBs(attributeReportIBsReader);
     }
     SuccessOrExit(err);
 
-    if (mSawAttributeReportsInCurrentReport && !mPendingMoreChunks)
+    if (mIsReporting && !mPendingMoreChunks)
     {
         mpCallback.OnReportEnd();
-        mIsInitialReport                    = true;
-        mSawAttributeReportsInCurrentReport = false;
+        mIsReporting = false;
     }
 
     SuccessOrExit(err = report.ExitContainer());
@@ -633,6 +624,15 @@ CHIP_ERROR ReadClient::ProcessAttributePath(AttributePathIB::Parser & aAttribute
     return CHIP_NO_ERROR;
 }
 
+void ReadClient::NoteReportingData()
+{
+    if (!mIsReporting)
+    {
+        mpCallback.OnReportBegin();
+        mIsReporting = true;
+    }
+}
+
 CHIP_ERROR ReadClient::ProcessAttributeReportIBs(TLV::TLVReader & aAttributeReportIBsReader)
 {
     CHIP_ERROR err = CHIP_NO_ERROR;
@@ -657,6 +657,7 @@ CHIP_ERROR ReadClient::ProcessAttributeReportIBs(TLV::TLVReader & aAttributeRepo
             ReturnErrorOnFailure(ProcessAttributePath(path, attributePath));
             ReturnErrorOnFailure(status.GetErrorStatus(&errorStatus));
             ReturnErrorOnFailure(errorStatus.DecodeStatusIB(statusIB));
+            NoteReportingData();
             mpCallback.OnAttributeData(attributePath, nullptr, statusIB);
         }
         else if (CHIP_END_OF_TLV == err)
@@ -681,6 +682,7 @@ CHIP_ERROR ReadClient::ProcessAttributeReportIBs(TLV::TLVReader & aAttributeRepo
                 attributePath.mListOp = ConcreteDataAttributePath::ListOperation::ReplaceAll;
             }
 
+            NoteReportingData();
             mpCallback.OnAttributeData(attributePath, &dataReader, statusIB);
         }
     }
@@ -722,6 +724,7 @@ CHIP_ERROR ReadClient::ProcessEventReportIBs(TLV::TLVReader & aEventReportIBsRea
                 mReadPrepareParams.mEventNumber.SetValue(header.mEventNumber + 1);
             }
 
+            NoteReportingData();
             mpCallback.OnEventData(header, &dataReader, nullptr);
         }
         else if (err == CHIP_END_OF_TLV)
@@ -735,6 +738,7 @@ CHIP_ERROR ReadClient::ProcessEventReportIBs(TLV::TLVReader & aEventReportIBsRea
             ReturnErrorOnFailure(status.GetErrorStatus(&statusIBParser));
             ReturnErrorOnFailure(statusIBParser.DecodeStatusIB(statusIB));
 
+            NoteReportingData();
             mpCallback.OnEventData(header, nullptr, &statusIB);
         }
     }

diff --git a/src/app/ReadClient.h b/src/app/ReadClient.h
@@ -66,16 +66,18 @@ class ReadClient : public Messaging::ExchangeDelegate
         virtual ~Callback() = default;
 
         /**
-         * Used to signal the commencement of processing of the first attribute report received in a given exchange.
+         * Used to signal the commencement of processing of the first attribute or event report received in a given exchange.
          *
          * This object MUST continue to exist after this call is completed. The application shall wait until it
          * receives an OnDone call to destroy the object.
          *
+         * Once OnReportBegin has been called, either OnReportEnd or OnError will be called before OnDone.
+         *
          */
         virtual void OnReportBegin() {}
 
         /**
-         * Used to signal the completion of processing of the last attribute report in a given exchange.
+         * Used to signal the completion of processing of the last attribute or event report in a given exchange.
          *
          * This object MUST continue to exist after this call is completed. The application shall wait until it
          * receives an OnDone call to destroy the object.
@@ -395,6 +397,8 @@ class ReadClient : public Messaging::ExchangeDelegate
     CHIP_ERROR SendSubscribeRequestImpl(const ReadPrepareParams & aSubscribePrepareParams);
     void UpdateDataVersionFilters(const ConcreteDataAttributePath & aPath);
     static void OnResubscribeTimerCallback(System::Layer * apSystemLayer, void * apAppState);
+    // Called to ensure OnReportBegin is called before calling OnEventData or OnAttributeData
+    void NoteReportingData();
 
     /*
      * Called internally to signal the completion of all work on this object, gracefully close the
@@ -414,6 +418,7 @@ class ReadClient : public Messaging::ExchangeDelegate
     Messaging::ExchangeContext * mpExchangeCtx = nullptr;
     Callback & mpCallback;
     ClientState mState                = ClientState::Idle;
+    bool mIsReporting                 = false;
     bool mIsInitialReport             = true;
     bool mIsPrimingReports            = true;
     bool mPendingMoreChunks           = false;
@@ -424,7 +429,6 @@ class ReadClient : public Messaging::ExchangeDelegate
     FabricIndex mFabricIndex          = kUndefinedFabricIndex;
     InteractionType mInteractionType  = InteractionType::Read;
     Timestamp mEventTimestamp;
-    bool mSawAttributeReportsInCurrentReport = false;
 
     ReadClient * mpNext                 = nullptr;
     InteractionModelEngine * mpImEngine = nullptr;

diff --git a/src/darwin/Framework/CHIP/CHIPDevice.mm b/src/darwin/Framework/CHIP/CHIPDevice.mm
@@ -299,6 +299,7 @@ - (instancetype)initWithDevice:(chip::DeviceProxy *)device
 
     void OnSubscriptionEstablished(SubscriptionId aSubscriptionId) override;
 
+    void ReportData();
     void ReportError(CHIP_ERROR err);
     void ReportError(const StatusIB & status);
     void ReportError(NSError * _Nullable err);
@@ -1393,7 +1394,8 @@ - (instancetype)initWithPath:(const ConcreteEventPath &)path
     mEventReports = [NSMutableArray new];
 }
 
-void SubscriptionCallback::OnReportEnd()
+// Reports attribute and event data if any exists
+void SubscriptionCallback::ReportData()
 {
     __block NSArray * attributeReports = mAttributeReports;
     mAttributeReports = nil;
@@ -1409,9 +1411,10 @@ - (instancetype)initWithPath:(const ConcreteEventPath &)path
             mEventReportCallback(eventReports);
         });
     }
-    // Else we have a pending error already.
 }
 
+void SubscriptionCallback::OnReportEnd() { ReportData(); }
+
 void SubscriptionCallback::OnEventData(const EventHeader & aEventHeader, TLV::TLVReader * apData, const StatusIB * apStatus)
 {
     id _Nullable value = nil;
@@ -1485,7 +1488,12 @@ - (instancetype)initWithPath:(const ConcreteEventPath &)path
     [mAttributeReports addObject:[[CHIPAttributeReport alloc] initWithPath:aPath value:value error:error]];
 }
 
-void SubscriptionCallback::OnError(CHIP_ERROR aError) { ReportError([CHIPError errorForCHIPErrorCode:aError]); }
+void SubscriptionCallback::OnError(CHIP_ERROR aError)
+{
+    // If OnError is called after OnReportBegin, we should report the collected data
+    ReportData();
+    ReportError([CHIPError errorForCHIPErrorCode:aError]);
+}
 
 void SubscriptionCallback::OnDone(ReadClient *)
 {