Skip to content
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

Merged
merged 23 commits into from Jun 1, 2021
Merged

Part 4: feature/binary input basic cluster #7032

merged 23 commits into from Jun 1, 2021

Conversation

ghost
Copy link

@ghost ghost commented May 21, 2021

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.

src/app/zap_cluster_list.py Outdated Show resolved Hide resolved
src/app/zap_cluster_list.py Outdated Show resolved Hide resolved
src/app/zap_cluster_list.py Outdated Show resolved Hide resolved
src/app/zap_cluster_list.py Outdated Show resolved Hide resolved
@@ -735,6 +735,103 @@
}
]
},
{
"name": "Binary Input (Basic)",
Copy link
Contributor

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.

Copy link
Author

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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like unrelated changes.

Copy link
Author

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.

Copy link
Contributor

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

Copy link
Contributor

@vivien-apple vivien-apple left a 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.

Copy link
Contributor

@bzbarsky-apple bzbarsky-apple left a 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....

@woody-apple
Copy link
Contributor

@eve-cxrp Can you take a look at the merge conflicts please?

@andy31415 @saurabhst @jelderton @Damian-Nordic ?

Dimitri Mizenko and others added 23 commits June 1, 2021 14:25
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
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.
@github-actions
Copy link

github-actions bot commented Jun 1, 2021

Size increase report for "esp32-example-build" from 0f94665

File Section File VM
chip-all-clusters-app.elf .dram0.bss 0 184
chip-all-clusters-app.elf .flash.rodata 136 136
chip-all-clusters-app.elf .flash.text 52 52
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-pigweed-app.elf and ./pull_artifact/chip-pigweed-app.elf:

sections,vmsize,filesize

Comparing ./master_artifact/chip-all-clusters-app.elf and ./pull_artifact/chip-all-clusters-app.elf:

sections,vmsize,filesize
.dram0.bss,184,0
.flash.rodata,136,136
.debug_str,0,91
.debug_line,0,88
.flash.text,52,52
.debug_info,0,49
.strtab,0,48
.debug_frame,0,24
.symtab,0,16
.xt.prop._ZTVN4chip11DeviceLayer37DeviceNetworkProvisioningDelegateImplE,0,1
.debug_loc,0,-21
[Unmapped],0,-136


@andy31415 andy31415 merged commit 56ecd46 into project-chip:master Jun 1, 2021
nikita-s-wrk pushed a commit to nikita-s-wrk/connectedhomeip that referenced this pull request Sep 23, 2021
* 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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants