Restructure CHIP_ERROR numeric values #8179

kpschoedel · 2021-07-07T22:26:08Z

Problem

Large constants hurt code density on small processors, and CHIP_ERROR
constants are very common.

Summary of Changes

Changed the internal structure of CHIP_ERROR numbers so that commonly
used constants are small. The new representation uses bit fields managed
by a helper class, ::chip::ChipError.

Several examples have been using CHIP_CONFIG_CORE_ERROR_MAX as a
single application-level error code. Since this no longer exists, and
the new representation makes it straightforward, a portion of the
CHIP_ERROR space is now explicitly made available for SDK users.

Converted remaining uses of int for CHIP_ERROR values.

Changed error constant definitions to hexadecimal to make them easier to find
given the bit field representation.

Testing

No changes to functionality. Manual sanity test with chip-tool and
all-clusters-app.

bzbarsky-apple · 2021-07-08T03:21:47Z

@kpschoedel @andreilitvin Any idea why we're not getting codesize numbers here?

bzbarsky-apple · 2021-07-08T03:37:31Z

Any idea why we're not getting codesize numbers here?

Probably because of the thing #8189 is trying to fix: no completed bloat checks in the last 13 hours!

github-actions · 2021-07-08T05:29:23Z

Size increase report for "nrfconnect-example-build" from 8b5b24f

File	Section	File	VM
chip-lock.elf	device_handles	8	8
chip-lock.elf	rodata	-8	-8
chip-lock.elf	text	-1464	-1464
chip-shell.elf	device_handles	-4	-4
chip-shell.elf	rodata	-8	-8
chip-shell.elf	text	-476	-476

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

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

sections,vmsize,filesize
.debug_info,0,100235
.debug_abbrev,0,9218
.debug_line,0,951
.debug_str,0,742
.debug_ranges,0,280
.debug_loc,0,154
device_handles,8,8
.debug_frame,0,-4
.debug_aranges,0,-8
rodata,-8,-8
.symtab,0,-64
text,-1464,-1464

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

sections,vmsize,filesize
.debug_info,0,49803
.debug_abbrev,0,5395
.debug_line,0,1418
.debug_str,0,742
.debug_loc,0,602
.debug_ranges,0,136
device_handles,-4,-4
.debug_aranges,0,-8
.debug_frame,0,-8
rodata,-8,-8
.symtab,0,-32
text,-476,-476

github-actions · 2021-07-08T05:29:55Z

Size increase report for "esp32-example-build" from 8b5b24f

File	Section	File	VM
chip-persistent-storage.elf	.flash.text	-20	-20

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:

BLOAT EXECUTION FAILED WITH CODE 1:
bloaty: integer overflow

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

sections,vmsize,filesize
.debug_info,0,3214
.debug_str,0,797
.debug_abbrev,0,445
.debug_loc,0,29
[Unmapped],0,20
.debug_ranges,0,-16
.flash.text,-20,-20
.debug_line,0,-229

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

sections,vmsize,filesize
.debug_str,0,1030
.debug_info,0,677
.debug_abbrev,0,36
.debug_loc,0,-1
.debug_line,0,-66

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

BLOAT EXECUTION FAILED WITH CODE 1:
bloaty: integer overflow

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

BLOAT EXECUTION FAILED WITH CODE 1:
bloaty: integer overflow

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

BLOAT EXECUTION FAILED WITH CODE 1:
bloaty: integer overflow

github-actions · 2021-07-08T05:30:36Z

Size increase report for "gn_qpg-example-build" from 8b5b24f

File	Section	File	VM
chip-qpg6100-lighting-example.out	.text	-2896	-2896

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-qpg6100-lighting-example.out and ./pull_artifact/chip-qpg6100-lighting-example.out:

