embassy-rs / embassy

Modern embedded framework, using Rust and async.
https://embassy.dev
Apache License 2.0
5.15k stars 713 forks source link

stm32: no fifo field in Uart::Config, and broken FIFO with new_half_duplex mode #2875

Open qwerty19106 opened 4 months ago

qwerty19106 commented 4 months ago

Now FIFO is always enabled for usart_v4.
https://github.com/embassy-rs/embassy/blob/main/embassy-stm32/src/usart/mod.rs#L1407

        #[cfg(usart_v4)]
        {
            trace!("USART: set_fifoen: true (usart_v4)");
            w.set_fifoen(true);
        }

I use Uart::new_half_duplex on chip STM32G030C8 (usart_v4). I send ~16 bytes and then read to IDLE:

uart.write(&send_buf).await?;
uart.flush().await?;
let res = uart.read_until_idle().await?;

But due to w.set_fifoen(true); I get ~7 bytes which send over uart.write.

  1. Availability is there a reason why Uart::Config no have fifo: bool field?
  2. Possible blocking_flush should use TXFIFO Empty flag if FIFO is enabled.
  3. Why async flush() use blocking_flush? I think we should wait Transmission complete interrupt or TXFIFO empty interrupt
    https://github.com/embassy-rs/embassy/blob/main/embassy-stm32/src/usart/mod.rs#L1564
    async fn flush(&mut self) -> Result<(), Self::Error> {
        self.blocking_flush()
    }

I am ready to write PR about each point if it will clear how it should work.

Dirbaio commented 4 months ago

Availability is there a reason why Uart::Config no have fifo: bool field?

The reasoning was, there's very little reason to not want a FIFO. Having a FIFO is just better: it makes things faster, less likely to lose bytes if you have irq delays, etc. You might think if you don't enable the FIFO you don't have to worry about flushing etc, but that's not true. You still have a 1 word "FIFO" which is the DR itself, so you can still have problems if you don't flush correctly.

Possible blocking_flush should use TXFIFO Empty flag if FIFO is enabled.

If flush() is returning before the fifo is empty with the current code then yes, definitely.

Why async flush() use blocking_flush? I think we should wait Transmission complete interrupt or TXFIFO empty interrupt

No one has implemented async flush yet, so it uses blocking flush for now, since it at least works even if not optimally. A PR fixing this would be welcome.