Fixing the generation for Attribute Min Max used in endpoint_config.h #1617
Conversation
- Fixing the generation of min and max values to be auto assumed and generated correctly when either min or max are mentioned instead of both min and max
- Also making sure we do not always typecase everything to (uint16_t) but instead typecast the values based on size and sign
- Github: ZAP Incorrect min/max defaults for temperature #1615
There was a problem hiding this comment.
Summary of Changes
Hello @brdandu, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request refines the generation of attribute minimum and maximum values for endpoint_config.h. It introduces intelligent inference for missing min/max boundaries based on the attribute's data type and implements dynamic typecasting to ensure that generated values are correctly represented according to their size and signedness, improving the robustness and accuracy of the generated code.
Highlights
- Dynamic Typecasting for Attribute Values: The generator now dynamically determines the appropriate C type (e.g.,
uint8_t,int32_t) for min, max, and default attribute values. This replaces the previous hardcoded(uint16_t)cast, ensuring type correctness and preventing potential overflows or sign issues in the generatedendpoint_config.h. - Automatic Min/Max Inference: Improved the generation logic to automatically infer missing minimum or maximum attribute values. If only one boundary (min or max) is specified, the other is now calculated based on the attribute's data type (signed/unsigned and its size), providing more robust and complete attribute definitions.
- Refactored Attribute Collection: The
collectAttributesfunction was updated to acceptzclPackageIds. This enhancement allows the function to accurately retrieve the sign and size information for ZCL types, which is critical for the new dynamic typecasting and min/max inference logic.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces valuable improvements for generating attribute configurations. It correctly handles cases where only a min or max is specified by inferring the other bound based on the data type. Additionally, it replaces a hardcoded uint16_t cast with a dynamic type cast that respects the attribute's size and sign, which is a great enhancement for robustness.
I've identified one high-severity issue regarding potential precision loss when calculating default boundaries for large integer types and have provided a suggestion to use BigInt to resolve it. I've also suggested caching the result of types.getSignAndSizeOfZclType to improve performance. Otherwise, the changes are well-implemented and align with the pull request's objectives.
| let min, max | ||
| if (a.min != null) { | ||
| min = a.min | ||
| } else { | ||
| if (type_size_and_sign.isTypeSigned) { | ||
| // Signed min: -2^(typeSize*8-1) | ||
| min = -(2 ** (typeSize * 8 - 1)) | ||
| } else { | ||
| // Unsigned min: 0 | ||
| min = 0 | ||
| } | ||
| } | ||
| if (a.max != null) { | ||
| max = a.max | ||
| } else { | ||
| if (type_size_and_sign.isTypeSigned) { | ||
| // Signed max: 2^(typeSize*8-1) - 1 | ||
| max = 2 ** (typeSize * 8 - 1) - 1 | ||
| } else { | ||
| // Unsigned max: 2^(typeSize*8) - 1 | ||
| max = 2 ** (typeSize * 8) - 1 | ||
| } | ||
| } |
There was a problem hiding this comment.
The calculation of default min and max values using the ** operator on standard JavaScript numbers can lead to a loss of precision for integer types larger than 53 bits (e.g., 64-bit integers). This can result in incorrect boundary values being generated.
To ensure correctness for all integer sizes, these calculations should be performed using BigInt.
let min = a.min
let max = a.max
if (min == null) {
if (type_size_and_sign.isTypeSigned) {
// Signed min: -2^(typeSize*8-1)
min = -(2n ** (BigInt(typeSize) * 8n - 1n))
} else {
// Unsigned min: 0
min = 0n
}
}
if (max == null) {
if (type_size_and_sign.isTypeSigned) {
// Signed max: 2^(typeSize*8-1) - 1
max = 2n ** (BigInt(typeSize) * 8n - 1n) - 1n
} else {
// Unsigned max: 2^(typeSize*8) - 1
max = 2n ** (BigInt(typeSize) * 8n) - 1n
}
}adaea79 to
5737fcf
Compare
- Fixing the generation of min and max values to be auto assumed and generated correctly when either min or max are mentioned instead of both min and max
- Also making sure we do not always typecase everything to (uint16_t) but instead typecast the values based on size and sign
- Updating existing tests
- For unit testing removing max from BallastFactorAdjustment attribute and making sure 0xFF is generated for '{ (uint8_t)0xFF, (uint8_t)0x64, (uint8_t)0xFF }, /* BallastFactorAdjustment */'
- Github: ZAP#1615
90153da to
92f159f
Compare
| @@ -627,13 +628,19 @@ function endpoint_attribute_min_max_list(options) { | |||
| .forEach((tok) => { | |||
| switch (tok) { | |||
| case 'def': | |||
| defMinMaxItems.push(`(uint16_t)${defS}`) | |||
| defMinMaxItems.push( | |||
| `(${mm.isTypeSigned ? '' : 'u'}int${mm.typeSize * 8}_t)${defS}` | |||
There was a problem hiding this comment.
I suspect this will completely break things, since the actual type those structs store is in fact uint16_t, no?
There was a problem hiding this comment.
Did not cross check that. Just fixed this based on the typecasting being wrong
| @@ -613,7 +613,8 @@ function endpoint_attribute_min_max_list(options) { | |||
|
|
|||
| if (isNaN(def)) def = 0 | |||
| if (isNaN(min)) min = 0 | |||
| if (isNaN(max)) max = 0xffff | |||
| if (isNaN(max)) max = '0x' + 'FF'.repeat(mm.typeSize) | |||
There was a problem hiding this comment.
This is not the correct max for a signed type.
There was a problem hiding this comment.
| @@ -613,7 +613,8 @@ function endpoint_attribute_min_max_list(options) { | |||
|
|
|||
| if (isNaN(def)) def = 0 | |||
| if (isNaN(min)) min = 0 | |||
There was a problem hiding this comment.
This is not the correct min for a signed type. Should at least have a comment explaining why this is here in this form and why it's OK.
If the expectation is that this case never happens, shouldn't this error out instead of silently doing something wrong?
There was a problem hiding this comment.