Adding IsReadable Flag to be able to configure Write-only attributes #1619

Alami-Amine · 2025-07-04T17:03:24Z

Problem

In Matter SDK, we do not support write-only attributes as mandated by Spec.
the generated Metadata.h is not able to put the correct Access Privileges for a Write-only Attribute (should be std::nullopt)
Example here for the WriteOnlyInt8u Attribute in Unit Testing Cluster

Solution

The Main goal of my Changes is:

being able to add readable="false" to the ZAPXML for a Cluster, such as test-cluster.xml in this PR
having a functioning IsReadableAttribute flag in order to add a writeonly Attribute Qualifier in the ZAP Template idl/attribute_definition.zapt for it to added to generated .matter files

gemini-code-assist

Summary of Changes

Hello @Alami-Amine, I'm Gemini Code Assist¹! 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 isReadable flag 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 isReadable flag, I've added an IS_READABLE column to the ATTRIBUTE table 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_mask helper function (helper-zcl.js) now accepts the readable parameter and generates an ATTRIBUTE_MASK_READABLE flag 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 expose isReadableAttribute.
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 the readable attribute 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 the ATTRIBUTE_MASK_READABLE or ZAP_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.

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. ↩

gemini-code-assist

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.

gemini-code-assist · 2025-07-04T17:05:25Z

src-electron/db/db-mapping.js

+      isReadable: dbApi.fromDbBool(x.IS_READABLE),
+      isReadableAttribute: dbApi.fromDbBool(x.IS_READABLE),


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.

src-electron/generator/helper-endpointconfig.js

src-electron/generator/helper-zcl.js

test/gen-zigbee-4.test.js

zap-schema.dot

…tter" This reverts commit 4f81b1c.

brdandu · 2025-07-08T14:24:42Z

src-electron/generator/helper-zcl.js

