STM32: Serial: Overrun not handled properly in serial_readable

[boraozgen]

Description of defect

Introduced in #4502, the following code is supposed to handle overrun condition in the USART peripheral.

mbed-os/targets/TARGET_STM/serial_api.c

Lines 569 to 574 in d147abc

	/* To avoid a target blocking case, let's check for
	* possible OVERRUN error and discard it
	*/
	if (__HAL_UART_GET_FLAG(huart, UART_FLAG_ORE)) {
	__HAL_UART_CLEAR_OREFLAG(huart);
	}

The issue is that by clearing the overrun flag (ORE), the data register (USART_DR) is read and the data in the register is lost. Moreover, this also clears the RXNE flag, which results in serial_readable returning false, which was not the case before clearing ORE. In short, the clearing ORE does not belong to this function and should be done somewhere else.

A possible case where this leads to a endless loop is when you call getc (UnbufferedSerial::read) in the Serial RX interrupt. If the peripheral has been initialized and an overrun has occured before the interrupt is enabled, the first getc call on the first interrupt will block forever due to this issue.

Target(s) affected by this defect ?

STM32F4

Toolchain(s) (name and version) displaying this defect ?

GCC

What version of Mbed-os are you using (tag or sha) ?

mbed-os-99.99.99

What version(s) of tools are you using. List all that apply (E.g. mbed-cli)

Not relevant

How is this defect reproduced ?

We encountered this issue in a more complicated setup, but the steps are basically these:

• Construct (enable) UnbufferedSerial
• Send some bytes to the target
• Attach the RxIrq interrupt to a function which calls read

@jeromecoutant

[0xc0170]

cc @ARMmbed/team-st-mcd

[MubeenHCLite]

@boraozgen
The mentioned scenario does not produce any error or halts the program on mbed-os 6.13.
The code I have used is as below :

// Create a DigitalOutput object to toggle an LED whenever data is received.
static DigitalOut led(LED1);

// Create a UnbufferedSerial object with a default baud rate.
static UnbufferedSerial serial_port(PD_5, PD_6);

void on_rx_interrupt()
{
    char c;

    // Toggle the LED.
    led = !led;

    // Read the data to clear the receive interrupt.
    if (serial_port.read(&c, 1)) {
        // Echo the input back to the terminal.
        serial_port.write(&c, 1);
    }
}

int main(void)
{
    char c = '*';
    // Set desired properties (9600-8-N-1).
    serial_port.baud(9600);
    serial_port.format(
        /* bits */ 8,
        /* parity */ SerialBase::None,
        /* stop bit */ 1
    );

    for(int i=0; i< 10 ; i++)
        serial_port.write(&c, 1);

    ThisThread::sleep_for(10s);
    printf("%c",serial_port.read(&c, 1));

    ThisThread::sleep_for(10s);

    // Register a callback to process a Rx (receive) interrupt.
    serial_port.attach(&on_rx_interrupt, SerialBase::RxIrq);

    while(1){
        ThisThread::sleep_for(1s);
        printf("Endless loop\n");
    }
        
}

Please recheck once and let know if you see any issues.

[boraozgen]

Thank you for the repro! It's almost correct, however you should not do the printf("%c",serial_port.read(&c, 1)); (as it clears the overrun) and just send some data from the other side during the delay. After that, once the interrupt is attached, you will see that the program loops indefinitely in if (serial_port.read(&c, 1)).

[MubeenHCLite]

Thank you for the repro! It's almost correct, however you should not do the printf("%c",serial_port.read(&c, 1)); (as it clears the overrun) and just send some data from the other side during the delay. After that, once the interrupt is attached, you will see that the program loops indefinitely in if (serial_port.read(&c, 1)).

I have tried this scenario already, the result i same. The led toggles.

Please let know exactly which platform are you trying on and which veruon of mbed-os are you using.

[boraozgen]

6.13.0 on NUCLEO_F401RE.

Well it breaks out of the loop when the next byte is received, but until then it gets stuck in the readable loop, i.e. "Endless loop" does not print.

[MubeenHCLite]

@boraozgen
The problem statement mentioned in the description is different when seen in the reference manual for STM32F401RE. Please refer to the section 19.6.1. The register description says the process for clearing ORE is a read of USART_SR followed by a read of USART_DR from software. If not read the data is overwritten.

After the interrupt is enabled (callback is registered), the next byte received will clear the ORE flag in read operation. It will read the overwritten data anyhow.

And I do not see that it is stuck in a readable loop. For me the "Endless loop" is getting printed without any issues.
.

[boraozgen]

After the interrupt is enabled (callback is registered), the next byte received will clear the ORE flag in read operation. It will read the overwritten data anyhow.

Here is the problem: it recovers only when the next byte is received. If there are no bytes received, the program gets stuck in here:

mbed-os/targets/TARGET_STM/TARGET_STM32F4/serial_device.c

Line 259 in 7acd5b4

while (!serial_readable(obj));

You can reproduce it like this (modified from your code, see the comments):

void on_rx_interrupt()
{
    char c;

    // Toggle the LED.
    led = !led;

    // Read the data to clear the receive interrupt.
    if (serial_port.read(&c, 1)) { // Will get stuck here in the first interrupt. Recovers when more bytes are sent from the counterpart.
        // Echo the input back to the terminal.
        serial_port.write(&c, 1);
    }
}

int main(void)
{
    char c = '*';
    // Set desired properties (9600-8-N-1).
    serial_port.baud(9600);
    serial_port.format(
        /* bits */ 8,
        /* parity */ SerialBase::None,
        /* stop bit */ 1
    );

    for(int i=0; i< 10 ; i++)
        serial_port.write(&c, 1);

    ThisThread::sleep_for(10s); // Send some bytes from the counterpart here and wait until the interrupt is enabled

    // Register a callback to process a Rx (receive) interrupt.
    serial_port.attach(&on_rx_interrupt, SerialBase::RxIrq);

    while(1){
        ThisThread::sleep_for(1s);
        printf("Endless loop\n");
    }
        
}

[MubeenHCLite]

@boraozgen
Try enable your interrupt only after ORE and all are cleared. Trying reading data before enabling interrupt. It is not desired to enable interrupt abruptly with out clearing some pending flags.

[boraozgen]

Then these flags should be cleared in the attach function. However IMHO it is only a workaround to a faulty logic in the readable function. After all, the interrupt itself does not cause any problems, but the readable function does.

[MubeenHCLite]

@boraozgen
As mentioned in the comment from the code snippet, that part of code is required to avoid target blocking,
on other side you are not using the attach call in the right way.

Either you do your attach for callback with the configuration of UART itself or do a read before attach(This is not a usual use case, but i hope you are having an unavoidable use case scenario)