Expand attribute persistence provider api #27611
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…e to take the required attribute information directly rather than as a EmberAfAttributeMetadata structure.
…ing of uint8 and nullable uint8
… work for all uint types.
pullapprove
bot
requested review from
amitnj,
anush-apple,
arkq,
bzbarsky-apple,
carol-apple,
cecille,
chrisdecenzo,
chshu,
chulspro,
cliffamzn,
CodeChronos928,
Damian-Nordic,
dhrishi,
electrocucaracha,
emargolis,
gjc13,
harimau-qirex,
harsha-rajendran,
hawk248,
jelderton and
jepenven-silabs
|
PR #27611: Size comparison from ffc026f to 333ac54 Increases (47 builds for bl602, bl702, bl702l, cc32xx, efr32, esp32, k32w, linux, psoc6, qpg, telink)
Decreases (13 builds for bl702, bl702l, cc32xx, cyw30739, esp32, psoc6)
Full report (55 builds for bl602, bl702, bl702l, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, psoc6, qpg, telink)
|
tcarmelveilleux
approved these changes
Jul 7, 2023
…ts in some tests.
…unction for nullable types to avoid the error: The left operand of '==' is a garbage value, on some platforms.
|
PR #27611: Size comparison from 53e06c7 to 582b92c Increases (51 builds for bl602, bl702, bl702l, cc32xx, efr32, esp32, k32w, linux, nrfconnect, psoc6, qpg, telink)
Decreases (12 builds for bl702, bl702l, cc32xx, cyw30739, psoc6)
Full report (58 builds for bl602, bl702, bl702l, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
|
PR #27611: Size comparison from 53e06c7 to 81d062f Increases (3 builds for cc32xx, qpg)
Decreases (1 build for cc32xx)
Full report (4 builds for cc32xx, mbed, qpg)
|
mhazley
approved these changes
Jul 7, 2023
|
PR #27611: Size comparison from 19deed2 to 7f2423f Increases (1 build for cc32xx)
Decreases (1 build for cc32xx)
Full report (2 builds for cc32xx, mbed)
|
|
PR #27611: Size comparison from 19deed2 to e289e60 Increases (49 builds for bl702, bl702l, cc32xx, efr32, esp32, k32w, linux, nrfconnect, psoc6, qpg, telink)
Decreases (13 builds for bl602, bl702, bl702l, cc32xx, cyw30739, psoc6)
Full report (58 builds for bl602, bl702, bl702l, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
| * | ||
| * 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, |
There was a problem hiding this comment.
This is an API change to a public SDK API that will break consumers. Why is this ok?
| * @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() |
There was a problem hiding this comment.
Why is this reinventing various machinery from src/app/util/attribute-storage-null-handling.h?
| * @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> |
There was a problem hiding this comment.
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....)
There was a problem hiding this comment.
The std::is_integral will return true if T is a signed or unsigned integer. According to this documentation at least.
There was a problem hiding this comment.
I know it will; my point is that the actual code in there ends up assuming the value is unsigned, right?
There was a problem hiding this comment.
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.
| /** | ||
| * Read an attribute of type intX, uintX or bool from non-volatile memory. | ||
| * | ||
| * @param [in] aPath the attribute path for the data being persisted. |
There was a problem hiding this comment.
The data is not being persisted....
| { | ||
| 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 |
There was a problem hiding this comment.
No, ReadValue is implemented by the application in general. You can't make assumptions about how it will use the data it's handed.
| return err; | ||
| } | ||
|
|
||
| chip::Encoding::LittleEndian::Reader r(tempVal.data(), tempVal.size()); |
There was a problem hiding this comment.
Why the extra chip::? But again, the data here is native-endian.
| CHIP_ERROR WriteScalarValue(const ConcreteAttributePath & aPath, T & aValue) | ||
| { | ||
| uint8_t value[sizeof(T)]; | ||
| auto w = Encoding::LittleEndian::BufferWriter(value, sizeof(T)); |
There was a problem hiding this comment.
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR expands the
AttributePersistenceProviderAPI to provide simple to use methods to store and retrieve attributes from the KVS. The expansion allows for the storage and retrieval of all unsigned types and their nullable varieties as well as the storage and retrieval of boolean values.