Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expand attribute persistence provider api #27611

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
48915f3
Modified the AttributePersistenceProvider ReadValue function signitur…
hicklin Jul 3, 2023
1f989a9
Expanded AttributePersistanceProvider API to include reading and wirt…
hicklin Jul 3, 2023
b5ed2d5
Templated the AttributePersistanceProvider read and wiret function to…
hicklin Jul 3, 2023
c58e9a2
Fixed AttributePersistanceProvider accepted types. Added read helper …
hicklin Jul 4, 2023
1b1abfe
Merge branch 'project-chip:master' into expand_AttributePersistancePr…
hicklin Jul 4, 2023
e398672
Restyled by clang-format
restyled-commits Jul 4, 2023
6e3d3c7
Fixed mismatched size return error of DefaultAttributePersistenceProv…
hicklin Jul 4, 2023
4630dc1
Started work on tests for the AttributePersistenceProvider.
hicklin Jul 5, 2023
d60a795
Added unit tests for AttributePersistenceProvider testing the storage…
hicklin Jul 5, 2023
1643373
Changed the type of aSize in ReadValue to size_t
hicklin Jul 6, 2023
e1ec87e
Removed the dependancy on generated code in the AttributePersistencez…
hicklin Jul 6, 2023
4574741
Added static funtctions to get the KVS null representation for differ…
hicklin Jul 6, 2023
b5d456c
Added functions to read and write nullable bools and accompanying tests.
hicklin Jul 6, 2023
15230fc
Incorporated boolean tests in the scalar test.
hicklin Jul 6, 2023
476bf9c
Merge branch 'master' into expand_AttributePersistanceProvider_API
hicklin Jul 6, 2023
76db4d3
Added failure before init test
hicklin Jul 6, 2023
333ac54
Restyled by clang-format
restyled-commits Jul 6, 2023
4f9c481
Merge branch 'master' into expand_AttributePersistanceProvider_API
hicklin Jul 7, 2023
2d6f26a
Fixed after merge.
hicklin Jul 7, 2023
2d4cca0
Removed the failure on init test as it may have been causing seg faul…
hicklin Jul 7, 2023
09488ec
Renamed GetNull -> GetNullValueForNullableType
hicklin Jul 7, 2023
582b92c
Added the initialisation of valueReadBack and added a new templated f…
hicklin Jul 7, 2023
7c1bf20
Added handline of signed ints and accompanying tests.
hicklin Jul 7, 2023
81d062f
Added handline of nullable signed ints and accompanying tests.
hicklin Jul 7, 2023
de3b1c9
Type cast null.
hicklin Jul 7, 2023
35070d1
Merge branch 'master' into expand_AttributePersistanceProvider_API
hicklin Jul 7, 2023
7f2423f
Restyled by clang-format
restyled-commits Jul 7, 2023
9ab6af9
Changed shift bit to be af the same type are the return val.
hicklin Jul 7, 2023
e289e60
Added tests got GetNull functions
hicklin Jul 7, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
165 changes: 162 additions & 3 deletions src/app/AttributePersistenceProvider.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an API change to a public SDK API that will break consumers. Why is this ok?

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()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this reinventing various machinery from src/app/util/attribute-storage-null-handling.h?

{
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>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this have tested for T being unsigned too, if we are going to assume below that it is? (But watch out for bool....)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The std::is_integral will return true if T is a signed or unsigned integer. According to this documentation at least.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it will; my point is that the actual code in there ends up assuming the value is unsigned, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So concretely. Say this is invoked with T == int8_t, aValue == -1.

We will set up a 1-byte buffer.

We will then proceed to do w.EndianPut(uint64_t(aValue), 1); on the buffer writer. But for a signed value we should be doing w.EndianPutSigned, I would think. So we should have separate things for signed vs unsigned here @hicklin, instead of assuming things that might or might not be true about the implementation details of the buffer writer.

CHIP_ERROR WriteScalarValue(const ConcreteAttributePath & aPath, T & aValue)
{
uint8_t value[sizeof(T)];
auto w = Encoding::LittleEndian::BufferWriter(value, sizeof(T));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will completely break on a big-endian system for non-one-byte values, no? The values passed to AttributePersistenceProvider are native-endian for integers! This is very clearly documented above.

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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The data is not 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, ReadValue is implemented by the application in general. You can't make assumptions about how it will use the data it's handed.

// 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());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the extra chip::? But again, the data here is native-endian.

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
2 changes: 2 additions & 0 deletions src/app/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
12 changes: 6 additions & 6 deletions src/app/DefaultAttributePersistenceProvider.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
Expand All @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion src/app/DefaultAttributePersistenceProvider.h
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
14 changes: 7 additions & 7 deletions src/app/DeferredAttributePersistenceProvider.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
6 changes: 3 additions & 3 deletions src/app/DeferredAttributePersistenceProvider.h
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
1 change: 1 addition & 0 deletions src/app/tests/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ chip_test_suite("tests") {
test_sources = [
"TestAclEvent.cpp",
"TestAttributePathExpandIterator.cpp",
"TestAttributePersistenceProvider.cpp",
"TestAttributeValueDecoder.cpp",
"TestAttributeValueEncoder.cpp",
"TestBindingTable.cpp",
Expand Down
Loading