WriteClient: Encoding as many items as possible into a single list as part of ReplaceAll item operation

Alami-Amine · Alami-Amine · commit fc9d608e9373 · 2025-03-20T18:16:50.000+01:00
diff --git a/src/app/WriteClient.cpp b/src/app/WriteClient.cpp
@@ -187,7 +187,7 @@ CHIP_ERROR WriteClient::StartNewMessage()
     reservedSize = static_cast<uint16_t>(reservedSize + Crypto::CHIP_CRYPTO_AEAD_MIC_LENGTH_BYTES);
 
     // ... and the overhead for end of AttributeDataIBs (end of container), more chunks flag, end of WriteRequestMessage (another
-    // end of container).
+    // end of container), the End of the List (EndOfContainer as well) and End of AttributeDataIB (EndOfContainer)
     reservedSize = static_cast<uint16_t>(reservedSize + kReservedSizeForTLVEncodingOverhead);
 
 #if CONFIG_BUILD_FOR_HOST_UNIT_TEST
diff --git a/src/app/WriteClient.h b/src/app/WriteClient.h
@@ -41,6 +41,8 @@
 #include <system/SystemPacketBuffer.h>
 #include <system/TLVPacketBufferBackingStore.h>
 
+#include <app-common/zap-generated/ids/Attributes.h>
+#include <app-common/zap-generated/ids/Clusters.h>
 namespace chip {
 namespace app {
 
@@ -162,25 +164,71 @@ class WriteClient : public Messaging::ExchangeDelegate
 
     /**
      *  Encode a possibly-chunked list attribute value.  Will create a new chunk when necessary.
+     *
+     * This function will Attempt to to encode as many list items as possible into a SingleAttributeDataIB, which will be handled by
+     cluster server as a ReplaceAll Item operation
+     * If the list is too large, WriteRequest will be chunked and remaining items will be encoded as AppendItem operations, chunking
+     them as needed
+     *
+        // The items in this list will be decoded by servers as part of the ReplaceAll list.
+
      */
     template <class T>
-    CHIP_ERROR EncodeAttribute(const AttributePathParams & attributePath, const DataModel::List<T> & value,
+    CHIP_ERROR EncodeAttribute(const AttributePathParams & attributePath, const DataModel::List<T> & listValue,
                                const Optional<DataVersion> & aDataVersion = NullOptional)
     {
         // Here, we are using kInvalidEndpointId for missing endpoint id, which is used when sending group write requests.
         ConcreteDataAttributePath path =
             ConcreteDataAttributePath(attributePath.HasWildcardEndpointId() ? kInvalidEndpointId : attributePath.mEndpointId,
                                       attributePath.mClusterId, attributePath.mAttributeId, aDataVersion);
 
-        ReturnErrorOnFailure(EnsureMessage());
+        ListIndex nextItemToAppendIndex = kInvalidListIndex;
+        // TODO CHANGE NAME OF numEncodedItems and maybe not make it listindex?
+        uint16_t numSuccessfullyEncodedItems = kInvalidListIndex;
+        bool chunkingNeeded                  = false;
+
+        VerifyOrReturnError(!listValue.empty(), CHIP_ERROR_INVALID_LIST_LENGTH);
+
+        // BACKWARD COMPATIBILITY: Legacy OTA Requestor Servers( Pre-Matter 1.4) only support having an empty list for ReplaceAll
+        // when writing to the DefaultOTAProviders attribute and they will only Decode List Items that are  of the AppendItem List
+        // operation. So we must allow this for backward compatiblity.
+        // TODO remove this special case when Matter 1.3 is Sunsetted
+        bool encodeEmptyListAsReplaceAll =
+            (path.mClusterId == Clusters::OtaSoftwareUpdateRequestor::Id &&
+             path.mAttributeId == Clusters::OtaSoftwareUpdateRequestor::Attributes::DefaultOTAProviders::Id);
+
+        if (encodeEmptyListAsReplaceAll)
+        {
+            ReturnErrorOnFailure(EnsureMessage());
+            ReturnErrorOnFailure(EncodeSingleAttributeDataIB(path, DataModel::List<uint8_t>()));
+        }
+        else
+        {
+            // StartNewMessage was called to make sure we always create a new chunk when we have a new Attribute to Encode, this
+            // might be more efficient
+            // TODO should we keep this or just use EnsureMessage; if we choose EnsureMessage, we will have to adapt
+            // EnsureListStarted to make sure we create a New Chunk if it fails (could happens when a new Attribute is added)
+            ReturnErrorOnFailure(StartNewMessage());
 
-        // Encode an empty list for the chunking protocol.
-        ReturnErrorOnFailure(EncodeSingleAttributeDataIB(path, DataModel::List<uint8_t>()));
+            // TODO CHANGE NAME OF numEncodedItems
+            ReturnErrorOnFailure(
+                TryEncodeListIntoSingleAttributeDataIB(path, listValue, chunkingNeeded, numSuccessfullyEncodedItems));
+            if (!chunkingNeeded)
+            {
+                return CHIP_NO_ERROR;
+            }
+            // Start a new WriteRequest chunk.
+            ReturnErrorOnFailure(StartNewMessage());
+            nextItemToAppendIndex = numSuccessfullyEncodedItems;
+        }
 
         path.mListOp = ConcreteDataAttributePath::ListOperation::AppendItem;
-        for (size_t i = 0; i < value.size(); i++)
+
+        ListIndex startIndex = encodeEmptyListAsReplaceAll ? 0 : nextItemToAppendIndex;
+
+        for (ListIndex i = startIndex; i < listValue.size(); i++)
         {
-            ReturnErrorOnFailure(EncodeSingleAttributeDataIB(path, value.data()[i]));
+            ReturnErrorOnFailure(EncodeSingleAttributeDataIB(path, listValue[i]));
         }
 
         return CHIP_NO_ERROR;
@@ -254,31 +302,108 @@ class WriteClient : public Messaging::ExchangeDelegate
     /**
      *  Encode an attribute value that can be directly encoded using DataModel::Encode.
      */
-    template <class T>
+    template <class T, std::enable_if_t<!DataModel::IsFabricScoped<T>::value, int> = 0>
     CHIP_ERROR TryEncodeSingleAttributeDataIB(const ConcreteDataAttributePath & attributePath, const T & value)
     {
         chip::TLV::TLVWriter * writer = nullptr;
 
         ReturnErrorOnFailure(PrepareAttributeIB(attributePath));
         VerifyOrReturnError((writer = GetAttributeDataIBTLVWriter()) != nullptr, CHIP_ERROR_INCORRECT_STATE);
-        // Use if constexpr to make compile-time decision on which encoding method to use
-        if constexpr (DataModel::IsFabricScoped<T>::value)
-        {
-            ReturnErrorOnFailure(
-                DataModel::EncodeForWrite(*writer, chip::TLV::ContextTag(chip::app::AttributeDataIB::Tag::kData), value));
-        }
-        else
+        ReturnErrorOnFailure(DataModel::Encode(*writer, chip::TLV::ContextTag(chip::app::AttributeDataIB::Tag::kData), value));
+        ReturnErrorOnFailure(FinishAttributeIB());
+
+        return CHIP_NO_ERROR;
+    }
+
+    template <class T, std::enable_if_t<DataModel::IsFabricScoped<T>::value, int> = 0>
+    CHIP_ERROR TryEncodeSingleAttributeDataIB(const ConcreteDataAttributePath & attributePath, const T & value)
+    {
+        chip::TLV::TLVWriter * writer = nullptr;
+
+        ReturnErrorOnFailure(PrepareAttributeIB(attributePath));
+        VerifyOrReturnError((writer = GetAttributeDataIBTLVWriter()) != nullptr, CHIP_ERROR_INCORRECT_STATE);
+        ReturnErrorOnFailure(
+            DataModel::EncodeForWrite(*writer, chip::TLV::ContextTag(chip::app::AttributeDataIB::Tag::kData), value));
+        ReturnErrorOnFailure(FinishAttributeIB());
+
+        return CHIP_NO_ERROR;
+    }
+
+    CHIP_ERROR EnsureListStarted(const ConcreteDataAttributePath & attributePath)
+    {
+        chip::TLV::TLVWriter * writer = nullptr;
+
+        TLV::TLVType outerType;
+
+        ReturnErrorOnFailure(PrepareAttributeIB(attributePath));
+        VerifyOrReturnError((writer = GetAttributeDataIBTLVWriter()) != nullptr, CHIP_ERROR_INCORRECT_STATE);
+        ReturnErrorOnFailure(
+            writer->StartContainer(chip::TLV::ContextTag(chip::app::AttributeDataIB::Tag::kData), TLV::kTLVType_Array, outerType));
+
+        VerifyOrDie(outerType == kAttributeDataIBType);
+
+        return CHIP_NO_ERROR;
+    }
+
+    CHIP_ERROR EnsureListEnded()
+    {
+        chip::TLV::TLVWriter * writer = nullptr;
+        VerifyOrReturnError((writer = GetAttributeDataIBTLVWriter()) != nullptr, CHIP_ERROR_INCORRECT_STATE);
+
+        // In the event of Chunking (we have a CHIP_ERROR_NO_MEMORY), we need to Unreserve two more Bytes in order to be able to
+        // Append EndOfContainer of the List + EndOfContainer of the AttributeDataIB
+        VerifyOrDie(writer->UnreserveBuffer(kReservedSizeForEndOfListContainer + kReservedSizeForEndOfAttributeDataIB) ==
+                    CHIP_NO_ERROR);
+        ReturnErrorOnFailure(writer->EndContainer(kAttributeDataIBType));
+        ReturnErrorOnFailure(FinishAttributeIB());
+
+        return CHIP_NO_ERROR;
+    }
+
+    template <class T>
+    CHIP_ERROR TryEncodeListIntoSingleAttributeDataIB(const ConcreteDataAttributePath & attributePath,
+                                                      const DataModel::List<T> & list, bool & chunkingNeeded,
+                                                      ListIndex & numSuccessfullyEncodedItems)
+    {
+        CHIP_ERROR err = CHIP_NO_ERROR;
+        TLV::TLVWriter backupWriter;
+
+        ReturnErrorOnFailure(EnsureListStarted(attributePath));
+
+        chip::TLV::TLVWriter * writer = nullptr;
+        VerifyOrReturnError((writer = GetAttributeDataIBTLVWriter()) != nullptr, CHIP_ERROR_INCORRECT_STATE);
+
+        numSuccessfullyEncodedItems = 0;
+
+        for (auto & item : list)
         {
-            ReturnErrorOnFailure(DataModel::Encode(*writer, chip::TLV::ContextTag(chip::app::AttributeDataIB::Tag::kData), value));
+            mWriteRequestBuilder.GetWriteRequests().Checkpoint(backupWriter);
+            if constexpr (DataModel::IsFabricScoped<T>::value)
+            {
+                err = DataModel::EncodeForWrite(*writer, TLV::AnonymousTag(), item);
+            }
+            else
+            {
+                err = DataModel::Encode(*writer, TLV::AnonymousTag(), item);
+            }
+
+            if (err == CHIP_ERROR_NO_MEMORY || err == CHIP_ERROR_BUFFER_TOO_SMALL)
+            {
+                mWriteRequestBuilder.GetWriteRequests().Rollback(backupWriter);
+                chunkingNeeded = true;
+                break;
+            }
+            ReturnErrorOnFailure(err);
+            numSuccessfullyEncodedItems++;
         }
-        ReturnErrorOnFailure(FinishAttributeIB());
 
+        ReturnErrorOnFailure(EnsureListEnded());
         return CHIP_NO_ERROR;
     }
 
     /**
      * A wrapper for TryEncodeSingleAttributeDataIB which will start a new chunk when failed with CHIP_ERROR_NO_MEMORY or
-     * CHIP_ERROR_BUFFER_TOO_SMALL.
+     * CHIP_ERROR_BUFFER_TOO_SMALL. This will not be used for Lists
      */
     template <class T>
     CHIP_ERROR EncodeSingleAttributeDataIB(const ConcreteDataAttributePath & attributePath, const T & value)
@@ -418,6 +543,13 @@ class WriteClient : public Messaging::ExchangeDelegate
     static constexpr uint16_t kReservedSizeForTLVEncodingOverhead = kReservedSizeForIMRevision + kReservedSizeForMoreChunksFlag +
         kReservedSizeForEndOfContainer + kReservedSizeForEndOfContainer;
     bool mHasDataVersion = false;
+
+    // This was not added to kReservedSizeForTLVEncodingOverhead since it will be "Unreserved" before the others.
+    static constexpr uint32_t kReservedSizeForEndOfListContainer = 1;
+    // TODO should this be 2 ?
+    static constexpr uint32_t kReservedSizeForEndOfAttributeDataIB = 1;
+
+    static constexpr TLV::TLVType kAttributeDataIBType = TLV::kTLVType_Structure;
 };
 
 } // namespace app
diff --git a/src/app/tests/TestWriteInteraction.cpp b/src/app/tests/TestWriteInteraction.cpp
@@ -627,20 +627,24 @@ TEST_F_FROM_FIXTURE(TestWriteInteraction, TestWriteHandlerReceiveInvalidMessage)
     auto * engine = chip::app::InteractionModelEngine::GetInstance();
     EXPECT_EQ(engine->Init(&GetExchangeManager(), &GetFabricTable(), app::reporting::GetDefaultReportScheduler()), CHIP_NO_ERROR);
 
-    // Reserve all except the last 128 bytes, so that we make sure to chunk.
+    // Reserve all except the last 96 bytes, so that we make sure to chunk.
     app::WriteClient writeClient(&GetExchangeManager(), &writeCallback, Optional<uint16_t>::Missing(),
-                                 static_cast<uint16_t>(kMaxSecureSduLengthBytes - 128) /* reserved buffer size */);
+                                 static_cast<uint16_t>(kMaxSecureSduLengthBytes - 96) /* reserved buffer size */);
 
-    ByteSpan list[5];
+    // it was empirically determined that we need a list of at least 25 items to make sure we chunk (when the reserved buffer size =
+    // kMaxSecureSduLengthBytes - 96)
+    constexpr uint8_t kTestListLength = 30;
+    ByteSpan list[kTestListLength];
 
-    EXPECT_EQ(writeClient.EncodeAttribute(attributePath, app::DataModel::List<ByteSpan>(list, 5)), CHIP_NO_ERROR);
+    EXPECT_EQ(writeClient.EncodeAttribute(attributePath, app::DataModel::List<ByteSpan>(list, kTestListLength)), CHIP_NO_ERROR);
 
     GetLoopback().mSentMessageCount                 = 0;
     GetLoopback().mNumMessagesToDrop                = 1;
     GetLoopback().mNumMessagesToAllowBeforeDropping = 2;
     EXPECT_EQ(writeClient.SendWriteRequest(sessionHandle), CHIP_NO_ERROR);
     DrainAndServiceIO();
 
+    // TODO failure here, we have NULLPTR ERROR or a segmentation fault
     EXPECT_EQ(InteractionModelEngine::GetInstance()->GetNumActiveWriteHandlers(), 1u);
     EXPECT_EQ(GetLoopback().mSentMessageCount, 3u);
     EXPECT_EQ(GetLoopback().mDroppedMessageCount, 1u);
@@ -692,13 +696,13 @@ TEST_F(TestWriteInteraction, TestWriteHandlerInvalidateFabric)
     auto * engine = chip::app::InteractionModelEngine::GetInstance();
     EXPECT_EQ(engine->Init(&GetExchangeManager(), &GetFabricTable(), app::reporting::GetDefaultReportScheduler()), CHIP_NO_ERROR);
 
-    // Reserve all except the last 128 bytes, so that we make sure to chunk.
+    // Reserve all except the last 96 bytes, so that we make sure to chunk.
     app::WriteClient writeClient(&GetExchangeManager(), &writeCallback, Optional<uint16_t>::Missing(),
-                                 static_cast<uint16_t>(kMaxSecureSduLengthBytes - 128) /* reserved buffer size */);
+                                 static_cast<uint16_t>(kMaxSecureSduLengthBytes - 96) /* reserved buffer size */);
 
-    ByteSpan list[5];
+    ByteSpan list[30];
 
-    EXPECT_EQ(writeClient.EncodeAttribute(attributePath, app::DataModel::List<ByteSpan>(list, 5)), CHIP_NO_ERROR);
+    EXPECT_EQ(writeClient.EncodeAttribute(attributePath, app::DataModel::List<ByteSpan>(list, 30)), CHIP_NO_ERROR);
 
     GetLoopback().mDroppedMessageCount              = 0;
     GetLoopback().mSentMessageCount                 = 0;
diff --git a/src/controller/tests/TestWriteChunking.cpp b/src/controller/tests/TestWriteChunking.cpp
