nRF52 uart_write get stuck

[jeffbortolotti]

Using nRF52DK, I have found conditions where the uart_write function stays stuck on waiting the character to be sent(

RIOT/cpu/nrf5x_common/periph/uart.c

Line 146 in 8a74475

while (NRF_UART0->EVENTS_TXDRDY == 0) {}

).

The way to reproduce this is : use the example/microcoap_server, setup the IPv6/6lowPan connection with a host and send ping request every 5s for instance. (the 5s depends on the configuration of the debug functions you have turned on as they change the timing of the processing). I have turned on several enable_debug flags in several module as I'm debugging different ping instabilities(see note below).
Very often, I get the 6lowpan thread stuck on the uart_write function looping on this line :
while (NRF_UART0->EVENTS_TXDRDY == 0) {}

Note: there are other instabilities with the ping and this example/setup: 1/ sometimes the memory is full and thus the ping reply will not be sent. 2/ at some point when configuring the host for IPv6/6lowpan, the UART send garbage characters and then recover.

In my configuration, I have ENABLE_DEBUG(1) for :
pkg/nordic_softdevice_ble/src/ble-mac.c
pkg/nordic_softdevice_ble/src/gnrc_nordic_ble_6lowpan.c
sys/net/gnrc/netapi/gnrc_netapi.c
sys/net/gnrc/network_layer/icmpv6/echo/gnrc_icmpv6_echo.c
sys/net/gnrc/network_layer/icmpv6/gnrc_icmpv6.c
sys/net/gnrc/network_layer/ipv6/gnrc_ipv6.c
sys/net/gnrc/network_layer/sixlowpan/gnrc_sixlowpan.c
sys/net/gnrc/pktbuf_static/gnrc_pktbuf_static.c

In order to workaround this stuck/infinite loop condition, I have modified the uart_write function this way:
void uart_write(uart_t uart, const uint8_t data, size_t len)
{
int count=0;
if (uart == 0) {
for (size_t i = 0; i < len; i++) {
/ write data into transmit register /
NRF_UART0->TXD = data[i];
/ wait for any transmission to be done /
while ((NRF_UART0->EVENTS_TXDRDY == 0)&&(count<1000)) {count+=1;}
if (count >= 1000) dropped+=1;
/ reset ready flag */
NRF_UART0->EVENTS_TXDRDY = 0;
if (count>max_count) max_count=count;
count=0;
}
}
}
I have checked that a "normal" character transfer takes less than 400 while loop iterations, thus deciding to use 1000 as a detection for abnormal condition and decision to drop and continue.

I would like to get feedback on this workaround to discuss if we want to root cause the issue that may take time or put this ugly safeguard to avoid getting the system stuck.

[jfbortolotti]

Would it make sense to submit the workaround proposed below till there is a better understanding of the root cause ?
At least, this will avoid to get the system stuck forever.

[haukepetersen]

I am against the workaround, we should find the root cause and fix that. Looking at the error you see, it seems to me that something outside of the UART driver is causing the observed behavior, probably something related to STDIO trouble when 'spamming' it from multiple threads -> I bed we get the UART error when there is a context switch between lines 146 and 148. So the actual problem is rather the UART driver no being thread safe while STDIO is calling it from multiple threads. The main problem here is, that we can not make the STDIO fully thread safe, as it is used through the DEBUG macro also in interrupt contexts where mutexes are not available...

Could you try this:

void uart_write(uart_t uart, const uint8_t *data, size_t len)
{
    if (uart == 0) {
        for (size_t i = 0; i < len; i++) {
            /* reset ready flag */
            NRF_UART0->EVENTS_TXDRDY = 0;
            /* write data into transmit register */
            NRF_UART0->TXD = data[i];
            /* wait for any transmission to be done */
            while (NRF_UART0->EVENTS_TXDRDY == 0) {}
        }
    }
}

Now task switches should not bring the uart driver into a deadlock state...

[jfbortolotti]

I'll give it a try. I had the same thinking but was not able to prove it.
Just to be sure I'm not missing something, your change above still have the issue, you are just changing the window of the problem, but the problem will still exist, correct ?
Shouldn't the right fix be to make the uart driver thread safe ?

[haukepetersen]

https://docs.google.com/spreadsheets/d/15NwdGzZBBV5_FYY6X8z2LS5wJXT-xFU_TJC3l_7ZUfg/edit?usp=sharing

I am not 100% sure, but I think the fix should close the window completely.

Shouldn't the right fix be to make the uart driver thread safe ?

In general yes, but this is not really possible, as long as the UART is used from interrupt context as it is done currently.

[jfbortolotti]

Here are the results of my weekend testing: the proposed fix from @haukepetersen avoid the stuck condition. This is a cleaner solution than my proposal (counter based).
The reason is that even if a thread is interrupted by another thread between the write of the data and the test of the TXRDY flag, the bit will not be cleared and the interrupted thread will be able to exit its infinite loop.
The limitation of this solution is that one character may be lost as the check of the TXRDY of the interrupted thread has not really be checked before the higher priority thread writes its character.
The chances of this happening are quite limited in reality in my opinion(depends on uart speed, processor clock, ...).
Another possibility would be to make the uart_write thread safe with similar mechanisms that have been implemented for some other cpu/chips like :

RIOT/cpu/stm32f2/periph/uart.c

Line 153 in 8a74475

void uart_write(uart_t uart, const uint8_t *data, size_t len)

.

I'll do a PR with this proposal adding a comment for the thread unsafe limitation.