Fixes some more bugs in the chunking logic + adds a definitive chunking

validation test.

config/standalone/CHIPProjectConfig.h

@@ -75,4 +75,6 @@
 #define DYNAMIC_ENDPOINT_COUNT 4
 #endif
 
+#define CONFIG_IM_BUILD_FOR_UNIT_TEST 1
+
 #endif /* CHIPPROJECTCONFIG_H */

src/app/MessageDef/Builder.cpp

@@ -47,8 +47,7 @@ void Builder::ResetError()
 
 void Builder::ResetError(CHIP_ERROR aErr)
 {
-    mError              = aErr;
-    mOuterContainerType = chip::TLV::kTLVType_NotSpecified;
+    mError = aErr;
 }
 
 void Builder::EndOfContainer()

src/app/reporting/Engine.cpp

@@ -67,6 +67,8 @@ Engine::RetrieveClusterData(FabricIndex aAccessingFabricIndex, AttributeReportIB
     CHIP_ERROR err = CHIP_NO_ERROR;
 
     AttributeReportIB::Builder attributeReport = aAttributeReportIBs.CreateAttributeReport();
+    err                                        = attributeReport.GetError();
+    SuccessOrExit(attributeReport.GetError());
 
     ChipLogDetail(DataManagement, "<RE:Run> Cluster %" PRIx32 ", Attribute %" PRIx32 " is dirty", aPath.mClusterId,
                   aPath.mAttributeId);
@@ -95,10 +97,17 @@ CHIP_ERROR Engine::BuildSingleReportDataAttributeReportIBs(ReportDataMessage::Bu
     bool attributeDataWritten = false;
     bool hasMoreChunks        = true;
     TLV::TLVWriter backup;
+    const uint32_t kReservedSizeEndOfReportIBs = 1;
+
     aReportDataBuilder.Checkpoint(backup);
     auto attributeReportIBs = aReportDataBuilder.CreateAttributeReportIBs();
     SuccessOrExit(err = aReportDataBuilder.GetError());
 
+    //
+    // Reserve enough space for closing out the Report IB list
+    //
+    attributeReportIBs.GetWriter()->ReserveBuffer(kReservedSizeEndOfReportIBs);
+
     {
         // TODO: Figure out how AttributePathExpandIterator should handle read
         // vs write paths.
@@ -127,8 +136,8 @@ CHIP_ERROR Engine::BuildSingleReportDataAttributeReportIBs(ReportDataMessage::Bu
                     continue;
                 }
             }
-            // If we are processing a read request, or the initial report of a subscription, just regard all paths as dirty paths.
 
+            // If we are processing a read request, or the initial report of a subscription, just regard all paths as dirty paths.
             TLV::TLVWriter attributeBackup;
             attributeReportIBs.Checkpoint(attributeBackup);
             ConcreteReadAttributePath pathForRetrieval(readPath);
@@ -147,19 +156,48 @@ CHIP_ERROR Engine::BuildSingleReportDataAttributeReportIBs(ReportDataMessage::Bu
         hasMoreChunks = false;
     }
 exit:
+    //
+    // Running out of space is an error that we're expected to handle - the incompletely written DataIB has already been rolled back
+    // earlier to ensure only whole and complete DataIBs are present in the stream.
+    //
+    // We can safely clear out the error so that the rest of the machinery to close out the reports, etc. will function correctly.
+    // These are are guaranteed to not fail since we've already reserved memory for the remaining 'close out' TLV operations in this
+    // function and its callers.
+    //
     if ((err == CHIP_ERROR_BUFFER_TOO_SMALL) || (err == CHIP_ERROR_NO_MEMORY))
     {
         ChipLogDetail(DataManagement, "<RE:Run> We cannot put more chunks into this report. Enable chunking.");
+
+        //
+        // Reset the error tracked within the builder. Otherwise, any further attempts to write
+        // data through the builder will be blocked by that error.
+        //
+        attributeReportIBs.ResetError();
         err = CHIP_NO_ERROR;
     }
 
+    //
+    // Only close out the report if we haven't hit an error yet so far.
+    //
     if (err == CHIP_NO_ERROR)
     {
+        attributeReportIBs.GetWriter()->UnreserveBuffer(kReservedSizeEndOfReportIBs);
+
         attributeReportIBs.EndOfAttributeReportIBs();
         err = attributeReportIBs.GetError();
+
+        //
+        // We reserved space for this earlier - consequently, the call to end the ReportIBs should
+        // never fail, so assert if we do since that's a logic bug.
+        //
+        VerifyOrDie(err == CHIP_NO_ERROR);
     }
 
-    if (!attributeDataWritten || err != CHIP_NO_ERROR)
+    //
+    // Rollback the the entire ReportIB array if we never wrote any attributes
+    // AND never hit an error.
+    //
+    if (!attributeDataWritten && err == CHIP_NO_ERROR)
     {
         aReportDataBuilder.Rollback(backup);
     }
@@ -269,7 +307,7 @@ CHIP_ERROR Engine::BuildSingleReportDataEventReports(ReportDataMessage::Builder
     ChipLogDetail(DataManagement, "Fetched %zu events", eventCount);
 
 exit:
-    if (err != CHIP_NO_ERROR || eventCount == 0 || eventClean)
+    if (err == CHIP_NO_ERROR && (eventCount == 0 || eventClean))
     {
         aReportDataBuilder.Rollback(backup);
     }
@@ -289,6 +327,12 @@ CHIP_ERROR Engine::BuildAndSendSingleReportData(ReadHandler * apReadHandler)
     uint16_t reservedSize                      = 0;
     bool hasMoreChunks                         = false;
 
+    // Reserved size for the MoreChunks boolean flag, which takes up 1 byte for the control tag and 1 byte for the context tag.
+    const uint32_t kReservedSizeForMoreChunksFlag = 1 + 1;
+
+    // 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;
+
     VerifyOrExit(!bufHandle.IsNull(), err = CHIP_ERROR_NO_MEMORY);
 
     if (bufHandle->AvailableDataLength() > kMaxSecureSduLengthBytes)
@@ -298,6 +342,10 @@ CHIP_ERROR Engine::BuildAndSendSingleReportData(ReadHandler * apReadHandler)
 
     reportDataWriter.Init(std::move(bufHandle));
 
+#if CONFIG_IM_BUILD_FOR_UNIT_TEST
+    reportDataWriter.ReserveBuffer(mReservedSize);
+#endif
+
     // Always limit the size of the generated packet to fit within kMaxSecureSduLengthBytes regardless of the available buffer
     // capacity.
     // Also, we need to reserve some extra space for the MIC field.
@@ -314,15 +362,17 @@ CHIP_ERROR Engine::BuildAndSendSingleReportData(ReadHandler * apReadHandler)
         reportDataBuilder.SubscriptionId(subscriptionId);
     }
 
-    SuccessOrExit(err = reportDataWriter.ReserveBuffer(Engine::kReservedSizeForMoreChunksFlag));
+    SuccessOrExit(err = reportDataWriter.ReserveBuffer(kReservedSizeForMoreChunksFlag + kReservedSizeForEndOfReportMessage));
 
     err = BuildSingleReportDataAttributeReportIBs(reportDataBuilder, apReadHandler, &hasMoreChunks);
     SuccessOrExit(err);
 
     err = BuildSingleReportDataEventReports(reportDataBuilder, apReadHandler, &hasMoreChunks);
     SuccessOrExit(err);
 
-    SuccessOrExit(err = reportDataWriter.UnreserveBuffer(Engine::kReservedSizeForMoreChunksFlag));
+    SuccessOrExit(reportDataBuilder.GetError());
+
+    SuccessOrExit(err = reportDataWriter.UnreserveBuffer(kReservedSizeForMoreChunksFlag + kReservedSizeForEndOfReportMessage));
     if (hasMoreChunks)
     {
         reportDataBuilder.MoreChunkedMessages(hasMoreChunks);
@@ -333,7 +383,12 @@ CHIP_ERROR Engine::BuildAndSendSingleReportData(ReadHandler * apReadHandler)
     }
 
     reportDataBuilder.EndOfReportDataMessage();
-    SuccessOrExit(err = reportDataBuilder.GetError());
+
+    //
+    // Since we've already reserved space for both the MoreChunked/SuppressResponse flags, as well as
+    // the end-of-container flag for the end of the report, we should never hit an error closing out the message.
+    //
+    VerifyOrDie(reportDataBuilder.GetError() == CHIP_NO_ERROR);
 
     err = reportDataWriter.Finalize(&bufHandle);
     SuccessOrExit(err);

src/app/reporting/Engine.h

@@ -62,6 +62,10 @@ class Engine
 
     void Shutdown();
 
+#if CONFIG_IM_BUILD_FOR_UNIT_TEST
+    void SetWriterReserved(uint32_t aReservedSize) { mReservedSize = aReservedSize; }
+#endif
+
     /**
      * Main work-horse function that executes the run-loop.
      */
@@ -146,8 +150,9 @@ class Engine
      */
     ClusterInfo * mpGlobalDirtySet = nullptr;
 
-    constexpr static uint32_t kReservedSizeForMoreChunksFlag =
-        2; // According to TLV encoding, the TAG length is 1 byte and its type is 1 byte.
+#if CONFIG_IM_BUILD_FOR_UNIT_TEST
+    uint32_t mReservedSize = 0;
+#endif
 };
 
 }; // namespace reporting

src/app/util/ember-compatibility-functions.cpp

@@ -250,7 +250,6 @@ CHIP_ERROR ReadSingleClusterData(FabricIndex aAccessingFabricIndex, const Concre
     ReturnErrorOnFailure(attributeDataIBBuilder.GetError());
 
     attributePathIBBuilder = attributeDataIBBuilder.CreatePath();
-    ReturnErrorOnFailure(attributePathIBBuilder.GetError());
 
     attributePathIBBuilder.Endpoint(aPath.mEndpointId)
         .Cluster(aPath.mClusterId)

src/controller/python/chip/ChipDeviceCtrl.py

@@ -427,7 +427,7 @@ async def ReadAttribute(self, nodeid: int, attributes: typing.List[typing.Union[
             (Clusters.ClusterA.AttributeA):             Endpoint = *,           Cluster = specific,   Attribute = specific
             endpoint:                                   Endpoint = specific,    Cluster = *,          Attribute = *
             Clusters.ClusterA:                          Endpoint = *,           Cluster = specific,   Attribute = *
-            None:                                       Endpoint = *,           Cluster = *,          Attribute = *
+            '*' or ():                                  Endpoint = *,           Cluster = *,          Attribute = *
 
         The cluster and attributes specified above are to be selected from the generated cluster objects.
 
@@ -446,7 +446,7 @@ async def ReadAttribute(self, nodeid: int, attributes: typing.List[typing.Union[
             endpoint = None
             cluster = None
             attribute = None
-            if v == None or v == ('*'):
+            if v == ('*') or v == ():
                 # Wildcard
                 pass
             elif type(v) is not tuple:

src/controller/python/test/test_scripts/cluster_objects.py

@@ -111,11 +111,10 @@ async def TestReadRequests(cls, devCtrl):
             (0, Clusters.Basic.Attributes.ProductID),
             (0, Clusters.Basic.Attributes.HardwareVersion),
         ]
-        await devCtrl.ReadAttribute(nodeid=NODE_ID, attributes=req)
-
-        '''
-        Note: The following tests is disabled temporarily due to reports of random failure. However, the CI is still green.
-        Disable these tests to avoid making CI flaky before finding the root cause.
+        res = await devCtrl.ReadAttribute(nodeid=NODE_ID, attributes=req)
+        if (len(res) != 3):
+            raise AssertionError(
+                f"Got back {len(res)} data items instead of 3")
 
         logger.info("2: Reading Ex Cx A*")
         req = [
@@ -146,7 +145,6 @@ async def TestReadRequests(cls, devCtrl):
             None
         ]
         await devCtrl.ReadAttribute(nodeid=NODE_ID, attributes=req)
-        '''
 
     @classmethod
     async def RunTest(cls, devCtrl):

src/controller/tests/BUILD.gn

@@ -25,6 +25,7 @@ chip_test_suite("tests") {
 
   if (chip_device_platform != "mbed" && chip_device_platform != "efr32") {
     test_sources += [ "TestServerCommandDispatch.cpp" ]
+    test_sources += [ "TestReadChunking.cpp" ]
   }
 
   cflags = [ "-Wconversion" ]