Handle nullable attributes in TLV-to-attr-store interactions and Accessors #11665
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
bzbarsky-apple
force-pushed
the
nullable-attr-store
branch
from
ac107d4 to
81ca596
Compare
pullapprove
bot
requested review from
anush-apple,
austinh0,
balducci-apple,
carol-apple,
chrisdecenzo,
chulspro,
Damian-Nordic,
electrocucaracha,
emargolis,
erjiaqing,
franck-apple,
harimau-qirex,
hawk248,
holbrookt,
jelderton,
jepenven-silabs,
jmartinez-silabs,
kghost,
kpschoedel,
LuDuda,
lzgrablic02,
mlepage-google,
msandstedt and
pan-apple
bzbarsky-apple
force-pushed
the
nullable-attr-store
branch
from
2ccbff6 to
08a144f
Compare
|
PR #11665: Size comparison from 4374a54 to 08a144f Increases above 0.2%:
Increases (31 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
Decreases (3 builds for mbed, nrfconnect, p6)
Full report (38 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
bzbarsky-apple
force-pushed
the
nullable-attr-store
branch
from
08a144f to
67c2697
Compare
|
PR #11665: Size comparison from 4ae5f21 to 67c2697 Increases above 0.2%:
Increases (6 builds for k32w, qpg, telink)
Full report (7 builds for k32w, qpg, telink)
|
bzbarsky-apple
force-pushed
the
nullable-attr-store
branch
from
67c2697 to
7d06f77
Compare
|
PR #11665: Size comparison from 14eaa07 to 7d06f77 Increases above 0.2%:
Increases (31 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
Decreases (3 builds for mbed, nrfconnect, p6)
Full report (38 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
bzbarsky-apple
force-pushed
the
nullable-attr-store
branch
from
7d06f77 to
1ac8d2e
Compare
|
PR #11665: Size comparison from 2d0eb94 to 1ac8d2e Increases above 0.2%:
Increases (31 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
Decreases (3 builds for mbed, nrfconnect, p6)
Full report (38 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
andy31415
reviewed
Nov 12, 2021
andy31415
reviewed
Nov 12, 2021
andy31415
reviewed
Nov 12, 2021
bzbarsky-apple
force-pushed
the
nullable-attr-store
branch
from
fbbc35d to
a80407c
Compare
andy31415
approved these changes
Nov 12, 2021
|
PR #11665: Size comparison from 5d4bade to a80407c Increases above 0.2%:
Increases (31 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
Decreases (3 builds for mbed, nrfconnect, p6)
Full report (38 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
|
@msandstedt @saurabhst @jepenven-silabs @jmartinez-silabs @LuDuda @Damian-Nordic Please take a look? |
jmartinez-silabs
approved these changes
Nov 12, 2021
…tten. This implements the following semantics, for attributes we store in the attribute store (integers, booleans, strings, octet strings): Writes: * An attempt to write TLV null for non-nullable attributes leads to an error. * Writing TLV null to a nullable string or octet string sets its length to 0xFFFF or 0xFF depending on whether it's a long string or not. * Writing TLV null to a nullable integer or boolean uses the "NaU" or "NaS" ZCL representation to represent it. * Writing a TLV integer to an integer attribute makes a CanRepresentValue check and returns an error if it returns false. For now CanRepresentValue only disallows the NaU/NaS value for nullable attributes. Reads: * When reading a nullable integer or boolean, NaU/NaS is converted to TLV null. * When reading an integer or boolean all other cases go through a CanRepresentValue check (in case the value in the attr store is not actually valid) and return error if it returns false. * When reading a nullable string or octet string, length 0xFFFF/0xFF is converted to TLV null. * When reading a non-nullable string or octet string, length 0xFFFF/0xFF leads to an error.
For setters, the new behavior is: * For non-string attributes when setting an integer or boolean value, do a CanRepresentValue check and fail if that fails. For string attributes, ensure that strings with 0xFF or 0xFFFF as length cannot be passed in in practice. * For nullable attributes add a SetNull method. * For nullable attributes add a Set() method taking a Nullable<T> and calling either SetNull or the setter that expects a non-null value. For getters, the new behavior is: * When reading a nullable integer or boolean, NaU/NaS is converted to a null value stored in the Nullable. * When reading a non-nullable integer or boolean, return error if CanRepresentValue returns false on the value from the attr store. * When reading a nullable string or octet string, length 0xFFFF/0xFF is converted to a null value stored in the Nullable. * When reading a non-nullable string or octet string, length 0xFFFF/0xFF leads to an error. The XML changes are for two problems the new asserts caught: we had strings that were short-typed, but had sizes that would not fit in a single byte length.
bzbarsky-apple
force-pushed
the
nullable-attr-store
branch
from
a80407c to
41b50d4
Compare
|
PR #11665: Size comparison from ea10a5d to 41b50d4 Increases above 0.2%:
Increases (31 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
Decreases (3 builds for mbed, nrfconnect, p6)
Full report (38 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
PSONALl
pushed a commit
to PSONALl/connectedhomeip
that referenced
this pull request
Dec 3, 2021
…ssors (project-chip#11665) * Add a way to ask EmberAfAttributeMetadata whether the attribute is nullable. * Convert ZCL null values to/from TLV null when attributes are read/written. This implements the following semantics, for attributes we store in the attribute store (integers, booleans, strings, octet strings): Writes: * An attempt to write TLV null for non-nullable attributes leads to an error. * Writing TLV null to a nullable string or octet string sets its length to 0xFFFF or 0xFF depending on whether it's a long string or not. * Writing TLV null to a nullable integer or boolean uses the "NaU" or "NaS" ZCL representation to represent it. * Writing a TLV integer to an integer attribute makes a CanRepresentValue check and returns an error if it returns false. For now CanRepresentValue only disallows the NaU/NaS value for nullable attributes. Reads: * When reading a nullable integer or boolean, NaU/NaS is converted to TLV null. * When reading an integer or boolean all other cases go through a CanRepresentValue check (in case the value in the attr store is not actually valid) and return error if it returns false. * When reading a nullable string or octet string, length 0xFFFF/0xFF is converted to TLV null. * When reading a non-nullable string or octet string, length 0xFFFF/0xFF leads to an error. * Fix Accessors to handle nullable attributes. For setters, the new behavior is: * For non-string attributes when setting an integer or boolean value, do a CanRepresentValue check and fail if that fails. For string attributes, ensure that strings with 0xFF or 0xFFFF as length cannot be passed in in practice. * For nullable attributes add a SetNull method. * For nullable attributes add a Set() method taking a Nullable<T> and calling either SetNull or the setter that expects a non-null value. For getters, the new behavior is: * When reading a nullable integer or boolean, NaU/NaS is converted to a null value stored in the Nullable. * When reading a non-nullable integer or boolean, return error if CanRepresentValue returns false on the value from the attr store. * When reading a nullable string or octet string, length 0xFFFF/0xFF is converted to a null value stored in the Nullable. * When reading a non-nullable string or octet string, length 0xFFFF/0xFF leads to an error. The XML changes are for two problems the new asserts caught: we had strings that were short-typed, but had sizes that would not fit in a single byte length. * Add tests for nullable attributes. * Fix the Darwin tests by skipping the nullable read for now * Address review comments
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.
Reviewer note: Reviewing the commits separately may be a good idea. The two commits with substantive changes have long commit messages explaining them.
Problem
Nullable attributes not getting encoded as null on the wire when read and not allowing that encoding when written.
Change overview
Implement null handling for TLV and Accessors getters/setters.
Testing
New unit tests added.