Skip to content

cpu/lpc23xx: enable support for picolibc#14830

Merged
aabadie merged 2 commits intoRIOT-OS:masterfrom
benpicco:cpu/lpc23xx-picolibc
Sep 8, 2020
Merged

cpu/lpc23xx: enable support for picolibc#14830
aabadie merged 2 commits intoRIOT-OS:masterfrom
benpicco:cpu/lpc23xx-picolibc

Conversation

@benpicco
Copy link
Contributor

@benpicco benpicco commented Aug 23, 2020

Contribution description

lpc23xx works with the same ARM targeted picolibc as our Cortex-M targets. The linker script also looks almost the same, so adding support for TLS is just copy & paste.

So all that's left to do is enable support for picolibc in the build system.

Testing procedure

Only tried tests/thread_cooperation, examples/default and tests/malloc. All worked, but tests/malloc would only use the first heap segment as multi-heap support is not implemented yet in the picolibc support code.

  • tests/thread_cooperation:

newlibc

   text	   data	    bss	    dec	    hex	filename
  19068	    152	  19268	  38488	   9658	/home/benpicco/dev/RIOT/tests/thread_cooperation/bin/mcb2388/tests_thread_cooperation.elf

picolibc

   text	   data	    bss	    dec	    hex	filename
   9680	     52	  19336	  29068	   718c	/home/benpicco/dev/RIOT/tests/thread_cooperation/bin/mcb2388/tests_thread_cooperation.elf
  • examples/default:

newlibc

  text	   data	    bss	    dec	    hex	filename
  33776	    148	   7152	  41076	   a074	/home/benpicco/dev/RIOT/examples/default/bin/mcb2388/default.elf

picolibc

   text	   data	    bss	    dec	    hex	filename
  21432	     48	   7224	  28704	   7020	/home/benpicco/dev/RIOT/examples/default/bin/mcb2388/default.elf

Issues/PRs references

depends on #14827

@benpicco benpicco requested a review from maribu August 23, 2020 11:46
@benpicco benpicco requested a review from kaspar030 as a code owner August 23, 2020 11:46
@benpicco benpicco added Area: cpu Area: CPU/MCU ports Type: new feature The issue requests / The PR implemements a new feature for RIOT State: waiting for other PR State: The PR requires another PR to be merged first labels Aug 23, 2020
@benpicco benpicco force-pushed the cpu/lpc23xx-picolibc branch from ca93bc8 to 727e924 Compare August 24, 2020 16:08
@benpicco benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed State: waiting for other PR State: The PR requires another PR to be merged first labels Aug 24, 2020
# use the nano-specs of Newlib when available
USEMODULE += newlib_nano

ifeq (1,$(PICOLIBC))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we using a variable here? Shouldn't it be

ifeq (picolibc newlib,$(USEMODULE))
# whatever default you want to choose
endif

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just copied that from cpu/cortexm_common/Makefile.dep :)

Do you mean picolibc should be selected by USEMODULE += picolibc instead of PICOLIBC=1?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I think that would be better, I understand it is unrelated with this PR but it would be good to do this cleanup.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we do that as a follow-up?
The RISC-V port also uses this variable.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that this would be nicer but I think that this should be done in a follow-up, with all affected CPUs changed at once (cortexm, fe310 and lpc23xx).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There you go #15011

Copy link
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

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

I trust @benpicco's testing and code changes are OK. Would you mind opening a follow-up PR that replaces the PICOLIBC variable usage by a check on USEMODULE instead ?

ACK

@aabadie aabadie merged commit 9eef7a4 into RIOT-OS:master Sep 8, 2020
@benpicco benpicco deleted the cpu/lpc23xx-picolibc branch September 8, 2020 08:35
@aabadie aabadie self-assigned this Sep 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: cpu Area: CPU/MCU ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: new feature The issue requests / The PR implemements a new feature for RIOT

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants