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.

Sign up for GitHub

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

Jump to bottom

[Fixed] OTAServerDelegate class for vendor logic #7537

Merged
merged 8 commits into from
Jun 16, 2021
Merged

[Fixed] OTAServerDelegate class for vendor logic #7537

merged 8 commits into from
Jun 16, 2021

Conversation

holbrookt
Copy link
Contributor

@holbrookt holbrookt commented Jun 10, 2021 •
edited
Loading

This is a resubmit of #6476 with fixes for the breakages that lead to #7534

Problem

Need a way to separate common code from application/vendor specific code for OTA Server Cluster, which doesn't require the developer to fork the repo.

Change overview

Testing

Ran chip-tool and all-clusters-app on Linux machine, confirmed that I can send query-image apply-update-request and notify-update-applied. The response to apply-update-request and notify-update-applied is EMBER_ZCL_STATUS_UNSUP_COMMAND as expected, because there is no OTAServerDelegate implemented yet. The response to query-image is a TLV parse failure because CHAR_STRING is not supported yet

holbrookt and others added 2 commits June 10, 2021 23:18
@holbrookt @tcarmelveilleux
OTAServerDelegate class for vendor logic (project-chip#6476)
e8b3ce6 
* OTAServerDelegate class for vendor logic

- define OTAServerDelegate class for implementing vendor OTA Server
logic
- add some common logic for OTA Server Cluster commands
- use OTA_SERVER_CLUSTER_EXAMPLE_IMPL to conditionally build example
logic
- add OTA Server Cluster to all-clusters-app and chip-tool
- fix AnnounceOTAServer command direction

* add build macros to all-clusters-app esp32 build files

* use virtual delegate methods instead of static singleton

* fix whitespace

* add OTA Server to all-clusters-app, remove ota-client callbacks

ota-client response callbacks are automatically generated now

* generated files

* remove src/darwin edits

* edit zap files to include just ota clusters

* regenerate gen files after zap fix

* add multiple endpoint support and EMBER_ZCL_STATUS_INVALID_ARGUMENT

* remove OTA Server Cluster from all-clusters-app Endpoint 1

- There should only be one OTA Server Cluster, in endpoint 0

* fix darwin tests

- enable Cluster Revision attribute reporting for OTA Server Cluster in
all-clusters-app
- change darwin test to use endpoint 0 instead of endpoint 1

* edit helper.js in darwin to fix OTA Server test

- note: tv-app files were also modified and tv-app build is failing

* generated files after merge conflict

* resolve merge conflicts + regen

* default destructor for OTAServerDelegate

Co-authored-by: Tennessee Carmel-Veilleux <[email protected]>

* error logging and value change for EMBER_ZCL_STATUS_INVALID_ARGUMENT

* regenerate

* regenerated files after merge conflict fix

* add generated tv-app files

* Revert "add generated tv-app files"

This reverts commit 48acb90.

Co-authored-by: Tennessee Carmel-Veilleux <[email protected]>
@holbrookt
change logging types and regenerate files
4f8acda
@holbrookt holbrookt mentioned this pull request Jun 10, 2021
Merged
@andy31415
Copy link
Contributor

@holbrookt - needs merge due to generated code

mspang
mspang approved these changes Jun 13, 2021
View reviewed changes
src/app/clusters/ota-server/ota-server.cpp Outdated
return true;
}

void chip::app::clusters::OTAServer::SetDelegate(chip::EndpointId endpointId, OTAServerDelegate * delegate)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are doing "using namespace chip::app::clusters" only to have to spell out the entire thing here?

Can't we put the entire file content within that namespace?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The entire file should not be in that namespace because the emberAf... functions are defined in the global namespace.

But I did remove the unnecessary scopes here

mspang
mspang requested changes Jun 13, 2021
View reviewed changes
Copy link
Contributor

@mspang mspang left a comment

Choose a reason for hiding this comment

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

Please fix the namespacing.

@holbrookt holbrookt requested a review from mspang June 14, 2021 20:43
@github-actions
Copy link

Size increase report for "esp32-example-build" from 26e00e1