@@ -2462,6 +2463,7 @@ async function attribute_mask(
  isSingleton,
  prefixString,
  postfixString,
+  readable,


Could we add a comment // readable only used by Matter and not Zigbee

brdandu · 2025-07-08T14:27:22Z

test/custom-xml-device-type.test.js

@@ -195,7 +195,7 @@ test('Generation with custom xml with Device Type', async () => {

  let zapConfigVer2 = genResult.content['zap-config-version-2.h']
  expect(zapConfigVer2).toContain(
-    '{ 0x0302, ZCL_INT8U_ATTRIBUTE_TYPE, 1, (ATTRIBUTE_MASK_MANUFACTURER_SPECIFIC| ATTRIBUTE_MASK_CLIENT), { (uint8_t*)0  } }, /* 2 Cluster: Custom Cluster, Attribute: A9, Side: client*/ \\'
+    '{ 0x0302, ZCL_INT8U_ATTRIBUTE_TYPE, 1, (ATTRIBUTE_MASK_MANUFACTURER_SPECIFIC| ATTRIBUTE_MASK_CLIENT| ATTRIBUTE_MASK_READABLE), { (uint8_t*)0  } }, /* 2 Cluster: Custom Cluster, Attribute: A9, Side: client*/ \\'


Why is this changing? Is there a corresponding xml change for this?

brdandu · 2025-07-08T14:28:06Z

test/custom-xml-device-type.test.js

@@ -278,7 +278,7 @@ test(`Load .zap file with custom xml with Device Type`, async () => {

  let endpointConfig = genResult.content['endpoint-config.c']
  expect(endpointConfig).toContain(
-    '{ 0x00000000, ZAP_TYPE(BOOLEAN), 1, 0, ZAP_SIMPLE_DEFAULT(0) }, /* OnOff */  \\'
+    '{ 0x00000000, ZAP_TYPE(BOOLEAN), 1, ZAP_ATTRIBUTE_MASK(READABLE), ZAP_SIMPLE_DEFAULT(0) }, /* OnOff */  \\'


Why is this changing? Is there a corresponding xml change for this?

brdandu · 2025-07-08T14:28:18Z

test/endpoint-config.test.js

@@ -208,7 +208,7 @@ test(
      '{ ZAP_REPORT_DIRECTION(REPORTED), 0x0029, 0x00000101, 0x00000000, ZAP_CLUSTER_MASK(SERVER), 0x0000, {{ 0, 65534, 0 }} }, /* lock state */'
    )
    expect(epc).toContain(
-      '{ 0x00000004, ZAP_TYPE(CHAR_STRING), 33, ZAP_ATTRIBUTE_MASK(TOKENIZE), ZAP_LONG_DEFAULTS_INDEX(0) }'
+      '{ 0x00000004, ZAP_TYPE(CHAR_STRING), 33, ZAP_ATTRIBUTE_MASK(TOKENIZE) | ZAP_ATTRIBUTE_MASK(READABLE), ZAP_LONG_DEFAULTS_INDEX(0) }'


Why is this changing? Is there a corresponding xml change for this?

brdandu · 2025-07-08T14:28:31Z

test/gen-matter-3-1.test.js

@@ -123,7 +123,7 @@ test(
  { ZAP_CLUSTER_INDEX(80), 1, 0 }, \\
 }`)
    expect(ept).toContain(
-      `{ 0x00000005, ZAP_TYPE(ENUM8), 1, ZAP_ATTRIBUTE_MASK(EXTERNAL_STORAGE) | ZAP_ATTRIBUTE_MASK(NULLABLE), ZAP_EMPTY_DEFAULT() }, /* LastNetworkingStatus */`
+      `{ 0x00000005, ZAP_TYPE(ENUM8), 1, ZAP_ATTRIBUTE_MASK(EXTERNAL_STORAGE) | ZAP_ATTRIBUTE_MASK(READABLE) | ZAP_ATTRIBUTE_MASK(NULLABLE), ZAP_EMPTY_DEFAULT() }, /* LastNetworkingStatus */`


Why is this changing? Is there a corresponding xml change for this?

brdandu · 2025-07-08T14:32:48Z

test/gen-zigbee-4.test.js

    expect(cfgVer2).toContain(
-      `0x0022, ZCL_SECURITY_KEY_ATTRIBUTE_TYPE, 16, (ATTRIBUTE_MASK_WRITABLE| ATTRIBUTE_MASK_CLIENT), { (uint8_t*)&(generatedDefaults[28]) } }, /* 25 Cluster: Green Power, Attribute: gp link key, Side: client*/`
+      `0x0022, ZCL_SECURITY_KEY_ATTRIBUTE_TYPE, 16, (ATTRIBUTE_MASK_WRITABLE| ATTRIBUTE_MASK_CLIENT| ATTRIBUTE_MASK_READABLE), { (uint8_t*)&(generatedDefaults[28]) } }, /* 25 Cluster: Green Power, Attribute: gp link key, Side: client*/`


Revert to old test

brdandu · 2025-07-08T14:33:06Z

test/gen-zigbee-5.test.js

@@ -133,11 +133,11 @@ test(

    // Test GENERATED_ATTRIBUTES for the same attribute name but different attribute code
    expect(cfgVer3).toContain(
-      '{ 0x0000, ZCL_INT16U_ATTRIBUTE_TYPE, 2, (ATTRIBUTE_MASK_WRITABLE| ATTRIBUTE_MASK_MANUFACTURER_SPECIFIC), { (uint8_t*)0x0000  } }, /* 51 Cluster: Sample Mfg Specific Cluster 2, Attribute: ember sample attribute 2, Side: server*/'


Same as above

brdandu · 2025-07-08T14:33:46Z

test/multi-protocol.test.js

@@ -148,13 +148,13 @@ test(

    // Global attribute test
    expect(zigbeeEndpointConfigGen).toContain(
-      '{ 0xFFFD, ZCL_INT16U_ATTRIBUTE_TYPE, 2, (ATTRIBUTE_MASK_SINGLETON), { (uint8_t*)3  } }, /* 13 Cluster: Basic, Attribute: cluster revision, Side: server*/'
+      '{ 0xFFFD, ZCL_INT16U_ATTRIBUTE_TYPE, 2, (ATTRIBUTE_MASK_SINGLETON| ATTRIBUTE_MASK_READABLE), { (uint8_t*)3  } }, /* 13 Cluster: Basic, Attribute: cluster revision, Side: server*/'


Same as above. Revert

brdandu · 2025-07-08T14:34:01Z

test/startup.test.js

@@ -346,10 +346,10 @@ test(
    )
    // Check for the specific string in the generated content
    expect(genResultMatter.content['endpoint-config.c']).not.toContain(
-      `{ 0x0000FFFD, ZAP_TYPE(INT16U), 2, 0, ZAP_SIMPLE_DEFAULT(1) }`
+      `{ 0x0000FFFD, ZAP_TYPE(INT16U), 2, ZAP_ATTRIBUTE_MASK(READABLE), ZAP_SIMPLE_DEFAULT(1) }`


Same as above. revert

brdandu · 2025-07-08T14:34:58Z

zcl-builtin/matter/data-model/chip/test-cluster.xml

@@ -236,7 +236,7 @@ limitations under the License.
    <attribute side="server" code="0x4028" define="NULLABLE_RANGE_RESTRICTED_INT16U" type="int16u" min="100" max="1000" writable="true" isNullable="true" optional="false" default="200">nullable_range_restricted_int16u</attribute>
    <attribute side="server" code="0x4029" define="NULLABLE_RANGE_RESTRICTED_INT16S" type="int16s" min="-150" max="200" writable="true" isNullable="true" optional="false" default="-5">nullable_range_restricted_int16s</attribute>

-    <attribute side="server" code="0x402A" define="WRITE_ONLY_INT8U" type="INT8U" writable="true" default="0" optional="true">write_only_int8u</attribute>
+    <attribute side="server" code="0x402A" define="WRITE_ONLY_INT8U" type="INT8U" writable="true" readable="false" default="0" optional="true">write_only_int8u</attribute>


If this is leading to any existing tests change then better to add a new attribute and make readable= false for that so it is easier to just test the change corresponding to your changes.

Alami-Amine · 2025-07-07T11:55:25Z

src-electron/generator/helper-endpointconfig.js

@@ -1096,6 +1096,7 @@ async function collectAttributes(db, sessionId, endpointTypes, options) {
        }
        if (a.isSingleton) mask.push('singleton')
        if (a.isWritable) mask.push('writable')
+        if (a.isReadable) mask.push('readable')


Suggested change

if (a.isReadable) mask.push('readable')

Question for Reviewers

would it be sane/logical to do this change to NOT have a ZAP_ATTRIBUTE_MASK(READABLE) created? this will make the changes less "invasive" and the whole point of my changes is to have a isReadableAttribute that can be used in Zap Templates to add a writeonly (as can be seen in idl/attribute_definition.zapt) ?

Adding Readable Flag and Attribute Mask

ee8543a

gemini-code-assist bot reviewed Jul 4, 2025

View reviewed changes

Alami-Amine added 4 commits July 4, 2025 19:22

removing TODO

2285184

adding readable=false to writeOnly attribute in unit testing ZAPXML

58f5862

Removing Attribute Mask changes that impact Zigbee and NOT Matter

4f81b1c

Revert "Removing Attribute Mask changes that impact Zigbee and NOT Ma…

e1b4b89

…tter" This reverts commit 4f81b1c.

Alami-Amine force-pushed the AA/AddingReadableFlag branch from 81a1b78 to dc668a8 Compare July 7, 2025 14:49

moving readable argument to end of attribute_mask function

04be82f

Alami-Amine force-pushed the AA/AddingReadableFlag branch from dc668a8 to 04be82f Compare July 7, 2025 15:26

Fixing Unit Test

acff128

Alami-Amine marked this pull request as ready for review July 7, 2025 16:30

Alami-Amine requested review from paulr34, tecimovic and brdandu as code owners July 7, 2025 16:30

brdandu requested review from andy31415 and bzbarsky-apple July 7, 2025 16:40

Adding schema

29762e6

Alami-Amine changed the title ~~[Draft] Adding Readable Flag to allow Write-only attributes~~ Adding IsReadable Flag to be able to configure Write-only attributes Jul 7, 2025

brdandu requested changes Jul 8, 2025

View reviewed changes

Alami-Amine commented Jul 8, 2025

View reviewed changes

Removing Attribute_mask changes that impact Zigbee Only

8a6bb3e

		isReadable: dbApi.fromDbBool(x.IS_READABLE),
		isReadableAttribute: dbApi.fromDbBool(x.IS_READABLE),

Adding IsReadable Flag to be able to configure Write-only attributes #1619

Are you sure you want to change the base?

Adding IsReadable Flag to be able to configure Write-only attributes #1619

Uh oh!

Conversation

Alami-Amine commented Jul 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Solution

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Jul 4, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Alami-Amine commented Jul 4, 2025 •

edited

Loading