Remove one kind of Timer #8488

kpschoedel · 2021-07-19T23:10:07Z

Problem

System::Layer has two functionally equivalent kinds of timer,
which is a maintenance burden. One of them is very little
used, and not currently supported on all platforms.

Change overview

Remove the kind of timer that takes a Callback<>.

Testing

Revised the unit test. The changed calls are both in ESP32,
which is tested in CI using QEMU.

kpschoedel · 2021-07-19T23:11:11Z

If the consensus is that the Callback style is better, I'd rather convert all the System::Timer uses than keep both.

Damian-Nordic

While I see some pros of the Callback-based timers (they allow to statically allocate memory for the callback and hence guarantee that some critical operations will be executed), I've never understood why the code for processing them is so different than the timers allocated from the common pool, and the Callback class seems too heavy for the job. Thus I support the change 👍

src/platform/ESP32/BLEManagerImpl.h

src/platform/ESP32/bluedroid/BLEManagerImpl.cpp

#### Problem System::Layer has two functionally equivalent kinds of timer, which is a maintenance burden. One of them is very little used, and not currently supported on all platforms. #### Change overview Remove the kind of timer that takes a `Callback<>`. #### Testing Revised the unit test. The changed calls are both in ESP32, which is tested in CI using QEMU.

github-actions · 2021-07-20T19:58:10Z

Size increase report for "esp32-example-build" from 32c1393

File	Section	File	VM
chip-shell.elf	.flash.rodata	-4	-4
chip-shell.elf	.dram0.bss	0	-136
chip-shell.elf	.flash.text	-404	-404
chip-temperature-measurement-app.elf	.flash.rodata	-4	-4
chip-temperature-measurement-app.elf	.dram0.bss	0	-136
chip-temperature-measurement-app.elf	.flash.text	-436	-436
chip-lock-app.elf	.flash.rodata	-4	-4
chip-lock-app.elf	.dram0.bss	0	-136
chip-lock-app.elf	.flash.text	-432	-432
chip-all-clusters-app.elf	.dram0.bss	0	-144
chip-all-clusters-app.elf	.flash.text	-544	-544

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
[Unmapped],0,408
.flash.rodata,-4,-4
.debug_aranges,0,-56
.xt.prop._ZN4chip8Callback10Cancelable6CancelEv,0,-76
.xt.prop._ZN4chip8Callback13CallbackDeque7DequeueEPNS0_10CancelableE,0,-76
.shstrtab,0,-115
.dram0.bss,-136,0
.symtab,0,-144
.debug_frame,0,-168
.strtab,0,-329
.flash.text,-404,-404
.debug_ranges,0,-688
.debug_loc,0,-1772
.debug_str,0,-2092
.debug_line,0,-2624
.debug_abbrev,0,-6109
.debug_info,0,-56783

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

sections,vmsize,filesize
.debug_loc,0,-1
.debug_abbrev,0,-69
.debug_info,0,-906
.debug_str,0,-1276

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

sections,vmsize,filesize
.debug_loc,0,-1
.debug_abbrev,0,-69
.debug_info,0,-906
.debug_str,0,-1292

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

sections,vmsize,filesize
.flash.rodata,-4,-4
.debug_aranges,0,-56
.xt.prop._ZN4chip8Callback10Cancelable6CancelEv,0,-76
.xt.prop._ZN4chip8Callback13CallbackDeque7DequeueEPNS0_10CancelableE,0,-76
.shstrtab,0,-119
.dram0.bss,-136,0
.symtab,0,-144
.debug_frame,0,-168
.strtab,0,-329
.flash.text,-436,-436
.debug_ranges,0,-648
.debug_loc,0,-1638
.debug_str,0,-2089
.debug_line,0,-2607
[Unmapped],0,-3656
.debug_abbrev,0,-7644
.debug_info,0,-85110

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

sections,vmsize,filesize
[Unmapped],0,436
.flash.rodata,-4,-4
.debug_aranges,0,-56
.xt.prop._ZN4chip8Callback10Cancelable6CancelEv,0,-76
.xt.prop._ZN4chip8Callback13CallbackDeque7DequeueEPNS0_10CancelableE,0,-76
.shstrtab,0,-119
.dram0.bss,-136,0
.symtab,0,-144
.debug_frame,0,-168
.strtab,0,-329
.flash.text,-432,-432
.debug_ranges,0,-688
.debug_loc,0,-1698
.debug_str,0,-2084
.debug_line,0,-2624
.debug_abbrev,0,-12479
.debug_info,0,-135719

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

sections,vmsize,filesize

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

sections,vmsize,filesize
[Unmapped],0,544
.shstrtab,0,-2
.debug_aranges,0,-56
.symtab,0,-96
.dram0.bss,-144,0
.strtab,0,-278
.debug_frame,0,-304
.debug_ranges,0,-328
.flash.text,-544,-544
.debug_loc,0,-1526
.debug_str,0,-2086
.debug_line,0,-2090
.debug_abbrev,0,-9792
.debug_info,0,-114634

