sys/byteorder: clean up implementation by maribu · Pull Request #20313 · RIOT-OS/RIOT

maribu · 2024-01-30T14:51:24Z

Contribution description

This changes the implementation to be solely build upon endian.h and unaligned.h.

This turns byteorder.h basically in syntactic sugar on top of the <endian.h> API, reducing the complexity of the implementation and, hence, the maintenance effort.

Note that yields a small ROM reduction as well yeah!

make BOARD=nrf52840dk RIOT_CI_BUILD=1 BUILD_IN_DOCKER=1 -C tests/unittests

Yields before this commit:

   text	  data	   bss	   dec	   hex	filename
 417788	  2200	 28640	448628	 6d874	/data/riotbuild/riotbase/tests/unittests/bin/nrf52840dk/tests_unittests.elf

And with this commit:

   text	  data	   bss	   dec	   hex	filename
 417756	  2200	 28640	448596	 6d854	/data/riotbuild/riotbase/tests/unittests/bin/nrf52840dk/tests_unittests.elf

Update

This uncovered some bugs in <endian.h> that were not detected by the unit tests, as the unit test did not cover the magic constants but only that the right implementation was chosen. Due to the preprocessor handling undifined macros as 0 in #if directives, the correct implementation on little endian system was chosen anyway.

In addition to fixing the bug, the unit test was extended, so that the same bug would be detected now.

Testing procedure

The unit tests in core cover sys/byteorder (historically, byteorder.h was once in core):

make BOARD=nrf52840dk BUILD_IN_DOCKER=1 -C tests/unittests tests-core flash test -j

make: Entering directory '/home/maribu/Repos/software/RIOT/byteorder/tests/unittests'
Launching build container using image "docker.io/riot/riotbuild:latest".
podman run --rm --tty --userns keep-id -v '/etc/zoneinfo/Europe/Berlin:/etc/localtime:ro' -v '/home/maribu/Repos/software/RIOT/byteorder:/data/riotbuild/riotbase:delegated' -v '/home/maribu/.cargo/registry:/data/riotbuild/.cargo/registry:delegated' -v '/home/maribu/.cargo/git:/data/riotbuild/.cargo/git:delegated' -e 'RIOTBASE=/data/riotbuild/riotbase' -e 'CCACHE_BASEDIR=/data/riotbuild/riotbase' -v '/home/maribu/.cache/RIOT:/data/riotbuild/build:delegated' -e 'BUILD_DIR=/data/riotbuild/build' -e 'RIOTPROJECT=/data/riotbuild/riotbase' -e 'RIOTCPU=/data/riotbuild/riotbase/cpu' -e 'RIOTBOARD=/data/riotbuild/riotbase/boards' -e 'RIOTMAKE=/data/riotbuild/riotbase/makefiles'    -v '/home/maribu/Repos/software/boards/riot:/data/riotbuild/external/riot:delegated' -v '/home/maribu/Repos/software/miot-pcbs/RIOT/boards:/data/riotbuild/external/boards:delegated' -v '/home/maribu/Repos/software/RIOT/master/.git:/home/maribu/Repos/software/RIOT/master/.git:delegated' -e 'BOARD=nrf52840dk' -e 'DISABLE_MODULE=auto_init auto_init_%' -e 'DEFAULT_MODULE=test_utils_interactive_sync test_utils_print_stack_usage' -e 'FEATURES_REQUIRED=' -e 'FEATURES_BLACKLIST=highlevel_stdio' -e 'FEATURES_OPTIONAL=' -e 'USEMODULE=core_mbox embunit' -e 'USEPKG='  -w '/data/riotbuild/riotbase/tests/unittests/' 'docker.io/riot/riotbuild:latest' make 'BOARD=nrf52840dk'  'EXTERNAL_BOARD_DIRS=/data/riotbuild/external/riot /data/riotbuild/external/boards' -j tests-core
Building application "tests_unittests" for "nrf52840dk" with MCU "nrf52".

"make" -C /data/riotbuild/riotbase/pkg/cmsis/ 
"make" -C /data/riotbuild/riotbase/boards/common/init
"make" -C /data/riotbuild/riotbase/boards/nrf52840dk
"make" -C /data/riotbuild/riotbase/core
"make" -C /data/riotbuild/riotbase/core/lib
"make" -C /data/riotbuild/riotbase/cpu/nrf52
"make" -C /data/riotbuild/riotbase/drivers
"make" -C /data/riotbuild/riotbase/sys
"make" -C /data/riotbuild/riotbase/tests/unittests/tests-core
"make" -C /data/riotbuild/riotbase/drivers/periph_common
"make" -C /data/riotbuild/riotbase/boards/common/nrf52xxxdk
"make" -C /data/riotbuild/riotbase/sys/div
"make" -C /data/riotbuild/riotbase/cpu/cortexm_common
"make" -C /data/riotbuild/riotbase/sys/embunit
"make" -C /data/riotbuild/riotbase/cpu/nrf52/periph
"make" -C /data/riotbuild/riotbase/sys/isrpipe
"make" -C /data/riotbuild/riotbase/cpu/nrf52/vectors
"make" -C /data/riotbuild/riotbase/sys/libc
"make" -C /data/riotbuild/riotbase/cpu/nrf5x_common
"make" -C /data/riotbuild/riotbase/sys/malloc_thread_safe
"make" -C /data/riotbuild/riotbase/sys/newlib_syscalls_default
"make" -C /data/riotbuild/riotbase/sys/stdio_uart
"make" -C /data/riotbuild/riotbase/sys/test_utils/interactive_sync
"make" -C /data/riotbuild/riotbase/sys/test_utils/print_stack_usage
"make" -C /data/riotbuild/riotbase/sys/tsrb
"make" -C /data/riotbuild/riotbase/cpu/nrf5x_common/periph
"make" -C /data/riotbuild/riotbase/cpu/cortexm_common/periph
   text	  data	   bss	   dec	   hex	filename
  29216	   188	  4144	 33548	  830c	/data/riotbuild/riotbase/tests/unittests/bin/nrf52840dk/tests_unittests.elf
/home/maribu/Repos/software/RIOT/byteorder/dist/tools/openocd/openocd.sh flash /home/maribu/Repos/software/RIOT/byteorder/tests/unittests/bin/nrf52840dk/tests_unittests.elf
### Flashing Target ###
Open On-Chip Debugger 0.12.0+dev-snapshot (2024-01-17-08:38)
Licensed under GNU GPL v2
For bug reports, read
	http://openocd.org/doc/doxygen/bugs.html
DEPRECATED! use 'adapter serial' not 'jlink serial'
swd
Info : J-Link OB-SAM3U128-V2-NordicSemi compiled Nov  7 2022 16:21:57
Info : Hardware version: 1.00
Info : VTarget = 3.300 V
Info : clock speed 1000 kHz
Info : SWD DPIDR 0x2ba01477
Info : [nrf52.cpu] Cortex-M4 r0p1 processor detected
Info : [nrf52.cpu] target has 6 breakpoints, 4 watchpoints
Info : [nrf52.cpu] Examination succeed
Info : starting gdb server for nrf52.cpu on 0
Info : Listening on port 38937 for gdb connections
    TargetName         Type       Endian TapName            State       
--  ------------------ ---------- ------ ------------------ ------------
 0* nrf52.cpu          cortex_m   little nrf52.cpu          unknown
[nrf52.cpu] halted due to debug-request, current mode: Thread 
xPSR: 0x01000000 pc: 0x000009ec msp: 0x20000200
Info : nRF52840-xxAA(build code: AA) 1024kB Flash, 256kB RAM
Warn : Adding extra erase range, 0x000072dc .. 0x00007fff
auto erase enabled
wrote 29404 bytes from file /home/maribu/Repos/software/RIOT/byteorder/tests/unittests/bin/nrf52840dk/tests_unittests.elf in 1.205779s (23.814 KiB/s)
verified 29404 bytes in 0.099144s (289.628 KiB/s)
shutdown command invoked
Done flashing
r
Due to limitations in the gcoap API it is currently not possible to use a dual stack setup
/home/maribu/Repos/software/RIOT/byteorder/dist/tools/pyterm/pyterm -p "/dev/ttyACM1" -b "115200" --no-reconnect --noprefix --no-repeat-command-on-empty-line 
Connect to serial port /dev/ttyACM1
Welcome to pyterm!
Type '/exit' to exit.
READY
s
START
..........................................................................................................
OK (106 tests)

make: Leaving directory '/home/maribu/Repos/software/RIOT/byteorder/tests/unittests'

tl;dr

READY
s
START
..........................................................................................................
OK (106 tests)

Issues/PRs references

None

Teufelchen1

🧼 🧽 Clean! ✨

sys/include/byteorder.h

riot-ci · 2024-01-30T18:05:50Z

Murdock results

✔️ PASSED

ebfaa36 pkg/tinydtls: fix conflict with endian.h

Success	Failures	Total	Runtime
8626	0	8627	09m:18s

Artifacts

Documentation preview

maribu · 2024-01-31T07:08:50Z

This has revealed some bugs in <endian.h>, which have been fixed and unit tests have been added that would have caught them. Should I split the fixes out into a separate PR, or should I just update the PR description?

Teufelchen1

I think this can stay in one PR. Just a whitespace nit left.

sys/libc/include/endian.h

The script to fix the vendor header files has been renamed to `fix_headers.sh` and now does two things: 1. Strip bogus type qualifiers in front of padding (as before) 2. Strip bogus `LITTLE_ENDIAN` defines.

Ran the `fix_headers.sh` to fix the vendor header files and removed the no longer needed work around for them.

The constants BIG_ENDIAN etc. were not consistently defined. This bug was not caught by the unit tests, as the preprocessor would treat undefined macros as being `0` in numeric comparisons, hence the following test did select the correct implementation: ```C #if BYTE_ORDER == LITTLE_ENDIAN /* correct implementation for all currently supported boards */ #endif ``` This adds now an explicit test to the unit tests to ensure that the magic numbers are consistently the correct values. Hence, this bug should no longer be able to sneak in. Co-authored-by: Teufelchen <[email protected]>

There is no need to use `__uint16_t` instead of `uint16_t` etc., so let's just go with `uint16_t` and have AVR GCC happy.

Older versions of newlib already provide the magic endian numbers via `machine/endian.h`, which may be indirectly included. This changes the header to only provide the macros if the are not provided otherwise. For sanity, it checks if the values are indeed the expected magic numbers, even if provided from other sources.

maribu · 2024-01-31T13:52:53Z

@benpicco Could you take a look at the first two commits of this PR?

I dropped the work around for the LITTLE_ENDIAN macro in the vendor header and just stripped that macro from the header files. We already are sanitizing the header files with a shell script for C++ compatibility, so I just extended (and renamed) that script to also handle that issue.

Move the <endian.h> header from `sys/libc/include` to `sys/include`.

maribu · 2024-01-31T19:25:44Z

OK, this has been quite a journey, but everything is green now. I reorganized the commits into a logically sensible order. Everything should be ready for another round of review.

Teufelchen1

LGTM, did not run any tests, I assume the CI is enough.
(the timing of your request makes it look like I speed-ed through it in less than 3 minutes, not the case)

This changes the implementation to be solely build upon `endian.h` and `unaligned.h`. This turns `byteorder.h` basically in syntactic sugar on top of the `<endian.h>` API, reducing the complexity of the implementation and, hence, the maintenance effort. Note that yields a small ROM reduction as well *yeah!* ``` make BOARD=nrf52840dk RIOT_CI_BUILD=1 BUILD_IN_DOCKER=1 -C tests/unittests ``` Yields before this commit: ``` text data bss dec hex filename 417788 2200 28640 448628 6d874 /data/riotbuild/riotbase/tests/unittests/bin/nrf52840dk/tests_unittests.elf ``` And with this commit: ``` text data bss dec hex filename 417756 2200 28640 448596 6d854 /data/riotbuild/riotbase/tests/unittests/bin/nrf52840dk/tests_unittests.elf ```

maribu · 2024-02-02T05:38:29Z

This time it was just a CI glitch...

maribu · 2024-02-02T08:07:22Z

Woohooo! 🎉

Thanks for the review! :)

github-actions bot added the Area: sys Area: System label Jan 30, 2024

Teufelchen1 approved these changes Jan 30, 2024

View reviewed changes

benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 30, 2024

mguetschow reviewed Jan 30, 2024

View reviewed changes

sys/include/byteorder.h Show resolved Hide resolved

maribu force-pushed the sys/byteorder/cleanup-implementation branch from 43f844d to 4a7097a Compare January 30, 2024 18:39

maribu requested a review from leandrolanzieri as a code owner January 30, 2024 21:29

github-actions bot added the Area: pkg Area: External package ports label Jan 30, 2024

maribu force-pushed the sys/byteorder/cleanup-implementation branch from 0384d19 to 83a0c79 Compare January 31, 2024 07:00

maribu requested a review from miri64 as a code owner January 31, 2024 07:00

github-actions bot added Area: tests Area: tests and testing framework and removed Area: pkg Area: External package ports labels Jan 31, 2024

maribu force-pushed the sys/byteorder/cleanup-implementation branch from 83a0c79 to 1e8337e Compare January 31, 2024 07:12

Teufelchen1 requested changes Jan 31, 2024

View reviewed changes

sys/libc/include/endian.h Outdated Show resolved Hide resolved

maribu force-pushed the sys/byteorder/cleanup-implementation branch 2 times, most recently from 8fdc667 to 16ebc02 Compare January 31, 2024 13:14

maribu and others added 4 commits January 31, 2024 14:46

cpu/sam0_common: Update script to fix vendor headers

1425dc0

The script to fix the vendor header files has been renamed to `fix_headers.sh` and now does two things: 1. Strip bogus type qualifiers in front of padding (as before) 2. Strip bogus `LITTLE_ENDIAN` defines.

cpu/sam0_common: fix vendor header files

b42b199

Ran the `fix_headers.sh` to fix the vendor header files and removed the no longer needed work around for them.

sys/libc/endian: fix compilation on AVR

0d01356

There is no need to use `__uint16_t` instead of `uint16_t` etc., so let's just go with `uint16_t` and have AVR GCC happy.

maribu force-pushed the sys/byteorder/cleanup-implementation branch from 16ebc02 to 2328a32 Compare January 31, 2024 13:46

maribu requested review from benpicco, dylad and keestux as code owners January 31, 2024 13:46

github-actions bot added Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: doc Area: Documentation Area: cpu Area: CPU/MCU ports labels Jan 31, 2024

maribu force-pushed the sys/byteorder/cleanup-implementation branch from 2328a32 to ad67d24 Compare January 31, 2024 13:50

github-actions bot added the Area: pkg Area: External package ports label Jan 31, 2024

sys/endian: move form sys/libc

9723a8d

Move the <endian.h> header from `sys/libc/include` to `sys/include`.

maribu force-pushed the sys/byteorder/cleanup-implementation branch from a307f15 to 944ce26 Compare January 31, 2024 19:14

maribu requested a review from Teufelchen1 February 1, 2024 10:52

Teufelchen1 approved these changes Feb 1, 2024

View reviewed changes

maribu added this pull request to the merge queue Feb 1, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 1, 2024

maribu added 3 commits February 1, 2024 18:24

pkg/esp8266_sdk: make use of <endian.h>

48b2552

pkg/tinydtls: fix conflict with endian.h

ebfaa36

maribu force-pushed the sys/byteorder/cleanup-implementation branch from 944ce26 to ebfaa36 Compare February 1, 2024 17:25

maribu enabled auto-merge February 1, 2024 17:25

maribu added this pull request to the merge queue Feb 1, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 1, 2024

maribu added this pull request to the merge queue Feb 2, 2024

Merged via the queue into RIOT-OS:master with commit 3b3da09 Feb 2, 2024

maribu deleted the sys/byteorder/cleanup-implementation branch February 2, 2024 08:07

MrKevinWeiss added this to the Release 2024.04 milestone Apr 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sys/byteorder: clean up implementation#20313

sys/byteorder: clean up implementation#20313
maribu merged 9 commits intoRIOT-OS:masterfrom
maribu:sys/byteorder/cleanup-implementation

maribu commented Jan 30, 2024 •

edited

Loading

Uh oh!

Teufelchen1 left a comment

Uh oh!

Uh oh!

riot-ci commented Jan 30, 2024 •

edited

Loading

Uh oh!

maribu commented Jan 31, 2024

Uh oh!

Teufelchen1 left a comment

Uh oh!

Uh oh!

maribu commented Jan 31, 2024

Uh oh!

maribu commented Jan 31, 2024

Uh oh!

Teufelchen1 left a comment •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

maribu commented Feb 2, 2024

Uh oh!

maribu commented Feb 2, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

maribu commented Jan 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Update

Testing procedure

tl;dr

Issues/PRs references

Uh oh!

Teufelchen1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

riot-ci commented Jan 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Murdock results

Artifacts

Uh oh!

maribu commented Jan 31, 2024

Uh oh!

Teufelchen1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maribu commented Jan 31, 2024

Uh oh!

maribu commented Jan 31, 2024

Uh oh!

Teufelchen1 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

maribu commented Feb 2, 2024

Uh oh!

maribu commented Feb 2, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

maribu commented Jan 30, 2024 •

edited

Loading

riot-ci commented Jan 30, 2024 •

edited

Loading

Teufelchen1 left a comment •

edited

Loading