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.

Sign up for GitHub

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

Jump to bottom

sys/libc: add endian.h #20310

Merged
merged 2 commits into from
Jan 30, 2024
Merged

sys/libc: add endian.h #20310

merged 2 commits into from
Jan 30, 2024

Conversation

maribu
Copy link
Member

@maribu maribu commented Jan 30, 2024

Contribution description

This adds the libc endian.h header compatible with glibc / FreeBSD / NetBSD / musl / ...

It is a standardized and lean API that provides basically the same features as byteorder.h.

Testing procedure

A unit test was added:

make BOARD=nrf52840dk -C tests/unittests tests-libc flash test 
make: Entering directory '/home/maribu/Repos/software/RIOT/master/tests/unittests'
Building application "tests_unittests" for "nrf52840dk" with MCU "nrf52".

"make" -C /home/maribu/Repos/software/RIOT/master/pkg/cmsis/ 
"make" -C /home/maribu/Repos/software/RIOT/master/boards/common/init
"make" -C /home/maribu/Repos/software/RIOT/master/boards/nrf52840dk
"make" -C /home/maribu/Repos/software/RIOT/master/boards/common/nrf52xxxdk
"make" -C /home/maribu/Repos/software/RIOT/master/core
"make" -C /home/maribu/Repos/software/RIOT/master/core/lib
"make" -C /home/maribu/Repos/software/RIOT/master/cpu/nrf52
"make" -C /home/maribu/Repos/software/RIOT/master/cpu/cortexm_common
"make" -C /home/maribu/Repos/software/RIOT/master/cpu/cortexm_common/periph
"make" -C /home/maribu/Repos/software/RIOT/master/cpu/nrf52/periph
"make" -C /home/maribu/Repos/software/RIOT/master/cpu/nrf52/vectors
"make" -C /home/maribu/Repos/software/RIOT/master/cpu/nrf5x_common
"make" -C /home/maribu/Repos/software/RIOT/master/cpu/nrf5x_common/periph
"make" -C /home/maribu/Repos/software/RIOT/master/drivers
"make" -C /home/maribu/Repos/software/RIOT/master/drivers/periph_common
"make" -C /home/maribu/Repos/software/RIOT/master/sys
"make" -C /home/maribu/Repos/software/RIOT/master/sys/div
"make" -C /home/maribu/Repos/software/RIOT/master/sys/embunit
"make" -C /home/maribu/Repos/software/RIOT/master/sys/isrpipe
"make" -C /home/maribu/Repos/software/RIOT/master/sys/libc
"make" -C /home/maribu/Repos/software/RIOT/master/sys/malloc_thread_safe
"make" -C /home/maribu/Repos/software/RIOT/master/sys/newlib_syscalls_default
"make" -C /home/maribu/Repos/software/RIOT/master/sys/stdio_uart
"make" -C /home/maribu/Repos/software/RIOT/master/sys/test_utils/interactive_sync
"make" -C /home/maribu/Repos/software/RIOT/master/sys/test_utils/print_stack_usage
"make" -C /home/maribu/Repos/software/RIOT/master/sys/tsrb
"make" -C /home/maribu/Repos/software/RIOT/master/tests/unittests/tests-libc
   text	  data	   bss	   dec	   hex	filename
  10796	   120	  3248	 14164	  3754	/home/maribu/Repos/software/RIOT/master/tests/unittests/bin/nrf52840dk/tests_unittests.elf
/home/maribu/Repos/software/RIOT/master/dist/tools/openocd/openocd.sh flash /home/maribu/Repos/software/RIOT/master/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 42119 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: 0x00002464 msp: 0x20000200
Info : nRF52840-xxAA(build code: AA) 1024kB Flash, 256kB RAM
Warn : Adding extra erase range, 0x00002aa4 .. 0x00002fff
auto erase enabled
wrote 10916 bytes from file /home/maribu/Repos/software/RIOT/master/tests/unittests/bin/nrf52840dk/tests_unittests.elf in 0.518442s (20.562 KiB/s)
verified 10916 bytes in 0.059716s (178.514 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/master/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 (3 tests)

make: Leaving directory '/home/maribu/Repos/software/RIOT/master/tests/unittests'
tl;dr
READY
s
START
...
OK (3 tests)

Issues/PRs references

As follow up, the byteorder.h could be adapted to use this, which IMO would improve readability. But that may be somewhat subjective, so let's not add this here.

@maribu maribu requested a review from miri64 as a code owner January 30, 2024 09:22
@github-actions github-actions bot added Area: tests Area: tests and testing framework Area: sys Area: System labels Jan 30, 2024
@maribu
Copy link
Member Author

maribu commented Jan 30, 2024 •
edited
Loading

Regarding the design decisions. The state of support by the different C libaries used:

  • avrlibc: No endian.h
  • picolibc: Standard compliant endian.h placed in machine/endian.h
  • newlib: Non-standard compliant endian.h placed in machine/endian.h. Incompatible to the picoblic variant

So, it was just easier to provide a standard compliant one based on the builtins provided by both GCC and clang.

@maribu
sys/libc: add endian.h
68018fe 
This provides glibc, NetBSD, FreeBSD compatible endian.h header with a
lean and simple API to convert between host byte order to little endian
and big endian and the other way around.
@maribu maribu 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
@Teufelchen1
Copy link
Contributor

Teufelchen1 commented Jan 30, 2024 •
edited
Loading

Hello 🐴

Looks reasonable to me. A few questions / nitpicks:

  • test_libc_endian(void) loses your intend. Could you add a comment on why this test is needed? I would imagine something like "Check that a good working <endian.h> is included (picolib, newlib, sys/libc/inlclude/endian.h)"?
  • You state "It is a standardized and lean API that provides basically the same features as byteorder.h", so my naive question: Why can't we get rid of byteorder.h?
  • Could you fix the static test on line length?
  • And on a personal note, how did this PR come to be?

@maribu maribu mentioned this pull request Jan 30, 2024
Merged
1 task
Teufelchen1
Teufelchen1 approved these changes Jan 30, 2024
View reviewed changes
Copy link
Contributor

@Teufelchen1 Teufelchen1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@maribu
Copy link
Member Author

maribu commented Jan 30, 2024

  • test_libc_endian(void) loses your intend. Could you add a comment on why this test is needed? I would imagine something like "Check that a good working <endian.h> is included (picolib, newlib, sys/libc/inlclude/endian.h)"?

I extended and documented the unit test.

You state "It is a standardized and lean API that provides basically the same features as byteorder.h", so my naive question: Why can't we get rid of byteorder.h?

I was actually hoping to do that. But that is a bit controversial and a bit of work. But if this gets in, PRs to replace the use of byteorder.h will follow.

Note: We likely have to still ship byteorder.h for some time to come due to deprecation procedure, even if it is no longer used and there is indeed a general agreement to drop it.

In the very least, I could clean up byteorder.h a bit while keeping the API.

Could you fix the static test on line length?

It seems the fixup improving the unit test solved this already, now the static test is green :)

And on a personal note, how did this PR come to be?

While reviewing #20301 I noticed a bug in the byte order conversion. A quick peek in the related driver w5100 showed the same bug.

My first reaction was "Y R U not using byteorder.h?!?". Then I looked into byteorder.h and realized that I would also have rather free coded that instead of working with byteorder.h :D But as this has shown: Free coding even simple stuff can easily go wrong and slip through code reviews, so let's have a standard API for that.

@maribu maribu enabled auto-merge January 30, 2024 11:33
@riot-ci
Copy link

riot-ci commented Jan 30, 2024

Murdock results

✔️ PASSED

6615f47 tests/unittests: Add tests for endian.h

Success Failures Total Runtime
8629 0 8629 11m:59s

Artifacts

@maribu maribu added this pull request to the merge queue Jan 30, 2024
Merged via the queue into RIOT-OS:master with commit e085ad0 Jan 30, 2024
25 checks passed
@maribu maribu deleted the libc/endian branch January 30, 2024 14:41
@maribu
Copy link
Member Author

maribu commented Jan 30, 2024

Thx :)

@MrKevinWeiss MrKevinWeiss added this to the Release 2024.04 milestone Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Teufelchen1 Teufelchen1 Teufelchen1 approved these changes

@miri64 miri64 Awaiting requested review from miri64 miri64 is a code owner

Assignees
No one assigned
Labels
Area: sys Area: System Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Milestone
Release 2024.04
Development

Successfully merging this pull request may close these issues.
4 participants
@maribu @Teufelchen1 @riot-ci @MrKevinWeiss