sys: stdio_null: add null driver by basilfx · Pull Request #10741 · RIOT-OS/RIOT

basilfx · 2019-01-09T22:46:27Z

Contribution description

This PR provides a null driver for STDIO, so that it doesn't depend on UART or RTT.

I created this to test #10215, where the bootloader easily exceeds the default 4KiB on EFM32 targets, because due to periph_uart still being included (which includes other big stuff). With this null driver, I could save an additional 2KiB. This makes sense for something like a bootloader, but anything that depends on reading from UART will break.

I've added a warning with DEVELHELP=1, because I assume you enabled it to show some output.

It took me five minutes to create, so if this doesn't make any sense, that I don't feel bad ;-)

Testing procedure

Add USEMODULE=stdio_null to any application to see what it saves. In particular, add this to bootloaders/riotboot/Makefile and observe that tests/riotboot still works.

Here is an example for tests/minimal.

As usual:

basilfx:minimal/ (feature/stdio_null) $ BOARD=samr21-xpro make -j8
Building application "tests_minimal" for "samr21-xpro" with MCU "samd21".

"make" -C boards/samr21-xpro
"make" -C core
"make" -C cpu/samd21
"make" -C drivers
"make" -C sys
"make" -C cpu/cortexm_common
"make" -C cpu/sam0_common
"make" -C cpu/samd21/periph
"make" -C sys/isrpipe
"make" -C drivers/periph_common
"make" -C sys/newlib_syscalls_default
"make" -C cpu/sam0_common/periph
"make" -C cpu/cortexm_common/periph
"make" -C sys/pm_layered
"make" -C sys/stdio_uart
"make" -C sys/tsrb
   text	   data	    bss	    dec	    hex	filename
   3260	     36	   2604	   5900	   170c	tests/minimal/bin/samr21-xpro/tests_minimal.elf

With USEMODULE=stdio_null:

basilfx:minimal/ (feature/stdio_null) $ USEMODULE=stdio_null BOARD=samr21-xpro make -j8
Building application "tests_minimal" for "samr21-xpro" with MCU "samd21".

"make" -C boards/samr21-xpro
"make" -C core
"make" -C cpu/samd21
"make" -C drivers
"make" -C sys
"make" -C cpu/cortexm_common
"make" -C drivers/periph_common
"make" -C sys/isrpipe
"make" -C cpu/sam0_common
"make" -C cpu/sam0_common/periph
"make" -C sys/newlib_syscalls_default
"make" -C sys/pm_layered
"make" -C cpu/samd21/periph
"make" -C sys/stdio_null
"make" -C sys/stdio_uart
"make" -C sys/tsrb
"make" -C cpu/cortexm_common/periph
   text	   data	    bss	    dec	    hex	filename
   2232	     16	   2524	   4772	   12a4	tests/minimal/bin/samr21-xpro/tests_minimal.elf

With DEVELHELP=1 USEMODULE=stdio_null:

basilfx:minimal/ (feature/stdio_null) $ DEVELHELP=1 USEMODULE=stdio_null BOARD=samr21-xpro make -j8
Building application "tests_minimal" for "samr21-xpro" with MCU "samd21".

"make" -C boards/samr21-xpro
"make" -C core
"make" -C cpu/samd21
"make" -C drivers
"make" -C sys
"make" -C cpu/cortexm_common
"make" -C cpu/sam0_common
"make" -C cpu/sam0_common/periph
"make" -C sys/newlib_syscalls_default
"make" -C drivers/periph_common
"make" -C cpu/samd21/periph
"make" -C sys/pm_layered
"make" -C cpu/cortexm_common/periph
"make" -C sys/stdio_null
Makefile:2: STDIO disabled via stdio_null, but DEVELHELP enabled
   text    data     bss     dec     hex filename
  10088     120    2532   12740    31c4 tests/minimal/bin/samr21-xpro/tests_minimal.elf

Issues/PRs references

#9503
#10215

sys/stdio_null/stdio_null.c

miri64 · 2019-01-10T09:31:18Z

I don't understand. Why not just exclude the stdio_uart module?

basilfx · 2019-01-10T10:41:17Z

That produces the following error (using origin/master):

basilfx:minimal/ (master) $ DISABLE_MODULE=stdio_uart BOARD=slstk3402a make -j4
Required modules were disabled using DISABLE_MODULE: stdio_uart


EXPECT ERRORS!


