[IM] Fix verious corner cases for event chunking (#16346)

* [IM] Fix verious corner cases for event chunking * Fix typo * Lift timeout by 5 min * Update src/app/reporting/Engine.cpp Co-authored-by: Boris Zbarsky <[email protected]> * Make test a bit faster * Fix * Cleanup Co-authored-by: Andrei Litvin <[email protected]> Co-authored-by: Boris Zbarsky <[email protected]>
project-chip · Nov 29, 2023 · 3385333 · 3385333
1 parent 42aef61
commit 3385333
Show file tree

Hide file tree

Showing 8 changed files with 599 additions and 32 deletions.
diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml
@@ -73,7 +73,7 @@ jobs:
               timeout-minutes: 20
               run: scripts/run_in_build_env.sh "ninja -C ./out"
             - name: Run Tests
-              timeout-minutes: 5
+              timeout-minutes: 10
               run: scripts/tests/gn_tests.sh
             # TODO Log Upload https://github.com/project-chip/connectedhomeip/issues/2227
             # TODO https://github.com/project-chip/connectedhomeip/issues/1512

diff --git a/src/app/EventManagement.cpp b/src/app/EventManagement.cpp
@@ -760,7 +760,16 @@ CHIP_ERROR EventManagement::FetchEventsSince(TLVWriter & aWriter, ObjectList<Eve
     }
 
 exit:
-    aEventMin = context.mCurrentEventNumber + 1;
+    if (err == CHIP_ERROR_BUFFER_TOO_SMALL || err == CHIP_ERROR_NO_MEMORY)
+    {
+        // We failed to fetch the current event because the buffer is too small, we will start from this one the next time.
+        aEventMin = context.mCurrentEventNumber;
+    }
+    else
+    {
+        // For all other cases, continue from the next event.
+        aEventMin = context.mCurrentEventNumber + 1;
+    }
     aEventCount += context.mEventCount;
     return err;
 }

diff --git a/src/app/ReadClient.cpp b/src/app/ReadClient.cpp
@@ -417,10 +417,8 @@ CHIP_ERROR ReadClient::ProcessReportData(System::PacketBufferHandle && aPayload)
     CHIP_ERROR err = CHIP_NO_ERROR;
     ReportDataMessage::Parser report;
 
-    bool isEventReportsPresent       = false;
-    bool isAttributeReportIBsPresent = false;
-    bool suppressResponse            = true;
-    uint64_t subscriptionId          = 0;
+    bool suppressResponse   = true;
+    uint64_t subscriptionId = 0;
     EventReportIBs::Parser eventReportIBs;
     AttributeReportIBs::Parser attributeReportIBs;
     System::PacketBufferTLVReader reader;
@@ -475,33 +473,28 @@ CHIP_ERROR ReadClient::ProcessReportData(System::PacketBufferHandle && aPayload)
     }
     SuccessOrExit(err);
 
-    err                   = report.GetEventReports(&eventReportIBs);
-    isEventReportsPresent = (err == CHIP_NO_ERROR);
+    err = report.GetEventReports(&eventReportIBs);
     if (err == CHIP_END_OF_TLV)
     {
         err = CHIP_NO_ERROR;
     }
-    SuccessOrExit(err);
-
-    if (isEventReportsPresent)
+    else if (err == CHIP_NO_ERROR)
     {
         chip::TLV::TLVReader EventReportsReader;
         eventReportIBs.GetReader(&EventReportsReader);
         err = ProcessEventReportIBs(EventReportsReader);
-        SuccessOrExit(err);
     }
+    SuccessOrExit(err);
 
-    err                         = report.GetAttributeReportIBs(&attributeReportIBs);
-    isAttributeReportIBsPresent = (err == CHIP_NO_ERROR);
+    err = report.GetAttributeReportIBs(&attributeReportIBs);
     if (err == CHIP_END_OF_TLV)
     {
         err = CHIP_NO_ERROR;
     }
-    SuccessOrExit(err);
-
-    if (isAttributeReportIBsPresent)
+    else if (err == CHIP_NO_ERROR)
     {
         TLV::TLVReader attributeReportIBsReader;
+        mSawAttributeReportsInCurrentReport = true;
         attributeReportIBs.GetReader(&attributeReportIBsReader);
 
         if (mIsInitialReport)
@@ -511,13 +504,14 @@ CHIP_ERROR ReadClient::ProcessReportData(System::PacketBufferHandle && aPayload)
         }
 
         err = ProcessAttributeReportIBs(attributeReportIBsReader);
-        SuccessOrExit(err);
+    }
+    SuccessOrExit(err);
 
