Separate System::Layer and Timer into per-implementation files. #8800

kpschoedel · 2021-08-04T19:54:53Z

Problem

Various implementations of System::Layer are provided by #if sections,
which prevents mock versions for testing and out-of-tree platform
implementations.

Change overview

Split platform #if sections of System::Layer and System::Timer
into separate WatchableEventManager implementations.

This change does for timers what PR #6561 did for socket I/O events.
The intent is that the public WatchableEventManager interface here
will become pure virtual methods of System::Layer, and the various
versions of WatchableEventManager will become its implementations.

System::Timer remains as an Object-pool based utility class for
implementing the Timer API.

Part of issue #5556 rework system layer event loop
Part of issue #7715 Virtualize System and Inet interfaces
Fixes #7757 Integrate timer platform details with WatchableEventManager.

Testing

Relying on CI in general, since no externally visible functionality
should have changed. Sanity-checked the libevent implementation (not in
CI) using chip-tool.

#### Problem Various implementations of System::Layer are provided by `#if` sections, which prevents mock versions for testing and out-of-tree platform implementations. #### Change overview Split platform `#if` sections of `System::Layer` and `System::Timer` into separate `WatchableEventManager` implementations. This change does for timers what PR project-chip#6561 did for socket I/O events. The intent is that the public `WatchableEventManager` interface here will become pure virtual methods of `System::Layer`, and the various versions of `WatchableEventManager` will become its implementations. `System::Timer` remains as an `Object`-pool based utility class for implementing the Timer API. - Make `Timer` list not LwIP-specific, mutex it, and use it in preference to iterating through the entire pool. - Move `IsEarlier` from `Timer` to `Clock`. - Fix dual definitions of `Clock::Platform` functions on Linux. #### Testing Relying on CI in general, since no externally visible functionality should have changed. Sanity-checked the libevent implementation (not in CI) using chip-tool.

mspang

There seems to be some debug logs left in here.

src/system/WatchableEventManagerSelect.cpp

src/include/platform/internal/GenericPlatformManagerImpl.cpp

src/include/platform/PlatformManager.h

src/include/platform/internal/GenericPlatformManagerImpl_FreeRTOS.cpp

src/platform/SystemEventSupport.cpp

src/system/SystemTimer.h

src/system/SystemMutex.h

- add config flag CHIP_SYSTEM_CONFIG_USE_TIMER_POOL - use std::lock_guard rather than inventing ScopedMutexLock - rename mWatchableEvents and WatchableEvents()

github-actions · 2021-08-09T22:38:43Z

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

File	Section	File	VM
chip-shell.elf	.flash.text	328	328
chip-shell.elf	.dram0.bss	0	88
chip-shell.elf	.iram0.text	52	52
chip-lock-app.elf	.flash.text	240	240
chip-lock-app.elf	.dram0.bss	0	88
chip-lock-app.elf	.iram0.text	52	52
chip-bridge-app.elf	.flash.text	292	292
chip-bridge-app.elf	.dram0.bss	0	88
chip-bridge-app.elf	.iram0.text	52	52
chip-temperature-measurement-app.elf	.flash.text	220	220
chip-temperature-measurement-app.elf	.dram0.bss	0	96
chip-temperature-measurement-app.elf	.iram0.text	40	40
chip-temperature-measurement-app.elf	.flash.rodata	8	8

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,22975
.debug_str,0,7971
.debug_line,0,865
.debug_abbrev,0,756
.debug_loc,0,-3

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

sections,vmsize,filesize
.debug_info,0,423923
.debug_line,0,33627
.debug_abbrev,0,28574
.debug_str,0,5381
.debug_loc,0,1543
.strtab,0,693
.debug_ranges,0,352
.debug_frame,0,344
.flash.text,328,328
.symtab,0,192
.debug_aranges,0,152
.dram0.bss,88,0
.xt.prop._ZN4chip6System5Mutex6UnlockEv,0,76
.xt.prop._ZNSt8_Rb_treeIhSt4pairIKhN4chip18OptionalQRCodeInfoEESt10_Select1stIS4_ESt4lessIhESaIS4_EE14_M_create_nodeIJRKS4_EEEPSt13_Rb_tree_nodeIS4_EDpOT_$isra$67,0,76
.iram0.text,52,52
.shstrtab,0,47
[1 Others],0,40
.xt.lit.chipDie,0,-48
.xt.prop._ZNSt8_Rb_treeIhSt4pairIKhN4chip18OptionalQRCodeInfoEESt10_Select1stIS4_ESt4lessIhESaIS4_EE14_M_create_nodeIJRKS4_EEEPSt13_Rb_tree_nodeIS4_EDpOT_$isra$68,0,-76
.xt.prop.chipDie,0,-76
[Unmapped],0,-380

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

