[Draft] Adding Readable Flag to allow Write-only attributes #1619
Conversation
There was a problem hiding this comment.
Summary of Changes
Hello @Alami-Amine, 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 introduces a fundamental change to how ZCL attributes are defined and handled by adding a 'readable' flag. This enables the system to properly distinguish between readable and write-only attributes, which is crucial for accurate ZCL specification compliance and device behavior. The changes span the database schema, data access layers, code generation logic, XML parsing, and the test infrastructure to ensure comprehensive support for this new attribute property.
Highlights
- Core Feature: 'Readable' Attribute Flag: I've introduced a new
isReadableflag for ZCL attributes, allowing for the definition of attributes that are write-only (not readable). This expands the flexibility of attribute definitions within the system. - Database Schema and Data Model Updates: To support the new
isReadableflag, I've added anIS_READABLEcolumn to theATTRIBUTEtable in the database schema. Correspondingly, the application's data mapping (db-mapping.js) and various database queries (query-attribute.js,query-config.js,query-endpoint.js,query-loader.js,query-session-zcl.js,query-zcl.js) have been updated to store, retrieve, and expose this new property. - Code Generation and Templating Integration: The
attribute_maskhelper function (helper-zcl.js) now accepts thereadableparameter and generates anATTRIBUTE_MASK_READABLEflag in the output. This change propagates to the ZAP templates (zap-config-version-2.zapt,zap-config-version-3.zapt,zap-config.zapt) to ensure the generated C/C++ code correctly reflects the readability of attributes. Matter-specific helpers (ClustersHelper.js) also now exposeisReadableAttribute. - XML Loader Enhancements: The ZCL attribute loaders for Dotdot and Silabs XML definitions (
zcl-loader-dotdot.js,zcl-loader-silabs.js) have been updated to parse and incorporate thereadableattribute from XML input, ensuring that attribute definitions from external sources correctly populate this new flag. - Test Suite Updates: I've updated numerous test cases across the codebase (
custom-xml-device-type.test.js,endpoint-config.test.js,gen-matter-3-1.test.js,gen-zigbee-3.test.js,gen-zigbee-4.test.js,gen-zigbee-5.test.js,helpers.test.js,multi-protocol.test.js,startup.test.js) to reflect the inclusion of theATTRIBUTE_MASK_READABLEorZAP_ATTRIBUTE_MASK(READABLE)in the generated output, verifying the correct behavior of the new feature.
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 a readable flag for attributes to support write-only attributes. The changes are comprehensive, touching the database schema, queries, data loaders, and templates. My review has highlighted several TODO comments within the code that need to be addressed to clarify intent and confirm correctness. Additionally, there's a minor code duplication that could be refactored for better maintainability and an outdated schema diagram that needs to be updated. Overall, the approach is solid, and addressing these points will help finalize the PR.
| isReadable: dbApi.fromDbBool(x.IS_READABLE), | ||
| isReadableAttribute: dbApi.fromDbBool(x.IS_READABLE), |
There was a problem hiding this comment.
The properties isReadable and isReadableAttribute are redundant as they are both mapped from x.IS_READABLE. This mirrors an existing redundancy with isWritable and isWritableAttribute (seen also on lines 702-703).
To improve maintainability and reduce confusion, consider consolidating these into single properties (e.g., just isReadable and isWritable) and updating consumers to use this single source of truth.
| //TODO: is this correct? | ||
| if (a.isReadable) mask.push('readable') | ||
| if (a.isNullable) mask.push('nullable') | ||
| if (a.mustUseTimedWrite) mask.push('must_use_timed_write') | ||
| let zap_type = 'UNKNOWN ATTRIBUTE TYPE' | ||
| if (a.typeInfo.atomicType) { | ||
| zap_type = a.typeInfo.atomicType | ||
| //TODO: is this an issue since struct items should always be readable? and thus do not need isReadable | ||
| } else if (a.typeInfo.type == dbEnum.zclType.struct) { |
There was a problem hiding this comment.
There are a couple of TODO comments here that should be addressed before this PR is merged.
- On line 1099: Please confirm if this logic is correct and remove the
TODO. - On line 1106: This is a good question. Typically, the readability of struct fields is governed by the readability of the parent attribute that contains the struct. If an attribute is readable, all its fields are. If you want to make individual struct fields conditionally readable, that would be a more complex feature. For now, assuming struct items are readable if the containing attribute is readable seems correct. Please clarify the intent and remove the
TODO.
src-electron/generator/helper-zcl.js
Outdated
| @@ -2452,8 +2453,10 @@ | |||
| * @param postfixString | |||
| * @returns attribute mask based on given values | |||
| */ | |||
| //TODO: is this change ok? shold I change mask anywhere else (where is it used?) | |||
There was a problem hiding this comment.
This TODO comment indicates some uncertainty about the change. Please verify that all call sites of attribute_mask are correctly handled with the new readable parameter and then remove this comment.
| @@ -164,11 +164,12 @@ test( | |||
| ) | |||
|
|
|||
| // Testing GENERATED ATTRIBUTES to see that they are refering to the correct generation defaults | |||
| //TODO ask reviewers: it looks like the gp link key attribute in Green Power should be Readable by default (?), and it seems that there are no Zigbee Attributes that are WriteOnly (?) | |||
There was a problem hiding this comment.
Regarding the TODO here:
The goal of this PR is to allow for write-only attributes. The current implementation in the ZCL loaders defaults isReadable to true unless readable="false" is explicitly set in the XML. This is a sound approach.
For the gp link key attribute, if it should be write-only, then its definition in the source ZCL file should be updated to readable="false". The test change to add ATTRIBUTE_MASK_READABLE is consistent with the new default behavior.
Please confirm this aligns with the requirements for the Green Power cluster and remove the TODO.
| @@ -3091,6 +3091,7 @@ digraph "SchemaCrawler_Diagram" { | |||
| <td align='left'> </td> | |||
| <td port='is_writable_74b410f9.end' align='left'>INTEGER</td> | |||
| </tr> | |||
| /* TODO: Add equivalent of IS_READABLE ? */ | |||
There was a problem hiding this comment.
This TODO comment points out that the schema diagram is missing the IS_READABLE field. Please update this diagram to reflect the changes in zap-schema.sql to keep the documentation consistent with the implementation.
No description provided.