Use custom ember-buffer decode on codegen data model Read as well. #36229
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
|
Review changes with SemanticDiff. |
|
PR #36229: Size comparison from 515bc3b to 91d5458 Full report (68 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
tcarmelveilleux
approved these changes
Oct 24, 2024
tcarmelveilleux
changed the title
woody-apple
approved these changes
Oct 24, 2024
Comment on lines
+387
to
+394
| bool isSigned = (mAttributeType == ZCL_INT8S_ATTRIBUTE_TYPE) // | ||
| || (mAttributeType == ZCL_INT16S_ATTRIBUTE_TYPE) // | ||
| || (mAttributeType == ZCL_INT24S_ATTRIBUTE_TYPE) // | ||
| || (mAttributeType == ZCL_INT32S_ATTRIBUTE_TYPE) // | ||
| || (mAttributeType == ZCL_INT40S_ATTRIBUTE_TYPE) // | ||
| || (mAttributeType == ZCL_INT48S_ATTRIBUTE_TYPE) // | ||
| || (mAttributeType == ZCL_INT56S_ATTRIBUTE_TYPE) // | ||
| || (mAttributeType == ZCL_INT64S_ATTRIBUTE_TYPE); |
There was a problem hiding this comment.
Is there a reason we are reinventing chip::app::IsSignedAttributeType?
There was a problem hiding this comment.
probably me not knowing all the functions we have ... will update it.
|
|
||
| if (mIsNullable && (value.uint_value == nullValue)) | ||
| { | ||
| // MaxValue is used for NULL setting |
There was a problem hiding this comment.
This comment does not make sense for signed types.
There was a problem hiding this comment.
I will re-write it. It does rely that we have a union and we store the null value as a uint64_t ... so we are looking at a static u64 cast on both sides here ...
Comment on lines
+456
to
+457
| case ZCL_INT8U_ATTRIBUTE_TYPE: // Unsigned 8-bit integer | ||
| return writer.Put(tag, static_cast<uint8_t>(value.uint_value)); |
There was a problem hiding this comment.
Why are we doing this? Assuming the value is in range at all, we can just Put() as a 64-bit signed/unsigned respectively depending on isSigned and don't need this whole big switch and the casting.
Comment on lines
+499
to
+500
| case 0xFF: | ||
| return writer.PutNull(tag); |
There was a problem hiding this comment.
This will incorrectly encode null even if !mIsNullable, instead of correctly erroring out.
| } | ||
| if (NumericAttributeTraits<float>::IsNullValue(value.value)) | ||
| { | ||
| return writer.PutNull(tag); |
There was a problem hiding this comment.
Again, this needs to check mIsNullable!
| } | ||
| if (NumericAttributeTraits<double>::IsNullValue(value.value)) | ||
| { | ||
| return writer.PutNull(tag); |
There was a problem hiding this comment.
Needs to check mIsNullable.
| @@ -73,6 +85,10 @@ class EmberAttributeDataBuffer | |||
| /// Takes into account internal mIsNullable. | |||
| CHIP_ERROR DecodeSignedInteger(chip::TLV::TLVReader & reader, EndianWriter & writer); | |||
|
|
|||
| /// Encodes the UNSIGNED integer into `writer`. | |||
There was a problem hiding this comment.
The code is not just for unsigned. What is this comment talking about?
yyzhong-g
pushed a commit
to yyzhong-g/connectedhomeip
that referenced
this pull request
Dec 12, 2024
…roject-chip#36229) * Use EmberAttributeDataBuffer for codegen provider _Read * Fix comments * Restyled by clang-format --------- Co-authored-by: Andrei Litvin <[email protected]> Co-authored-by: Restyled.io <[email protected]>
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 saves about 1KB of flash compared to the alternative (more template-heavy) approach.