@cladmi, I added some more notes in the PR description. It should simplify the review. Let me know if you see some application missing in the list.

All the application using USE_ETHOS_FOR_STDIO also must be updated.

I am re-running the tests.

All the application using USE_ETHOS_FOR_STDIO also must be updated.

Except tests/riotboot_flashwrite, they are all pulling-in the shell and thus stdio_rx, so not needed. I'll just update this one then.

@aabadie Then the dependency is not set, it is implicitly given by the shell which is against the code that says "if you set USE_ETHOS_FOR_STDIO you should require stdio_uart_rx.

You even asked about it ;) https://github.com/RIOT-OS/RIOT/pull/11310/files#r274874695 and I agree with you.

@cladmi @kaspar030, thanks for your initial reviews. I've updated this PR following your comments (stdio_rx -> stdin + added stdin dependency to missing applications).
How about adding stdin as a dependency of ethos ? I think it would be simpler and some macro checks could be removed.

You even asked about it ;) https://github.com/RIOT-OS/RIOT/pull/11310/files#r274874695 and I agree with you.

Sorry, I thought you were replying to @kaspar030... So I pushed some commits to address this. Should be good now. I tested gnrc_border_router and it works.

How about adding stdin as a dependency of ethos ? I think it would be simpler and some macro checks could be removed.

ethos itself doesn't need stdin, only if used also as stdio. It is totally valid to e.g., use stdio_rtt and then ethos on some other uart. We'd need a (pseudo-)module like "stdio_ethos".

We'd need a (pseudo-)module like "stdio_ethos".

Exactly what I did ;)

Now I'm wondering if ethos could be a dependency of stdio_ethos

Now I'm wondering if ethos could be a dependency of stdio_ethos

That would make sense!

That would make sense!

Done!

@cladmi, are you ok with the changes ?

On my side the tests look good now. However that pull request grew quite big for a simple fix and may better be split.

Right now the decription and testing procedure is not adapted anymore to all the changes.

Right now the decription and testing procedure is not adapted anymore to all the changes.

And what do you propose to move forward fast ?

1. Fixup of the applications that need stdio_uart
2. In parallel, introduce the stdio_ethos module with an stdio_uart_rx dependency
3. Then introduce stin with documentation also in stdio_base.h to say the module is required for having an input

Otherwise, to move forward fast, revert the original PR and take time to do it step by step so there is not need to go fast :)

to move forward fast, revert the original PR and take time to do it step by step so there is not need to go fast

This is nonsense: "to move forward fast, take your time".
Reverting the original PR would also break something useful (PM on STM32) and I'm happy that #11594 wasn't merged.
When #11310 was merged, it was difficult to evaluate that applications using getchar would have problems afterwards.

The current state of this PR fixes everything in one go. If you want to move fast, just ACK and merge this one. I can split out the tes application if you want.

An alternative (to move fast) could also to just add stdio_uart_rx to applications requiring it (as done in the 3 first commits), but it will leave the codebase in a non satisfactory state.

When you reported #11525, it would have been better if you provide the list of impacted applications precisely, since you seem to know it already, I have never seen this list, except here.

I can rework the history of this PR if you agree, and maybe move the test application in its own PR.

Maybe the best would be to just return EOF if MODULE_STDIN is not there, instead of -ENOTSUP.

The return code definitely has to be EOF. It is the only documented error case for getchar() AFAIK.

This tells the caller, who is supposed to check the value returned by getchar/getc/..., that there's an error. The documentation tells him what to do (USEMODULE += stdin).

I think the test applications that use getchar should have a check if the character read is EOF or not.

I don't think so. The check is basically never needed (in RIOT) when stdin is available, and it would only trigger for an application where getchar never works. So using getchar() with stdin disabled is actually a compile-time error.

The return code definitely has to be EOF. It is the only documented error case for getchar() AFAIK.

https://pubs.opengroup.org/onlinepubs/9699919799/functions/getchar.html refers to https://pubs.opengroup.org/onlinepubs/9699919799/functions/fgetc.html:

[…] If a read error occurs, the error indicator for the stream shall be set, fgetc() shall return EOF, and shall set errno to indicate the error.

Possible errnos are listet at the link. Either put it in the errno variable directly or the newlib reentrant (see #8619). [edit: the scheduler should do that... sorry only had one coffee yet]

From http://pubs.opengroup.org/onlinepubs/9699919799.2018edition/basedefs/stdio.h.html:

The <stdio.h> header shall define the following macro which shall expand to an integer constant expression with type int and a negative value:

EOF End-of-file return value.

I don't think stdio_read is supposed to return EOF directly, this should be done is syscall.c related function. For example, in newlib syscalls, one has _read_r: here the errno can be set to the _reent variable and EOF returned if stdio_read return value is < 0 (see vfs version for example).

But then look at other getchar implementation, e.g. atmega, msp430 they should be changed to correctly return EOF when stdio_read fails.

IMHO, this should be done in follow-up PRs and it needs a tracking issue. What do you think ?

edit: added links to the code

I don't think stdio_read is supposed to return EOF directly, this should be done is syscall.c related function.

Ah, right. Newlib's getchar implementation should handle EOF and errno handling. -ENOTSUP should be fine.

-ENOTSUP should be fine.

From the list of standard errors listed here, I would vote for EIO or ENXIO instead. ENOTSUP is not listed.

@kaspar030, let's move forward on this one.

So using getchar() with stdin disabled is actually a compile-time error.

Do you think of something like this ?

#ifdef MODULE_STDIN
ssize_t stdio_read(void* buffer, size_t count)
{
    return (ssize_t)isrpipe_read(&stdio_uart_isrpipe, buffer, count);
}
#endif

I noticed that tests/lua_loader was also broken because of this.
#11686

When #11310 was merged, it was difficult to evaluate that applications using getchar would have problems afterwards.

Removing that input works by default, should trigger checking what uses input.

The current state of this PR fixes everything in one go. If you want to move fast, just ACK and merge this one. I can split out the tes application if you want.

An alternative (to move fast) could also to just add stdio_uart_rx to applications requiring it (as done in the 3 first commits), but it will leave the codebase in a non satisfactory state.

No does not work, I noticed it must only be added if stdio_uart is used.
Adding it would blacklist boards using stdio_rtt…

EDIT: added output as reference

make --no-print-directory -C tests/xtimer_usleep info-boards-supported | wc -w
154

And get less when using the module

make --no-print-directory -C tests/xtimer_usleep info-boards-supported | wc -w
151

with this diff

diff --git a/tests/xtimer_usleep/Makefile b/tests/xtimer_usleep/Makefile
index a51966755..5cef84b3b 100644
--- a/tests/xtimer_usleep/Makefile
+++ b/tests/xtimer_usleep/Makefile
@@ -11,4 +11,6 @@ TEST_ON_CI_WHITELIST += samr21-xpro
 #CFLAGS += -DSLEEP_PIN=7
 #CFLAGS += -DSLEEP_PORT=PORT_F
 
+USEMODULE += stdio_uart_rx
+
 include $(RIOTBASE)/Makefile.include

When you reported #11525, it would have been better if you provide the list of impacted applications precisely, since you seem to know it already, I have never seen this list, except here.

If it was not clear from the original PR what can be impacted and how to solve it. Then the PR was not ready to be merged.

If it was not clear from the original PR what can be impacted and how to solve it. Then the PR was not ready to be merged.

#11525 is not a PR but the issue you opened initially.

Adding it would blacklist boards using stdio_rtt…

Now it doesn't. Because applications requiring inputs load the stdin module which itself only requires stdio_uart_rx if stdio_uart module is loaded.
There's no rx restriction with stdio_rtt.

I noticed that tests/lua_loader was also broken

I pushed a commit to fix this.

@cladmi, have you read #11598 (comment) before re-reviewing ?

ping @cladmi

there's a typo (expect -> expects), feel free to squash directly!

Fixed, rebased and squashed!

&go.