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

Update TV app to use the new IM changes #7906

Merged

Conversation

lazarkov
Copy link
Contributor

@lazarkov lazarkov commented Jun 25, 2021

Problem

TV example app was not using the IM changes introduced by this commit:
#5945

Change overview

  • Updated the tv.zap and enabled TV clusters
    - Updated the all-clusters.zap and disabled TV clusters
  • Updated TV cluster code to use IM logic
  • Created test_tv_suites.sh script for testign
  • Created *.yaml file for every TV cluster.

Testing

How was this tested? (at least one bullet point required)

  • Used the ./gn_build.sh to verify the building is successful
    - Run the ./scripts/tests/test_tv_suites.sh
  • Run the ./scripts/tests/test_suites.sh -a tv

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.

General architectural comment: We should have the "everyone has to do this" parts of cluster implementations in src/app/clusters and make callouts to apps for logic the app needs to provide. We should not be requiring everyone who wants to build a tv app to reinvent a bunch of wheels like encoding the responses and whatnot that are really shared code.

@lazarkov lazarkov force-pushed the feature/update-tv-app-per-new-im branch from 03ab61b to 6da8656 Compare June 30, 2021 12:24
@lazarkov lazarkov force-pushed the feature/update-tv-app-per-new-im branch 2 times, most recently from e42ea9a to 89a30df Compare June 30, 2021 12:37
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.

Not a full review; since there are unaddressed changes from last time (e.g. about pointers into a stack frame that goes away) I wasn't sure whether one was desired yet.

@lazarkov lazarkov force-pushed the feature/update-tv-app-per-new-im branch from d8b6202 to 6fb856f Compare June 30, 2021 17:48
@lazarkov lazarkov force-pushed the feature/update-tv-app-per-new-im branch 2 times, most recently from d9be430 to 3709f75 Compare July 1, 2021 09:05
@lazarkov lazarkov force-pushed the feature/update-tv-app-per-new-im branch from 3709f75 to fa8fa8e Compare July 1, 2021 09:12
@lazarkov
Copy link
Contributor Author

lazarkov commented Jul 1, 2021

@bzbarsky-apple Seems to me that the Darwin build failing has nothing to do with this PR.

/Users/runner/work/connectedhomeip/connectedhomeip/src/lib/core/CHIPTLV.h:274:20: error: unknown type name 'int8_t'; did you mean 'uint8_t'? CHIP_ERROR Get(int8_t & v); ^~~~~~ uint8_t

For example this line CHIPTLV.h:274 hasn't been changed in a year.

@lazarkov lazarkov force-pushed the feature/update-tv-app-per-new-im branch 2 times, most recently from 2ac636c to 17246f8 Compare July 1, 2021 15:25
@lazarkov lazarkov force-pushed the feature/update-tv-app-per-new-im branch 3 times, most recently from d9f41c0 to f556c0c Compare July 2, 2021 18:08
@woody-apple woody-apple dismissed bzbarsky-apple’s stale review July 2, 2021 18:37

Comments are resolved

@lazarkov lazarkov force-pushed the feature/update-tv-app-per-new-im branch from f556c0c to 72c603d Compare July 2, 2021 22:45
@lazarkov lazarkov force-pushed the feature/update-tv-app-per-new-im branch 2 times, most recently from 7bf97c2 to 7c7b2af Compare July 2, 2021 22:51
Problem
- After the IM changes TV app was not returning correct responses

Summary of Changes
- Updated all the TV clusters to return response using new TLVWriter
Problem
- All cluster app does not have zap regenerated files

Summary of Changes
- Run zap_regen_all.py script
Problem
- TV app is not covered with tests

Summary of Changes
- Added yaml files for all clusters that needs to be tested
Problem
- Tests does not have zap regenerated files

Summary of Changes
- Run zap_regen_all.py script
@lazarkov lazarkov force-pushed the feature/update-tv-app-per-new-im branch from 7c7b2af to 81f9195 Compare July 3, 2021 15:14
@lazarkov
Copy link
Contributor Author

lazarkov commented Jul 3, 2021

Resolved conflicts.

@github-actions
Copy link

github-actions bot commented Jul 3, 2021

Size increase report for "esp32-example-build" from d1e04c6

File Section File VM
chip-all-clusters-app.elf .flash.text 32 32
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-persistent-storage.elf and ./pull_artifact/chip-persistent-storage.elf:

sections,vmsize,filesize

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

sections,vmsize,filesize
.debug_str,0,-8

Comparing ./master_artifact/chip-shell.elf and ./pull_artifact/chip-shell.elf:

sections,vmsize,filesize

Comparing ./master_artifact/chip-temperature-measurement-app.elf and ./pull_artifact/chip-temperature-measurement-app.elf:

sections,vmsize,filesize
.debug_str,0,-8

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

sections,vmsize,filesize
.debug_info,0,72
.debug_line,0,64
.debug_loc,0,41
.debug_abbrev,0,39
.debug_str,0,32
.flash.text,32,32
.strtab,0,26
.debug_frame,0,24
.symtab,0,16
.debug_aranges,0,8
.debug_ranges,0,8
.shstrtab,0,2

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

sections,vmsize,filesize


@woody-apple woody-apple merged commit d68c20c into project-chip:master Jul 5, 2021
@franck-apple franck-apple added this to the Test Event 4 milestone Jul 6, 2021
andy31415 pushed a commit that referenced this pull request Jul 6, 2021
* Fix IM changes

Problem
- After the IM changes TV app was not returning correct responses

Summary of Changes
- Updated all the TV clusters to return response using new TLVWriter

* Running zap generator

Problem
- All cluster app does not have zap regenerated files

Summary of Changes
- Run zap_regen_all.py script

* Adding tests for TV app

Problem
- TV app is not covered with tests

Summary of Changes
- Added yaml files for all clusters that needs to be tested

* Running zap generator

Problem
- Tests does not have zap regenerated files

Summary of Changes
- Run zap_regen_all.py script

* Adding TV callbacks to all cluster example apps

* Updated the PR latests comments

* Update PR per latest comments

* Updated the PR per latest comments
nikita-s-wrk pushed a commit to nikita-s-wrk/connectedhomeip that referenced this pull request Sep 23, 2021
* Fix IM changes

Problem
- After the IM changes TV app was not returning correct responses

Summary of Changes
- Updated all the TV clusters to return response using new TLVWriter

* Running zap generator

Problem
- All cluster app does not have zap regenerated files

Summary of Changes
- Run zap_regen_all.py script

* Adding tests for TV app

Problem
- TV app is not covered with tests

Summary of Changes
- Added yaml files for all clusters that needs to be tested

* Running zap generator

Problem
- Tests does not have zap regenerated files

Summary of Changes
- Run zap_regen_all.py script

* Adding TV callbacks to all cluster example apps

* Updated the PR latests comments

* Update PR per latest comments

* Updated the PR per latest comments
@lazarkov lazarkov deleted the feature/update-tv-app-per-new-im branch December 23, 2021 19:32
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.

8 participants