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 <bzbarsky@apple.com>

* fix style

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

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

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

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;

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