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.

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

core: add stdio.h to replace stdout functions with stdio_null #20872

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

benpicco
Copy link
Contributor

Contribution description

So far stdio_null would just drop the stdio backend, all the expensive stdio formatting functions were still included.

This PR replaces all calls to printf() and friends with a no-op.

Testing procedure

Build e.g. tests/minimal which uses stdio_null:

master

   text	   data	    bss	    dec	    hex	filename
   9256	    108	   1312	  10676	   29b4	/home/benpicco/dev/RIOT-2/tests/minimal/bin/same54-xpro/tests_minimal.elf

this PR

   text	   data	    bss	    dec	    hex	filename
   5140	    108	   1312	   6560	   19a0	/home/benpicco/dev/RIOT/tests/minimal/bin/same54-xpro/tests_minimal.elf

Issues/PRs references

@github-actions github-actions bot added Platform: native Platform: This PR/issue effects the native platform Area: core Area: RIOT kernel. Handle PRs marked with this with care! Area: cpu Area: CPU/MCU ports labels Sep 26, 2024
@benpicco benpicco changed the title Stdio null frontend core: add stdio.h to replace stdout functions with stdio_null Sep 26, 2024
@benpicco benpicco removed the Platform: native Platform: This PR/issue effects the native platform label Sep 26, 2024
Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

lgtm. The CI has complaints, though

@mguetschow mguetschow added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Sep 27, 2024
@riot-ci
Copy link

riot-ci commented Sep 27, 2024

Murdock results

✔️ PASSED

1280166 dist/tools/headerguards: ignore if #include_next is used

Success Failures Total Runtime
10214 0 10215 16m:42s

Artifacts

@github-actions github-actions bot added the Platform: native Platform: This PR/issue effects the native platform label Oct 1, 2024
@github-actions github-actions bot added the Area: pkg Area: External package ports label Oct 2, 2024
We can't use the same header guard if we include two files with the same name.
@github-actions github-actions bot added the Area: tools Area: Supplementary tools label Oct 2, 2024

#ifndef CORE_STDIO_H
#define CORE_STDIO_H
#include_next "stdio.h"
Copy link
Member

Choose a reason for hiding this comment

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

That's a GNU extension, AIU supported by Clang as well.

I'm not saying we shouldn't do this (I'm glad of all libc headers we can do ourselves to avoid weird choices by the used libc), but it is in direct conflict with the first bulled of the coding convention. But if we start doing it, better change the code convention (or if that's what we do get a 2-ACK dispensation), and reap the fruit by using #pragma once, rather than creating a divide between what we do and what we claim to do.

Copy link
Member

Choose a reason for hiding this comment

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

+1 for updating the coding convention to match.

We actually already have an used of #include_next in the AVR code to automatically move format string to flash and use printf_P() to save lots of RAM.

Copy link
Contributor Author

@benpicco benpicco Oct 7, 2024

Choose a reason for hiding this comment

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

I added a line to CODING_CONVENTIONS.md.
I think being implemented by both GCC and Clang is enough of a safeguard to prevent opening the gates to some crazy things.

Copy link
Contributor Author

@benpicco benpicco Oct 18, 2024

Choose a reason for hiding this comment

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

We already use #include_next in esp32, tinyusb, posix and avr8_common - is the stdio case really any different?

Copy link
Member

Choose a reason for hiding this comment

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

I'd be more lenient in the cpu directories where there might easily just not be compilers other than the CPU specific one (AIU, neither ESP32 nor AVR can be built with off-the-shelf C compilers). Similarly in the POSIX case, its use is limited to native, which AIU also comes with a severe set of limitations by construction.

As for tinyusb and POSIX, I'd guess that neither reviewer nor author took care.

Copy link
Member

Choose a reason for hiding this comment

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

AVR can use upstream GCC (and we do so). Upstream clang is rather mature, but not feature complete.

ESP does indeed need magic binary only compilers.

@benpicco benpicco requested a review from jia200x as a code owner October 7, 2024 09:53
@github-actions github-actions bot added the Area: doc Area: Documentation label Oct 7, 2024
@benpicco benpicco added the Process: needs >1 ACK Integration Process: This PR requires more than one ACK label Oct 7, 2024
@github-actions github-actions bot added the Process: missing approvals Integration Process: PR needs more ACKS (handled by action) label Oct 7, 2024
CODING_CONVENTIONS.md Outdated Show resolved Hide resolved
CODING_CONVENTIONS.md Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the Area: doc Area: Documentation label Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: core Area: RIOT kernel. Handle PRs marked with this with care! Area: cpu Area: CPU/MCU ports Area: pkg Area: External package ports Area: tools Area: Supplementary tools CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: native Platform: This PR/issue effects the native platform Process: missing approvals Integration Process: PR needs more ACKS (handled by action) Process: needs >1 ACK Integration Process: This PR requires more than one ACK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants