Skip to content

Commit 5dd4b06

Browse files
hicklinrestyled-commitstcarmelveilleuxbzbarsky-apple
authored
Reverted the AttributePersistenceProvider and added a SafeAttributePersistenceProvider (project-chip#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 <tennessee.carmelveilleux@gmail.com> * Apply documentation suggestions from code review Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * 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 <bzbarsky@apple.com> * Restyled by clang-format * Refactored SafeAttributePersistenceProvider reducing the number of templated functions. Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Fixed minor bug in suggestions. * Restyled by whitespace --------- Co-authored-by: Restyled.io <commits@restyled.io> Co-authored-by: Tennessee Carmel-Veilleux <tennessee.carmelveilleux@gmail.com> Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
1 parent 97964db commit 5dd4b06

13 files changed

+288
-214
lines changed

‎src/app/AttributePersistenceProvider.h

+7-163
Original file line numberDiff line numberDiff line change
@@ -16,19 +16,17 @@
1616
#pragma once
1717

1818
#include <app/ConcreteAttributePath.h>
19-
#include <app/data-model/Nullable.h>
2019
#include <app/util/attribute-metadata.h>
21-
#include <cstring>
22-
#include <inttypes.h>
23-
#include <lib/support/BufferReader.h>
24-
#include <lib/support/BufferWriter.h>
2520
#include <lib/support/Span.h>
2621

2722
namespace chip {
2823
namespace app {
2924

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

3432
class AttributePersistenceProvider
@@ -61,171 +59,17 @@ class AttributePersistenceProvider
6159
* Read an attribute value from non-volatile memory.
6260
*
6361
* @param [in] aPath the attribute path for the data being persisted.
64-
* @param [in] aType the attribute type.
65-
* @param [in] aSize the attribute size.
62+
* @param [in] aMetadata the attribute metadata, as a convenience.
6663
* @param [in,out] aValue where to place the data. The size of the buffer
67-
* will be equal to `size`.
64+
* will be equal to `size` member of aMetadata.
6865
*
6966
* The data is expected to be in native endianness for
7067
* integers and floats. For strings, see the string
7168
* representation description in the WriteValue
7269
* documentation.
7370
*/
74-
virtual CHIP_ERROR ReadValue(const ConcreteAttributePath & aPath, EmberAfAttributeType aType, size_t aSize,
71+
virtual CHIP_ERROR ReadValue(const ConcreteAttributePath & aPath, const EmberAfAttributeMetadata * aMetadata,
7572
MutableByteSpan & aValue) = 0;
76-
77-
/**
78-
* Get the KVS representation of null for the given type.
79-
* @tparam T The type for which the null representation should be returned.
80-
* @return A value of type T that in the KVS represents null.
81-
*/
82-
template <typename T, std::enable_if_t<std::is_same<bool, T>::value, bool> = true>
83-
static uint8_t GetNullValueForNullableType()
84-
{
85-
return 0xff;
86-
}
87-
88-
/**
89-
* Get the KVS representation of null for the given type.
90-
* @tparam T The type for which the null representation should be returned.
91-
* @return A value of type T that in the KVS represents null.
92-
*/
93-
template <typename T, std::enable_if_t<std::is_unsigned<T>::value && !std::is_same<bool, T>::value, bool> = true>
94-
static T GetNullValueForNullableType()
95-
{
96-
T nullValue = 0;
97-
nullValue = T(~nullValue);
98-
return nullValue;
99-
}
100-
101-
/**
102-
* Get the KVS representation of null for the given type.
103-
* @tparam T The type for which the null representation should be returned.
104-
* @return A value of type T that in the KVS represents null.
105-
*/
106-
template <typename T, std::enable_if_t<std::is_signed<T>::value && !std::is_same<bool, T>::value, bool> = true>
107-
static T GetNullValueForNullableType()
108-
{
109-
T shiftBit = 1;
110-
return T(shiftBit << ((sizeof(T) * 8) - 1));
111-
}
112-
113-
// The following API provides helper functions to simplify the access of commonly used types.
114-
// The API may not be complete.
115-
// Currently implemented write and read types are: uint8_t, uint16_t, uint32_t, unit64_t and
116-
// their nullable varieties, and bool.
117-
118-
/**
119-
* Write an attribute value of type intX, uintX or bool to non-volatile memory.
120-
*
121-
* @param [in] aPath the attribute path for the data being written.
122-
* @param [in] aValue the data to write.
123-
*/
124-
template <typename T, std::enable_if_t<std::is_integral<T>::value, bool> = true>
125-
CHIP_ERROR WriteScalarValue(const ConcreteAttributePath & aPath, T & aValue)
126-
{
127-
uint8_t value[sizeof(T)];
128-
auto w = Encoding::LittleEndian::BufferWriter(value, sizeof(T));
129-
w.EndianPut(uint64_t(aValue), sizeof(T));
130-
131-
return WriteValue(aPath, ByteSpan(value));
132-
}
133-
134-
/**
135-
* Read an attribute of type intX, uintX or bool from non-volatile memory.
136-
*
137-
* @param [in] aPath the attribute path for the data being persisted.
138-
* @param [in,out] aValue where to place the data.
139-
*/
140-
template <typename T, std::enable_if_t<std::is_integral<T>::value, bool> = true>
141-
CHIP_ERROR ReadScalarValue(const ConcreteAttributePath & aPath, T & aValue)
142-
{
143-
uint8_t attrData[sizeof(T)];
144-
MutableByteSpan tempVal(attrData);
145-
// **Note** aType in the ReadValue function is only used to check if the value is of a string type. Since this template
146-
// function is only enabled for integral values, we know that this case will not occur, so we can pass the enum of an
147-
// arbitrary integral type. 0x20 is the ZCL enum type for ZCL_INT8U_ATTRIBUTE_TYPE.
148-
auto err = ReadValue(aPath, 0x20, sizeof(T), tempVal);
149-
if (err != CHIP_NO_ERROR)
150-
{
151-
return err;
152-
}
153-
154-
chip::Encoding::LittleEndian::Reader r(tempVal.data(), tempVal.size());
155-
r.RawReadLowLevelBeCareful(&aValue);
156-
return r.StatusCode();
157-
}
158-
159-
/**
160-
* Write an attribute value of type nullable intX, uintX or bool to non-volatile memory.
161-
*
162-
* @param [in] aPath the attribute path for the data being written.
163-
* @param [in] aValue the data to write.
164-
*/
165-
template <typename T, std::enable_if_t<std::is_integral<T>::value, bool> = true>
166-
CHIP_ERROR WriteScalarValue(const ConcreteAttributePath & aPath, DataModel::Nullable<T> & aValue)
167-
{
168-
if (aValue.IsNull())
169-
{
170-
auto nullVal = GetNullValueForNullableType<T>();
171-
return WriteScalarValue(aPath, nullVal);
172-
}
173-
return WriteScalarValue(aPath, aValue.Value());
174-
}
175-
176-
/**
177-
* Read an attribute of type nullable intX, uintX from non-volatile memory.
178-
*
179-
* @param [in] aPath the attribute path for the data being persisted.
180-
* @param [in,out] aValue where to place the data.
181-
*/
182-
template <typename T, std::enable_if_t<std::is_integral<T>::value && !std::is_same<bool, T>::value, bool> = true>
183-
CHIP_ERROR ReadScalarValue(const ConcreteAttributePath & aPath, DataModel::Nullable<T> & aValue)
184-
{
185-
T tempIntegral;
186-
187-
CHIP_ERROR err = ReadScalarValue(aPath, tempIntegral);
188-
if (err != CHIP_NO_ERROR)
189-
{
190-
return err;
191-
}
192-
193-
if (tempIntegral == GetNullValueForNullableType<T>())
194-
{
195-
aValue.SetNull();
196-
return CHIP_NO_ERROR;
197-
}
198-
199-
aValue.SetNonNull(tempIntegral);
200-
return CHIP_NO_ERROR;
201-
}
202-
203-
/**
204-
* Read an attribute of type nullable bool from non-volatile memory.
205-
*
206-
* @param [in] aPath the attribute path for the data being persisted.
207-
* @param [in,out] aValue where to place the data.
208-
*/
209-
template <typename T, std::enable_if_t<std::is_same<bool, T>::value, bool> = true>
210-
CHIP_ERROR ReadScalarValue(const ConcreteAttributePath & aPath, DataModel::Nullable<T> & aValue)
211-
{
212-
uint8_t tempIntegral;
213-
214-
CHIP_ERROR err = ReadScalarValue(aPath, tempIntegral);
215-
if (err != CHIP_NO_ERROR)
216-
{
217-
return err;
218-
}
219-
220-
if (tempIntegral == GetNullValueForNullableType<T>())
221-
{
222-
aValue.SetNull();
223-
return CHIP_NO_ERROR;
224-
}
225-
226-
aValue.SetNonNull(tempIntegral);
227-
return CHIP_NO_ERROR;
228-
}
22973
};
23074

23175
/**

‎src/app/BUILD.gn

+1
Original file line numberDiff line numberDiff line change
@@ -183,6 +183,7 @@ static_library("app") {
183183
"ReadHandler.cpp",
184184
"RequiredPrivilege.cpp",
185185
"RequiredPrivilege.h",
186+
"SafeAttributePersistenceProvider.h",
186187
"StatusResponse.cpp",
187188
"StatusResponse.h",
188189
"SubscriptionResumptionStorage.h",

‎src/app/DefaultAttributePersistenceProvider.cpp

+60-9
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@
2222
namespace chip {
2323
namespace app {
2424

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

@@ -33,20 +33,16 @@ CHIP_ERROR DefaultAttributePersistenceProvider::WriteValue(const ConcreteAttribu
3333
{
3434
return CHIP_ERROR_BUFFER_TOO_SMALL;
3535
}
36-
return mStorage->SyncSetKeyValue(
37-
DefaultStorageKeyAllocator::AttributeValue(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId).KeyName(),
38-
aValue.data(), static_cast<uint16_t>(aValue.size()));
36+
return mStorage->SyncSetKeyValue(aKey.KeyName(), aValue.data(), static_cast<uint16_t>(aValue.size()));
3937
}
4038

41-
CHIP_ERROR DefaultAttributePersistenceProvider::ReadValue(const ConcreteAttributePath & aPath, EmberAfAttributeType aType,
42-
size_t aSize, MutableByteSpan & aValue)
39+
CHIP_ERROR DefaultAttributePersistenceProvider::InternalReadValue(const StorageKeyName & aKey, EmberAfAttributeType aType,
40+
size_t aSize, MutableByteSpan & aValue)
4341
{
4442
VerifyOrReturnError(mStorage != nullptr, CHIP_ERROR_INCORRECT_STATE);
4543

4644
uint16_t size = static_cast<uint16_t>(min(aValue.size(), static_cast<size_t>(UINT16_MAX)));
47-
ReturnErrorOnFailure(mStorage->SyncGetKeyValue(
48-
DefaultStorageKeyAllocator::AttributeValue(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId).KeyName(),
49-
aValue.data(), size));
45+
ReturnErrorOnFailure(mStorage->SyncGetKeyValue(aKey.KeyName(), aValue.data(), size));
5046
EmberAfAttributeType type = aType;
5147
if (emberAfIsStringAttributeType(type))
5248
{
@@ -71,12 +67,45 @@ CHIP_ERROR DefaultAttributePersistenceProvider::ReadValue(const ConcreteAttribut
7167
return CHIP_NO_ERROR;
7268
}
7369

70+
CHIP_ERROR DefaultAttributePersistenceProvider::WriteValue(const ConcreteAttributePath & aPath, const ByteSpan & aValue)
71+
{
72+
return InternalWriteValue(DefaultStorageKeyAllocator::AttributeValue(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId),
73+
aValue);
74+
}
75+
76+
CHIP_ERROR DefaultAttributePersistenceProvider::ReadValue(const ConcreteAttributePath & aPath,
77+
const EmberAfAttributeMetadata * aMetadata, MutableByteSpan & aValue)
78+
{
79+
return InternalReadValue(DefaultStorageKeyAllocator::AttributeValue(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId),
80+
aMetadata->attributeType, aMetadata->size, aValue);
81+
}
82+
83+
CHIP_ERROR DefaultAttributePersistenceProvider::SafeWriteValue(const ConcreteAttributePath & aPath, const ByteSpan & aValue)
84+
{
85+
return InternalWriteValue(
86+
DefaultStorageKeyAllocator::SafeAttributeValue(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId), aValue);
87+
}
88+
89+
CHIP_ERROR DefaultAttributePersistenceProvider::SafeReadValue(const ConcreteAttributePath & aPath, MutableByteSpan & aValue)
90+
{
91+
return InternalReadValue(
92+
DefaultStorageKeyAllocator::SafeAttributeValue(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId), 0x20,
93+
aValue.size(), aValue);
94+
}
95+
7496
namespace {
7597

7698
AttributePersistenceProvider * gAttributeSaver = nullptr;
7799

78100
} // anonymous namespace
79101

102+
/**
103+
* Gets the global attribute saver.
104+
*
105+
* Note: When storing cluster attributes that are managed via AttributeAccessInterface, it is recommended to
106+
* use SafeAttributePersistenceProvider. See AttributePersistenceProvider and SafeAttributePersistenceProvider
107+
* class documentation for more information.
108+
*/
80109
AttributePersistenceProvider * GetAttributePersistenceProvider()
81110
{
82111
return gAttributeSaver;
@@ -90,5 +119,27 @@ void SetAttributePersistenceProvider(AttributePersistenceProvider * aProvider)
90119
}
91120
}
92121

122+
namespace {
123+
124+
SafeAttributePersistenceProvider * gSafeAttributeSaver = nullptr;
125+
126+
} // anonymous namespace
127+
128+
/**
129+
* Gets the global attribute safe saver.
130+
*/
131+
SafeAttributePersistenceProvider * GetSafeAttributePersistenceProvider()
132+
{
133+
return gSafeAttributeSaver;
134+
}
135+
136+
void SetSafeAttributePersistenceProvider(SafeAttributePersistenceProvider * aProvider)
137+
{
138+
if (aProvider != nullptr)
139+
{
140+
gSafeAttributeSaver = aProvider;
141+
}
142+
}
143+
93144
} // namespace app
94145
} // namespace chip

