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

Fix Window app commissioning flow #7526

Merged
merged 6 commits into from
Jun 16, 2021

Conversation

jmartinez-silabs
Copy link
Member

Problem

Commissioning flow is broken on window-app as the operational clusters have not been moved to endpoint 0 for this example common generated code.

Change overview

Modify window-app.zap to move all operational clusters (NetworkCommissioning, Basic, Diagnostics) to endpoint 0, Keep application cluster (WindowCovering) on endpoint 1

First commit is the changes to the .zap file.
2nd is the regeneration of the files

Testing

Manual testing, validated commissioning flow on python controller with NetworkCommissioning on endpoint 0 and validated WindowCovering cluster commmands are received on the EFR device using endpoint 1

@jmartinez-silabs jmartinez-silabs changed the title Window app Fix Window app commissioning flow Jun 10, 2021
@jmartinez-silabs
Copy link
Member Author

Investigating this ZAP failure in the CI. The CI does a regen and removes commands that should not be removed. It happends locally also, but once committed the regen does the opposite, re-adds the commands.

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.

Looks reasonable, though maybe removing instead of disabling the endpoint-0 bits from the other endpoint would have made more sense...

@bzbarsky-apple
Copy link
Contributor

I wonder whether the zap failure is similar to the one that keeps biting tv-app. @vivien-apple

@jmartinez-silabs
Copy link
Member Author

Looks reasonable, though maybe removing instead of disabling the endpoint-0 bits from the other endpoint would have made more sense...

@bzbarsky-apple I agree, Not sure how to effectively delete them in zap. In endpoint 1, I changed all of them from"Server" to not a value. Created endpoint 0 and set them to server. I thought the regen would delete them from endpoint 1 but it just disabled them. Is there another way to go?

@bzbarsky-apple
Copy link
Contributor

I thought the regen would delete them from endpoint 1 but it just disabled them. Is there another way to go?

I don't know. I have yet to use the ZAP UI; I've just ended up hand-editing .zap files as needed...

@jmartinez-silabs
Copy link
Member Author

@bzbarsky-apple I just validated with the zap team.
We do store both entries like that intentionally, at least for the disabled commands/attributes configuration just in case. However, I think we are planning a project to remove at least some of this duplcication due to the large size of the ZAP files.

Also the generation issue adding/removing the commands are due to a helper issue with incomming commands. and that helper should be replace by a new one for command parsing

@andy31415
Copy link
Contributor

@jmartinez-silabs - merge conflicts

@woody-apple
Copy link
Contributor

/rebase

@woody-apple
Copy link
Contributor

@jmartinez-silabs these look like real errors.

@saurabhst @msandstedt?

@jmartinez-silabs
Copy link
Member Author

@woody-apple Right those are real errors that I fixed with #7619 . I will rebase this PR, should fix it.

regen after rebase

Rebase and regen for updates
…_CLUSTER in the modified window-app.zap file

Regen files
Rebase and Regen2
@andy31415 andy31415 merged commit 844c65b into project-chip:master Jun 16, 2021
woody-apple added a commit that referenced this pull request Jun 16, 2021
woody-apple added a commit that referenced this pull request Jun 17, 2021
@bzbarsky-apple
Copy link
Contributor

@jmartinez-silabs This was reverted because of a merge issue that caused it to break the build... Needs to be rebased on ToT, codegen rerun, and the build issue addressed.

@jmartinez-silabs
Copy link
Member Author

@bzbarsky-apple ok thank you Boris. Will address this.

sharadb-amazon pushed a commit to sharadb-amazon/connectedhomeip that referenced this pull request Jun 17, 2021
nikita-s-wrk pushed a commit to nikita-s-wrk/connectedhomeip that referenced this pull request Sep 23, 2021
* move operational clusters to endpoint 0, keep the application cluster (window cover) in ep 1

* regenerate files with zap

regen after rebase

Rebase and regen for updates

* Rebase and regen

* Fix missed issue when rebabse didn't remove TRUSTED_ROOT_CERTIFICATES_CLUSTER in the modified window-app.zap file
Regen files

* Rebase and Regen

Rebase and Regen2

* rebase&regen
nikita-s-wrk pushed a commit to nikita-s-wrk/connectedhomeip that referenced this pull request Sep 23, 2021
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