sys/shell: ensure character is flushed when echoing.

[jcarrano]

Contribution description

When using a serial terminal without local echo, the current line would not get updated as the user typed because the shell module's readline() was not flushing each character.

This commit fixes that behavior. For additional clarity, fflush is turned into a macro (flush_if_needed) which expands to either a call to fflush() or empty, according to the standard library used.

This also fixes the erase/line editing behavior (the delete characters were not being flushed either.)

Testing procedure

Test that the shell is not broken by running

make -C tests/shell all flash
make -C tests/shell test

To test echo, I used miniterm.py (from pyserial) but any terminal with raw mode and no local echo should do:

$ miniterm.py --raw --eol CRLF /dev/ttyACM0 115200

Type a command. I used the 'test/shell' application.

Without this patch: you won't see anything on the screen until you type enter.

With this patch: you will see characters show up as you type. If you press backspace you will see the characters disappear too.

References

Mentioned by @neiljay in the mailing list some months ago: https://lists.riot-os.org/pipermail/devel/2018-July/005823.html

[leandrolanzieri]

Tested and now the echo works, also erasing characters.

[leandrolanzieri]

Why not defining this in the _putchar function if it is only needed with newlib?

[jcarrano]

Because we don't need to flush after every character. I do think, however, that it would be better if stdio was in unbuffered mode: it fits better with the HW.

[kaspar030]

This fix was long overdue. thx!

Please remove the macro though.

[jcarrano]

I force pushed the fix (no point in a fixup commit)

[MrKevinWeiss]

@kaspar030 Is it good to go?

[kaspar030]

Let me do some tests

[MrKevinWeiss]

Tested with pyterm on nucleo-l476rg and it seems good.

[MrKevinWeiss]

miniterm is a bit funky though, let's wait a bit.
copy paste error, it echos until 128 chars then freezes and requires a reset. Expected behaviour I guess...

[kaspar030]

I tested on nrf52dk using pyterm and picocom. I was curious if any tests might break when there's a possible double echo, but it seems fine. Let's also run the CI tests on samr21.
When those also pass, I'm happy here.

[kaspar030]

miniterm is a bit funky though, let's wait a bit.

maybe quickly add a change request, so noone gets trigger happy?

[kaspar030]

At least on the samr21-xpro all tests succeeded.

[jcarrano]

copy paste error, it echos until 128 chars then freezes and requires a reset. Expected behaviour I guess...

mmm, weird. In any case, it should not be affected by this change. Is it a thing with the terminal or with RIOT? To be sure the terminal is not messing things up, I always test with:

• raw encoding
• CRLF line endings (because that's what riot uses)
• No local echo, no local buffering

[MrKevinWeiss]

I used your miniterm setup

miniterm.py --raw --eol CRLF /dev/ttyACM0 115200
> main(): This is RIOT! (Version: 2018.10-RC1-597-gb34dc-tstxxx/10630)
test_shell.
> 00000000111111112222222233333333444444445555555566666666777777778888888899999999aaaaaaaabbbbbbbbccccccccddddddddeeeeeeeefffffff

sorry 127 bytes, I guess it gets full, I can't delete or press enter. It doesn't crash... Ideally it would discard the last byte and evaluate the new one.

I don't think it is part of this PR though.

[jcarrano]

@MrKevinWeiss see #10639 .

[MrKevinWeiss]

I am not sending at once, these are just typed characters (at the speed of me).