-        if (!mPendingMoreChunks)
-        {
-            mpCallback.OnReportEnd();
-            mIsInitialReport = true;
-        }
+    if (mSawAttributeReportsInCurrentReport && !mPendingMoreChunks)
+    {
+        mpCallback.OnReportEnd();
+        mIsInitialReport                    = true;
+        mSawAttributeReportsInCurrentReport = false;
     }
 
     SuccessOrExit(err = report.ExitContainer());

diff --git a/src/app/ReadClient.h b/src/app/ReadClient.h
@@ -339,7 +339,8 @@ class ReadClient : public Messaging::ExchangeDelegate
     FabricIndex mFabricIndex            = kUndefinedFabricIndex;
     InteractionType mInteractionType    = InteractionType::Read;
     Timestamp mEventTimestamp;
-    EventNumber mEventMin = 0;
+    EventNumber mEventMin                    = 0;
+    bool mSawAttributeReportsInCurrentReport = false;
 
     ReadClient * mpNext                 = nullptr;
     InteractionModelEngine * mpImEngine = nullptr;

diff --git a/src/app/reporting/Engine.cpp b/src/app/reporting/Engine.cpp
@@ -276,6 +276,7 @@ CHIP_ERROR Engine::BuildSingleReportDataAttributeReportIBs(ReportDataMessage::Bu
     if (!attributeDataWritten && err == CHIP_NO_ERROR)
     {
         aReportDataBuilder.Rollback(backup);
+        aReportDataBuilder.ResetError();
     }
 
     // hasMoreChunks + no data encoded is a flag that we have encountered some trouble when processing the attribute.
@@ -290,7 +291,7 @@ CHIP_ERROR Engine::BuildSingleReportDataAttributeReportIBs(ReportDataMessage::Bu
 }
 
 CHIP_ERROR Engine::BuildSingleReportDataEventReports(ReportDataMessage::Builder & aReportDataBuilder, ReadHandler * apReadHandler,
-                                                     bool * apHasMoreChunks, bool * apHasEncodedData)
+                                                     bool aBufferIsUsed, bool * apHasMoreChunks, bool * apHasEncodedData)
 {
     CHIP_ERROR err    = CHIP_NO_ERROR;
     size_t eventCount = 0;
@@ -318,8 +319,13 @@ CHIP_ERROR Engine::BuildSingleReportDataEventReports(ReportDataMessage::Builder
     }
 
     {
-        EventReportIBs::Builder & eventReportIBs = aReportDataBuilder.CreateEventReports();
+        // Just like what we do in BuildSingleReportDataAttributeReportIBs(), we need to reserve one byte for end of container tag
+        // when encoding events to ensure we can close the container successfully.
+        const uint32_t kReservedSizeEndOfReportIBs = 1;
+        EventReportIBs::Builder & eventReportIBs   = aReportDataBuilder.CreateEventReports();
         SuccessOrExit(err = aReportDataBuilder.GetError());
+        VerifyOrExit(eventReportIBs.GetWriter() != nullptr, err = CHIP_ERROR_INCORRECT_STATE);
+        SuccessOrExit(err = eventReportIBs.GetWriter()->ReserveBuffer(kReservedSizeEndOfReportIBs));
         err = eventManager.FetchEventsSince(*(eventReportIBs.GetWriter()), eventList, eventMin, eventCount,
                                             apReadHandler->GetSubjectDescriptor());
 
@@ -331,9 +337,13 @@ CHIP_ERROR Engine::BuildSingleReportDataEventReports(ReportDataMessage::Builder
         else if ((err == CHIP_ERROR_BUFFER_TOO_SMALL) || (err == CHIP_ERROR_NO_MEMORY))
         {
             // when first cluster event is too big to fit in the packet, ignore that cluster event.
+            // However, we may have encoded some attributes before, we don't skip it in that case.
             if (eventCount == 0)
             {
-                eventMin++;
+                if (!aBufferIsUsed)
+                {
+                    eventMin++;
+                }
                 ChipLogDetail(DataManagement, "<RE:Run> first cluster event is too big so that it fails to fit in the packet!");
                 err = CHIP_NO_ERROR;
             }
@@ -356,6 +366,7 @@ CHIP_ERROR Engine::BuildSingleReportDataEventReports(ReportDataMessage::Builder
             ExitNow();
         }
 
+        SuccessOrExit(err = eventReportIBs.GetWriter()->UnreserveBuffer(kReservedSizeEndOfReportIBs));
         eventReportIBs.EndOfEventReports();
         SuccessOrExit(err = eventReportIBs.GetError());
     }
@@ -367,9 +378,13 @@ CHIP_ERROR Engine::BuildSingleReportDataEventReports(ReportDataMessage::Builder
         *apHasEncodedData = !(eventCount == 0 || eventClean);
     }
 
-    if (err == CHIP_NO_ERROR && (eventCount == 0 || eventClean))
+    // Maybe encoding the attributes has already used up all space.
+    if ((err == CHIP_NO_ERROR || err == CHIP_ERROR_NO_MEMORY || err == CHIP_ERROR_BUFFER_TOO_SMALL) &&
+        (eventCount == 0 || eventClean))
     {
         aReportDataBuilder.Rollback(backup);
+        aReportDataBuilder.ResetError();
+        err = CHIP_NO_ERROR;
     }
 
     // hasMoreChunks + no data encoded is a flag that we have encountered some trouble when processing the attribute.
@@ -402,6 +417,9 @@ CHIP_ERROR Engine::BuildAndSendSingleReportData(ReadHandler * apReadHandler)
     // Reserved size for the end of report message, which is an end-of-container (i.e 1 byte for the control tag).
     const uint32_t kReservedSizeForEndOfReportMessage = 1;
 
+    // Reserved size for an empty EventReportIBs, so we can at least check if there are any events need to be reported.
+    const uint32_t kReservedSizeForEventReportIBs = 3; // type, tag, end of container
+
     VerifyOrExit(apReadHandler != nullptr, err = CHIP_ERROR_INVALID_ARGUMENT);
     VerifyOrExit(!bufHandle.IsNull(), err = CHIP_ERROR_NO_MEMORY);
 
@@ -433,7 +451,7 @@ CHIP_ERROR Engine::BuildAndSendSingleReportData(ReadHandler * apReadHandler)
     }
 
     SuccessOrExit(err = reportDataWriter.ReserveBuffer(kReservedSizeForMoreChunksFlag + kReservedSizeForIMRevision +
-                                                       kReservedSizeForEndOfReportMessage));
+                                                       kReservedSizeForEndOfReportMessage + kReservedSizeForEventReportIBs));
 
     {
         bool hasMoreChunksForAttributes = false;
@@ -444,8 +462,9 @@ CHIP_ERROR Engine::BuildAndSendSingleReportData(ReadHandler * apReadHandler)
         err = BuildSingleReportDataAttributeReportIBs(reportDataBuilder, apReadHandler, &hasMoreChunksForAttributes,
                                                       &hasEncodedAttributes);
         SuccessOrExit(err);
-
-        err = BuildSingleReportDataEventReports(reportDataBuilder, apReadHandler, &hasMoreChunksForEvents, &hasEncodedEvents);
+        SuccessOrExit(err = reportDataWriter.UnreserveBuffer(kReservedSizeForEventReportIBs));
+        err = BuildSingleReportDataEventReports(reportDataBuilder, apReadHandler, hasEncodedAttributes, &hasMoreChunksForEvents,
+                                                &hasEncodedEvents);
         SuccessOrExit(err);
 
         hasMoreChunks = hasMoreChunksForAttributes || hasMoreChunksForEvents;

diff --git a/src/app/reporting/Engine.h b/src/app/reporting/Engine.h
@@ -145,7 +145,7 @@ class Engine
     CHIP_ERROR BuildSingleReportDataAttributeReportIBs(ReportDataMessage::Builder & reportDataBuilder, ReadHandler * apReadHandler,
                                                        bool * apHasMoreChunks, bool * apHasEncodedData);
     CHIP_ERROR BuildSingleReportDataEventReports(ReportDataMessage::Builder & reportDataBuilder, ReadHandler * apReadHandler,
-                                                 bool * apHasMoreChunks, bool * apHasEncodedData);
+                                                 bool aBufferIsUsed, bool * apHasMoreChunks, bool * apHasEncodedData);
     CHIP_ERROR RetrieveClusterData(const Access::SubjectDescriptor & aSubjectDescriptor, bool aIsFabricFiltered,
                                    AttributeReportIBs::Builder & aAttributeReportIBs,
                                    const ConcreteReadAttributePath & aClusterInfo,

diff --git a/src/controller/tests/BUILD.gn b/src/controller/tests/BUILD.gn
@@ -27,6 +27,7 @@ chip_test_suite("tests") {
       chip_device_platform != "esp32") {
     test_sources += [ "TestServerCommandDispatch.cpp" ]
     test_sources += [ "TestReadChunking.cpp" ]
+    test_sources += [ "TestEventChunking.cpp" ]
     test_sources += [ "TestWriteChunking.cpp" ]
   }