tests/minimal/../../Makefile.include:736: tests/minimal/bin/slstk3402a/stdio_uart.a
tests/minimal/../../Makefile.include:738: *** BASELIBS value changed.  Stop.

There is a hard dependency on stdio_init():

RIOT/sys/newlib_syscalls_default/syscalls.c

Line 72 in aa2de01

stdio_init();

miri64 · 2019-01-10T10:42:23Z

There is a hard dependency on stdio_init():

RIOT/sys/newlib_syscalls_default/syscalls.c

Line 72 in aa2de01

stdio_init();

Is it possible to remove that dependency instead?

basilfx · 2019-01-10T10:49:45Z

Is it possible to remove that dependency instead?

Probably. But I don't know the impact on existing applications. First of all, vfs_bind_stdio() is still invoked from stdio_init(), so I guess it will break any application that writes to file descriptor 0, 1 or 3.

To me, this optional driver is similar to telling an application to write to /dev/null, without changing the application to remove everything related to reading/writing to stdio.

It's a self-contained alternative. Otherwise we would have to do a lot of #ifdefs anywhere we use stdio_%.

kaspar030 · 2019-01-10T11:04:56Z

Is it possible to remove that dependency instead?

"I have a branch(tm)". Disabling stdio completely needs some ground work. I'll start PR'ing.

miri64 · 2019-01-10T11:31:08Z

Probably. But I don't know the impact on existing applications. First of all, vfs_bind_stdio() is still invoked from stdio_init(), so I guess it will break any application that writes to file descriptor 0, 1 or 3.

To me, this optional driver is similar to telling an application to write to /dev/null, without changing the application to remove everything related to reading/writing to stdio.

It's a self-contained alternative. Otherwise we would have to do a lot of #ifdefs anywhere we use stdio_%.

Ok, right. As an optional alternative I am fine with this approach. Nevertheless, we should investigate if an alternative without any dependency to stdio (but without #ifdefs around every printf()) is viable.

kaspar030 · 2019-01-10T11:34:42Z

Nevertheless, we should investigate if an alternative without any dependency to stdio (but without #ifdefs around every printf()) is viable.

Well, one way is to use LOG_ macros instead of printf, as they can be disabled completely. Dropping stdio_uart and periph/uart goes a long way, but being able to also drop newlib's stdio code has the most wins...

+1 for adding this, even if the only user is the bootloader on EFM32.

basilfx · 2019-01-10T13:34:54Z

I fixed the comments by @smlng and I also committed 43adad8 to ensure Makefile.dep doesn't compile stdio_uart whenever stdio_rtt or stdio_null is selected. That was still the case.

jcarrano · 2019-01-14T17:06:41Z

I added commit to #9258 (this one) that provides this same functionality, but without adding a null driver. Instead I intercept read() and write() (actually, the reentrant versions). I think it is important to be honest about the fact that it is a stub and set error codes appropriately.

Space savings are similar, but I need to rebase my branch on top of yours to do a proper comparison.

danpetry · 2019-01-16T11:33:55Z

Personally I think that we should look at working on a full solution rather than a quick fix, unless this is urgently needed by anyone other than @basilfx ...

basilfx · 2019-01-16T12:53:39Z

I would need it to get the boot loader fit. We could also replace it later on.

stale · 2019-08-10T02:08:15Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

kaspar030 · 2019-11-19T10:39:00Z

@basilfx would you rebase pls?

basilfx · 2019-11-19T12:01:06Z

@kaspar030 Done! I also updated the PR description after retesting. Still seems to work ;-)

kaspar030 · 2019-11-19T14:10:44Z

Here is an example for tests/minimal.

Could you add this module to tests/minimal, so it is actually compiled at least somewhere?

kaspar030 · 2019-11-19T14:11:12Z

Could you add this module to tests/minimal, so it is actually compiled at least somewhere?

or to riotboot?

basilfx · 2019-11-19T17:32:02Z

Added it to tests/minimal. Also took the liberty of squashing the PR.

basilfx:minimal/ (feature/stdio_null) $ BOARD=samr21-xpro make -j8
Building application "tests_minimal" for "samr21-xpro" with MCU "samd21".

"make" -C boards/samr21-xpro
"make" -C core
"make" -C cpu/samd21
"make" -C drivers
"make" -C sys
"make" -C cpu/cortexm_common
"make" -C cpu/sam0_common
"make" -C cpu/samd21/periph
"make" -C drivers/periph_common
"make" -C sys/newlib_syscalls_default
"make" -C sys/pm_layered
"make" -C cpu/cortexm_common/periph
"make" -C cpu/sam0_common/periph
"make" -C sys/stdio_null
   text    data     bss     dec     hex filename
   2236      16    2524    4776    12a8 tests/minimal/bin/samr21-xpro/tests_minimal.elf

basilfx · 2019-11-19T19:28:16Z

I'll create a separate PR (#12744) to isolate that last commit.

aabadie · 2019-11-22T17:53:37Z

#12768 has just been merged so Murdock should pass now (I retriggered it).

basilfx · 2019-11-26T20:48:21Z

Rebased. Murdock is happy. I think the comments by @smlng have been resolved some time ago already :-)

