-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Part 4: feature/binary input basic cluster #7032
Conversation
@@ -735,6 +735,103 @@ | |||
} | |||
] | |||
}, | |||
{ | |||
"name": "Binary Input (Basic)", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remove the Silabs
definitions from src/app/zap-templates/zlc/data_model/Silabs/general.xml
and src/app/zcl/zap-templates/data_model/Silabs/general-thread.xml
and add your own into src/app/zap-templates/zcl/input-output-value.xml
? It will be easier to compare the definition to the spec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure I can do this. The only think is that I don't know which side effects this introduces. For example src/app/zap-templates/zcl/data-model/silabs/ha-devices.xml
does reference the Binary Input Basic cluster.
@@ -158,6 +156,3 @@ typedef void (*TvChannelTvChannelListListAttributeCallback)(void * context, uint | |||
typedef void (*TargetNavigatorTargetNavigatorListListAttributeCallback)(void * context, uint16_t count, | |||
_NavigateTargetTargetInfo * entries); | |||
typedef void (*TestClusterListInt8uListAttributeCallback)(void * context, uint16_t count, uint8_t * entries); | |||
typedef void (*TestClusterListOctetStringListAttributeCallback)(void * context, uint16_t count, chip::ByteSpan * entries); | |||
typedef void (*TestClusterListStructOctetStringListAttributeCallback)(void * context, uint16_t count, | |||
_TestListStructOctet * entries); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like unrelated changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know why this happened. I can only tell that a more up to date ZAP Tool seems to remove this lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It happened because of the pieces removed from src/controller/data_model/controller-clusters.zap
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving for TE3 under the assumption that the changes mentioned in https://github.com/project-chip/connectedhomeip/pull/7032/files?file-filters%5B%5D=.h&file-filters%5B%5D=.js&file-filters%5B%5D=.m&file-filters%5B%5D=.mm&file-filters%5B%5D=.py#r638728211 are removed.
It is probably some conflicts into the ZAP file for something else since I did update ZAP into #7189 and have not noticed those changes.
It seems like the result of this PR is mostly to expose a set of accessors to facilitate the manipulation of the input basic
cluster attributes.
It makes perfect sense to facilitate it imho, but I would rather prefer to autogenerate helpers code such as what is proposed in #7183, instead of having a lot of new clusters introduced into src/app/clusters
.
examples/all-clusters-app/all-clusters-common/all-clusters-app.zap
Outdated
Show resolved
Hide resolved
examples/all-clusters-app/all-clusters-common/all-clusters-app.zap
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, codegen never got run across the tree here....
@eve-cxrp Can you take a look at the merge conflicts please? |
used a custom patched zap tool. based on 535ebf3, because the current zap tool is not working properly
- replace emberAfReadAttribute with emberAfReadServerAttribute - replace emberAfWriteAttribute with emberAfWriteServerAttribute - add emberAfBinaryInputBasicClusterPrintln if not defined - let the caller do the logging - expose emberAfBinaryInputBasicClusterGetPresentValue - expose emberAfBinaryInputBasicClusterGetOutOfService
This reverts commit 99492f2.
The default values are populated by the attribute storage.
Other things are using c.clusterName, so we should do that here as well to match.
Size increase report for "esp32-example-build" from 0f94665
Full report output
|
* format endpointClusterWithInit for easier pull requests * add support for emberAfBinaryInputBasicClusterServerInitCallback * fix emberAfBinaryInputBasicClusterServerInitCallback Note: relies on project-chip/zap#154 * Add new clusters to zap_cluster_list.py * add initial binary input basic cluster impl. * fix typo in binary-input-server.cpp * add binary input basic to controller-clusters.zap used a custom patched zap tool. based on 535ebf3, because the current zap tool is not working properly * Restyled by whitespace * Restyled by clang-format * add fixes && expose more binary input APIs - replace emberAfReadAttribute with emberAfReadServerAttribute - replace emberAfWriteAttribute with emberAfWriteServerAttribute - add emberAfBinaryInputBasicClusterPrintln if not defined - let the caller do the logging - expose emberAfBinaryInputBasicClusterGetPresentValue - expose emberAfBinaryInputBasicClusterGetOutOfService * Revert "Add new clusters to zap_cluster_list.py" This reverts commit 99492f2. * add only binary input basic to zap_clusters_list.py * remove emberAfBinaryInputBasicClusterServerInitCallback impl. The default values are populated by the attribute storage. * fix year of copyright * fix includes in binary-input-basic-server.cpp * add binary-input-basic cluster to the all-clusters-app.zap * try to fix all-cluster-app/esp32 * another fix for the all-cluster-app/esp32 * Fix endpoint config generation for clusters with complicated names. Other things are using c.clusterName, so we should do that here as well to match. * Restore the octet string tests that were improperly removed * Remove unnecessary init function from Binary Input cluster * Move Binary Input cluster from endpoint 0 to endpoint 1 in all-clusters-app * Regenerate generated files Co-authored-by: Vivien Nicolas <[email protected]> Co-authored-by: Restyled.io <[email protected]> Co-authored-by: Boris Zbarsky <[email protected]>
Problem
The matter project has currently no support for the binary input (basic) cluster. This PR adds a support for that.
Change overview
Limitations
Testing
Tested on custom accessory with contact sensor. Test setup was similar to TE2.
HW Platform
Nordic nRF52840
Notes
Related PRs
I have also created a issue regarding the name.