sys: make uart_stdio RX optional (attempt #2) #11310

vincent-d · 2019-03-29T13:09:08Z

Contribution description

This is a rebase of #7974 that fixes conflicts. This allows to not block the low power mode if the shell is not used.

Issues/PRs references

jcarrano · 2019-04-01T13:42:00Z

If I understand this correctly, it is currently impossible to have UART RX compiled in and be able to go to low power mode, right? So if the user wants to use the RX functionality, he has no way to disable it at runtime to be able to go to sleep.

I'd rather not force the user into choosing either RX or low power. It is better to provide a run-time toggle for the RX function, or to bypass the low power block.

From #7947 I get that this is only a problem on chips that don't have a low power capable UART. In those chips, bypassing the mode blockage would mean that there is no input until the MCU wakes up again. In #7947 (comment) you describe a possible solution which I believe is a good one.

Also, I'm trying to find where in the RIOT code is the lpm block, but I cannot. Which CPUs have this limitation?

kaspar030 · 2019-04-01T13:49:03Z

If I understand this correctly, it is currently impossible to have UART RX compiled in and be able to go to low power mode, right?

Nope. On stm32, if an RX callback is configured and uart is initialized (and not powered off through uart_poweroff()), LPM is blocked such that RX still works.
The problem is that stdio_uart always initializes both RX and TX.

kaspar030 · 2019-04-01T13:50:53Z

Thanks @vincent-d for taking over!

kaspar030 · 2019-04-01T13:52:12Z

Makefile.dep

@@ -396,8 +396,20 @@ ifneq (,$(filter stdio_rtt,$(USEMODULE)))
  USEMODULE += xtimer
 endif

-ifneq (,$(filter stdio_uart,$(USEMODULE)))
+ifneq (,$(filter shell,$(USEMODULE)))
+  USEMODULE += stdio_uart_rx


this'll not work with stdio_rtt

maybe depend on an intermediate module like "stdio_input" and make that depend on stdio_uart_rx if stdio_uart is used?

just checked for stdio_uart being used, is it good enough?

kaspar030 · 2019-04-01T13:52:21Z

Makefile.dep

+  USEMODULE += stdio_uart_rx
+endif
+
+ifneq (,$(filter rtt_stdio,$(USEMODULE)))


and this is unrelated?

rebase mess I guess ;)

jcarrano · 2019-04-01T14:07:00Z

On stm32, [...]

I'm not familiar with STM32, but googling around seems to indicate that it has the ability to be woken up from the UART. If that is the case then it is something that should be addressed by the periph implementation.

LPM is blocked such that RX still works.

Ok, but what happens if the block is ignored and the device sleeps? Does anything break? Will the RX work after it wakes up again?

If it is OK for a certain application to not have RX, then it does not matter if the answer is yer or no, and ignoring the blockage is enough to fix the issue (or, rather, work around the lack of low power uart in stm32).

If the answer is "yes" (meaning after wake up the RX is still working) then ignoring the blockage, in combination with something like what @vincent-d proposes in #7947 (comment) is good too.

In both cases, removing RX is a bit of an extreme measure.

kaspar030 · 2019-04-01T14:31:39Z

I'm not familiar with STM32, but googling around seems to indicate that it has the ability to be woken up from the UART. If that is the case then it is something that should be addressed by the periph implementation.

This usually loses the first byte, which may or may not be acceptable.

Ok, but what happens if the block is ignored and the device sleeps? Does anything break? Will the RX work after it wakes up again?

On stm32 (and most platforms I know), yes, unless the MCU sleeps too deep. On stm32, there's e.g., the standby mode, which would lose IO configuration and all RAM contents.

If it is OK for a certain application to not have RX, then it does not matter if the answer is yer or no, and ignoring the blockage is enough to fix the issue (or, rather, work around the lack of low power uart in stm32).

There's many ways to tackle this issue. As of now, if uart is initialized and powered with an RX callback, RX works, but blocks LPM. As long as we don't have a way to specify that losing bytes is acceptable, it should be that way.

"certain applications" should not initialize uart with RX enabled (e.g., they should not pass an rx callback).

This PR tackles the issue by making stdio RX optional, by not configuring it trough an RX callback.

More elaborate alternatives would make stdio RX sleep controllable. Maybe make it only active when someone is waiting in a read() call comes to mind.
IMO, until we have that functionality, making this a compile-time option makes sense. This is also nice for output-only applications that want to save the RX buffer.

aabadie · 2019-04-11T08:19:39Z

@kaspar030 @jcarrano, I think this is in good shape and already provide an enhancement regarding power management, without the shell. I'm +1 for merging it. What's your opinion (e.g. do you ACK) ?

sys/stdio_uart/stdio_uart.c

kaspar030 · 2019-04-11T09:02:46Z

What's your opinion (e.g. do you ACK) ?

Just gave it a last look. Unfortunately I just realized that as is I think it would break ethos if used with the same uart as stdio (e.g., on the border router or our suit examples).

kaspar030 · 2019-04-11T11:30:48Z

Please squash!

vincent-d · 2019-04-11T12:27:55Z

Squashed

aabadie · 2019-04-12T11:59:24Z

sys/stdio_uart/stdio_uart.c

+    cb = (uart_rx_cb_t) isrpipe_write_one;
+    arg = &stdio_uart_isrpipe;
+#else
+#ifdef USE_ETHOS_FOR_STDIO


I'm wondering if we could make stdio_uart_rx a dependency of ethos in the build system instead of preprocessor macros ?

I am not sure about this... @kaspar030 any opinion?

