Adding to zcl_attributes_server and enabled_attributes_for_cluster_and_side helpers to replace chip_server_cluster_attributes #899

brdandu · 2023-01-19T18:05:58Z

chip_server_cluster_attributes is returning server attributes which are not actually enabled because the server side cluster is disabled.
The reason chip_server_cluster_attributes is picking the server side attributes which are not enabled is because those are enabled in the .zap file(Which is the case for saving user selections and ease of use). However the helper is not actually checking if the cluster is enabled as well.
all_user_cluster_attributes_irrespective_of_manufacturing_specification is solving this issue correctly and actually only showing attributes which are truly enabled.
Github: ZAP#898

bzbarsky-apple

I'm a little confused by what this is trying to address.

There are two ways to use chip_server_cluster_attributes.

If includeAll is true, then it just returns all the attributes that are defined in the relevant cluster. Whether that cluster is enabled or not might affect whether chip_server_cluster_attributes is reached at all.

If includeAll is false, then it returns all the attributes that are enabled in the relevant cluster. Again, whether that cluster is enabled or not affects whether it's reached, but in practice in this mode chip_server_cluster_attributes should not be getting invoked for disabled clusters at all.

How is any of this related to the helpers being changed here?

src-electron/db/query-attribute.js

src-electron/generator/helper-session.js

brdandu · 2023-01-19T20:40:15Z

I'm a little confused by what this is trying to address.

There are two ways to use chip_server_cluster_attributes.

If includeAll is true, then it just returns all the attributes that are defined in the relevant cluster. Whether that cluster is enabled or not might affect whether chip_server_cluster_attributes is reached at all.

If includeAll is false, then it returns all the attributes that are enabled in the relevant cluster. Again, whether that cluster is enabled or not affects whether it's reached, but in practice in this mode chip_server_cluster_attributes should not be getting invoked for disabled clusters at all.

How is any of this related to the helpers being changed here?

I switched {{#chip_server_cluster_attributes}} in MatterIDL.zapt to {{#chip_server_cluster_attributes includeAll=false}} but that is not removing all the server attributes which should not be generated because the corresponding server cluster is disabled.

bzbarsky-apple · 2023-01-19T21:30:59Z

I switched {{#chip_server_cluster_attributes}} in MatterIDL.zapt to {{#chip_server_cluster_attributes includeAll=false}}

The includeAll=false needs to be on the chip_server_clusters, no? And that's the default behavior, by the way....

The real problem, it seems to me, is that chip_server_cluster_attributes is supposed to be used inside chip_server_clusters, which only iterates the enabled ones unless includeAll=true. The documentation for the function even says that.

But MatterIDL.zapt uses it inside all_user_clusters for some reason, which iterates the disabled ones?

brdandu · 2023-01-20T15:39:50Z

chip_server_clusters

I switched {{#chip_server_cluster_attributes}} in MatterIDL.zapt to {{#chip_server_cluster_attributes includeAll=false}}

The includeAll=false needs to be on the chip_server_clusters, no? And that's the default behavior, by the way....

The real problem, it seems to me, is that chip_server_cluster_attributes is supposed to be used inside chip_server_clusters, which only iterates the enabled ones unless includeAll=true. The documentation for the function even says that.

But MatterIDL.zapt uses it inside all_user_clusters for some reason, which iterates the disabled ones?

I am looking at ChipCallbackTypes.zapt which uses {{#chip_client_clusters}} and {{#chip_server_cluster_attributes}}. I see Identify server cluster attributes generated in ChipCallbackTypes.h even though the Identify server cluster is disabled. I do not see includeAll=true here(Assuming includeAll is false by default like you mentioned).

bzbarsky-apple · 2023-01-20T21:03:59Z

I am looking at ChipCallbackTypes.zapt which uses {{#chip_client_clusters}} and {{#chip_server_cluster_attributes}}.

That one is a hack which explicitly relies on "we enable the server attributes, but then actually enable the client clusters".

That is, it wants the observed behavior. It should really be using server clusters and includeAll=true, but the owners of that code have sort of abandoned it....

That is, this is codegen for the java "control everything" client. It should not depend on what server clusters are enabled (none of them, in practice) for controllers.

brdandu · 2023-01-23T14:29:04Z

I am looking at ChipCallbackTypes.zapt which uses {{#chip_client_clusters}} and {{#chip_server_cluster_attributes}}.

That one is a hack which explicitly relies on "we enable the server attributes, but then actually enable the client clusters".

That is, it wants the observed behavior. It should really be using server clusters and includeAll=true, but the owners of that code have sort of abandoned it....

That is, this is codegen for the java "control everything" client. It should not depend on what server clusters are enabled (none of them, in practice) for controllers.

To do further analysis here. Which of the following files are not hacks and not abandoned?

   modified:   src/controller/java/zap-generated/CHIPAttributeTLVValueDecoder.cpp
modified:   src/controller/java/zap-generated/CHIPCallbackTypes.h
modified:   src/controller/java/zap-generated/CHIPClustersWrite-JNI.cpp
modified:   src/controller/java/zap-generated/CHIPReadCallbacks.cpp
modified:   src/controller/java/zap-generated/CHIPReadCallbacks.h
modified:   src/controller/java/zap-generated/chip/devicecontroller/ChipClusters.java
modified:   src/controller/java/zap-generated/chip/devicecontroller/ChipIdLookup.java
modified:   src/controller/java/zap-generated/chip/devicecontroller/ClusterInfoMapping.java
modified:   src/controller/java/zap-generated/chip/devicecontroller/ClusterReadMapping.java
modified:   src/controller/java/zap-generated/chip/devicecontroller/ClusterWriteMapping.java
modified:   src/controller/python/chip/clusters/CHIPClusters.py
    modified:   src/darwin/Framework/CHIP/zap-generated/MTRAttributeTLVValueDecoder.mm
modified:   src/darwin/Framework/CHIP/zap-generated/MTRBaseClusters.h
modified:   src/darwin/Framework/CHIP/zap-generated/MTRBaseClusters.mm
modified:   src/darwin/Framework/CHIP/zap-generated/MTRCallbackBridge.h
modified:   src/darwin/Framework/CHIP/zap-generated/MTRCallbackBridge.mm
modified:   src/darwin/Framework/CHIP/zap-generated/MTRClusters.h
modified:   src/darwin/Framework/CHIP/zap-generated/MTRClusters.mm
    modified:   zzz_generated/chip-tool/zap-generated/test/Commands.h
modified:   zzz_generated/darwin-framework-tool/zap-generated/cluster/Commands.h
modified:   zzz_generated/darwin/controller-clusters/zap-generated/CHIPClientCallbacks.h

bzbarsky-apple · 2023-01-23T15:15:46Z

Which of the following files are not hacks and not abandoned?

Files are not hacks. Specific things in a file might be hacks.

The src/controller/java stuff is all more or less abandoned, at the moment, as far as I understand. Maybe new maintainers can be found for it; please check with the Google folks.

The python, darwin and chip-tool bits are not abandoned, but they also have the behavior they want to have right now. So any PR that changes their output needs to be looked at very carefully to see what changed about the output and why.

brdandu · 2023-01-23T20:52:18Z

Which of the following files are not hacks and not abandoned?

Files are not hacks. Specific things in a file might be hacks.

The src/controller/java stuff is all more or less abandoned, at the moment, as far as I understand. Maybe new maintainers can be found for it; please check with the Google folks.

The python, darwin and chip-tool bits are not abandoned, but they also have the behavior they want to have right now. So any PR that changes their output needs to be looked at very carefully to see what changed about the output and why.

@andy31415 would you know anything about the java code. Is that still being used or can it be removed?

andy31415 · 2023-01-23T21:14:38Z

abandoned

I do not believe the java stuff was abandoned. Historically we had to move some of the files to be code generated by the python code generator because a single large file would crash the gcc NDK compiler with internal error.

However converting handlebars to python jinja2 templates is a non-trivial effort, so we converted only what was crashing and not the rest. Which puts us in the awkward state of having 2 code generators even for java itself.

andy31415 · 2023-01-23T21:19:27Z

A lot of controller/java/zap-generated are actually used by our builds currently:

https://github.com/project-chip/connectedhomeip/blob/master/src/controller/java/BUILD.gn#L49

sources = [
...
    "zap-generated/CHIPAttributeTLVValueDecoder.cpp",
    "zap-generated/CHIPClustersWrite-JNI.cpp",
    "zap-generated/CHIPEventTLVValueDecoder.cpp",
    "zap-generated/CHIPInvokeCallbacks.cpp",
    "zap-generated/CHIPInvokeCallbacks.h",
    "zap-generated/CHIPReadCallbacks.cpp",
    "zap-generated/CHIPReadCallbacks.h",
  ]

CHIPCallbackTypes.h seems to not be in the list, however I wonder if that is a bug since it is used in CHIPClustersWrite-JNI.cpp and such.

Java files are also used below:

    "zap-generated/chip/devicecontroller/ChipClusters.java",
  "zap-generated/chip/devicecontroller/ChipEventStructs.java",
  "zap-generated/chip/devicecontroller/ChipIdLookup.java",
  "zap-generated/chip/devicecontroller/ChipStructs.java",
  "zap-generated/chip/devicecontroller/ClusterInfoMapping.java",
  "zap-generated/chip/devicecontroller/ClusterReadMapping.java",
  "zap-generated/chip/devicecontroller/ClusterWriteMapping.java",
]

bzbarsky-apple · 2023-01-24T19:58:33Z

@brdandu To be clear, the Java stuff is used. It's just not particularly actively maintained and hasn't been in a while as far as I can tell.

brdandu · 2023-01-25T16:06:56Z

@brdandu To be clear, the Java stuff is used. It's just not particularly actively maintained and hasn't been in a while as far as I can tell.

ok I will make some changes to the templates here and run them by you.

src-electron/db/db-mapping.js

src-electron/db/query-attribute.js

src-electron/generator/helper-attribute.js

src-electron/generator/helper-session.js

src-electron/generator/helper-zcl.js

codecov-commenter · 2023-03-10T19:46:12Z

Codecov Report

Merging #899 (2bc8864) into master (ac6dd83) will increase coverage by 0.23%.
The diff coverage is 80.62%.

❗ Current head 2bc8864 differs from pull request most recent head f01d94f. Consider uploading reports for the commit f01d94f to get more accurate results

@@            Coverage Diff             @@
##           master     #899      +/-   ##
==========================================
+ Coverage   66.24%   66.48%   +0.23%     
==========================================
  Files         155      156       +1     
  Lines       16708    16952     +244     
  Branches     3602     3681      +79     
==========================================
+ Hits        11068    11270     +202     
- Misses       5640     5682      +42

Impacted Files	Coverage Δ
src-electron/db/query-attribute.js	`64.60% <0.00%> (-0.86%)`	⬇️
src-electron/generator/helper-session.js	`63.65% <33.33%> (-0.28%)`	⬇️
...nerator/matter/controller/java/templates/helper.js	`66.54% <74.62%> (+2.22%)`	⬆️
...rator/matter/controller/python/templates/helper.js	`52.11% <76.66%> (+16.39%)`	⬆️
src-electron/db/db-mapping.js	`98.70% <81.81%> (-0.61%)`	⬇️
src-electron/db/query-bitmap.js	`91.66% <81.81%> (-8.34%)`	⬇️
src-electron/db/query-enum.js	`94.44% <81.81%> (-5.56%)`	⬇️
src-electron/db/query-number.js	`83.33% <81.81%> (-1.29%)`	⬇️
src-electron/db/query-data-type.js	`89.39% <86.66%> (-0.81%)`	⬇️
src-electron/util/zcl-util.js	`73.55% <88.23%> (+0.62%)`	⬆️
... and 6 more

... and 2 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

src-electron/generator/helper-attribute.js

src-electron/generator/matter/app/zap-templates/templates/chip/helper.js

src-electron/generator/helper-zcl.js

tecimovic

This seems like a reasonable step towards replacing stateful helpers with stateless helpers. Work in progress...

docs/api.md

src-electron/db/query-attribute.js

src-electron/db/query-bitmap.js

src-electron/generator/helper-session.js

src-electron/generator/matter/app/zap-templates/templates/chip/helper.js

src-electron/generator/matter/controller/python/templates/helper.js

src-electron/db/query-attribute.js

src-electron/db/query-bitmap.js

src-electron/generator/matter/controller/python/templates/helper.js

…server helper to replace chip_server_cluster_attributes - chip_server_cluster_attributes is returning server attributes which are not actually enabled because the server side cluster is disabled. - The reason chip_server_cluster_attributes is picking the server side attributes which are not enabled is because those are enabled in the .zap file(Which is the case for saving user selections and ease of use). However the helper is not actually checking if the cluster is enabled as well. - enabled_attributes_for_cluster_and_side is solving this issue correctly and actually only showing attributes which are truly enabled. - Extending endpointTypeAttributeExtended such that there is a common place for an attribute map - Updating endpointTypeAttributeExtended with everything that attributeExportMapping had such that it can act as a common place for all other temporary maps that have been created across our code. Also sorting endpointTypeAttributeExtended for easier readability and avoiding duplicate keys - Updating zcl_attributes_server block helper such that it can behave like chip_server_cluster_attributes. - Updating attribute map such that it can be used more widely - Adding additional helpers to remove the stateful helpers - Adding removeKeys option for removing certain columns which are not needed in a block helper such as enabled_attributes_for_cluster_and_side and zcl_attributes_server - Adding if_unsupported_attribute_type and if_attribute_complex to check for unsupported and complex zcl data types - Deprecating old stateful helpers with new stateless helpers - Adding select data type using type name and cluster id to data_types, enums, bitmap, numbers and structs - Adding if_unsupported_attribute_callback helper and if_basic_attribute if helpers for java code generation - Deprecating java and python zcl type helpers with new zcl type helpers such that they do not need any stateful information coming from parent block helpers. For eg chipType and chipCallback.type - Adding tests for all the newly introduced helpers. - Updating Api documentation - Adding if_is_data_type_signed and as_zcl_data_type_size to determine sign and size of zcl data types - if_is_data_type_signed and as_zcl_data_type_size take type name and cluster id to return size and sign of the zcl data types - Github: ZAP#898

brdandu requested review from andy31415, tecimovic and bzbarsky-apple January 19, 2023 18:05

brdandu self-assigned this Jan 19, 2023

brdandu force-pushed the bug/fixServerSideEnabledClustersAndAttributes/ZAP#898 branch from e6e8677 to 0c5a305 Compare January 19, 2023 18:07

bzbarsky-apple reviewed Jan 19, 2023

View reviewed changes

src-electron/db/query-attribute.js Outdated Show resolved Hide resolved

src-electron/db/query-attribute.js Outdated Show resolved Hide resolved

src-electron/generator/helper-session.js Outdated Show resolved Hide resolved

brdandu linked an issue Jan 20, 2023 that may be closed by this pull request

chip_server_cluster_attributes returning attributes of a clusters which are disabled #898

Closed

brdandu force-pushed the bug/fixServerSideEnabledClustersAndAttributes/ZAP#898 branch 2 times, most recently from 2d8516b to 013b170 Compare March 3, 2023 19:07

brdandu force-pushed the bug/fixServerSideEnabledClustersAndAttributes/ZAP#898 branch from 52ee508 to 1bd167f Compare March 9, 2023 15:33

brdandu requested a review from vivien-apple March 9, 2023 15:33

brdandu force-pushed the bug/fixServerSideEnabledClustersAndAttributes/ZAP#898 branch from 1bd167f to 8b2ddae Compare March 9, 2023 15:36

brdandu force-pushed the bug/fixServerSideEnabledClustersAndAttributes/ZAP#898 branch 2 times, most recently from f53ae4d to fd8af8c Compare March 9, 2023 19:00

bzbarsky-apple reviewed Mar 9, 2023

View reviewed changes

bzbarsky-apple reviewed Mar 10, 2023

View reviewed changes

src-electron/generator/helper-zcl.js Outdated Show resolved Hide resolved

brdandu force-pushed the bug/fixServerSideEnabledClustersAndAttributes/ZAP#898 branch 3 times, most recently from db03a4f to e7acecb Compare March 13, 2023 20:41

bzbarsky-apple reviewed Mar 13, 2023

View reviewed changes

src-electron/generator/helper-attribute.js Outdated Show resolved Hide resolved

src-electron/generator/matter/app/zap-templates/templates/chip/helper.js Outdated Show resolved Hide resolved

src-electron/generator/helper-zcl.js Outdated Show resolved Hide resolved

brdandu force-pushed the bug/fixServerSideEnabledClustersAndAttributes/ZAP#898 branch 2 times, most recently from 0e414d7 to 237bf3d Compare March 14, 2023 18:29

tecimovic approved these changes Mar 15, 2023

View reviewed changes

bzbarsky-apple reviewed Mar 15, 2023

View reviewed changes

brdandu force-pushed the bug/fixServerSideEnabledClustersAndAttributes/ZAP#898 branch from 2bc8864 to f01d94f Compare March 15, 2023 20:14

bzbarsky-apple reviewed Mar 16, 2023

View reviewed changes

src-electron/db/query-attribute.js Outdated Show resolved Hide resolved

src-electron/db/query-bitmap.js Outdated Show resolved Hide resolved

bzbarsky-apple reviewed Mar 16, 2023

View reviewed changes

brdandu force-pushed the bug/fixServerSideEnabledClustersAndAttributes/ZAP#898 branch from f01d94f to efed03c Compare March 16, 2023 19:22

brdandu requested a review from bzbarsky-apple March 16, 2023 19:38

bzbarsky-apple approved these changes Mar 16, 2023

View reviewed changes

brdandu force-pushed the bug/fixServerSideEnabledClustersAndAttributes/ZAP#898 branch from efed03c to 4f26132 Compare March 16, 2023 20:51

brdandu force-pushed the bug/fixServerSideEnabledClustersAndAttributes/ZAP#898 branch from 4f26132 to 8bff12d Compare March 16, 2023 21:07

brdandu merged commit feb352c into project-chip:master Mar 16, 2023

andy31415 mentioned this pull request Mar 17, 2023

Update MatterIDL codegen to not take into account enabled for client attributes project-chip/connectedhomeip#25737

Closed

tecimovic mentioned this pull request Apr 11, 2023

Make zap files smaller #992

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding to zcl_attributes_server and enabled_attributes_for_cluster_and_side helpers to replace chip_server_cluster_attributes #899

Adding to zcl_attributes_server and enabled_attributes_for_cluster_and_side helpers to replace chip_server_cluster_attributes #899

brdandu commented Jan 19, 2023

bzbarsky-apple left a comment

brdandu commented Jan 19, 2023

bzbarsky-apple commented Jan 19, 2023

brdandu commented Jan 20, 2023

bzbarsky-apple commented Jan 20, 2023

brdandu commented Jan 23, 2023 •

edited

Loading

bzbarsky-apple commented Jan 23, 2023 •

edited

Loading

brdandu commented Jan 23, 2023

andy31415 commented Jan 23, 2023

andy31415 commented Jan 23, 2023

bzbarsky-apple commented Jan 24, 2023

brdandu commented Jan 25, 2023

codecov-commenter commented Mar 10, 2023 •

edited

Loading

tecimovic left a comment

Adding to zcl_attributes_server and enabled_attributes_for_cluster_and_side helpers to replace chip_server_cluster_attributes #899

Adding to zcl_attributes_server and enabled_attributes_for_cluster_and_side helpers to replace chip_server_cluster_attributes #899

Conversation

brdandu commented Jan 19, 2023

bzbarsky-apple left a comment

Choose a reason for hiding this comment

brdandu commented Jan 19, 2023

bzbarsky-apple commented Jan 19, 2023

brdandu commented Jan 20, 2023

bzbarsky-apple commented Jan 20, 2023

brdandu commented Jan 23, 2023 • edited Loading

bzbarsky-apple commented Jan 23, 2023 • edited Loading

brdandu commented Jan 23, 2023

andy31415 commented Jan 23, 2023

andy31415 commented Jan 23, 2023

bzbarsky-apple commented Jan 24, 2023

brdandu commented Jan 25, 2023

codecov-commenter commented Mar 10, 2023 • edited Loading

Codecov Report

tecimovic left a comment

Choose a reason for hiding this comment

brdandu commented Jan 23, 2023 •

edited

Loading

bzbarsky-apple commented Jan 23, 2023 •

edited

Loading

codecov-commenter commented Mar 10, 2023 •

edited

Loading