Expand attribute persistence provider api (#27611)

* Modified the AttributePersistenceProvider ReadValue function signiture to take the required attribute information directly rather than as a EmberAfAttributeMetadata structure. * Expanded AttributePersistanceProvider API to include reading and wirting of uint8 and nullable uint8 * Templated the AttributePersistanceProvider read and wiret function to work for all uint types. * Fixed AttributePersistanceProvider accepted types. Added read helper for type bool. * Restyled by clang-format * Fixed mismatched size return error of DefaultAttributePersistenceProvider * Started work on tests for the AttributePersistenceProvider. * Added unit tests for AttributePersistenceProvider testing the storage and retrival of all unsigned types and their nullable veriaties, bool and ByteSpan. Tested for small buffer errors. * Changed the type of aSize in ReadValue to size_t * Removed the dependancy on generated code in the AttributePersistencezprovider.h * Added static funtctions to get the KVS null representation for different types. * Added functions to read and write nullable bools and accompanying tests. * Incorporated boolean tests in the scalar test. * Added failure before init test * Restyled by clang-format * Fixed after merge. * Removed the failure on init test as it may have been causing seg faults in some tests. * Renamed GetNull -> GetNullValueForNullableType * Added the initialisation of valueReadBack and added a new templated function for nullable types to avoid the error: The left operand of '==' is a garbage value, on some platforms. * Added handline of signed ints and accompanying tests. * Added handline of nullable signed ints and accompanying tests. * Type cast null. * Restyled by clang-format * Changed shift bit to be af the same type are the return val. * Added tests got GetNull functions --------- Co-authored-by: Restyled.io <[email protected]>
project-chip · Dec 11, 2023 · 4c19e03 · 4c19e03
1 parent 634b80a
commit 4c19e03
Show file tree

Hide file tree

Showing 10 changed files with 591 additions and 23 deletions.
diff --git a/src/app/AttributePersistenceProvider.h b/src/app/AttributePersistenceProvider.h
@@ -16,7 +16,12 @@
 #pragma once
 
 #include <app/ConcreteAttributePath.h>
+#include <app/data-model/Nullable.h>
 #include <app/util/attribute-metadata.h>
+#include <cstring>
+#include <inttypes.h>
+#include <lib/support/BufferReader.h>
+#include <lib/support/BufferWriter.h>
 #include <lib/support/Span.h>
 
 namespace chip {
@@ -56,17 +61,171 @@ class AttributePersistenceProvider
      * Read an attribute value from non-volatile memory.
      *
      * @param [in]     aPath the attribute path for the data being persisted.
-     * @param [in]     aMetadata the attribute metadata, as a convenience.
+     * @param [in]     aType the attribute type.
+     * @param [in]     aSize the attribute size.
      * @param [in,out] aValue where to place the data.  The size of the buffer
-     *                 will be equal to `size` member of aMetadata.
+     *                 will be equal to `size`.
      *
      *                 The data is expected to be in native endianness for
      *                 integers and floats.  For strings, see the string
      *                 representation description in the WriteValue
      *                 documentation.
      */
-    virtual CHIP_ERROR ReadValue(const ConcreteAttributePath & aPath, const EmberAfAttributeMetadata * aMetadata,
+    virtual CHIP_ERROR ReadValue(const ConcreteAttributePath & aPath, EmberAfAttributeType aType, size_t aSize,
                                  MutableByteSpan & aValue) = 0;
+
+    /**
+     * Get the KVS representation of null for the given type.
+     * @tparam T The type for which the null representation should be returned.
+     * @return A value of type T that in the KVS represents null.
+     */
+    template <typename T, std::enable_if_t<std::is_same<bool, T>::value, bool> = true>
+    static uint8_t GetNullValueForNullableType()
+    {
+        return 0xff;
+    }
+
+    /**
+     * Get the KVS representation of null for the given type.
+     * @tparam T The type for which the null representation should be returned.
+     * @return A value of type T that in the KVS represents null.
+     */
+    template <typename T, std::enable_if_t<std::is_unsigned<T>::value && !std::is_same<bool, T>::value, bool> = true>
+    static T GetNullValueForNullableType()
+    {
+        T nullValue = 0;
+        nullValue   = T(~nullValue);
+        return nullValue;
+    }
+
+    /**
+     * Get the KVS representation of null for the given type.
+     * @tparam T The type for which the null representation should be returned.
+     * @return A value of type T that in the KVS represents null.
+     */
+    template <typename T, std::enable_if_t<std::is_signed<T>::value && !std::is_same<bool, T>::value, bool> = true>
+    static T GetNullValueForNullableType()
+    {
+        T shiftBit = 1;
+        return T(shiftBit << ((sizeof(T) * 8) - 1));
+    }
+
+    // The following API provides helper functions to simplify the access of commonly used types.
+    // The API may not be complete.
+    // Currently implemented write and read types are: uint8_t, uint16_t, uint32_t, unit64_t and
+    // their nullable varieties, and bool.
+
+    /**
+     * Write an attribute value of type intX, uintX or bool to non-volatile memory.
+     *
+     * @param [in] aPath the attribute path for the data being written.
+     * @param [in] aValue the data to write.
+     */
+    template <typename T, std::enable_if_t<std::is_integral<T>::value, bool> = true>
+    CHIP_ERROR WriteScalarValue(const ConcreteAttributePath & aPath, T & aValue)
+    {
+        uint8_t value[sizeof(T)];
+        auto w = Encoding::LittleEndian::BufferWriter(value, sizeof(T));
+        w.EndianPut(uint64_t(aValue), sizeof(T));
+
+        return WriteValue(aPath, ByteSpan(value));
+    }
+
+    /**
+     * Read an attribute of type intX, uintX or bool from non-volatile memory.
+     *
+     * @param [in]     aPath the attribute path for the data being persisted.
+     * @param [in,out] aValue where to place the data.
+     */
+    template <typename T, std::enable_if_t<std::is_integral<T>::value, bool> = true>
+    CHIP_ERROR ReadScalarValue(const ConcreteAttributePath & aPath, T & aValue)
+    {
+        uint8_t attrData[sizeof(T)];
+        MutableByteSpan tempVal(attrData);
+        // **Note** aType in the ReadValue function is only used to check if the value is of a string type. Since this template
+        // function is only enabled for integral values, we know that this case will not occur, so we can pass the enum of an
+        // arbitrary integral type. 0x20 is the ZCL enum type for ZCL_INT8U_ATTRIBUTE_TYPE.
+        auto err = ReadValue(aPath, 0x20, sizeof(T), tempVal);
+        if (err != CHIP_NO_ERROR)
+        {
+            return err;
+        }
+
+        chip::Encoding::LittleEndian::Reader r(tempVal.data(), tempVal.size());
+        r.RawReadLowLevelBeCareful(&aValue);
+        return r.StatusCode();
+    }
+
+    /**
+     * Write an attribute value of type nullable intX, uintX or bool to non-volatile memory.
+     *
+     * @param [in] aPath the attribute path for the data being written.
+     * @param [in] aValue the data to write.
+     */
+    template <typename T, std::enable_if_t<std::is_integral<T>::value, bool> = true>
+    CHIP_ERROR WriteScalarValue(const ConcreteAttributePath & aPath, DataModel::Nullable<T> & aValue)
+    {
+        if (aValue.IsNull())
+        {
+            auto nullVal = GetNullValueForNullableType<T>();
+            return WriteScalarValue(aPath, nullVal);
+        }
+        return WriteScalarValue(aPath, aValue.Value());
+    }
+
+    /**
+     * Read an attribute of type nullable intX, uintX from non-volatile memory.
+     *
+     * @param [in]     aPath the attribute path for the data being persisted.
+     * @param [in,out] aValue where to place the data.
+     */
+    template <typename T, std::enable_if_t<std::is_integral<T>::value && !std::is_same<bool, T>::value, bool> = true>
+    CHIP_ERROR ReadScalarValue(const ConcreteAttributePath & aPath, DataModel::Nullable<T> & aValue)
+    {
+        T tempIntegral;
+
+        CHIP_ERROR err = ReadScalarValue(aPath, tempIntegral);
+        if (err != CHIP_NO_ERROR)
+        {
+            return err;
+        }
+
+        if (tempIntegral == GetNullValueForNullableType<T>())
+        {
+            aValue.SetNull();
+            return CHIP_NO_ERROR;
+        }
+
+        aValue.SetNonNull(tempIntegral);
+        return CHIP_NO_ERROR;
+    }
+
+    /**
+     * Read an attribute of type nullable bool from non-volatile memory.
+     *
+     * @param [in]     aPath the attribute path for the data being persisted.
+     * @param [in,out] aValue where to place the data.
+     */
+    template <typename T, std::enable_if_t<std::is_same<bool, T>::value, bool> = true>
+    CHIP_ERROR ReadScalarValue(const ConcreteAttributePath & aPath, DataModel::Nullable<T> & aValue)
+    {
+        uint8_t tempIntegral;
+
+        CHIP_ERROR err = ReadScalarValue(aPath, tempIntegral);
+        if (err != CHIP_NO_ERROR)
+        {
+            return err;
+        }
+
+        if (tempIntegral == GetNullValueForNullableType<T>())
+        {
+            aValue.SetNull();
+            return CHIP_NO_ERROR;
+        }
+
+        aValue.SetNonNull(tempIntegral);
+        return CHIP_NO_ERROR;
+    }
 };
 
 /**

diff --git a/src/app/BUILD.gn b/src/app/BUILD.gn
@@ -88,7 +88,9 @@ static_library("app") {
     "CommandResponseHelper.h",
     "CommandSender.cpp",
     "DefaultAttributePersistenceProvider.cpp",
+    "DefaultAttributePersistenceProvider.h",
     "DeferredAttributePersistenceProvider.cpp",
+    "DeferredAttributePersistenceProvider.h",
     "DeviceProxy.cpp",
     "DeviceProxy.h",
     "EventManagement.cpp",

diff --git a/src/app/DefaultAttributePersistenceProvider.cpp b/src/app/DefaultAttributePersistenceProvider.cpp
@@ -27,8 +27,8 @@ CHIP_ERROR DefaultAttributePersistenceProvider::WriteValue(const ConcreteAttribu
     VerifyOrReturnError(mStorage != nullptr, CHIP_ERROR_INCORRECT_STATE);
 
     // TODO: we may want to have a small cache for values that change a lot, so
-    // we only write them once a bunch of changes happen or on timer or
-    // shutdown.
+    //  we only write them once a bunch of changes happen or on timer or
+    //  shutdown.
     if (!CanCastTo<uint16_t>(aValue.size()))
     {
         return CHIP_ERROR_BUFFER_TOO_SMALL;
@@ -38,16 +38,16 @@ CHIP_ERROR DefaultAttributePersistenceProvider::WriteValue(const ConcreteAttribu
         aValue.data(), static_cast<uint16_t>(aValue.size()));
 }
 
-CHIP_ERROR DefaultAttributePersistenceProvider::ReadValue(const ConcreteAttributePath & aPath,
-                                                          const EmberAfAttributeMetadata * aMetadata, MutableByteSpan & aValue)
+CHIP_ERROR DefaultAttributePersistenceProvider::ReadValue(const ConcreteAttributePath & aPath, EmberAfAttributeType aType,
+                                                          size_t aSize, MutableByteSpan & aValue)
 {
     VerifyOrReturnError(mStorage != nullptr, CHIP_ERROR_INCORRECT_STATE);
 
     uint16_t size = static_cast<uint16_t>(min(aValue.size(), static_cast<size_t>(UINT16_MAX)));
     ReturnErrorOnFailure(mStorage->SyncGetKeyValue(
         DefaultStorageKeyAllocator::AttributeValue(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId).KeyName(),
         aValue.data(), size));
-    EmberAfAttributeType type = aMetadata->attributeType;
+    EmberAfAttributeType type = aType;
     if (emberAfIsStringAttributeType(type))
     {
         // Ensure that we've read enough bytes that we are not ending up with
@@ -65,7 +65,7 @@ CHIP_ERROR DefaultAttributePersistenceProvider::ReadValue(const ConcreteAttribut
     else
     {
         // Ensure we got the expected number of bytes for all other types.
-        VerifyOrReturnError(size == aMetadata->size, CHIP_ERROR_INCORRECT_STATE);
+        VerifyOrReturnError(size == aSize, CHIP_ERROR_INVALID_ARGUMENT);
     }
     aValue.reduce_size(size);
     return CHIP_NO_ERROR;

diff --git a/src/app/DefaultAttributePersistenceProvider.h b/src/app/DefaultAttributePersistenceProvider.h
@@ -50,7 +50,7 @@ class DefaultAttributePersistenceProvider : public AttributePersistenceProvider
 
     // AttributePersistenceProvider implementation.
     CHIP_ERROR WriteValue(const ConcreteAttributePath & aPath, const ByteSpan & aValue) override;
-    CHIP_ERROR ReadValue(const ConcreteAttributePath & aPath, const EmberAfAttributeMetadata * aMetadata,
+    CHIP_ERROR ReadValue(const ConcreteAttributePath & aPath, EmberAfAttributeType aType, size_t aSize,
                          MutableByteSpan & aValue) override;
 
 protected:

diff --git a/src/app/DeferredAttributePersistenceProvider.cpp b/src/app/DeferredAttributePersistenceProvider.cpp
@@ -42,25 +42,25 @@ void DeferredAttribute::Flush(AttributePersistenceProvider & persister)
     mValue.Release();
 }
 
-CHIP_ERROR DeferredAttributePersistenceProvider::WriteValue(const ConcreteAttributePath & path, const ByteSpan & value)
+CHIP_ERROR DeferredAttributePersistenceProvider::WriteValue(const ConcreteAttributePath & aPath, const ByteSpan & aValue)
 {
     for (DeferredAttribute & da : mDeferredAttributes)
     {
-        if (da.Matches(path))
+        if (da.Matches(aPath))
         {
-            ReturnErrorOnFailure(da.PrepareWrite(System::SystemClock().GetMonotonicTimestamp() + mWriteDelay, value));
+            ReturnErrorOnFailure(da.PrepareWrite(System::SystemClock().GetMonotonicTimestamp() + mWriteDelay, aValue));
             FlushAndScheduleNext();
             return CHIP_NO_ERROR;
         }
     }
 
-    return mPersister.WriteValue(path, value);
+    return mPersister.WriteValue(aPath, aValue);
 }
 
-CHIP_ERROR DeferredAttributePersistenceProvider::ReadValue(const ConcreteAttributePath & path,
-                                                           const EmberAfAttributeMetadata * metadata, MutableByteSpan & value)
+CHIP_ERROR DeferredAttributePersistenceProvider::ReadValue(const ConcreteAttributePath & aPath, EmberAfAttributeType aType,
+                                                           size_t aSize, MutableByteSpan & aValue)
 {
-    return mPersister.ReadValue(path, metadata, value);
+    return mPersister.ReadValue(aPath, aType, aSize, aValue);
 }
 
 void DeferredAttributePersistenceProvider::FlushAndScheduleNext()

diff --git a/src/app/DeferredAttributePersistenceProvider.h b/src/app/DeferredAttributePersistenceProvider.h
@@ -66,9 +66,9 @@ class DeferredAttributePersistenceProvider : public AttributePersistenceProvider
      *
      * For other attributes, immediately pass the write operation to the decorated persister.
      */
-    CHIP_ERROR WriteValue(const ConcreteAttributePath & path, const ByteSpan & value) override;
-    CHIP_ERROR ReadValue(const ConcreteAttributePath & path, const EmberAfAttributeMetadata * metadata,
-                         MutableByteSpan & value) override;
+    CHIP_ERROR WriteValue(const ConcreteAttributePath & aPath, const ByteSpan & aValue) override;
+    CHIP_ERROR ReadValue(const ConcreteAttributePath & aPath, EmberAfAttributeType aType, size_t aSize,
+                         MutableByteSpan & aValue) override;
 
 private:
     void FlushAndScheduleNext();

diff --git a/src/app/tests/BUILD.gn b/src/app/tests/BUILD.gn
@@ -112,6 +112,7 @@ chip_test_suite("tests") {
   test_sources = [
     "TestAclEvent.cpp",
     "TestAttributePathExpandIterator.cpp",
+    "TestAttributePersistenceProvider.cpp",
     "TestAttributeValueDecoder.cpp",
     "TestAttributeValueEncoder.cpp",
     "TestBindingTable.cpp",