hm, ethos itself doesn't need stdio_uart_rx.
stdio-over-ethos should probably be a pseudomodule...

I guess as is this is fine for now.

aabadie · 2019-05-09T07:14:41Z

@kaspar030, can you give a final review and eventually ACK this one ?

kaspar030

ACK.

aabadie · 2019-05-09T14:14:08Z

Thanks @kaspar030 :)

…_rx_optional" This change was breaking (dependencies for) ethos, shell, etc, as well as an unknown number of external apps (API change.) See RIOT-OS#11525 for more details. Fixes RIOT-OS#11525.

…ional" This change was breaking (dependencies for) ethos, shell, etc, as well as an unknown number of external apps (API change.) See RIOT-OS#11525 for more details. Fixes RIOT-OS#11525.

…ional" This change was breaking (dependencies for) ethos, and other serial users, as well as an unknown number of external apps (API change.). See RIOT-OS#11525 for more details. Fixes RIOT-OS#11525.

benemorius · 2019-06-05T20:30:16Z

I'm not familiar with STM32, but googling around seems to indicate that it has the ability to be woken up from the UART. If that is the case then it is something that should be addressed by the periph implementation.

This usually loses the first byte, which may or may not be acceptable.

Is that due to taking too long to wake up and clear the receive buffer or is it actually a documented hardware limitation? I don't think I've ever had to tolerate that problem on any platform, but it's possible I've avoided it by shortening the wakeup sequence.

I recall some time ago moving the main oscillator startup to a later point and making it conditional, but I don't remember if that was for the uart or just to avoid wasting power for xtimer overflow wakeups which come very frequently on EFM32.

vincent-d added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 29, 2019

vincent-d requested review from haukepetersen, miri64 and kaspar030 March 29, 2019 13:09

kaspar030 mentioned this pull request Apr 1, 2019

sys: make uart_stdio RX optional #7974

Closed

kaspar030 reviewed Apr 1, 2019

View reviewed changes

aabadie mentioned this pull request Apr 11, 2019

cpu/stm32_common: enable EXTI interrupt for rtt #11286

Closed

aabadie added the Area: pm Area: (Low) power management label Apr 11, 2019

kaspar030 reviewed Apr 11, 2019

View reviewed changes

sys/stdio_uart/stdio_uart.c Show resolved Hide resolved

kaspar030 added the CI: run tests If set, CI server will run tests on hardware for the labeled PR label Apr 11, 2019

sys: make uart_stdio RX optional

002057f

vincent-d force-pushed the make_uart_stdio_rx_optional branch from 386c187 to 002057f Compare April 11, 2019 12:27

kaspar030 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 Apr 11, 2019

aabadie mentioned this pull request Apr 12, 2019

examples/lorawan: add the possibility to use ABP activation procedure #11237

Merged

aabadie reviewed Apr 12, 2019

View reviewed changes

kaspar030 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 May 9, 2019

kaspar030 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label May 9, 2019

kaspar030 approved these changes May 9, 2019

View reviewed changes

kaspar030 merged commit a9f1a85 into RIOT-OS:master May 9, 2019

cladmi mentioned this pull request May 14, 2019

API change, uart input not working anymore on previously working setups #11525

Closed

jcarrano mentioned this pull request May 28, 2019

sys: Revert #11310 (make stdio_rx optional) #11594

Closed

kYc0o mentioned this pull request Jun 3, 2019

pm: provide way to disable uart_stdio rx #7947

Closed

aabadie mentioned this pull request Jun 7, 2019

Add stdin pseudo module and set it as dependency for applications using getchar #11598

Merged

roberthartung mentioned this pull request Jul 30, 2019

tests/periph_pm: make usage more intuitive and move shell commands to sys/ #11731

Merged

vincent-d deleted the make_uart_stdio_rx_optional branch November 10, 2020 11:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sys: make uart_stdio RX optional (attempt #2) #11310

sys: make uart_stdio RX optional (attempt #2) #11310

vincent-d commented Mar 29, 2019

jcarrano commented Apr 1, 2019

kaspar030 commented Apr 1, 2019

kaspar030 commented Apr 1, 2019

kaspar030 Apr 1, 2019

kaspar030 Apr 1, 2019

vincent-d Apr 3, 2019

kaspar030 Apr 1, 2019

vincent-d Apr 3, 2019

jcarrano commented Apr 1, 2019

kaspar030 commented Apr 1, 2019

aabadie commented Apr 11, 2019

kaspar030 commented Apr 11, 2019

kaspar030 commented Apr 11, 2019

vincent-d commented Apr 11, 2019

aabadie Apr 12, 2019

vincent-d Apr 17, 2019

kaspar030 May 9, 2019

aabadie commented May 9, 2019

kaspar030 left a comment

aabadie commented May 9, 2019

benemorius commented Jun 5, 2019

sys: make uart_stdio RX optional (attempt #2) #11310

sys: make uart_stdio RX optional (attempt #2) #11310

Conversation

vincent-d commented Mar 29, 2019

Contribution description

Issues/PRs references

jcarrano commented Apr 1, 2019

kaspar030 commented Apr 1, 2019

kaspar030 commented Apr 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcarrano commented Apr 1, 2019

kaspar030 commented Apr 1, 2019

aabadie commented Apr 11, 2019

kaspar030 commented Apr 11, 2019

kaspar030 commented Apr 11, 2019

vincent-d commented Apr 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aabadie commented May 9, 2019

kaspar030 left a comment

Choose a reason for hiding this comment

aabadie commented May 9, 2019

benemorius commented Jun 5, 2019