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

Enabling list attriubtes on the app clusters and updating the TV example app #6582

Conversation

lazarkov
Copy link
Contributor

@lazarkov lazarkov commented May 7, 2021

Problem

We do not have list attributes enabled on app clusters.

Summary of Changes

  • Updated all the app clusters where list attributes are used
  • Updated the TV example app to use the new attributes

Note

Everything which is not supported by the CHIP framework currently is noted with TODO comment.

Test

  • Tested locally using chip-tool client and tv-server
  • Used the ./scripts/tools/zap_regen_all.py and ./gn_build.sh to verify the building is successful

@lazarkov
Copy link
Contributor Author

lazarkov commented May 7, 2021

  • Added the list attribute to all the xml files
  • Updated the includes folder and every cluster manager now populates the list with example data
  • Updated the cluster-init.cpp file, now every cluster once it is initialised has the logic which stores the example list

@woody-apple
Copy link
Contributor

@vivien-apple ?

@lazarkov lazarkov changed the title Adding list attriubtes to the clusters and updating the TV example app Enabling list attriubtes to the clusters and updating the TV example app May 7, 2021
@lazarkov lazarkov changed the title Enabling list attriubtes to the clusters and updating the TV example app Enabling list attriubtes on the app clusters and updating the TV example app May 7, 2021
@woody-apple
Copy link
Contributor

@lazarkov conflicts here :(

@lazarkov lazarkov force-pushed the feature/update-tv-example-app-with-list-attributes branch from 5d3b94f to 53838c0 Compare May 11, 2021 16:44
@lazarkov
Copy link
Contributor Author

@woody-apple update the PR. Conflicts should be resolved now.

@woody-apple
Copy link
Contributor

/rebase

@woody-apple
Copy link
Contributor

@lazarkov Sorry, still failing build - and now conflicted again with tv app :(

@bzbarsky-apple
Copy link
Contributor

@lazarkov I would recommend splitting this into two changesets within the PR: one with the manual changes and one with the auto-generated changes. That will make review easier and also make merging/rebasing easier: the generated PR can just be dropped nad then regenerated after merging/rebasing the manual changes.

@lazarkov lazarkov force-pushed the feature/update-tv-example-app-with-list-attributes branch from 53838c0 to 25914a2 Compare May 17, 2021 16:28
Problem
We do not have list attributes enabled on app clusters.

Summary of Changes
- Updated all the app clusters where list attributes are used
- Updated the TV example app to use the new attributes

Test
- Tested locally using chip-tool client and newly created tv-server
- Used the zap_regen_all.py and gn_build.sh to verify the building is successful
@lazarkov lazarkov force-pushed the feature/update-tv-example-app-with-list-attributes branch from 25914a2 to 91c71f5 Compare May 17, 2021 16:35
@lazarkov
Copy link
Contributor Author

lazarkov commented May 17, 2021

@bzbarsky-apple I understand it's hard to follow as gen/ folder polutes the PR.
I do not understand the advice of splitting of the PRs. For example if I add only xml changes without gen/ folder changes CI will fail as it compares if the gen/ folder the CI builds vs the one I've submitted. So it will expect to have the changes in gen/ folder

Now regarding the PR, on how to follow what actually is the change:
I've added support for all the list attributes in the clusters as they were not supported before.

  1. Follow the changes in the cluster .xml files. Every TODO: List is now removed and list attribute is added
  2. I've created the ClusterManager.cpp file which stores the list attribute for any cluster
  3. I've called the ClusterManager on cluster-init.cpp on every cluster initialization
  4. I've passed the "fake list data" for every cluster which is stored on cluster init.

Clusters who have now list attribute supported are: TV Channel, Content Launcher, Application Launcher, Audio Output, Media Input and Target navigator.

If there's a good way to split PR, please advise.
If there's any better way to do a PR, please advise.

@andy31415 andy31415 merged commit fdb60ef into project-chip:master May 17, 2021
@bzbarsky-apple
Copy link
Contributor

I do not understand the advice of splitting of the PRs.

@lazarkov I did not say to split up into multiple _PR_s. I said to split up into multiple changesets within a single PR. This makes review significantly simpler, because instead of needing to find the non-generated files in the overall diff you just have two separate diffs: one with the manual changes, one with the generated ones.

See #6804 for an example. If you look at https://github.com/project-chip/connectedhomeip/pull/6804/commits there are two commits: one with the manual changes, and one with the generated stuff.

@lazarkov lazarkov deleted the feature/update-tv-example-app-with-list-attributes branch December 23, 2021 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants