Some interface cleanup and make AttributeValueEncoder/Decoder more similar (#33028)

* Uniform API: provide subject descriptor only for both value encode and decode

* Split out AttributeEncodeState and decouple things to use less friend classes

* Fix tests

* Slight move around based on pahole results, to try to make size differences a bit smaller

* Fix typo ... amazingly enough tests still pass, so test read is maybe broken

* Two more replacements for state compare logic

* Restyle

* Fix logic error in Descripto creation for test

* Update argument name

* Remove useless comments

* move AttributeEncodeState to a separate file

* Make the copy from a pointer logic cleaner

* Fix value passing

* Fix const correctness

* Fix one more compile

* Put back AccessingFabricIndex

* Updated back the comments and file layout

---------

Co-authored-by: Andrei Litvin <andreilitvin@google.com>

Author: andy31415

src/app/AttributeEncodeState.h

@@ -0,0 +1,103 @@
+/*
+ *    Copyright (c) 2021-2024 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.
+ */
+#pragma once
+
+#include <lib/core/DataModelTypes.h>
+
+namespace chip {
+namespace app {
+
+/// Maintains the internal state of list encoding
+///
+/// List encoding is generally assumed incremental and chunkable (i.e.
+/// partial encoding is ok.). For this purpose the class maintains two
+/// pieces of data:
+///   - AllowPartialData tracks if partial encoding is acceptable in the
+///     current encoding state (to be used for atomic/non-atomic list item writes)
+///   - CurrentEncodingListIndex representing the list index that is next
+///     to be encoded in the output. kInvalidListIndex means that a new list
+///     encoding has been started.
+class AttributeEncodeState
+{
+public:
+    AttributeEncodeState() = default;
+
+    /// Allows the encode state to be initialized from an OPTIONAL
+    /// other encoding state
+    ///
+    /// if other is nullptr, this is the same as the default initializer.
+    AttributeEncodeState(const AttributeEncodeState * other)
+    {
+        if (other != nullptr)
+        {
+            *this = *other;
+        }
+        else
+        {
+            mCurrentEncodingListIndex = kInvalidListIndex;
+            mAllowPartialData         = false;
+        }
+    }
+
+    bool AllowPartialData() const { return mAllowPartialData; }
+    ListIndex CurrentEncodingListIndex() const { return mCurrentEncodingListIndex; }
+
+    AttributeEncodeState & SetAllowPartialData(bool allow)
+    {
+        mAllowPartialData = allow;
+        return *this;
+    }
+
+    AttributeEncodeState & SetCurrentEncodingListIndex(ListIndex idx)
+    {
+        mCurrentEncodingListIndex = idx;
+        return *this;
+    }
+
+    void Reset()
+    {
+        mCurrentEncodingListIndex = kInvalidListIndex;
+        mAllowPartialData         = false;
+    }
+
+private:
+    /**
+     * If set to kInvalidListIndex, indicates that we have not encoded any data for the list yet and
+     * need to start by encoding an empty list before we start encoding any list items.
+     *
+     * When set to a valid ListIndex value, indicates the index of the next list item that needs to be
+     * encoded (i.e. the count of items encoded so far).
+     */
+    ListIndex mCurrentEncodingListIndex = kInvalidListIndex;
+
+    /**
+     * When an attempt to encode an attribute returns an error, the buffer may contain tailing dirty data
+     * (since the put was aborted).  The report engine normally rolls back the buffer to right before encoding
+     * of the attribute started on errors.
+     *
+     * When chunking a list, EncodeListItem will atomically encode list items, ensuring that the
+     * state of the buffer is valid to send (i.e. contains no trailing garbage), and return an error
+     * if the list doesn't entirely fit.  In this situation, mAllowPartialData is set to communicate to the
+     * report engine that it should not roll back the list items.
+     *
+     * TODO: There might be a better name for this variable.
+     */
+    bool mAllowPartialData = false;
+};
+
+} // namespace app
+} // namespace chip

src/app/AttributeValueEncoder.cpp

@@ -32,13 +32,13 @@ CHIP_ERROR AttributeValueEncoder::EnsureListStarted()
 {
     VerifyOrDie(mCurrentEncodingListIndex == kInvalidListIndex);
 
-    mEncodingInitialList = (mEncodeState.mCurrentEncodingListIndex == kInvalidListIndex);
+    mEncodingInitialList = (mEncodeState.CurrentEncodingListIndex() == kInvalidListIndex);
     if (mEncodingInitialList)
     {
         // Clear mAllowPartialData flag here since this encode procedure is not atomic.
         // The most common error in this function is CHIP_ERROR_NO_MEMORY / CHIP_ERROR_BUFFER_TOO_SMALL, just revert and try
         // next time is ok.
-        mEncodeState.mAllowPartialData = false;
+        mEncodeState.SetAllowPartialData(false);
 
         AttributeReportBuilder builder;
 
@@ -59,7 +59,7 @@ CHIP_ERROR AttributeValueEncoder::EnsureListStarted()
         ReturnErrorOnFailure(
             mAttributeReportIBsBuilder.GetWriter()->ReserveBuffer(kEndOfAttributeReportIBByteCount + kEndOfListByteCount));
 
-        mEncodeState.mCurrentEncodingListIndex = 0;
+        mEncodeState.SetCurrentEncodingListIndex(0);
     }
     else
     {
@@ -72,7 +72,7 @@ CHIP_ERROR AttributeValueEncoder::EnsureListStarted()
 
     // After encoding the initial list start, the remaining items are atomically encoded into the buffer. Tell report engine to not
     // revert partial data.
-    mEncodeState.mAllowPartialData = true;
+    mEncodeState.SetAllowPartialData(true);
 
     return CHIP_NO_ERROR;
 }
@@ -105,7 +105,7 @@ void AttributeValueEncoder::EnsureListEnded()
         // 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;
+        mEncodeState.SetAllowPartialData(false);
     }
 }
 

src/app/AttributeValueEncoder.h

@@ -16,6 +16,8 @@
  */
 #pragma once
 
+#include <access/SubjectDescriptor.h>
+#include <app/AttributeEncodeState.h>
 #include <app/AttributeReportBuilder.h>
 #include <app/ConcreteAttributePath.h>
 #include <app/MessageDef/AttributeReportIBs.h>
@@ -51,9 +53,9 @@ class AttributeValueEncoder
             // If we are encoding for a fabric filtered attribute read and the fabric index does not match that present in the
             // request, skip encoding this list item.
             VerifyOrReturnError(!mAttributeValueEncoder.mIsFabricFiltered ||
-                                    aArg.GetFabricIndex() == mAttributeValueEncoder.mAccessingFabricIndex,
+                                    aArg.GetFabricIndex() == mAttributeValueEncoder.AccessingFabricIndex(),
                                 CHIP_NO_ERROR);
-            return mAttributeValueEncoder.EncodeListItem(mAttributeValueEncoder.mAccessingFabricIndex, std::forward<T>(aArg));
+            return mAttributeValueEncoder.EncodeListItem(mAttributeValueEncoder.AccessingFabricIndex(), std::forward<T>(aArg));
         }
 
         template <typename T, std::enable_if_t<!DataModel::IsFabricScoped<T>::value, bool> = true>
@@ -66,42 +68,11 @@ class AttributeValueEncoder
         AttributeValueEncoder & mAttributeValueEncoder;
     };
 
-    class AttributeEncodeState
-    {
-    public:
-        AttributeEncodeState() : mAllowPartialData(false), mCurrentEncodingListIndex(kInvalidListIndex) {}
-        bool AllowPartialData() const { return mAllowPartialData; }
-
-    private:
-        friend class AttributeValueEncoder;
-        /**
-         * When an attempt to encode an attribute returns an error, the buffer may contain tailing dirty data
-         * (since the put was aborted).  The report engine normally rolls back the buffer to right before encoding
-         * of the attribute started on errors.
-         *
-         * When chunking a list, EncodeListItem will atomically encode list items, ensuring that the
-         * state of the buffer is valid to send (i.e. contains no trailing garbage), and return an error
-         * if the list doesn't entirely fit.  In this situation, mAllowPartialData is set to communicate to the
-         * report engine that it should not roll back the list items.
-         *
-         * TODO: There might be a better name for this variable.
-         */
-        bool mAllowPartialData = false;
-        /**
-         * If set to kInvalidListIndex, indicates that we have not encoded any data for the list yet and
-         * need to start by encoding an empty list before we start encoding any list items.
-         *
-         * When set to a valid ListIndex value, indicates the index of the next list item that needs to be
-         * encoded (i.e. the count of items encoded so far).
-         */
-        ListIndex mCurrentEncodingListIndex = kInvalidListIndex;
-    };
-
-    AttributeValueEncoder(AttributeReportIBs::Builder & aAttributeReportIBsBuilder, FabricIndex aAccessingFabricIndex,
+    AttributeValueEncoder(AttributeReportIBs::Builder & aAttributeReportIBsBuilder, Access::SubjectDescriptor subjectDescriptor,
                           const ConcreteAttributePath & aPath, DataVersion aDataVersion, bool aIsFabricFiltered = false,
                           const AttributeEncodeState & aState = AttributeEncodeState()) :
         mAttributeReportIBsBuilder(aAttributeReportIBsBuilder),
-        mAccessingFabricIndex(aAccessingFabricIndex), mPath(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId),
+        mSubjectDescriptor(subjectDescriptor), mPath(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId),
         mDataVersion(aDataVersion), mIsFabricFiltered(aIsFabricFiltered), mEncodeState(aState)
     {}
 
@@ -169,17 +140,19 @@ class AttributeValueEncoder
         if (err == CHIP_NO_ERROR)
         {
             // The Encode procedure finished without any error, clear the state.
-            mEncodeState = AttributeEncodeState();
+            mEncodeState.Reset();
         }
         return err;
     }
 
     bool TriedEncode() const { return mTriedEncode; }
 
+    const Access::SubjectDescriptor & GetSubjectDescriptor() const { return mSubjectDescriptor; }
+
     /**
      * The accessing fabric index for this read or subscribe interaction.
      */
-    FabricIndex AccessingFabricIndex() const { return mAccessingFabricIndex; }
+    FabricIndex AccessingFabricIndex() const { return GetSubjectDescriptor().fabricIndex; }
 
     /**
      * AttributeValueEncoder is a short lived object, and the state is persisted by mEncodeState and restored by constructor.
@@ -195,7 +168,7 @@ class AttributeValueEncoder
     {
         // EncodeListItem must be called after EnsureListStarted(), thus mCurrentEncodingListIndex and
         // mEncodeState.mCurrentEncodingListIndex are not invalid values.
-        if (mCurrentEncodingListIndex < mEncodeState.mCurrentEncodingListIndex)
+        if (mCurrentEncodingListIndex < mEncodeState.CurrentEncodingListIndex())
         {
             // We have encoded this element in previous chunks, skip it.
             mCurrentEncodingListIndex++;
@@ -226,7 +199,7 @@ class AttributeValueEncoder
         }
 
         mCurrentEncodingListIndex++;
-        mEncodeState.mCurrentEncodingListIndex++;
+        mEncodeState.SetCurrentEncodingListIndex(mCurrentEncodingListIndex);
         mEncodedAtLeastOneListItem = true;
         return CHIP_NO_ERROR;
     }
@@ -266,20 +239,20 @@ class AttributeValueEncoder
      */
     void EnsureListEnded();
 
-    bool mTriedEncode = false;
     AttributeReportIBs::Builder & mAttributeReportIBsBuilder;
-    const FabricIndex mAccessingFabricIndex;
+    const Access::SubjectDescriptor mSubjectDescriptor;
     ConcreteDataAttributePath mPath;
     DataVersion mDataVersion;
+    bool mTriedEncode      = false;
     bool mIsFabricFiltered = false;
     // mEncodingInitialList is true if we're encoding a list and we have not
     // 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;
+    bool mEncodedAtLeastOneListItem     = false;
     ListIndex mCurrentEncodingListIndex = kInvalidListIndex;
+    AttributeEncodeState mEncodeState;
 };
 
 } // namespace app

src/app/BUILD.gn

@@ -281,6 +281,7 @@ static_library("attribute-access") {
     "AttributeAccessInterfaceCache.h",
     "AttributeAccessInterfaceRegistry.cpp",
     "AttributeAccessInterfaceRegistry.h",
+    "AttributeEncodeState.h",
     "AttributeReportBuilder.cpp",
     "AttributeReportBuilder.h",
     "AttributeValueDecoder.h",

src/app/ConcreteAttributePath.h

@@ -101,7 +101,7 @@ struct ConcreteReadAttributePath : public ConcreteAttributePath
  */
 struct ConcreteDataAttributePath : public ConcreteAttributePath
 {
-    enum class ListOperation
+    enum class ListOperation : uint8_t
     {
         NotList,     // Path points to an attribute that isn't a list.
         ReplaceAll,  // Path points to an attribute that is a list, indicating that the contents of the list should be replaced in

src/app/ReadHandler.cpp

@@ -841,7 +841,7 @@ void ReadHandler::PersistSubscription()
 void ReadHandler::ResetPathIterator()
 {
     mAttributePathExpandIterator = AttributePathExpandIterator(mpAttributePathList);
-    mAttributeEncoderState       = AttributeValueEncoder::AttributeEncodeState();
+    mAttributeEncoderState.Reset();
 }
 
 void ReadHandler::AttributePathIsDirty(const AttributePathParams & aAttributeChanged)
@@ -870,7 +870,7 @@ void ReadHandler::AttributePathIsDirty(const AttributePathParams & aAttributeCha
         // our iterator to point back to the beginning of that cluster. This ensures that the receiver will get a coherent view of
         // the state of the cluster as present on the server
         mAttributePathExpandIterator.ResetCurrentCluster();
-        mAttributeEncoderState = AttributeValueEncoder::AttributeEncodeState();
+        mAttributeEncoderState.Reset();
     }
 
     // ReportScheduler will take care of verifying the reportability of the handler and schedule the run

src/app/ReadHandler.h

@@ -412,8 +412,8 @@ class ReadHandler : public Messaging::ExchangeDelegate
     /// or after the min interval is reached if it has not yet been reached.
     void ForceDirtyState();
 
-    const AttributeValueEncoder::AttributeEncodeState & GetAttributeEncodeState() const { return mAttributeEncoderState; }
-    void SetAttributeEncodeState(const AttributeValueEncoder::AttributeEncodeState & aState) { mAttributeEncoderState = aState; }
+    const AttributeEncodeState & GetAttributeEncodeState() const { return mAttributeEncoderState; }
+    void SetAttributeEncodeState(const AttributeEncodeState & aState) { mAttributeEncoderState = aState; }
     uint32_t GetLastWrittenEventsBytes() const { return mLastWrittenEventsBytes; }
 
     // Returns the number of interested paths, including wildcard and concrete paths.
@@ -562,7 +562,7 @@ class ReadHandler : public Messaging::ExchangeDelegate
 
     // The detailed encoding state for a single attribute, used by list chunking feature.
     // The size of AttributeEncoderState is 2 bytes for now.
-    AttributeValueEncoder::AttributeEncodeState mAttributeEncoderState;
+    AttributeEncodeState mAttributeEncoderState;
 
     // Current Handler state
     HandlerState mState            = HandlerState::Idle;

src/app/dynamic_server/DynamicDispatcher.cpp

@@ -123,7 +123,7 @@ Status DetermineAttributeStatus(const ConcreteAttributePath & aPath, bool aIsWri
 
 CHIP_ERROR ReadSingleClusterData(const SubjectDescriptor & aSubjectDescriptor, bool aIsFabricFiltered,
                                  const ConcreteReadAttributePath & aPath, AttributeReportIBs::Builder & aAttributeReports,
-                                 AttributeValueEncoder::AttributeEncodeState * aEncoderState)
+                                 AttributeEncodeState * aEncoderState)
 {
     Status status = DetermineAttributeStatus(aPath, /* aIsWrite = */ false);
     return aAttributeReports.EncodeAttributeStatus(aPath, StatusIB(status));

src/app/reporting/Engine.cpp

@@ -82,7 +82,7 @@ bool Engine::IsClusterDataVersionMatch(const SingleLinkedListNode<DataVersionFil
 CHIP_ERROR
 Engine::RetrieveClusterData(const SubjectDescriptor & aSubjectDescriptor, bool aIsFabricFiltered,
                             AttributeReportIBs::Builder & aAttributeReportIBs, const ConcreteReadAttributePath & aPath,
-                            AttributeValueEncoder::AttributeEncodeState * aEncoderState)
+                            AttributeEncodeState * aEncoderState)
 {
     ChipLogDetail(DataManagement, "<RE:Run> Cluster %" PRIx32 ", Attribute %" PRIx32 " is dirty", aPath.mClusterId,
                   aPath.mAttributeId);
@@ -199,7 +199,7 @@ CHIP_ERROR Engine::BuildSingleReportDataAttributeReportIBs(ReportDataMessage::Bu
             attributeReportIBs.Checkpoint(attributeBackup);
             ConcreteReadAttributePath pathForRetrieval(readPath);
             // Load the saved state from previous encoding session for chunking of one single attribute (list chunking).
-            AttributeValueEncoder::AttributeEncodeState encodeState = apReadHandler->GetAttributeEncodeState();
+            AttributeEncodeState encodeState = apReadHandler->GetAttributeEncodeState();
             err = RetrieveClusterData(apReadHandler->GetSubjectDescriptor(), apReadHandler->IsFabricFiltered(), attributeReportIBs,
                                       pathForRetrieval, &encodeState);
             if (err != CHIP_NO_ERROR)
@@ -226,7 +226,7 @@ CHIP_ERROR Engine::BuildSingleReportDataAttributeReportIBs(ReportDataMessage::Bu
                     // We met a error during writing reports, one common case is we are running out of buffer, rollback the
                     // attributeReportIB to avoid any partial data.
                     attributeReportIBs.Rollback(attributeBackup);
-                    apReadHandler->SetAttributeEncodeState(AttributeValueEncoder::AttributeEncodeState());
+                    apReadHandler->SetAttributeEncodeState(AttributeEncodeState());
 
                     if (!IsOutOfWriterSpaceError(err))
                     {
@@ -256,7 +256,7 @@ CHIP_ERROR Engine::BuildSingleReportDataAttributeReportIBs(ReportDataMessage::Bu
             }
             SuccessOrExit(err);
             // Successfully encoded the attribute, clear the internal state.
-            apReadHandler->SetAttributeEncodeState(AttributeValueEncoder::AttributeEncodeState());
+            apReadHandler->SetAttributeEncodeState(AttributeEncodeState());
         }
         // We just visited all paths interested by this read handler and did not abort in the middle of iteration, there are no more
         // chunks for this report.

src/app/reporting/Engine.h

@@ -170,8 +170,7 @@ class Engine
                                                  bool aBufferIsUsed, bool * apHasMoreChunks, bool * apHasEncodedData);
     CHIP_ERROR RetrieveClusterData(const Access::SubjectDescriptor & aSubjectDescriptor, bool aIsFabricFiltered,
                                    AttributeReportIBs::Builder & aAttributeReportIBs,
-                                   const ConcreteReadAttributePath & aClusterInfo,
-                                   AttributeValueEncoder::AttributeEncodeState * apEncoderState);
+                                   const ConcreteReadAttributePath & aClusterInfo, AttributeEncodeState * apEncoderState);
     CHIP_ERROR CheckAccessDeniedEventPaths(TLV::TLVWriter & aWriter, bool & aHasEncodedData, ReadHandler * apReadHandler);
 
     // If version match, it means don't send, if version mismatch, it means send.