sections,vmsize,filesize
.debug_info,0,22975
.debug_str,0,7983
.debug_line,0,865
.debug_abbrev,0,756
.debug_loc,0,5

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

sections,vmsize,filesize
.debug_info,0,1749794
.debug_abbrev,0,81030
.debug_line,0,69181
.debug_str,0,8991
.debug_loc,0,1716
.strtab,0,658
.debug_ranges,0,328
.debug_frame,0,280
.flash.text,240,240
.debug_aranges,0,112
.symtab,0,112
.xt.prop._ZSt34__uninitialized_move_if_noexcept_aIPN4chip18OptionalQRCodeInfoES2_SaIS1_EET0_T_S5_S4_RT1_$isra$130,0,100
.dram0.bss,88,0
.xt.prop._ZNSt8_Rb_treeIhSt4pairIKhN4chip18OptionalQRCodeInfoEESt10_Select1stIS4_ESt4lessIhESaIS4_EE14_M_create_nodeIJRKS4_EEEPSt13_Rb_tree_nodeIS4_EDpOT_$isra$72,0,76
.iram0.text,52,52
[1 Others],0,-34
.xt.lit.chipDie,0,-48
.xt.prop._ZNSt8_Rb_treeIhSt4pairIKhN4chip18OptionalQRCodeInfoEESt10_Select1stIS4_ESt4lessIhESaIS4_EE14_M_create_nodeIJRKS4_EEEPSt13_Rb_tree_nodeIS4_EDpOT_$isra$73,0,-76
.xt.prop.chipDie,0,-76
.xt.prop._ZSt34__uninitialized_move_if_noexcept_aIPN4chip18OptionalQRCodeInfoES2_SaIS1_EET0_T_S5_S4_RT1_$isra$138,0,-100
[Unmapped],0,-292

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

sections,vmsize,filesize
.debug_info,0,761304
.debug_line,0,53458
.debug_abbrev,0,47867
.debug_str,0,4986
.debug_loc,0,1433
.strtab,0,658
.debug_ranges,0,328
.flash.text,292,292
.debug_frame,0,280
.debug_aranges,0,112
.symtab,0,112
.dram0.bss,88,0
.iram0.text,52,52
.shstrtab,0,-34
.xt.lit.chipDie,0,-48
.xt.prop.chipDie,0,-76
[Unmapped],0,-344

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

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

sections,vmsize,filesize
.debug_info,0,31204
.debug_str,0,4454
.debug_line,0,2446
.debug_abbrev,0,2129
.debug_loc,0,-5

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

sections,vmsize,filesize
.debug_info,0,743859
.debug_line,0,52029
.debug_abbrev,0,46769
.debug_str,0,4987
.debug_loc,0,1680
.strtab,0,658
.debug_ranges,0,312
.debug_frame,0,280
.flash.text,220,220
.debug_aranges,0,112
.symtab,0,112
.dram0.bss,96,0
.iram0.text,40,40
.flash.rodata,8,8
.shstrtab,0,-30
.xt.lit.chipDie,0,-48
.xt.prop.chipDie,0,-76
[Unmapped],0,-268

github-actions · 2021-08-09T22:39:13Z

Size increase report for "nrfconnect-example-build" from f446fd2

