stm32-rs / stm32l0xx-hal

A hardware abstraction layer (HAL) for the STM32L0 series microcontrollers written in Rust
BSD Zero Clause License
96 stars 60 forks source link

Order Issue for USART flushing #179

Closed allexoll closed 3 years ago

allexoll commented 3 years ago

I believe the uart flush has the operation in the wrong order. please correct me if i'm wrong.

                fn flush(&mut self) -> nb::Result<(), Self::Error> {
                    // NOTE(unsafe) atomic read with no side effects
                    let isr = unsafe { (*$USARTX::ptr()).isr.read() };

                    // Frame complete, set the TC Clear Flag
                    unsafe {
                        (*$USARTX::ptr()).icr.write(|w| {w.tccf().set_bit()});
                    }

                    // Check TC bit on ISR
                    if isr.tc().bit_is_set() {
                        Ok(())
                    } else {
                        Err(nb::Error::WouldBlock)
                    }
                }

i believe the write to tccf should be done after checking for TC bit. the way it is done now means that we clear the TC flag before checking it, meaning that the condition will always return WouldBlock.

I'm happy to do a PR if i'm correct.

dzarda commented 3 years ago
isr.tc().bit_is_set()

As far as I know this does not perform the status register read, but reads the isr variable that was obtained on the first line.

allexoll commented 3 years ago

you can have a race condition if the TC flag is set between the read and the clear flag. that way, you will clear the flag, but the tc flag won't be detected. i think the correct implementation should be test flag, and clear flag only if the flag is set.

dzarda commented 3 years ago

Got it, I must agree

hannobraun commented 3 years ago

Fixed in #188.