github-actions · 2021-07-20T19:58:23Z

Size increase report for "nrfconnect-example-build" from 32c1393

File	Section	File	VM
chip-lock.elf	device_handles	-8	-8
chip-lock.elf	[LOAD #3 [RW]]	0	-27
chip-lock.elf	bss	0	-37
chip-lock.elf	text	-296	-296
chip-shell.elf	device_handles	4	4
chip-shell.elf	bss	0	-32
chip-shell.elf	text	-292	-292

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
.shstrtab,0,3
device_handles,-8,-8
[LOAD #3 [RW]],-27,0
bss,-37,0
.debug_aranges,0,-48
.symtab,0,-144
.debug_frame,0,-168
.strtab,0,-211
text,-296,-296
.debug_ranges,0,-424
.debug_line,0,-1034
.debug_loc,0,-1584
.debug_str,0,-1840
.debug_abbrev,0,-6228
.debug_info,0,-65994

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

sections,vmsize,filesize
device_handles,4,4
.shstrtab,0,3
bss,-32,0
.debug_aranges,0,-48
.symtab,0,-144
.debug_frame,0,-168
.strtab,0,-211
text,-292,-292
.debug_ranges,0,-424
.debug_line,0,-1037
.debug_loc,0,-1601
.debug_str,0,-1827
.debug_abbrev,0,-3741
.debug_info,0,-35570

github-actions · 2021-07-20T19:58:52Z

Size increase report for "gn_qpg-example-build" from 32c1393

File	Section	File	VM
chip-qpg6100-lighting-example.out	.heap	0	48
chip-qpg6100-lighting-example.out	.bss	0	-48
chip-qpg6100-lighting-example.out	.text	-208	-208

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
[Unmapped],0,208
.heap,48,0
.shstrtab,0,-1
.debug_aranges,0,-40
.bss,-48,0
.symtab,0,-112
.strtab,0,-139
.debug_frame,0,-164
.text,-208,-208
.debug_ranges,0,-544
.debug_line,0,-1079
.debug_loc,0,-1170
.debug_str,0,-1851
.debug_abbrev,0,-6121
.debug_info,0,-64107

kpschoedel · 2021-07-21T14:30:16Z

~~Marking WIP — merging would cause a conflict with #8542.~~

Removed WIP — #8542 replaced by #8551 with no conflict.

* Remove one kind of Timer #### Problem System::Layer has two functionally equivalent kinds of timer, which is a maintenance burden. One of them is very little used, and not currently supported on all platforms. #### Change overview Remove the kind of timer that takes a `Callback<>`. #### Testing Revised the unit test. The changed calls are both in ESP32, which is tested in CI using QEMU. * review

boring-cyborg bot added esp32 platform system labels Jul 19, 2021

pullapprove bot requested review from chrisdecenzo, gerickson, jelderton, jmartinez-silabs, LuDuda, saurabhst, woody-apple and yufengwangca July 19, 2021 23:10

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

mspang approved these changes Jul 19, 2021

View reviewed changes

Damian-Nordic reviewed Jul 20, 2021

View reviewed changes

src/platform/ESP32/BLEManagerImpl.h Outdated Show resolved Hide resolved

src/platform/ESP32/bluedroid/BLEManagerImpl.cpp Outdated Show resolved Hide resolved

jmartinez-silabs approved these changes Jul 20, 2021

View reviewed changes

kpschoedel added 2 commits July 20, 2021 10:32

review

a5f7a0a

kpschoedel force-pushed the x7725-system-event-4 branch from f0a4014 to a5f7a0a Compare July 20, 2021 14:51

woody-apple added the SDK Approved label Jul 20, 2021

kpschoedel changed the title ~~Remove one kind of Timer~~ [WIP] Remove one kind of Timer Jul 21, 2021

kpschoedel changed the title ~~[WIP] Remove one kind of Timer~~ Remove one kind of Timer Jul 21, 2021

woody-apple approved these changes Jul 22, 2021

View reviewed changes

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

yufengwangca merged commit 773af5a into project-chip:master Jul 22, 2021

kpschoedel deleted the x7725-system-event-4 branch July 22, 2021 13:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove one kind of Timer #8488

Remove one kind of Timer #8488

kpschoedel commented Jul 19, 2021

kpschoedel commented Jul 19, 2021

Damian-Nordic left a comment

github-actions bot commented Jul 20, 2021

github-actions bot commented Jul 20, 2021

github-actions bot commented Jul 20, 2021

kpschoedel commented Jul 21, 2021 •

edited

Loading

Remove one kind of Timer #8488

Remove one kind of Timer #8488

Conversation

kpschoedel commented Jul 19, 2021

Problem

Change overview

Testing

kpschoedel commented Jul 19, 2021

Damian-Nordic left a comment

Choose a reason for hiding this comment

github-actions bot commented Jul 20, 2021

github-actions bot commented Jul 20, 2021

github-actions bot commented Jul 20, 2021

kpschoedel commented Jul 21, 2021 • edited Loading

kpschoedel commented Jul 21, 2021 •

edited

Loading