sections,vmsize,filesize
.debug_info,0,98115
.debug_abbrev,0,7738
[Unmapped],0,2896
.debug_str,0,707
.debug_line,0,413
.debug_ranges,0,232
.debug_aranges,0,-16
.debug_frame,0,-48
.debug_loc,0,-1941
.text,-2896,-2896
.symtab,0,-4128

Comparing ./master_artifact/chip-qpg6100-lighting-example.out.map and ./pull_artifact/chip-qpg6100-lighting-example.out.map:

BLOAT EXECUTION FAILED WITH CODE 1:
bloaty: unknown file type for file './pull_artifact/chip-qpg6100-lighting-example.out.map'

andy31415 · 2021-07-08T13:34:25Z

@kpschoedel - it looks like darwin/Framwork/CHIP/ChipError.h redefines CHIP_ERROR :(

@sagar-apple - any ideas how to resolve this? Eventually on large platforms (darwin, linux, ios, android) we would want CHIP_ERROR to be a class, so we ca attach line information of origin to them. This would require a 'single definition' place.

mspang · 2021-07-08T20:18:57Z

examples/platform/esp32/shell_extension/heap_trace.cpp

 {
    heap_trace_dump();
    streamer_printf(streamer_get(), "Free heap %d/%d\n", heap_caps_get_free_size(MALLOC_CAP_8BIT),
                    heap_caps_get_total_size(MALLOC_CAP_8BIT));
-    return 0;
+    return CHIP_ERROR;


CHIP_NO_ERROR?

(I guess this isn't being compiled?)

sagar-apple · 2021-07-08T21:58:30Z

@kpschoedel - it looks like darwin/Framwork/CHIP/ChipError.h redefines CHIP_ERROR :(

@sagar-apple - any ideas how to resolve this? Eventually on large platforms (darwin, linux, ios, android) we would want CHIP_ERROR to be a class, so we ca attach line information of origin to them. This would require a 'single definition' place.

CHIPError is already a class in the darwin Framework but as you've pointed out, that class doesn't map to the underlying CHIP_ERROR correctly. This is a known problem. I'm unsure if there's a straightforward solution here. Errors that we want to allow consumers of the SDK to use(for case handling and such) need to be exposed as part of the CHIPErrorDomain in ObjC, afaik that involves a redefining them (maybe we can declare the enum values to map to CHIP_ERROR values?). For all other error types, we already just map them to "unknown" and print out the underlying CHIP_ERROR in the description.

As a start if we can define a underlying object for CHIP_ERROR with description and code information, I think we should be able to rewrite the ObjC CHIPError to just more directly wrap the underlying CHIP_ERROR objects while updating the values in the enum in ObjC to be equal to the raw CHIP_ERROR ones.

kpschoedel · 2021-07-09T17:34:50Z

For this particular PR I think we can just make the Darwin integer typedef match (assuming there are no more lurking signed/unsigned issues).

andy31415 · 2021-07-09T19:41:18Z

@kpschoedel - do we need the qpg6100 changes in this PR?

kpschoedel · 2021-07-09T19:43:31Z

@kpschoedel - do we need the qpg6100 changes in this PR?

No, it's just that git and I hate each other.

bzbarsky-apple · 2021-07-09T19:37:21Z

src/lib/core/CHIPError.h

+    /// Construct a `CHIP_ERROR` encapsulating @a value inside the @a range.
+    static BaseType Encapsulate(Range range, BaseType value)
+    {
+        return MakeInteger(range, (value & MakeMask(kValueStart, kValueLength)));


So this is relying on kValueStart being 0, right? Otherwise I'd expect to see:

Suggested change

return MakeInteger(range, (value & MakeMask(kValueStart, kValueLength)));

return MakeInteger(range, (value & MakeMask(0, kValueLength)));

Otherwise you are effectively left-shifting by kValueLength after taking the bits that are already shifted by kValueLength,

Yes, good catch.

bzbarsky-apple · 2021-07-09T19:39:12Z

src/lib/core/CHIPError.h

+    static constexpr BaseType MakeInteger(SdkPart part, BaseType code)
+    {
+        return MakeInteger(Range::kSDK,
+                           MakeField(kSdkPartStart, static_cast<std::underlying_type<SdkPart>::type>(part)) |


Suggested change

MakeField(kSdkPartStart, static_cast<std::underlying_type<SdkPart>::type>(part)) |

MakeField(kSdkPartStart, to_underlying(part)) |

after including lib/support/TypeTraits.h

Thanks, that's a nice addition.

bzbarsky-apple · 2021-07-09T19:40:57Z

src/lib/core/CHIPError.h

+    template <SdkPart part, BaseType code>
+    struct MakeSdkErrorConstant
+    {
+        static_assert(FitsInField(kSdkPartLength, static_cast<std::underlying_type<SdkPart>::type>(part)), "part is too large");


Suggested change

static_assert(FitsInField(kSdkPartLength, static_cast<std::underlying_type<SdkPart>::type>(part)), "part is too large");

static_assert(FitsInField(kSdkPartLength, to_underlying(part)), "part is too large");

bzbarsky-apple · 2021-07-09T19:48:38Z

src/system/SystemError.cpp

-    // CHIP_SYSTEM_LWIP_ERROR_MAX all fits inside err_t.  See static_assert in
-    // MapErrorLwIP.
-    const err_t lError = static_cast<err_t>(-lErrorWithoutOffset);
+    const err_t lError = static_cast<err_t>(-ChipError::GetValue(aError));


Suggested change

const err_t lError = static_cast<err_t>(-ChipError::GetValue(aError));

const err_t lError = -static_cast<err_t>(ChipError::GetValue(aError));

to undo the operations we did in MapErrorLwIP, where we negated, then cast.

#### Problem Large constants hurt code density on small processors, and `CHIP_ERROR` constants are very common. #### Summary of Changes The internal details of `CHIP_ERROR` numbers change so that commonly used constants are small. The new representation uses bit fields managed by a helper class, `::chip::ChipError`. Several examples have been using `CHIP_CONFIG_CORE_ERROR_MAX` as a single application-level error code. Since this no longer exists, and the new representation makes it straightforward, a portion of the `CHIP_ERROR` space is now explicitly available for SDK users. #### Testing No changes to functionality. Manual sanity test with chip-tool and all-clusters-app.

* Restructure CHIP_ERROR numeric values #### Problem Large constants hurt code density on small processors, and `CHIP_ERROR` constants are very common. #### Summary of Changes The internal details of `CHIP_ERROR` numbers change so that commonly used constants are small. The new representation uses bit fields managed by a helper class, `::chip::ChipError`. Several examples have been using `CHIP_CONFIG_CORE_ERROR_MAX` as a single application-level error code. Since this no longer exists, and the new representation makes it straightforward, a portion of the `CHIP_ERROR` space is now explicitly available for SDK users. #### Testing No changes to functionality. Manual sanity test with chip-tool and all-clusters-app. * fix examples/platform/esp32/shell_extension/heap_trace.cpp * Darwin typedef * review * zap regen

boring-cyborg bot added the examples label Jul 7, 2021

pullapprove bot requested review from andy31415, bzbarsky-apple, chrisdecenzo, Damian-Nordic, hawk248, jepenven-silabs and msandstedt July 7, 2021 22:29

pullapprove bot added the review - pending label Jul 7, 2021

msandstedt approved these changes Jul 8, 2021

View reviewed changes

andy31415 approved these changes Jul 8, 2021

View reviewed changes

mspang reviewed Jul 8, 2021

View reviewed changes

kpschoedel force-pushed the error-bitmask branch from 79badad to 90ba798 Compare July 9, 2021 19:45

bzbarsky-apple reviewed Jul 9, 2021

View reviewed changes

bzbarsky-apple approved these changes Jul 9, 2021

View reviewed changes

pullapprove bot added review - approved and removed review - pending labels Jul 9, 2021

kpschoedel added 3 commits July 9, 2021 15:57

fix examples/platform/esp32/shell_extension/heap_trace.cpp

1a45fdf

Darwin typedef

29cbf4b

review

8b573f3

kpschoedel force-pushed the error-bitmask branch from 90ba798 to 8b573f3 Compare July 9, 2021 20:10

kpschoedel mentioned this pull request Jul 10, 2021

Remove error erasures #8272

Merged

zap regen

d21fa14

mspang approved these changes Jul 12, 2021

View reviewed changes

mspang merged commit 6ed99f4 into project-chip:master Jul 12, 2021

kpschoedel deleted the error-bitmask branch July 12, 2021 20:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restructure CHIP_ERROR numeric values #8179

Restructure CHIP_ERROR numeric values #8179

kpschoedel commented Jul 7, 2021

bzbarsky-apple commented Jul 8, 2021

bzbarsky-apple commented Jul 8, 2021

github-actions bot commented Jul 8, 2021

github-actions bot commented Jul 8, 2021

github-actions bot commented Jul 8, 2021

andy31415 commented Jul 8, 2021 •

edited

Loading

mspang Jul 8, 2021 •

edited

Loading

sagar-apple commented Jul 8, 2021

kpschoedel commented Jul 9, 2021

andy31415 commented Jul 9, 2021

kpschoedel commented Jul 9, 2021

bzbarsky-apple Jul 9, 2021

kpschoedel Jul 9, 2021

bzbarsky-apple Jul 9, 2021

kpschoedel Jul 9, 2021

bzbarsky-apple Jul 9, 2021

bzbarsky-apple Jul 9, 2021

kpschoedel Jul 9, 2021

	return MakeInteger(range, (value & MakeMask(kValueStart, kValueLength)));
	return MakeInteger(range, (value & MakeMask(0, kValueLength)));

	MakeField(kSdkPartStart, static_cast<std::underlying_type<SdkPart>::type>(part)) \|
	MakeField(kSdkPartStart, to_underlying(part)) \|

	static_assert(FitsInField(kSdkPartLength, static_cast<std::underlying_type<SdkPart>::type>(part)), "part is too large");
	static_assert(FitsInField(kSdkPartLength, to_underlying(part)), "part is too large");

	const err_t lError = static_cast<err_t>(-ChipError::GetValue(aError));
	const err_t lError = -static_cast<err_t>(ChipError::GetValue(aError));

Restructure CHIP_ERROR numeric values #8179

Restructure CHIP_ERROR numeric values #8179

Conversation

kpschoedel commented Jul 7, 2021

Problem

Summary of Changes

Testing

bzbarsky-apple commented Jul 8, 2021

bzbarsky-apple commented Jul 8, 2021

github-actions bot commented Jul 8, 2021

github-actions bot commented Jul 8, 2021

github-actions bot commented Jul 8, 2021

andy31415 commented Jul 8, 2021 • edited Loading

mspang Jul 8, 2021 • edited Loading

Choose a reason for hiding this comment

sagar-apple commented Jul 8, 2021

kpschoedel commented Jul 9, 2021

andy31415 commented Jul 9, 2021

kpschoedel commented Jul 9, 2021

bzbarsky-apple Jul 9, 2021

Choose a reason for hiding this comment

kpschoedel Jul 9, 2021

Choose a reason for hiding this comment

bzbarsky-apple Jul 9, 2021

Choose a reason for hiding this comment

kpschoedel Jul 9, 2021

Choose a reason for hiding this comment

bzbarsky-apple Jul 9, 2021

Choose a reason for hiding this comment

bzbarsky-apple Jul 9, 2021

Choose a reason for hiding this comment

kpschoedel Jul 9, 2021

Choose a reason for hiding this comment

andy31415 commented Jul 8, 2021 •

edited

Loading

mspang Jul 8, 2021 •

edited

Loading