Improve encoding of lists where first item can't fit in packet. (#28346)

* Improve encoding of lists where first item can't fit in packet.

When the first item of a list can't fit in the current packet, it's better to
just send the packet without the list at all, then encode as many items as we
can in the packet that follows before we start doing IB-per-item.

Otherwise (before this change) we encode an empty list IB, then end up doing
IB-per-item for the whole list.

* Improve unit tests.

src/app/AttributeAccessInterface.cpp

@@ -130,6 +130,19 @@ void AttributeValueEncoder::EnsureListEnded()
 
     AttributeReportBuilder builder;
     VerifyOrDie(builder.FinishAttribute(mAttributeReportIBsBuilder) == CHIP_NO_ERROR);
+
+    if (!mEncodedAtLeastOneListItem)
+    {
+        // If we have not managed to encode any list items, we don't actually
+        // want to output the single "empty list" IB that will then be followed
+        // by one-IB-per-item in the next packet.  Just have the reporting
+        // engine roll back our entire attribute and put us in the next packet.
+        //
+        // If we succeeded at encoding the whole list (i.e. the list is in fact
+        // empty and we fit in the packet), mAllowPartialData will be ignored,
+        // so it's safe to set it to false even if encoding succeeded.
+        mEncodeState.mAllowPartialData = false;
+    }
 }
 
 } // namespace app

src/app/AttributeAccessInterface.h

@@ -292,6 +292,7 @@ class AttributeValueEncoder
 
         mCurrentEncodingListIndex++;
         mEncodeState.mCurrentEncodingListIndex++;
+        mEncodedAtLeastOneListItem = true;
         return CHIP_NO_ERROR;
     }
 
@@ -340,6 +341,8 @@ class AttributeValueEncoder
     // started chunking it yet, so we're encoding a single attribute report IB
     // for the whole list, not one per item.
     bool mEncodingInitialList = false;
+    // mEncodedAtLeastOneListItem becomes true once we successfully encode a list item.
+    bool mEncodedAtLeastOneListItem = false;
     AttributeEncodeState mEncodeState;
     ListIndex mCurrentEncodingListIndex = kInvalidListIndex;
 };

src/app/tests/TestReadInteraction.cpp

@@ -1309,8 +1309,8 @@ void TestReadInteraction::TestSetDirtyBetweenChunks(nlTestSuite * apSuite, void
                     !aPath.IsListItemOperation())
                 {
                     mGotStartOfSecondReport = true;
-                    // Wait for an actual data chunk.
-                    return;
+                    // We always have data chunks, so go ahead to mark things
+                    // dirty as needed.
                 }
 
                 if (!mGotStartOfSecondReport)
@@ -2359,12 +2359,12 @@ void TestReadInteraction::TestSubscribeWildcard(nlTestSuite * apSuite, void * ap
 #else
         // When EventList is not enabled, the packet boundaries shift and for the first
         // report for the list attribute we receive two of its items in the initial list,
-        // then 4 additional items.  For the second report we receive 0 items in
-        // the initial list followed by 6 additional items.
+        // then 4 additional items.  For the second report we receive 3 items in
+        // the initial list followed by 3 additional items.
         //
-        // Thus we should receive 29*2 + 4 + 6 = 68 attribute data when the eventlist
+        // Thus we should receive 29*2 + 4 + 3 = 65 attribute data when the eventlist
         // attribute is not available.
-        constexpr size_t kExpectedAttributeResponse = 68;
+        constexpr size_t kExpectedAttributeResponse = 65;
 #endif
         NL_TEST_ASSERT(apSuite, delegate.mNumAttributeResponse == kExpectedAttributeResponse);
         NL_TEST_ASSERT(apSuite, delegate.mNumArrayItems == 12);

src/controller/tests/TestReadChunking.cpp

@@ -70,6 +70,8 @@ constexpr AttributeId kTestListAttribute = 6;
 constexpr AttributeId kTestBadAttribute =
     7; // Reading this attribute will return CHIP_ERROR_NO_MEMORY but nothing is actually encoded.
 
+constexpr int kListAttributeItems = 5;
+
 class TestReadChunking
 {
 public:
@@ -125,6 +127,116 @@ DECLARE_DYNAMIC_ENDPOINT(testEndpoint5, testEndpoint5Clusters);
 
 uint8_t sAnStringThatCanNeverFitIntoTheMTU[4096] = { 0 };
 
+// Buffered callback class that lets us count the number of attribute data IBs
+// we receive.  BufferedReadCallback has all its ReadClient::Callback bits
+// private, so we can't just inherit from it and call our super-class functions.
+class TestBufferedReadCallback : public ReadClient::Callback
+{
+public:
+    TestBufferedReadCallback(ReadClient::Callback & aNextCallback) : mBufferedCallback(aNextCallback) {}
+
+    // Workaround for all the methods on BufferedReadCallback being private.
+    ReadClient::Callback & NextCallback() { return *static_cast<ReadClient::Callback *>(&mBufferedCallback); }
+
+    void OnReportBegin() override { NextCallback().OnReportBegin(); }
+    void OnReportEnd() override { NextCallback().OnReportEnd(); }
+
+    void OnAttributeData(const ConcreteDataAttributePath & aPath, TLV::TLVReader * apData, const StatusIB & aStatus) override
+    {
+        if (apData)
+        {
+            ++mAttributeDataIBCount;
+
+            TLV::TLVReader reader(*apData);
+            do
+            {
+                if (reader.GetType() != TLV::TLVType::kTLVType_Array)
+                {
+                    // Not a list.
+                    break;
+                }
+
+                TLV::TLVType containerType;
+                CHIP_ERROR err = reader.EnterContainer(containerType);
+                if (err != CHIP_NO_ERROR)
+                {
+                    mDecodingFailed = true;
+                    break;
+                }
+
+                err = reader.Next();
+                if (err == CHIP_END_OF_TLV)
+                {
+                    mSawEmptyList = true;
+                }
+                else if (err != CHIP_NO_ERROR)
+                {
+                    mDecodingFailed = true;
+                    break;
+                }
+            } while (false);
+        }
+        else
+        {
+            ++mAttributeStatusIBCount;
+        }
+
+        NextCallback().OnAttributeData(aPath, apData, aStatus);
+    }
+
+    void OnError(CHIP_ERROR aError) override { NextCallback().OnError(aError); }
+
+    void OnEventData(const EventHeader & aEventHeader, TLV::TLVReader * apData, const StatusIB * apStatus) override
+    {
+        NextCallback().OnEventData(aEventHeader, apData, apStatus);
+    }
+
+    void OnDone(ReadClient * apReadClient) override { NextCallback().OnDone(apReadClient); }
+
+    void OnSubscriptionEstablished(SubscriptionId aSubscriptionId) override
+    {
+        NextCallback().OnSubscriptionEstablished(aSubscriptionId);
+    }
+
+    CHIP_ERROR OnResubscriptionNeeded(ReadClient * apReadClient, CHIP_ERROR aTerminationCause) override
+    {
+        return NextCallback().OnResubscriptionNeeded(apReadClient, aTerminationCause);
+    }
+
+    void OnDeallocatePaths(ReadPrepareParams && aReadPrepareParams) override
+    {
+        NextCallback().OnDeallocatePaths(std::move(aReadPrepareParams));
+    }
+
+    CHIP_ERROR OnUpdateDataVersionFilterList(DataVersionFilterIBs::Builder & aDataVersionFilterIBsBuilder,
+                                             const Span<AttributePathParams> & aAttributePaths,
+                                             bool & aEncodedDataVersionList) override
+    {
+        return NextCallback().OnUpdateDataVersionFilterList(aDataVersionFilterIBsBuilder, aAttributePaths, aEncodedDataVersionList);
+    }
+
+    CHIP_ERROR GetHighestReceivedEventNumber(Optional<EventNumber> & aEventNumber) override
+    {
+        return NextCallback().GetHighestReceivedEventNumber(aEventNumber);
+    }
+
+    void OnUnsolicitedMessageFromPublisher(ReadClient * apReadClient) override
+    {
+        NextCallback().OnUnsolicitedMessageFromPublisher(apReadClient);
+    }
+
+    void OnCASESessionEstablished(const SessionHandle & aSession, ReadPrepareParams & aSubscriptionParams) override
+    {
+        NextCallback().OnCASESessionEstablished(aSession, aSubscriptionParams);
+    }
+
+    BufferedReadCallback mBufferedCallback;
+    bool mSawEmptyList               = false;
+    bool mDecodingFailed             = false;
+    uint32_t mAttributeDataIBCount   = 0;
+    uint32_t mAttributeStatusIBCount = 0;
+};
+
 class TestReadCallback : public app::ReadClient::Callback
 {
 public:
@@ -138,10 +250,13 @@ class TestReadCallback : public app::ReadClient::Callback
 
     void OnSubscriptionEstablished(SubscriptionId aSubscriptionId) override { mOnSubscriptionEstablished = true; }
 
+    void OnError(CHIP_ERROR aError) override { mReadError = aError; }
+
     uint32_t mAttributeCount        = 0;
     bool mOnReportEnd               = false;
     bool mOnSubscriptionEstablished = false;
-    app::BufferedReadCallback mBufferedCallback;
+    CHIP_ERROR mReadError           = CHIP_NO_ERROR;
+    TestBufferedReadCallback mBufferedCallback;
 };
 
 void TestReadCallback::OnAttributeData(const app::ConcreteDataAttributePath & aPath, TLV::TLVReader * apData,
@@ -276,7 +391,7 @@ CHIP_ERROR TestAttrAccess::Read(const app::ConcreteReadAttributePath & aPath, ap
     {
     case kTestListAttribute:
         return aEncoder.EncodeList([](const auto & encoder) {
-            for (int i = 0; i < 5; i++)
+            for (int i = 0; i < kListAttributeItems; i++)
             {
                 ReturnErrorOnFailure(encoder.Encode((uint8_t) gIterationCount));
             }
@@ -446,38 +561,74 @@ void TestReadChunking::TestListChunking(nlTestSuite * apSuite, void * apContext)
     app::AttributePathParams attributePath(kTestEndpointId3, app::Clusters::UnitTesting::Id, kTestListAttribute);
     app::ReadPrepareParams readParams(sessionHandle);
 
-    readParams.mpAttributePathParamsList    = &attributePath;
-    readParams.mAttributePathParamsListSize = 1;
+    // Read the path twice, so we get two lists.  This make it easier to check
+    // for what happens when one of the lists starts near the end of a packet
+    // boundary.
+
+    AttributePathParams pathList[] = { attributePath, attributePath };
+
+    readParams.mpAttributePathParamsList    = pathList;
+    readParams.mAttributePathParamsListSize = ArraySize(pathList);
+
+    constexpr size_t maxPacketSize = kMaxSecureSduLengthBytes;
+    bool gotSuccessfulEncode       = false;
+    bool gotFailureResponse        = false;
 
     //
-    // We've empirically determined that by reserving 950 bytes in the packet buffer, we can fit 2
-    // AttributeDataIBs into the packet. ~30-40 bytes covers a single AttributeDataIB, but let's 2-3x that
-    // to ensure we'll sweep from fitting 2 IBs to 3-4 IBs.
+    // Make sure we start off the packet size large enough that we can fit a
+    // single status response in it.  Verify that we get at least one status
+    // response.  Then sweep up over packet sizes until we're big enough to hold
+    // something like 7 IBs (at 30-40 bytes each, so call it 200 bytes) and check
+    // the behavior for all those cases.
     //
-    for (int i = 100; i > 0; i--)
+    for (uint32_t packetSize = 30; packetSize < 200; packetSize++)
     {
         TestReadCallback readCallback;
 
-        ChipLogDetail(DataManagement, "Running iteration %d\n", i);
+        ChipLogDetail(DataManagement, "Running iteration %d\n", packetSize);
 
-        gIterationCount = (uint32_t) i;
+        gIterationCount = packetSize;
 
-        app::InteractionModelEngine::GetInstance()->GetReportingEngine().SetWriterReserved(static_cast<uint32_t>(850 + i));
+        app::InteractionModelEngine::GetInstance()->GetReportingEngine().SetWriterReserved(
+            static_cast<uint32_t>(maxPacketSize - packetSize));
 
         app::ReadClient readClient(engine, &ctx.GetExchangeManager(), readCallback.mBufferedCallback,
                                    app::ReadClient::InteractionType::Read);
 
         NL_TEST_ASSERT(apSuite, readClient.SendRequest(readParams) == CHIP_NO_ERROR);
 
         ctx.DrainAndServiceIO();
-        NL_TEST_ASSERT(apSuite, readCallback.mOnReportEnd);
 
-        //
-        // Always returns the same number of attributes read (merged by buffered read callback). The content is checked in
-        // TestReadCallback::OnAttributeData
-        //
-        NL_TEST_ASSERT(apSuite, readCallback.mAttributeCount == 1);
-        readCallback.mAttributeCount = 0;
+        // Up until our packets are big enough, we might just keep getting
+        // errors due to the inability to encode even a single IB in a packet.
+        // But once we manage a successful encode, we should not have any more failures.
+        if (!gotSuccessfulEncode && readCallback.mReadError != CHIP_NO_ERROR)
+        {
+            gotFailureResponse = true;
+            // Check for the right error type.
+            NL_TEST_ASSERT(apSuite,
+                           StatusIB(readCallback.mReadError).mStatus == Protocols::InteractionModel::Status::ResourceExhausted);
+        }
+        else
+        {
+            gotSuccessfulEncode = true;
+
+            NL_TEST_ASSERT(apSuite, readCallback.mOnReportEnd);
+
+            //
+            // Always returns the same number of attributes read (merged by buffered read callback). The content is checked in
+            // TestReadCallback::OnAttributeData.  The attribute count is 1
+            // because the buffered callback treats the second read's path as being
+            // just a replace of the first read's path and buffers it all up as a
+            // single value.
+            //
+            NL_TEST_ASSERT(apSuite, readCallback.mAttributeCount == 1);
+            readCallback.mAttributeCount = 0;
+
+            // Check that we never saw an empty-list data IB.
+            NL_TEST_ASSERT(apSuite, !readCallback.mBufferedCallback.mDecodingFailed);
+            NL_TEST_ASSERT(apSuite, !readCallback.mBufferedCallback.mSawEmptyList);
+        }
 
         NL_TEST_ASSERT(apSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 0);
 
@@ -490,6 +641,9 @@ void TestReadChunking::TestListChunking(nlTestSuite * apSuite, void * apContext)
         }
     }
 
+    // If this fails, our smallest packet size was not small enough.
+    NL_TEST_ASSERT(apSuite, gotFailureResponse);
+
     emberAfClearDynamicEndpoint(0);
 }