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

Add CHIP_ERROR object methods #8784

Merged
merged 4 commits into from
Aug 5, 2021

Conversation

kpschoedel
Copy link
Contributor

Problem

Having CHIP_ERROR be a class type provides type safety and the option to
trace the source of errors. Since all platforms now use the class type,
transitional code that provided for CHIP_ERROR to be either a class or
integer type is no longer required and can be removed, which simplifies
the class and some notation for using errors.

Change overview

  • Remove the CHIP_CONFIG_ERROR_CLASS configuration option.
  • Add instance methods to replace the static ChipError functions, which
    were present to allow transitional integer overloads. To avoid
    breaking PRs in flight, the static methods remain present for now,
    marked “DO NOT USE”; they will be removed in a final cleanup PR.

Testing

Existing tests should confirm no change to functionality.

@kpschoedel kpschoedel force-pushed the x8340-error-class-7 branch from 86939d8 to 2f2e1f4 Compare August 4, 2021 14:06
#### Problem

Having CHIP_ERROR be a class type provides type safety and the option to
trace the source of errors. Since all platforms now use the class type,
transitional code that provided for CHIP_ERROR to be either a class or
integer type is no longer required and can be removed, which simplifies
the class and some notation for using errors.

#### Change overview

- Remove the `CHIP_CONFIG_ERROR_CLASS` configuration option.
- Add instance methods to replace the static `ChipError` functions, which
  were present to allow transitional integer overloads. To avoid
  breaking PRs in flight, the static methods remain present for now,
  marked “DO NOT USE”; they will be removed in a final cleanup PR.

#### Testing

Existing tests should confirm no change to functionality.
@kpschoedel kpschoedel force-pushed the x8340-error-class-7 branch from 2f2e1f4 to 3201c96 Compare August 4, 2021 14:57
@andy31415
Copy link
Contributor

@kpschoedel - merge conflicts

@github-actions
Copy link

github-actions bot commented Aug 4, 2021

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

File Section File VM
chip-ipv6only-app.elf .flash.text -172 -172
chip-all-clusters-app.elf .flash.text -36 -36
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-ipv6only-app.elf and ./pull_artifact/chip-ipv6only-app.elf:

sections,vmsize,filesize
.debug_info,0,14460
.debug_str,0,666
[Unmapped],0,172
.debug_abbrev,0,116
.debug_line,0,10
.flash.text,-172,-172

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

sections,vmsize,filesize
.debug_info,0,14460
.debug_str,0,674
.debug_abbrev,0,116
.debug_line,0,10

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

sections,vmsize,filesize
.debug_info,0,2570
.debug_str,0,424
.debug_abbrev,0,141
.debug_loc,0,17
.debug_line,0,-4

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

sections,vmsize,filesize
.debug_info,0,85029
.debug_abbrev,0,3565
.debug_str,0,417
[Unmapped],0,36
.debug_loc,0,22
.riscv.attributes,0,2
.shstrtab,0,2
.strtab,0,-2
.debug_frame,0,-12
.flash.text,-36,-36
.debug_ranges,0,-40
.debug_line,0,-987

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

sections,vmsize,filesize
.debug_info,0,37588
.debug_abbrev,0,2619
.debug_str,0,428
.strtab,0,48
.debug_ranges,0,-8
.debug_loc,0,-55
.debug_line,0,-160

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

sections,vmsize,filesize
.debug_info,0,1016768
.debug_abbrev,0,15787
.debug_line,0,314
[Unmapped],0,64
.strtab,0,1
.shstrtab,0,-1
.debug_ranges,0,-8
.flash.text,-64,-64
.debug_loc,0,-363
.debug_str,0,-1306

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

sections,vmsize,filesize
.debug_info,0,66531
.debug_abbrev,0,2897
.debug_str,0,420
[Unmapped],0,60
.debug_ranges,0,-8
.flash.text,-60,-60
.debug_line,0,-308
.debug_loc,0,-364


@bzbarsky-apple
Copy link
Contributor

So switching to class-type errors was not a problem in terms of codesize? I guess as long as CHIP_CONFIG_ERROR_SOURCE is false the codesize is fine? @kpschoedel

@kpschoedel
Copy link
Contributor Author

So switching to class-type errors was not a problem in terms of codesize? I guess as long as CHIP_CONFIG_ERROR_SOURCE is false the codesize is fine? @kpschoedel

Yes, I checked that before embarking on this conversion. With all the relevant things inline the code for a class-wrapped integer is generally identical to a bare one.

@mspang mspang merged commit 0fe17ec into project-chip:master Aug 5, 2021
@kpschoedel kpschoedel deleted the x8340-error-class-7 branch August 5, 2021 14:07
@kpschoedel
Copy link
Contributor Author

I'll now start monitoring PRs for the obsolete functions and submit a final cleanup when it's safe.

nikita-s-wrk pushed a commit to nikita-s-wrk/connectedhomeip that referenced this pull request Sep 23, 2021
* Add CHIP_ERROR object methods

#### Problem

Having CHIP_ERROR be a class type provides type safety and the option to
trace the source of errors. Since all platforms now use the class type,
transitional code that provided for CHIP_ERROR to be either a class or
integer type is no longer required and can be removed, which simplifies
the class and some notation for using errors.

#### Change overview

- Remove the `CHIP_CONFIG_ERROR_CLASS` configuration option.
- Add instance methods to replace the static `ChipError` functions, which
  were present to allow transitional integer overloads. To avoid
  breaking PRs in flight, the static methods remain present for now,
  marked “DO NOT USE”; they will be removed in a final cleanup PR.

#### Testing

Existing tests should confirm no change to functionality.

* Rebase and regen to fix ZAP/Android after project-chip#8680

* Add assertions for bit fields
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