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

periph/uart: break detection and sleep helper #9420

Closed
wants to merge 4 commits into from

Conversation

ZetaR60
Copy link
Contributor

@ZetaR60 ZetaR60 commented Jun 26, 2018

This is my attempt to solve the UART sleep issue.

Background

Entering a CPU sleep mode is problematic when using UART RX. There are several possible methods to detect when it is safe to sleep the UART, but many of them have problems:

  • sleep after not receiving for t seconds - can cause data loss on waking
  • require a keep-alive character to stay awake (variant of above) - requires modification of software on the other end of the UART
  • use a command (sent over the UART) to sleep - requires intervention by the user
  • use a UART that is able to receive while sleeping - requires specialized hardware
  • probably some others I haven't thought of

The method used here is to detect the disconnection of hardware that is driving the RX line by detecting a break condition. When a driver is attached to RX, it will hold the line in an idle state (high), and only bring it low long enough to transmit a character. Reception of a null character (all zeros) along with a frame error (no high stop bit) should indicate that the line has be non-idle for longer than the expected character time. When this condition is detected it should be safe to sleep, because there is no properly configured driver attached to the RX line (if it is improperly configured or noisy, communication will be unreliable anyway, regardless of sleeping or not). Waking can simply use a pin interrupt with a rising flank to detect the line returning to idle.

In order to detect the break condition a few criteria must be met:

  • The port the user is attaching to for communication must be the UART, and not something else such as USB (unless the adapter has been modified to pass on a break condition).
  • RX must have a pull-down resistor

I suspect that anything not meeting the first criterion is probably not suitable for very low-power use, because it has been designed with user friendliness as a priority and not power saving. Anything with a built in USB to UART adapter (or similar) that is designed for low-power use will provide some facility for sleeping the adapter and signaling this condition to the CPU. For example, a popular USB to UART adpater chip FT230 has an output to indicate that it is suspended, and can even be programmed to pull the UART RX line low during suspend.

As for the second criterion: I propose that anything that is used in low-power applications will be required to designed to be used in that application in order to be effective. Even if there is no USB to UART adapter chip, what does sleeping the CPU matter when you are wasting tens of milliwatts on common ICs not intended for very low power. For example, many popular linear regulators such as LM317 have a minimum output current of over a milliamp. If a board has been designed for low-power applications it will require putting thought into the interaction between the hardware and the sleep modes programmed into the firmware. Thus, either it has already been designed for low-power use and provides its own signal for the CPU to sleep, or it is yet to be released and asking for a single resistor pull-down to be added to the design will be an insignificant request (as is the case for my board that I am designing).

Contribution description

This PR adds an API change to drivers/include/periph/uart.c that adds break detection functionality, and a sleep helper function which uses the break detection to sleep the UART. These API changes do not affect systems that don't implement them. Also, a default sleep helper is implemented in drivers/periph_common/uart.c, and break detection has been implemented for the ATmegas.

Issues/PRs references

#7947

Initial status

I am simulating a UART disconnect on the Arduino UNO by forcing the RX pin low with a wire. Since the USB adapter is connected via a 1k resistor this is safe to do. Digital pin 2, which is an asynchronous interrupt pin, is tied to RX to detect the return to idle. It is partially working with problems. I can't seem to get it to say it is sleeping without printing that it is also waking. There is also some garbage and contact bounce issues creating multiple sleeps.

user@user ~/Desktop/RIOT_break_condition/examples/arduino_hello-world $ make BOARD=arduino-uno term
/home/user/Desktop/RIOT_break_condition/dist/tools/pyterm/pyterm -p "/dev/ttyACM0" -b "9600"
2018-06-26 14:27:41,288 - INFO # Connect to serial port /dev/ttyACM0
Welcome to pyterm!
Type '/exit' to exit.
2018-06-26 14:27:45,104 - INFO # 

2018-06-26 14:27:45,157 - INFO # [uart] UART0 now set to sleep on break condition
2018-06-26 14:27:45,251 - INFO # main(): This is RIOT! (Version: UNKNOWN (builddir: /home/user/Desktop/RIOT_break_condition))
2018-06-26 14:27:45,267 - INFO # Hello Arduino!
hello
2018-06-26 14:27:53,274 - INFO # Echo: hello
2018-06-26 14:27:59,193 - INFO # [uart] UART0 going to sleep�!��W�u�UART0 waking up
2018-06-26 14:27:59,652 - INFO # [uart] UART0 going to sleep�!��W�u�UART0 waking up
2018-06-26 14:27:59,738 - INFO # [uart] UART0 going to sleep�!��W�u�UART0 waking up
2018-06-26 14:28:00,278 - INFO # [uart] UART0 going to sleep�!��W�u�UART0 waking up
2018-06-26 14:28:00,356 - INFO # [uart] UART0 going to sleep�!��W�u�UART0 waking up
2018-06-26 14:28:00,413 - INFO # [uart] UART0 going to sleep�!��W�u�UART0 waking up
2018-06-26 14:28:00,471 - INFO # [uart] UART0 going to sleep�!��W�u�UART0 waking up
hello
2018-06-26 14:28:04,288 - INFO # Echo: hello
/exit
2018-06-26 14:28:10,523 - INFO # Exiting Pyterm

@ZetaR60 ZetaR60 added Type: new feature The issue requests / The PR implemements a new feature for RIOT Area: drivers Area: Device drivers Area: pm Area: (Low) power management labels Jun 26, 2018
@dylad
Copy link
Member

dylad commented Jun 27, 2018

@ZetaR60 This is interesting ! I'm wondering if you loose the first character during the wakeup ?