File	Section	File	VM
chip-shell.elf	text	212	212
chip-shell.elf	bss	0	152
chip-shell.elf	[LOAD #3 [RW]]	0	8
chip-shell.elf	device_handles	-4	-4
chip-lock.elf	text	184	184
chip-lock.elf	bss	0	160
chip-lock.elf	device_handles	8	8

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

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

sections,vmsize,filesize
.debug_info,0,341594
.debug_line,0,23940
.debug_abbrev,0,12338
.debug_str,0,2381
.debug_loc,0,1643
.strtab,0,448
.debug_ranges,0,424
.debug_frame,0,216
text,212,212
bss,152,0
.symtab,0,144
.debug_aranges,0,88
[LOAD #3 [RW]],8,0
device_handles,-4,-4

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

sections,vmsize,filesize
.debug_info,0,608678
.debug_line,0,43797
.debug_abbrev,0,21729
.debug_str,0,2381
.debug_loc,0,1847
.strtab,0,465
.debug_ranges,0,424
.debug_frame,0,216
text,184,184
bss,160,0
.symtab,0,144
.debug_aranges,0,88
device_handles,8,8
.shstrtab,0,-1

github-actions · 2021-08-09T22:40:31Z

Size increase report for "gn_qpg-example-build" from f446fd2

File	Section	File	VM
chip-qpg6100-lighting-example.out	.text	336	336
chip-qpg6100-lighting-example.out	.bss	0	8
chip-qpg6100-lighting-example.out	.heap	0	-8

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

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'

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

sections,vmsize,filesize
.debug_info,0,509261
.debug_line,0,34038
.debug_abbrev,0,21517
.debug_str,0,2691
.debug_loc,0,1525
.strtab,0,744
.text,336,336
.symtab,0,320
.debug_frame,0,276
.debug_ranges,0,224
.debug_aranges,0,120
.bss,8,0
.heap,-8,0
[Unmapped],0,-336

mspang · 2021-08-10T18:47:02Z

@saurabhst @jmartinez-silabs @jelderton

#### Problem As of project-chip#8800, the `System::Timer` class is no longer an inherent part of the `System::Layer` timer implementation; it is a utility class that can be used by implementations. However, project-chip#8800 did not include any example of this kind of use, and did not adequately document `System::Timer`. #### Change overview - The libevent implementation no longer uses `System::Timer`. - Added documentation for `System::Timer`. - Moved documentation for `System::Layer` into the header. #### Testing Manual verification using chip-tool. The libevent implementation is currently provided only as an illustrative example, and is not exercised in CI.

…ect-chip#8800) * Separate System::Layer and Timer into per-implementation files. #### Problem Various implementations of System::Layer are provided by `#if` sections, which prevents mock versions for testing and out-of-tree platform implementations. #### Change overview Split platform `#if` sections of `System::Layer` and `System::Timer` into separate `WatchableEventManager` implementations. This change does for timers what PR project-chip#6561 did for socket I/O events. The intent is that the public `WatchableEventManager` interface here will become pure virtual methods of `System::Layer`, and the various versions of `WatchableEventManager` will become its implementations. `System::Timer` remains as an `Object`-pool based utility class for implementing the Timer API. - Make `Timer` list not LwIP-specific, mutex it, and use it in preference to iterating through the entire pool. - Move `IsEarlier` from `Timer` to `Clock`. - Fix dual definitions of `Clock::Platform` functions on Linux. #### Testing Relying on CI in general, since no externally visible functionality should have changed. Sanity-checked the libevent implementation (not in CI) using chip-tool. * rebase and regen zap * Remove obsolete TODO. * restyle * remove leftover debug logging * restyle * convert outdated ChipError::FormatError() * review - add config flag CHIP_SYSTEM_CONFIG_USE_TIMER_POOL - use std::lock_guard rather than inventing ScopedMutexLock - rename mWatchableEvents and WatchableEvents()

boring-cyborg bot added ble darwin examples inet linux platform system labels Aug 4, 2021

pullapprove bot requested review from chrisdecenzo, jelderton, jmartinez-silabs, LuDuda, mspang, pan-apple, saurabhst, shana-apple and woody-apple August 4, 2021 19:55

pullapprove bot added the review - pending label Aug 4, 2021

andy31415 added the SDK Approved label Aug 4, 2021

kpschoedel added 2 commits August 4, 2021 18:20

rebase and regen zap

786e5fd

kpschoedel force-pushed the x7725-system-event-5 branch from 148b67d to 786e5fd Compare August 4, 2021 22:23

boring-cyborg bot added the controller label Aug 4, 2021

kpschoedel added 3 commits August 5, 2021 10:12

Merge remote-tracking branch 'origin/master' into x7725-system-event-5

ada442b

Merge remote-tracking branch 'origin/master' into x7725-system-event-5

88a5bd5

Remove obsolete TODO.

b9c8a85

restyled-io bot mentioned this pull request Aug 5, 2021

Restyle Separate System::Layer and Timer into per-implementation files. #8815

Closed

restyle

7280ed5

mspang approved these changes Aug 6, 2021

View reviewed changes

src/system/WatchableEventManagerSelect.cpp Outdated Show resolved Hide resolved

remove leftover debug logging

3e8753a

restyled-io bot mentioned this pull request Aug 6, 2021

Restyle Separate System::Layer and Timer into per-implementation files. #8835

Closed

kpschoedel added 2 commits August 6, 2021 10:01

restyle

c2f9c5e

convert outdated ChipError::FormatError()

95e84bc