‎src/app/DefaultAttributePersistenceProvider.h

+11-2
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
#pragma once
1717

1818
#include <app/AttributePersistenceProvider.h>
19+
#include <app/SafeAttributePersistenceProvider.h>
1920
#include <lib/core/CHIPPersistentStorageDelegate.h>
2021
#include <lib/support/DefaultStorageKeyAllocator.h>
2122

@@ -30,7 +31,7 @@ namespace app {
3031
* of this class, since it can't be constructed automatically without knowing
3132
* what PersistentStorageDelegate is to be used.
3233
*/
33-
class DefaultAttributePersistenceProvider : public AttributePersistenceProvider
34+
class DefaultAttributePersistenceProvider : public AttributePersistenceProvider, public SafeAttributePersistenceProvider
3435
{
3536
public:
3637
DefaultAttributePersistenceProvider() {}
@@ -50,11 +51,19 @@ class DefaultAttributePersistenceProvider : public AttributePersistenceProvider
5051

5152
// AttributePersistenceProvider implementation.
5253
CHIP_ERROR WriteValue(const ConcreteAttributePath & aPath, const ByteSpan & aValue) override;
53-
CHIP_ERROR ReadValue(const ConcreteAttributePath & aPath, EmberAfAttributeType aType, size_t aSize,
54+
CHIP_ERROR ReadValue(const ConcreteAttributePath & aPath, const EmberAfAttributeMetadata * aMetadata,
5455
MutableByteSpan & aValue) override;
5556

57+
// SafeAttributePersistenceProvider implementation.
58+
CHIP_ERROR SafeWriteValue(const ConcreteAttributePath & aPath, const ByteSpan & aValue) override;
59+
CHIP_ERROR SafeReadValue(const ConcreteAttributePath & aPath, MutableByteSpan & aValue) override;
60+
5661
protected:
5762
PersistentStorageDelegate * mStorage;
63+
64+
private:
65+
CHIP_ERROR InternalWriteValue(const StorageKeyName & aKey, const ByteSpan & aValue);
66+
CHIP_ERROR InternalReadValue(const StorageKeyName & aKey, EmberAfAttributeType aType, size_t aSize, MutableByteSpan & aValue);
5867
};
5968

6069
} // namespace app

‎src/app/DeferredAttributePersistenceProvider.cpp

+3-3
Original file line numberDiff line numberDiff line change
@@ -57,10 +57,10 @@ CHIP_ERROR DeferredAttributePersistenceProvider::WriteValue(const ConcreteAttrib
5757
return mPersister.WriteValue(aPath, aValue);
5858
}
5959

60-
CHIP_ERROR DeferredAttributePersistenceProvider::ReadValue(const ConcreteAttributePath & aPath, EmberAfAttributeType aType,
61-
size_t aSize, MutableByteSpan & aValue)
60+
CHIP_ERROR DeferredAttributePersistenceProvider::ReadValue(const ConcreteAttributePath & aPath,
61+
const EmberAfAttributeMetadata * aMetadata, MutableByteSpan & aValue)
6262
{
63-
return mPersister.ReadValue(aPath, aType, aSize, aValue);
63+
return mPersister.ReadValue(aPath, aMetadata, aValue);
6464
}
6565

6666
void DeferredAttributePersistenceProvider::FlushAndScheduleNext()

‎src/app/DeferredAttributePersistenceProvider.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,7 @@ class DeferredAttributePersistenceProvider : public AttributePersistenceProvider
6767
* For other attributes, immediately pass the write operation to the decorated persister.
6868
*/
6969
CHIP_ERROR WriteValue(const ConcreteAttributePath & aPath, const ByteSpan & aValue) override;
70-
CHIP_ERROR ReadValue(const ConcreteAttributePath & aPath, EmberAfAttributeType aType, size_t aSize,
70+
CHIP_ERROR ReadValue(const ConcreteAttributePath & aPath, const EmberAfAttributeMetadata * aMetadata,
7171
MutableByteSpan & aValue) override;
7272

7373
private:

0 commit comments

Comments
 (0)