@jnohlgard
Copy link
Member

This looks promising!

@jnohlgard
Copy link
Member

It is not entirely clear from the documentation that the uart_break_init function is not meant to be called from the application/board init, and that the uart_break_sleep function is the actual initialization function that should be called from the board or application init.
I think the problem comes from that this PR is introducing two things in the same API: One interface for a generic callback for UART line break conditions, and one interface for UART power management configuration, where the power management is implemented in terms of the generic callback.
The gpio_t argument to uart_break_sleep would not be necessary if the periph/uart driver is responsible for handling automatic module sleep instead. Adding a generic callback for line break conditions can be provided as a separate enhancement. The fiddling with GPIO IRQs that the sleep and wake up functions need to do can be done with much less overhead if it is part of the UART driver implementation. A generic line break callback can then be implemented as called from the line break ISR before executing the uart_poweroff part, in case the application needs this functionality.
What are the arguments/use cases for a run time configurability of the UART line break sleep enable setting?
The configuration for sleep on line break can be implemented as a part of periph_conf.h or as a function, e.g. uart_break_sleep(uart_t dev, unsigned enable), where enable is 0 or non-0. The compiler would probably do a better job of optimizing if it is a static configuration in periph_conf, but we would lose runtime configurability.

What do you think about this?

@ZetaR60
Copy link
Contributor Author

ZetaR60 commented Jun 27, 2018

I'm wondering if you loose the first character during the wakeup ?

No, because the wakeup is detecting the line's rise from low (non-idle) to high (idle), which will be a long time prior to the first character being sent. It's the amount of time between you plugging in the cable, and actually doing something with that cable. As you can see with the "hello" message, no characters are lost. The going to sleep / waking up messages are debug only, and typically it would be completely invisible to the user.

It is not entirely clear from the documentation that the uart_break_init function is not meant to be called from the application/board init, and that the uart_break_sleep function is the actual initialization function that should be called from the board or application init.

I will add a note to clarify.

I think the problem comes from that this PR is introducing two things in the same API: One interface for a generic callback for UART line break conditions, and one interface for UART power management configuration, where the power management is implemented in terms of the generic callback.

The reason for implementing power management using the generic callback is that the sleep code does not need to be CPU specific. Almost all of the changes to cpu/atmega_common/periph/uart.c are to support uart_poweron and uart_poweroff properly, and not to support break conditions. The actual break condition code consists of a few lines in uart_break_init to set variables, and a few lines in isr_handler to detect the condition and execute the callback.

The gpio_t argument to uart_break_sleep would not be necessary if the periph/uart driver is responsible for handling automatic module sleep instead. Adding a generic callback for line break conditions can be provided as a separate enhancement.

On the ATmegas at least, there is not a standard way of knowing what the UART pins are. You do not need to know your RX and TX pin numbers to use the UART. Also, the GPIO pin may not be the same as the RX pin if it does not support asynchronous interrupts (as is the case with the ATmega328P). Ergo, you need to supply the pin information somehow. It could be changed to a macro though.

The fiddling with GPIO IRQs that the sleep and wake up functions need to do can be done with much less overhead if it is part of the UART driver implementation. A generic line break callback can then be implemented as called from the line break ISR before executing the uart_poweroff part, in case the application needs this functionality.

I don't see how the overhead will be significantly less. You would still be executing the same code, but it would just be duplicated for every CPU. I suppose you could get rid of a few of the globals, but you could do that by other methods as well. I will work on reducing the memory overhead.

What are the arguments/use cases for a run time configurability of the UART line break sleep enable setting? The configuration for sleep on line break can be implemented as a part of periph_conf.h or as a function, e.g. uart_break_sleep(uart_t dev, unsigned enable), where enable is 0 or non-0. The compiler would probably do a better job of optimizing if it is a static configuration in periph_conf, but we would lose runtime configurability.

I guess there is not much use case for this. I will make things a bit more static and optimizable.

@dylad
Copy link
Member

dylad commented Jun 27, 2018

@ZetaR60 Correct me if I'm wrong, but the interrupt is triggered when a start bit is issued on the line right ?
So depending on the MCU wakeup time from its sleep mode, there is a possibility to loose the first character at least.

@ZetaR60
Copy link
Contributor Author

ZetaR60 commented Jun 27, 2018

@dylad No, the method of detection is for RX to be pulled low when the RX driver is either turned off or physically disconnected (this is called a break condition). When the driver is reconnected it will pull the line high, which is the idle state for UART. This is what is detected to wake the UART.

@ZetaR60 ZetaR60 force-pushed the RIOT_break_condition branch from 3c67432 to ccf810d Compare June 30, 2018 04:45
@ZetaR60
Copy link
Contributor Author

ZetaR60 commented Jun 30, 2018

Rebased to pick up fixes to GPIO from #9456.

I have managed to get rid of most of the garbage by a variety of methods such as flushing the buffer and rejecting bytes that have frame errors. However, I have not be able to eliminate the issues caused by contact bounce, only mitigate them somewhat with an aggressive time delay (which is highly undesirable because it is in an ISR; it's for testing only at this stage). I am concerned that contact bounce may prevent simple methods of break detection unreliable, because it can prevent null + frame error from appearing.

@ZetaR60
Copy link
Contributor Author

ZetaR60 commented Jul 3, 2018

I have been thinking about the break detection method, and I believe that it is going to be very difficult to get 100% reliability due to the contact bounce. I think that using the timeout method and losing one character is preferable to the system occasionally not sleeping on a power critical application. Closing this PR. I will salvage the ATmega UART sleep support in another PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: drivers Area: Device drivers Area: pm Area: (Low) power management Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants