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

Do a better job of not suppressing rejected promises in ZAP helpers. #8839

Merged

Conversation

bzbarsky-apple
Copy link
Contributor

There are two fundamental changes here:

  1. Make Clusters.init actually reject this.ready on errors.

  2. Make the various catch() bits we have re-throw the error after
    logging. The only reason we need these at all is that there are
    outstanding ZAP bugs that end up suppressing these errors
    somewhere, so if we don't log them we don't notice them happening
    at all.

Problem

We are suppressing some errors when we should not.

Change overview

Make sure to propagate them.

Testing

Tried adding various exceptions to the code and ensured that they got logged better, and in some cases failed codegen when we used to not fail it.

There are two fundamental changes here:

1) Make Clusters.init actually reject this.ready on errors.

2) Make the various catch() bits we have re-throw the error after
   logging.  The only reason we need these at all is that there are
   outstanding ZAP bugs that end up suppressing these errors
   somewhere, so if we don't log them we don't notice them happening
   at all.
@github-actions
Copy link

github-actions bot commented Aug 6, 2021

Size increase report for "esp32-example-build" from 075a974

File Section File VM
chip-lock-app.elf .flash.text 64 64
chip-temperature-measurement-app.elf .flash.text -60 -60
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

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

sections,vmsize,filesize
.flash.text,64,64
[Unmapped],0,-64

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

sections,vmsize,filesize

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

sections,vmsize,filesize

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

sections,vmsize,filesize
[Unmapped],0,60
.flash.text,-60,-60

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

sections,vmsize,filesize

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

sections,vmsize,filesize

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

sections,vmsize,filesize

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

sections,vmsize,filesize


@pullapprove pullapprove bot requested review from selissia and removed request for kedars August 11, 2021 03:04
@woody-apple woody-apple merged commit eb924d4 into project-chip:master Aug 11, 2021
@bzbarsky-apple bzbarsky-apple deleted the propagate-zap-errors-better branch August 11, 2021 04:31
nikita-s-wrk pushed a commit to nikita-s-wrk/connectedhomeip that referenced this pull request Sep 23, 2021
…roject-chip#8839)

There are two fundamental changes here:

1) Make Clusters.init actually reject this.ready on errors.

2) Make the various catch() bits we have re-throw the error after
   logging.  The only reason we need these at all is that there are
   outstanding ZAP bugs that end up suppressing these errors
   somewhere, so if we don't log them we don't notice them happening
   at all.
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.

None yet

4 participants