Skip to content

Commit

Permalink
Reverted the AttributePersistenceProvider and added a SafeAttributePe…
Browse files Browse the repository at this point in the history 
…rsistenceProvider (#28302)

* Renamed AttributePersistenceProvider to SafeAttributePersistenceProvider. Reverted to the previous AttributePersistenceProvider. Updated the tests and examples that used the AttributePersistenceProvider.

* Restyled by whitespace

* Restyled by clang-format

* Restyled by gn

* Fixed the key generated for safe attributes.

Co-authored-by: Tennessee Carmel-Veilleux <[email protected]>

* Apply documentation suggestions from code review

Co-authored-by: Boris Zbarsky <[email protected]>

* Removed unused aPath and import.

* Fixed some docs. Removed size from the SafeAttributePersistenceProvider::SafeReadValue method.

* Replaced the aType input var from the SafeAttributePersistenceProvider::SafeReadValue method with documentation on the expectet types.

* Replaced the SafeAttributePersistenceProvider::GetNullValueForNullableType logic with that used in NumericAttributeTraits::GetNullValue. Mode the GetNullValueForNullableType methods private and commented out the relevant unit tests.

* Restyled by clang-format

* Replaced the SafeAttributePersistenceProvider::GetNullValueForNullableType methods with NumericAttributeTraits.

* Apply documentation suggestions from code review

Co-authored-by: Boris Zbarsky <[email protected]>

* Restyled by clang-format

* Refactored SafeAttributePersistenceProvider reducing the number of templated functions.

Co-authored-by: Boris Zbarsky <[email protected]>

* Fixed minor bug in suggestions.

* Restyled by whitespace

---------

Co-authored-by: Restyled.io <[email protected]>
Co-authored-by: Tennessee Carmel-Veilleux <[email protected]>
Co-authored-by: Boris Zbarsky <[email protected]>
  • Loading branch information
@hicklin @restyled-commits
@tcarmelveilleux @bzbarsky-apple @pull
4 people authored and pull[bot] committed Dec 11, 2023
1 parent 4060e54 commit 1360306
Show file tree
Hide file tree
Showing 13 changed files with 288 additions and 214 deletions.
170 changes: 7 additions & 163 deletions src/app/AttributePersistenceProvider.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,19 +16,17 @@
#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 {
namespace app {

/**
* Interface for persisting attribute values.
* Interface for persisting attribute values. This will write attributes in storage with platform endianness for scalars
* and uses a different key space from SafeAttributePersistenceProvider.
* When storing cluster attributes that are managed via the AttributeAccessInterface, it is recommended to
* use SafeAttributePersistenceProvider.
*/

class AttributePersistenceProvider
Expand Down Expand Up @@ -61,171 +59,17 @@ class AttributePersistenceProvider
* Read an attribute value from non-volatile memory.
*
* @param [in] aPath the attribute path for the data being persisted.
* @param [in] aType the attribute type.
* @param [in] aSize the attribute size.
* @param [in] aMetadata the attribute metadata, as a convenience.
* @param [in,out] aValue where to place the data. The size of the buffer
* will be equal to `size`.
* will be equal to `size` member of aMetadata.
*
* 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, EmberAfAttributeType aType, size_t aSize,
virtual CHIP_ERROR ReadValue(const ConcreteAttributePath & aPath, const EmberAfAttributeMetadata * aMetadata,
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;
}
};

/**
Expand Down
1 change: 1 addition & 0 deletions src/app/BUILD.gn
View file
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,7 @@ static_library("app") {
"ReadHandler.cpp",
"RequiredPrivilege.cpp",
"RequiredPrivilege.h",
"SafeAttributePersistenceProvider.h",
"StatusResponse.cpp",
"StatusResponse.h",
"SubscriptionResumptionStorage.h",
Expand Down
69 changes: 60 additions & 9 deletions src/app/DefaultAttributePersistenceProvider.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
namespace chip {
namespace app {

CHIP_ERROR DefaultAttributePersistenceProvider::WriteValue(const ConcreteAttributePath & aPath, const ByteSpan & aValue)
CHIP_ERROR DefaultAttributePersistenceProvider::InternalWriteValue(const StorageKeyName & aKey, const ByteSpan & aValue)
{
VerifyOrReturnError(mStorage != nullptr, CHIP_ERROR_INCORRECT_STATE);

Expand All @@ -33,20 +33,16 @@ CHIP_ERROR DefaultAttributePersistenceProvider::WriteValue(const ConcreteAttribu
{
return CHIP_ERROR_BUFFER_TOO_SMALL;
}
return mStorage->SyncSetKeyValue(
DefaultStorageKeyAllocator::AttributeValue(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId).KeyName(),
aValue.data(), static_cast<uint16_t>(aValue.size()));
return mStorage->SyncSetKeyValue(aKey.KeyName(), aValue.data(), static_cast<uint16_t>(aValue.size()));
}

CHIP_ERROR DefaultAttributePersistenceProvider::ReadValue(const ConcreteAttributePath & aPath, EmberAfAttributeType aType,
size_t aSize, MutableByteSpan & aValue)
CHIP_ERROR DefaultAttributePersistenceProvider::InternalReadValue(const StorageKeyName & aKey, 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));
ReturnErrorOnFailure(mStorage->SyncGetKeyValue(aKey.KeyName(), aValue.data(), size));
EmberAfAttributeType type = aType;
if (emberAfIsStringAttributeType(type))
{
Expand All @@ -71,12 +67,45 @@ CHIP_ERROR DefaultAttributePersistenceProvider::ReadValue(const ConcreteAttribut
return CHIP_NO_ERROR;
}

CHIP_ERROR DefaultAttributePersistenceProvider::WriteValue(const ConcreteAttributePath & aPath, const ByteSpan & aValue)
{
return InternalWriteValue(DefaultStorageKeyAllocator::AttributeValue(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId),
aValue);
}

CHIP_ERROR DefaultAttributePersistenceProvider::ReadValue(const ConcreteAttributePath & aPath,
const EmberAfAttributeMetadata * aMetadata, MutableByteSpan & aValue)
{
return InternalReadValue(DefaultStorageKeyAllocator::AttributeValue(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId),
aMetadata->attributeType, aMetadata->size, aValue);
}

CHIP_ERROR DefaultAttributePersistenceProvider::SafeWriteValue(const ConcreteAttributePath & aPath, const ByteSpan & aValue)
{
return InternalWriteValue(
DefaultStorageKeyAllocator::SafeAttributeValue(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId), aValue);
}

CHIP_ERROR DefaultAttributePersistenceProvider::SafeReadValue(const ConcreteAttributePath & aPath, MutableByteSpan & aValue)
{
return InternalReadValue(
DefaultStorageKeyAllocator::SafeAttributeValue(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId), 0x20,
aValue.size(), aValue);
}

namespace {

AttributePersistenceProvider * gAttributeSaver = nullptr;

} // anonymous namespace

/**
* Gets the global attribute saver.
*
* Note: When storing cluster attributes that are managed via AttributeAccessInterface, it is recommended to
* use SafeAttributePersistenceProvider. See AttributePersistenceProvider and SafeAttributePersistenceProvider
* class documentation for more information.
*/
AttributePersistenceProvider * GetAttributePersistenceProvider()
{
return gAttributeSaver;
Expand All @@ -90,5 +119,27 @@ void SetAttributePersistenceProvider(AttributePersistenceProvider * aProvider)
}
}

namespace {

SafeAttributePersistenceProvider * gSafeAttributeSaver = nullptr;

} // anonymous namespace

/**
* Gets the global attribute safe saver.
*/
SafeAttributePersistenceProvider * GetSafeAttributePersistenceProvider()
{
return gSafeAttributeSaver;
}

void SetSafeAttributePersistenceProvider(SafeAttributePersistenceProvider * aProvider)
{
if (aProvider != nullptr)
{
gSafeAttributeSaver = aProvider;
}
}

} // namespace app
} // namespace chip
13 changes: 11 additions & 2 deletions src/app/DefaultAttributePersistenceProvider.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
#pragma once

#include <app/AttributePersistenceProvider.h>
#include <app/SafeAttributePersistenceProvider.h>
#include <lib/core/CHIPPersistentStorageDelegate.h>
#include <lib/support/DefaultStorageKeyAllocator.h>

Expand All @@ -30,7 +31,7 @@ namespace app {
* of this class, since it can't be constructed automatically without knowing
* what PersistentStorageDelegate is to be used.
*/
class DefaultAttributePersistenceProvider : public AttributePersistenceProvider
class DefaultAttributePersistenceProvider : public AttributePersistenceProvider, public SafeAttributePersistenceProvider
{
public:
DefaultAttributePersistenceProvider() {}
Expand All @@ -50,11 +51,19 @@ class DefaultAttributePersistenceProvider : public AttributePersistenceProvider

// AttributePersistenceProvider implementation.
CHIP_ERROR WriteValue(const ConcreteAttributePath & aPath, const ByteSpan & aValue) override;
CHIP_ERROR ReadValue(const ConcreteAttributePath & aPath, EmberAfAttributeType aType, size_t aSize,
CHIP_ERROR ReadValue(const ConcreteAttributePath & aPath, const EmberAfAttributeMetadata * aMetadata,
MutableByteSpan & aValue) override;

// SafeAttributePersistenceProvider implementation.
CHIP_ERROR SafeWriteValue(const ConcreteAttributePath & aPath, const ByteSpan & aValue) override;
CHIP_ERROR SafeReadValue(const ConcreteAttributePath & aPath, MutableByteSpan & aValue) override;

protected:
PersistentStorageDelegate * mStorage;

private:
CHIP_ERROR InternalWriteValue(const StorageKeyName & aKey, const ByteSpan & aValue);
CHIP_ERROR InternalReadValue(const StorageKeyName & aKey, EmberAfAttributeType aType, size_t aSize, MutableByteSpan & aValue);
};

} // namespace app
Expand Down
6 changes: 3 additions & 3 deletions src/app/DeferredAttributePersistenceProvider.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,10 +57,10 @@ CHIP_ERROR DeferredAttributePersistenceProvider::WriteValue(const ConcreteAttrib
return mPersister.WriteValue(aPath, aValue);
}

CHIP_ERROR DeferredAttributePersistenceProvider::ReadValue(const ConcreteAttributePath & aPath, EmberAfAttributeType aType,
size_t aSize, MutableByteSpan & aValue)
CHIP_ERROR DeferredAttributePersistenceProvider::ReadValue(const ConcreteAttributePath & aPath,
const EmberAfAttributeMetadata * aMetadata, MutableByteSpan & aValue)
{
return mPersister.ReadValue(aPath, aType, aSize, aValue);
return mPersister.ReadValue(aPath, aMetadata, aValue);
}

void DeferredAttributePersistenceProvider::FlushAndScheduleNext()
Expand Down
2 changes: 1 addition & 1 deletion src/app/DeferredAttributePersistenceProvider.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ class DeferredAttributePersistenceProvider : public AttributePersistenceProvider
* For other attributes, immediately pass the write operation to the decorated persister.
*/
CHIP_ERROR WriteValue(const ConcreteAttributePath & aPath, const ByteSpan & aValue) override;
CHIP_ERROR ReadValue(const ConcreteAttributePath & aPath, EmberAfAttributeType aType, size_t aSize,
CHIP_ERROR ReadValue(const ConcreteAttributePath & aPath, const EmberAfAttributeMetadata * aMetadata,
MutableByteSpan & aValue) override;

private:
Expand Down
Loading

0 comments on commit 1360306

Please sign in to comment.