File Section File VM
chip-all-clusters-app.elf .flash.text 396 396
chip-all-clusters-app.elf .flash.rodata 264 264
chip-all-clusters-app.elf .dram0.bss 0 96
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
.debug_info,0,2300
.debug_line,0,1217
.debug_loc,0,650
.flash.text,396,396
.debug_abbrev,0,286
.flash.rodata,264,264
.debug_str,0,147
.dram0.bss,96,0
.strtab,0,59
.debug_ranges,0,56
.debug_frame,0,48
.debug_aranges,0,32
.symtab,0,32
.shstrtab,0,1
[Unmapped],0,-264

@holbrookt
Copy link
Contributor Author

Please fix the namespacing.

fixed in latest commits.

@mspang mspang merged commit 1323ca4 into project-chip:master Jun 16, 2021
@woody-apple woody-apple mentioned this pull request Jun 16, 2021
Merged
nikita-s-wrk pushed a commit to nikita-s-wrk/connectedhomeip that referenced this pull request Sep 23, 2021
@holbrookt @tcarmelveilleux
[Fixed] OTAServerDelegate class for vendor logic (project-chip#7537)
05c82ed 
* OTAServerDelegate class for vendor logic (project-chip#6476)

* OTAServerDelegate class for vendor logic

- define OTAServerDelegate class for implementing vendor OTA Server
logic
- add some common logic for OTA Server Cluster commands
- use OTA_SERVER_CLUSTER_EXAMPLE_IMPL to conditionally build example
logic
- add OTA Server Cluster to all-clusters-app and chip-tool
- fix AnnounceOTAServer command direction

* add build macros to all-clusters-app esp32 build files

* use virtual delegate methods instead of static singleton

* fix whitespace

* add OTA Server to all-clusters-app, remove ota-client callbacks

ota-client response callbacks are automatically generated now

* generated files

* remove src/darwin edits

* edit zap files to include just ota clusters

* regenerate gen files after zap fix

* add multiple endpoint support and EMBER_ZCL_STATUS_INVALID_ARGUMENT

* remove OTA Server Cluster from all-clusters-app Endpoint 1

- There should only be one OTA Server Cluster, in endpoint 0

* fix darwin tests

- enable Cluster Revision attribute reporting for OTA Server Cluster in
all-clusters-app
- change darwin test to use endpoint 0 instead of endpoint 1

* edit helper.js in darwin to fix OTA Server test

- note: tv-app files were also modified and tv-app build is failing

* generated files after merge conflict

* resolve merge conflicts + regen

* default destructor for OTAServerDelegate

Co-authored-by: Tennessee Carmel-Veilleux <[email protected]>

* error logging and value change for EMBER_ZCL_STATUS_INVALID_ARGUMENT

* regenerate

* regenerated files after merge conflict fix

* add generated tv-app files

* Revert "add generated tv-app files"

This reverts commit 48acb90.

Co-authored-by: Tennessee Carmel-Veilleux <[email protected]>

* change logging types and regenerate files

* regenerate after merge conflict

* fix namespacing

* regenerate files

* wrap SetDelegate() in namespace scope

Co-authored-by: Tennessee Carmel-Veilleux <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmartinez-silabs jmartinez-silabs jmartinez-silabs approved these changes

@bzbarsky-apple bzbarsky-apple bzbarsky-apple approved these changes

@andy31415 andy31415 andy31415 approved these changes

@mspang mspang mspang approved these changes

@chrisdecenzo chrisdecenzo Awaiting requested review from chrisdecenzo

@jelderton jelderton Awaiting requested review from jelderton

@LuDuda LuDuda Awaiting requested review from LuDuda

@sagar-apple sagar-apple Awaiting requested review from sagar-apple

@saurabhst saurabhst Awaiting requested review from saurabhst

@selissia selissia Awaiting requested review from selissia

@woody-apple woody-apple Awaiting requested review from woody-apple

Assignees
No one assigned
Labels
app controller darwin examples review - approved
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement OTA Software Update cluster (Server side)
5 participants
@holbrookt @andy31415 @mspang @bzbarsky-apple @jmartinez-silabs