cpu/atmega_common/periph/uart: use TX_ISR to check TX end#12973
cpu/atmega_common/periph/uart: use TX_ISR to check TX end#12973aabadie merged 2 commits intoRIOT-OS:masterfrom
Conversation
|
Maybe this should be optional? It adds some code, and is only needed if |
Could this be somehow related to the periph_uart_nonblocking used with some sam0 cpus ? |
|
@aabadie do you think I should add the optional future part? fixing RIOT_VERSION and with the branch rebased on master.
There is some change in |
|
I'll trigger the CI now to see if there are any other issue and not have to run it tomorrow when it might be a bigger queu. |
aabadie
left a comment
There was a problem hiding this comment.
do you think I should add the optional future part?
Maybe not for the moment.
I have a couple of questions, see below.
| * @brief Allocate variable to hold transmission status, set when there | ||
| * is data in UDRn or in the Transmit Shift Register | ||
| */ | ||
| static volatile uint8_t _tx_pending = 0; |
There was a problem hiding this comment.
Is volatile really required ?
There was a problem hiding this comment.
AFAIK variables used inside and out of ISR should be volatile.
There was a problem hiding this comment.
AFAIK variables used inside and out of ISR should be volatile.
@fjmolinas: No, they shouldn't. volatile only prevents that access are are optimized and that the order of volatile accesses is not changed in regard to each other. When you need to access a variable from inside and outside ISR the most readable approach is to use C11 atomics, the next best is to use irq_disable() ... irq_restore() when accessing the variable from outside of the ISR.
See https://www.kernel.org/doc/html/latest/process/volatile-considered-harmful.html and https://blog.regehr.org/archives/28
|
I think you can squash now. Unfortunately I can't test so I'll have to trust yours. |
OK I'll do a run of tests on my ci to verify status is the same for avr boards. |
For atmega boards a TX has not actually completed until UDRn is empty as well as the Transmit Shift Register. To avoid resetting an UART before a TX has completed we use the TXCn flash and ISR to set a variables that indicates TX is ongoing. This allows not reseting the UART while there are ongoing TX pending. This fixes an issue where part of the last byte is not shifted out of the TX shift register causing rubish on the first TX following an uart_init.
cff0bbe to
fb388bf
Compare
|
Squashed |
maribu
left a comment
There was a problem hiding this comment.
Sorry for the late comment. See inline. I can open a PR to address this.
| * @brief Allocate variable to hold transmission status, set when there | ||
| * is data in UDRn or in the Transmit Shift Register | ||
| */ | ||
| static volatile uint8_t _tx_pending = 0; |
There was a problem hiding this comment.
AFAIK variables used inside and out of ISR should be volatile.
@fjmolinas: No, they shouldn't. volatile only prevents that access are are optimized and that the order of volatile accesses is not changed in regard to each other. When you need to access a variable from inside and outside ISR the most readable approach is to use C11 atomics, the next best is to use irq_disable() ... irq_restore() when accessing the variable from outside of the ISR.
See https://www.kernel.org/doc/html/latest/process/volatile-considered-harmful.html and https://blog.regehr.org/archives/28
|
Hi @maribu thanks for the feedback, if you want to open the PR yourself it would be nice, otherwise I'll do it when I understand your comments better. I'll read those links closely, to get rid of the |
Contribution description
For atmega boards a TX has not actually completed until UDRn is empty
as well as the Transmit Shift Register.
To avoid resetting an UART before a TX has completed we use the TXCn
flash and ISR to set a variables that indicates TX is ongoing. This
allows not reseting the UART while there are ongoing TX pending.
This fixes an issue where part of the last byte is not shifted out
of the TX shift register causing rubish on the first TX following an
uart_init.
Testing procedure
BOARD=arduino-uno make -C tests/sys_arduino flash termand see a clean print.Issues/PRs references