comments addressed

aabadie

Looks good and the CI is green. Let's go with this one.

ACK and go!

basilfx added Type: new feature The issue requests / The PR implemements a new feature for RIOT CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jan 9, 2019

smlng previously requested changes Jan 10, 2019

View reviewed changes

sys/stdio_null/stdio_null.c Outdated Show resolved Hide resolved

sys/stdio_null/stdio_null.c Outdated Show resolved Hide resolved

kaspar030 mentioned this pull request Jan 10, 2019

make: handle disabled default modules before dependencies #10744

Merged

basilfx force-pushed the feature/stdio_null branch from 04f15bd to 43adad8 Compare January 10, 2019 13:31

stale bot added the State: stale State: The issue / PR has no activity for >185 days label Aug 10, 2019

stale bot closed this Sep 10, 2019

kaspar030 mentioned this pull request Nov 18, 2019

sys/stdio: add stdio_none #12740

Closed

kaspar030 reopened this Nov 19, 2019

stale bot removed the State: stale State: The issue / PR has no activity for >185 days label Nov 19, 2019

basilfx force-pushed the feature/stdio_null branch from 43adad8 to 103a50d Compare November 19, 2019 11:59

basilfx force-pushed the feature/stdio_null branch 2 times, most recently from 830c695 to 2dff32c Compare November 19, 2019 17:30

basilfx mentioned this pull request Nov 19, 2019

boards: pic32-clicker: check for periph_uart #12744

Closed

basilfx force-pushed the feature/stdio_null branch from dc96484 to 2dff32c Compare November 19, 2019 19:39

aabadie added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Nov 22, 2019

basilfx added 2 commits November 26, 2019 21:12

sys: stdio_null: add null driver

810a069

tests: minimal: use stdio_null

05bdab8

basilfx force-pushed the feature/stdio_null branch from 2dff32c to 05bdab8 Compare November 26, 2019 20:13

aabadie approved these changes Dec 3, 2019

View reviewed changes

aabadie merged commit bd254df into RIOT-OS:master Dec 3, 2019

fjmolinas added this to the Release 2020.01 milestone Dec 13, 2019

miri64 mentioned this pull request Jan 17, 2020

boards: add support for nrf52840-dongle #13146

Merged

8 tasks

basilfx deleted the feature/stdio_null branch October 20, 2020 19:01

Conversation

basilfx commented Jan 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

Uh oh!

Uh oh!

miri64 commented Jan 10, 2019

Uh oh!

basilfx commented Jan 10, 2019

Uh oh!

miri64 commented Jan 10, 2019

Uh oh!

basilfx commented Jan 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kaspar030 commented Jan 10, 2019

Uh oh!

miri64 commented Jan 10, 2019

Uh oh!

kaspar030 commented Jan 10, 2019

Uh oh!

basilfx commented Jan 10, 2019

Uh oh!

jcarrano commented Jan 14, 2019

Uh oh!

danpetry commented Jan 16, 2019

Uh oh!

basilfx commented Jan 16, 2019

Uh oh!

stale bot commented Aug 10, 2019

Uh oh!

kaspar030 commented Nov 19, 2019

Uh oh!

basilfx commented Nov 19, 2019

Uh oh!

kaspar030 commented Nov 19, 2019

Uh oh!

kaspar030 commented Nov 19, 2019

Uh oh!

basilfx commented Nov 19, 2019

Uh oh!

basilfx commented Nov 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aabadie commented Nov 22, 2019

Uh oh!

basilfx commented Nov 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aabadie left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

basilfx commented Jan 9, 2019 •

edited

Loading

basilfx commented Jan 10, 2019 •

edited

Loading

basilfx commented Nov 19, 2019 •

edited

Loading

basilfx commented Nov 26, 2019 •

edited

Loading