Update

src/app/AttributeAccessInterface.cpp

@@ -0,0 +1,69 @@
+/*
+ *
+ *    Copyright (c) 2021 Project CHIP Authors
+ *    All rights reserved.
+ *
+ *    Licensed under the Apache License, Version 2.0 (the "License");
+ *    you may not use this file except in compliance with the License.
+ *    You may obtain a copy of the License at
+ *
+ *        http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *    Unless required by applicable law or agreed to in writing, software
+ *    distributed under the License is distributed on an "AS IS" BASIS,
+ *    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *    See the License for the specific language governing permissions and
+ *    limitations under the License.
+ */
+
+#include <app/AttributeAccessInterface.h>
+
+namespace chip {
+namespace app {
+CHIP_ERROR AttributeValueEncoder::EncodeAttributePathIB(AttributePathIB::Builder & aAttributePathIBBuilder)
+{
+    aAttributePathIBBuilder.Endpoint(mPath.mEndpointId).Cluster(mPath.mClusterId).Attribute(mPath.mAttributeId);
+
+    // Encode the list index field if we are encoding a single element in an list.
+    if (mCurrentEncodingListIndex != kInvalidListIndex)
+    {
+        aAttributePathIBBuilder.ListIndex(mCurrentEncodingListIndex);
+    }
+
+    return aAttributePathIBBuilder.EndOfAttributePathIB().GetError();
+}
+
+CHIP_ERROR AttributeValueEncoder::EncodeEmptyList()
+{
+    if (mCurrentEncodingListIndex == kInvalidListIndex)
+    {
+        if (mEncodeState.mCurrentEncodingListIndex == kInvalidListIndex)
+        {
+            // This part is not atomic, the
+            mEncodeState.mAllowPartialData = false;
+            // Spec 10.5.4.3.1, 10.5.4.6 (Replace a list w/ Multiple IBs)
+            // Put a empty array before encoding the first array element for list chunking.
+            AttributeReportIB::Builder attributeReportIBBuilder = mAttributeReportIBsBuilder.CreateAttributeReport();
+            AttributeDataIB::Builder attributeDataIBBuilder     = attributeReportIBBuilder.CreateAttributeData();
+
+            // mCurrentEncodingListIndex is an invalid list index now, thus EncodeAttributePathIB won't encode it in the
+            // AttributePathIB.
+            ReturnErrorOnFailure(EncodeAttributePathIB(attributeDataIBBuilder.CreatePath()));
+
+            ReturnErrorOnFailure(
+                TagBoundEncoder(attributeDataIBBuilder.GetWriter(), TLV::ContextTag(to_underlying(AttributeDataIB::Tag::kData)))
+                    .EncodeList([](const TagBoundEncoder &) -> CHIP_ERROR { return CHIP_NO_ERROR; }));
+
+            attributeDataIBBuilder.DataVersion(mDataVersion).EndOfAttributeDataIB();
+            ReturnErrorOnFailure(attributeDataIBBuilder.GetError());
+            ReturnErrorOnFailure(attributeReportIBBuilder.EndOfAttributeReportIB().GetError());
+            mEncodeState.mCurrentEncodingListIndex = 0;
+        }
+        mCurrentEncodingListIndex = 0;
+    }
+
+    return CHIP_NO_ERROR;
+}
+
+} // namespace app
+} // namespace chip

src/app/AttributeAccessInterface.h

@@ -43,11 +43,11 @@ namespace chip {
 namespace app {
 
 /**
- * The AttributeVValueEncoder is a helper class for filling report payloads into AttributeReportIBs.
+ * The AttributeValueEncoder is a helper class for filling report payloads into AttributeReportIBs.
  * The attribute value encoder can be initialized with a AttributeEncodeState for saving and recovering its state between encode
  * sessions (chunkings).
  *
- * When Encode returned recoverable errors (e.g. CHIP_ERROR_NO_MEMORY) the state can be used to initialize the AttributeValueEncoder
+ * When Encode returns recoverable errors (e.g. CHIP_ERROR_NO_MEMORY) the state can be used to initialize the AttributeValueEncoder
  * for future use on the same attribute path.
  */
 class AttributeValueEncoder
@@ -96,19 +96,11 @@ class AttributeValueEncoder
         AttributeReportIB::Builder attributeReportIBBuilder = mAttributeReportIBsBuilder.CreateAttributeReport();
         AttributeDataIB::Builder attributeDataIBBuilder     = attributeReportIBBuilder.CreateAttributeData();
         AttributePathIB::Builder attributePathIBBuilder     = attributeDataIBBuilder.CreatePath();
-        attributePathIBBuilder.Endpoint(mPath.mEndpointId).Cluster(mPath.mClusterId).Attribute(mPath.mAttributeId);
 
-        // Encode the list index field if we are encoding a single element in an list.
-        if (mCurrentEncodingListIndex != kInvalidListIndex)
-        {
-            attributePathIBBuilder.ListIndex(mCurrentEncodingListIndex);
-        }
-
-        ReturnErrorOnFailure(attributePathIBBuilder.EndOfAttributePathIB().GetError());
-
-        ReturnErrorOnFailure(
-            TagBoundEncoder(attributeDataIBBuilder.GetWriter(), TLV::ContextTag(to_underlying(AttributeDataIB::Tag::kData)))
-                .Encode(std::forward<Ts>(aArgs)...));
+        ReturnErrorOnFailure(EncodeAttributePathIB(attributePathIBBuilder));
+        ReturnErrorOnFailure(DataModel::Encode(*attributeDataIBBuilder.GetWriter(),
+                                               TLV::ContextTag(to_underlying(AttributeDataIB::Tag::kData)),
+                                               std::forward<Ts>(aArgs)...));
 
         attributeDataIBBuilder.DataVersion(mDataVersion);
 
@@ -119,35 +111,9 @@ class AttributeValueEncoder
     template <typename... Ts>
     CHIP_ERROR EncodeListItem(Ts... aArgs)
     {
-        if (mCurrentEncodingListIndex == kInvalidListIndex)
-        {
-            if (mEncodeState.mCurrentEncodingListIndex == kInvalidListIndex)
-            {
-                // Spec 10.5.4.3.1, 10.5.4.6 (Replace a list w/ Multiple IBs)
-                // Put a empty array before encoding the first array element for list chunking.
-                AttributeReportIB::Builder attributeReportIBBuilder = mAttributeReportIBsBuilder.CreateAttributeReport();
-                AttributeDataIB::Builder attributeDataIBBuilder     = attributeReportIBBuilder.CreateAttributeData();
-                AttributePathIB::Builder attributePathIBBuilder     = attributeDataIBBuilder.CreatePath();
-
-                ReturnErrorOnFailure(attributePathIBBuilder.Endpoint(mPath.mEndpointId)
-                                         .Cluster(mPath.mClusterId)
-                                         .Attribute(mPath.mAttributeId)
-                                         .EndOfAttributePathIB()
-                                         .GetError());
-
-                ReturnErrorOnFailure(
-                    TagBoundEncoder(attributeDataIBBuilder.GetWriter(), TLV::ContextTag(to_underlying(AttributeDataIB::Tag::kData)))
-                        .EncodeList([](const TagBoundEncoder &) -> CHIP_ERROR { return CHIP_NO_ERROR; }));
-
-                attributeDataIBBuilder.DataVersion(mDataVersion).EndOfAttributeDataIB();
-                ReturnErrorOnFailure(attributeDataIBBuilder.GetError());
-                ReturnErrorOnFailure(attributeReportIBBuilder.EndOfAttributeReportIB().GetError());
-                mEncodeState.mCurrentEncodingListIndex = 0;
-            }
-            mCurrentEncodingListIndex = 0;
-        }
+        ReturnErrorOnFailure(EncodeEmptyList());
 
-        // We make Encode atomic here, so need to tell the caller do not rollback when we met any error during encoding.
+        // We make Encode atomic here, so need to tell the caller to not rollback when we encounter an error during encoding.
         mEncodeState.mAllowPartialData = true;
 
         if (mCurrentEncodingListIndex < mEncodeState.mCurrentEncodingListIndex)
@@ -163,7 +129,9 @@ class AttributeValueEncoder
         CHIP_ERROR err = Encode(std::forward<Ts>(aArgs)...);
         if (err != CHIP_NO_ERROR)
         {
-            // Rollback on error, then EncodeListItem is atomic.
+            // For list chunking, ReportEngine should not rollback the buffer when CHIP_NO_MEMORY or similar error occurred.
+            // However, the error might be raised in the middle of encoding procedure, then the buffer may contain partial data,
+            // unclosed containers etc. This line clears all possible partial data and makes EncodeListItem is atomic.
             mAttributeReportIBsBuilder.Rollback(backup);
             return err;
         }
@@ -202,9 +170,16 @@ class AttributeValueEncoder
 
     AttributeEncodeState GetState() { return mEncodeState; }
 
-    void FinishManualEncode() {}
-
 private:
+    /**
+     * EncodeListReportHeader encodes the first item of one report with lists (an empty list).
+     *
+     * If internal state indicates we have already encoded the empty list, this function will do nothing and return CHIP_NO_ERROR.
+     */
+    CHIP_ERROR EncodeEmptyList();
+
+    CHIP_ERROR EncodeAttributePathIB(AttributePathIB::Builder &);
+
     bool mTriedEncode = false;
     AttributeReportIBs::Builder & mAttributeReportIBsBuilder;
     const FabricIndex mAccessingFabricIndex;

src/app/BUILD.gn

@@ -35,6 +35,7 @@ static_library("app") {
   output_name = "libCHIPDataModel"
 
   sources = [
+    "AttributeAccessInterface.cpp",
     "AttributePathExpandIterator.cpp",
     "AttributePathExpandIterator.h",
     "AttributePathParams.cpp",

src/app/clusters/descriptor/descriptor.cpp

@@ -59,7 +59,7 @@ CHIP_ERROR DescriptorAttrAccess::ReadPartsAttribute(EndpointId endpoint, Attribu
 
     if (endpoint == 0x00)
     {
-        err = aEncoder.EncodeList([](const AttributeValueEncoder::ListEncodeHelper & encoder) -> CHIP_ERROR {
+        err = aEncoder.EncodeList([](auto encoder) -> CHIP_ERROR {
             for (uint16_t index = 0; index < emberAfEndpointCount(); index++)
             {
                 if (emberAfEndpointIndexIsEnabled(index))
@@ -85,7 +85,7 @@ CHIP_ERROR DescriptorAttrAccess::ReadPartsAttribute(EndpointId endpoint, Attribu
 
 CHIP_ERROR DescriptorAttrAccess::ReadDeviceAttribute(EndpointId endpoint, AttributeValueEncoder & aEncoder)
 {
-    CHIP_ERROR err = aEncoder.EncodeList([&endpoint](const AttributeValueEncoder::ListEncodeHelper & encoder) -> CHIP_ERROR {
+    CHIP_ERROR err = aEncoder.EncodeList([&endpoint](auto encoder) -> CHIP_ERROR {
         Descriptor::Structs::DeviceType::Type deviceStruct;
         uint16_t index = emberAfIndexFromEndpoint(endpoint);
 
@@ -99,18 +99,17 @@ CHIP_ERROR DescriptorAttrAccess::ReadDeviceAttribute(EndpointId endpoint, Attrib
 
 CHIP_ERROR DescriptorAttrAccess::ReadClientServerAttribute(EndpointId endpoint, AttributeValueEncoder & aEncoder, bool server)
 {
-    CHIP_ERROR err =
-        aEncoder.EncodeList([&endpoint, server](const AttributeValueEncoder::ListEncodeHelper & encoder) -> CHIP_ERROR {
-            uint16_t clusterCount = emberAfClusterCount(endpoint, server);
+    CHIP_ERROR err = aEncoder.EncodeList([&endpoint, server](auto encoder) -> CHIP_ERROR {
+        uint16_t clusterCount = emberAfClusterCount(endpoint, server);
 
-            for (uint8_t clusterIndex = 0; clusterIndex < clusterCount; clusterIndex++)
-            {
-                EmberAfCluster * cluster = emberAfGetNthCluster(endpoint, clusterIndex, server);
-                ReturnErrorOnFailure(encoder.Encode(cluster->clusterId));
-            }
+        for (uint8_t clusterIndex = 0; clusterIndex < clusterCount; clusterIndex++)
+        {
+            EmberAfCluster * cluster = emberAfGetNthCluster(endpoint, clusterIndex, server);
+            ReturnErrorOnFailure(encoder.Encode(cluster->clusterId));
+        }
 
-            return CHIP_NO_ERROR;
-        });
+        return CHIP_NO_ERROR;
+    });
 
     return err;
 }

src/app/clusters/general_diagnostics_server/general_diagnostics_server.cpp

@@ -81,7 +81,7 @@ CHIP_ERROR GeneralDiagosticsAttrAccess::ReadListIfSupported(CHIP_ERROR (Platform
 
     if ((PlatformMgr().*getter)(faultList) == CHIP_NO_ERROR)
     {
-        err = aEncoder.EncodeList([&faultList](const AttributeValueEncoder::ListEncodeHelper & encoder) -> CHIP_ERROR {
+        err = aEncoder.EncodeList([&faultList](auto encoder) -> CHIP_ERROR {
             for (auto fault : faultList)
             {
                 ReturnErrorOnFailure(encoder.Encode(fault));
@@ -105,7 +105,7 @@ CHIP_ERROR GeneralDiagosticsAttrAccess::ReadNetworkInterfaces(AttributeValueEnco
 
     if (ConnectivityMgr().GetNetworkInterfaces(&netifs) == CHIP_NO_ERROR)
     {
-        err = aEncoder.EncodeList([&netifs](const AttributeValueEncoder::ListEncodeHelper & encoder) -> CHIP_ERROR {
+        err = aEncoder.EncodeList([&netifs](auto encoder) -> CHIP_ERROR {
             for (DeviceLayer::NetworkInterface * ifp = netifs; ifp != nullptr; ifp = ifp->Next)
             {
                 ReturnErrorOnFailure(encoder.Encode(*ifp));

src/app/clusters/mode-select-server/mode-select-server.cpp

@@ -63,7 +63,7 @@ CHIP_ERROR ModeSelectAttrAccess::Read(const ConcreteReadAttributePath & aPath, A
             return CHIP_NO_ERROR;
         }
         CHIP_ERROR err;
-        err = aEncoder.EncodeList([modeOptionsProvider](const AttributeValueEncoder::ListEncodeHelper & encoder) -> CHIP_ERROR {
+        err = aEncoder.EncodeList([modeOptionsProvider](auto encoder) -> CHIP_ERROR {
             const auto * end = modeOptionsProvider.end();
             for (auto * it = modeOptionsProvider.begin(); it != end; ++it)
             {

src/app/clusters/operational-credentials-server/operational-credentials-server.cpp

@@ -89,7 +89,7 @@ CHIP_ERROR OperationalCredentialsAttrAccess::ReadCommissionedFabrics(EndpointId
 
 CHIP_ERROR OperationalCredentialsAttrAccess::ReadFabricsList(EndpointId endpoint, AttributeValueEncoder & aEncoder)
 {
-    return aEncoder.EncodeList([](const AttributeValueEncoder::ListEncodeHelper & encoder) -> CHIP_ERROR {
+    return aEncoder.EncodeList([](auto encoder) -> CHIP_ERROR {
         for (auto & fabricInfo : Server::GetInstance().GetFabricTable())
         {
             if (!fabricInfo.IsInitialized())
@@ -114,7 +114,7 @@ CHIP_ERROR OperationalCredentialsAttrAccess::ReadFabricsList(EndpointId endpoint
 
 CHIP_ERROR OperationalCredentialsAttrAccess::ReadRootCertificates(EndpointId endpoint, AttributeValueEncoder & aEncoder)
 {
-    return aEncoder.EncodeList([](const AttributeValueEncoder::ListEncodeHelper & encoder) -> CHIP_ERROR {
+    return aEncoder.EncodeList([](auto encoder) -> CHIP_ERROR {
         for (auto & fabricInfo : Server::GetInstance().GetFabricTable())
         {
             ByteSpan cert;

src/app/clusters/software_diagnostics_server/software_diagnostics_server.cpp

@@ -104,7 +104,7 @@ CHIP_ERROR SoftwareDiagosticsAttrAccess::ReadThreadMetrics(AttributeValueEncoder
 
     if (DeviceLayer::PlatformMgr().GetThreadMetrics(&threadMetrics) == CHIP_NO_ERROR)
     {
-        err = aEncoder.EncodeList([&threadMetrics](const AttributeValueEncoder::ListEncodeHelper & encoder) -> CHIP_ERROR {
+        err = aEncoder.EncodeList([&threadMetrics](auto encoder) -> CHIP_ERROR {
             for (DeviceLayer::ThreadMetrics * thread = threadMetrics; thread != nullptr; thread = thread->Next)
             {
                 ReturnErrorOnFailure(encoder.Encode(*thread));

src/app/clusters/test-cluster-server/test-cluster-server.cpp

@@ -136,7 +136,7 @@ CHIP_ERROR TestAttrAccess::Write(const ConcreteDataAttributePath & aPath, Attrib
 
 CHIP_ERROR TestAttrAccess::ReadListInt8uAttribute(AttributeValueEncoder & aEncoder)
 {
-    return aEncoder.EncodeList([](const AttributeValueEncoder::ListEncodeHelper & encoder) -> CHIP_ERROR {
+    return aEncoder.EncodeList([](auto encoder) -> CHIP_ERROR {
         for (uint8_t index = 0; index < kAttributeListLength; index++)
         {
             ReturnErrorOnFailure(encoder.Encode(gListUint8Data[index]));
@@ -166,7 +166,7 @@ CHIP_ERROR TestAttrAccess::WriteListInt8uAttribute(AttributeValueDecoder & aDeco
 
 CHIP_ERROR TestAttrAccess::ReadListOctetStringAttribute(AttributeValueEncoder & aEncoder)
 {
-    return aEncoder.EncodeList([](const AttributeValueEncoder::ListEncodeHelper & encoder) -> CHIP_ERROR {
+    return aEncoder.EncodeList([](auto encoder) -> CHIP_ERROR {
         for (uint8_t index = 0; index < kAttributeListLength; index++)
         {
             ByteSpan span(gListOctetStringData[index].Data(), gListOctetStringData[index].Length());
@@ -200,7 +200,7 @@ CHIP_ERROR TestAttrAccess::WriteListOctetStringAttribute(AttributeValueDecoder &
 
 CHIP_ERROR TestAttrAccess::ReadListStructOctetStringAttribute(AttributeValueEncoder & aEncoder)
 {
-    return aEncoder.EncodeList([](const AttributeValueEncoder::ListEncodeHelper & encoder) -> CHIP_ERROR {
+    return aEncoder.EncodeList([](auto encoder) -> CHIP_ERROR {
         for (uint8_t index = 0; index < kAttributeListLength; index++)
         {
             Structs::TestListStructOctet::Type structOctet;
@@ -246,7 +246,7 @@ CHIP_ERROR TestAttrAccess::WriteListStructOctetStringAttribute(AttributeValueDec
 
 CHIP_ERROR TestAttrAccess::ReadListNullablesAndOptionalsStructAttribute(AttributeValueEncoder & aEncoder)
 {
-    return aEncoder.EncodeList([](const AttributeValueEncoder::ListEncodeHelper & encoder) -> CHIP_ERROR {
+    return aEncoder.EncodeList([](auto encoder) -> CHIP_ERROR {
         // Just encode a single default-initialized
         // entry for now.
         Structs::NullablesAndOptionalsStruct::Type entry;

src/app/tests/TestAttributeValueEncoder.cpp

@@ -156,7 +156,7 @@ void TestEncodeListOfBools2(nlTestSuite * aSuite, void * aContext)
 {
     TestSetup test(aSuite);
     bool list[]    = { true, false };
-    CHIP_ERROR err = test.encoder.EncodeList([&list](const AttributeValueEncoder::ListEncodeHelper & encoder) -> CHIP_ERROR {
+    CHIP_ERROR err = test.encoder.EncodeList([&list](auto encoder) -> CHIP_ERROR {
         for (auto & item : list)
         {
             ReturnErrorOnFailure(encoder.Encode(item));
@@ -217,7 +217,7 @@ void TestEncodeListChunking(nlTestSuite * aSuite, void * aContext)
     AttributeValueEncoder::AttributeEncodeState state;
 
     bool list[]      = { true, false };
-    auto listEncoder = [&list](const AttributeValueEncoder::ListEncodeHelper & encoder) -> CHIP_ERROR {
+    auto listEncoder = [&list](auto encoder) -> CHIP_ERROR {
         for (auto & item : list)
         {
             ReturnErrorOnFailure(encoder.Encode(item));

src/app/util/mock/attribute-storage.cpp

@@ -246,7 +246,7 @@ CHIP_ERROR ReadSingleMockClusterData(FabricIndex aAccessingFabricIndex, const Co
             (apEncoderState == nullptr ? AttributeValueEncoder::AttributeEncodeState() : *apEncoderState);
         AttributeValueEncoder valueEncoder(aAttributeReports, aAccessingFabricIndex, aPath, 0, state);
 
-        CHIP_ERROR err = valueEncoder.EncodeList([](const AttributeValueEncoder::ListEncodeHelper & encoder) -> CHIP_ERROR {
+        CHIP_ERROR err = valueEncoder.EncodeList([](auto encoder) -> CHIP_ERROR {
             for (int i = 0; i < 6; i++)
             {
                 ReturnErrorOnFailure(encoder.Encode(chip::ByteSpan(mockAttribute4, sizeof(mockAttribute4))));