Poll for completion on SPI write.

[newAM]

Closes #130

---

Continuing discussion from #130 :

The suggested implementation was replacing this:

 self.check_send().ok(); 
 Ok(()) 

With this:

nb::block!(self.check_send())

The concern I have with that is it seems the last check_send().ok() was intended to clear the OVR error bit that may be set due to dropped reads.

Additionally, it does not seem that this works as intended, the CS still goes high before the write is completed with consecutive write calls.

[therealprof]

For this to work check_send might potentially need to be changed to first check for bsy and return nb::Error::WouldBlock if that's the case. If there's room in the buffer it will not return nb::Error::WouldBlock which is what we want so we can block on it.

[therealprof]

We can also combine the current current and the nb::block! in case we want to retain the current behaviour of ignoring any errors at that point.

[newAM]

Upon looking at the code again the final check_send().ok() does not appear to be clearing the OVR flag.

The STM provided HAL (in C) does clear the OVR flag when operating in write-only mode, but the procedure to clear that flag requires reading DR, then reading SR.

When an overrun condition occurs, the newly received value does not overwrite the previous one in the RXFIFO. The newly received value is discarded and all data transmitted subsequently is lost. Clearing the OVR bit is done by a read access to the SPI_DR register followed by a read access to the SPI_SR register.

---

For this to work check_send might potentially need to be changed to first check for bsy and return nb::Error::WouldBlock if that's the case. If there's room in the buffer it will not return nb::Error::WouldBlock which is what we want so we can block on it.

Along the lines of this?

    fn check_send(&mut self) -> nb::Result<(), Error> {
        let sr = self.spi.sr.read();

        Err(if sr.ovr().bit_is_set() {
            nb::Error::Other(Error::Overrun)
        } else if sr.modf().bit_is_set() {
            nb::Error::Other(Error::ModeFault)
        } else if sr.crcerr().bit_is_set() {
            nb::Error::Other(Error::Crc)
        } else if sr.txe().bit_is_set() && sr.bsy().bit_is_clear() {
            return Ok(());
        } else {
            nb::Error::WouldBlock
        })
    }

[therealprof]

Along the lines of this?

Yes, something like that might work. I'm actually not sure which other conditions would be reason to use nb::Error::WouldBlock. Maybe we should just explictly for the bsy condition up front and return nb::Error::WouldBlock and turn the else into a hard Error.

[therealprof]

@newAM Ping?

[newAM]

Sorry for the delay, got distracted with other projects 😅

I made the modification - both the first and second implementations fix the issues mentioned (BIDIMODE / BIDIOE race condition & back to back calls).

Yes, something like that might work. I'm actually not sure which other conditions would be reason to use nb::Error::WouldBlock. Maybe we should just explictly for the bsy condition up front and return nb::Error::WouldBlock and turn the else into a hard Error.

I am not sure of the other conditions either, which makes me hesitant to change the existing implementation too much. It does feel like this could be optimized better, but I have not spent enough time with this hardware to be confident about that.

[WasabiFan]

Still .ok() rather than returning the result?

[newAM]

I left it as ok() because that better preserves the previously existing behavior.

...as for whether or not it should be ok() or ? that is debatable. What do you think?

I think the previous intent was to clear the overrun flag if it was set, but that requires a read to DR followed by a read access to SR to clear.

[WasabiFan]

My understanding is that before it had no tangible effect (since it didn't read from DR) and now it blocks but still does not affect the flags. It seems a bit strange if we were to call this function and identify a failure but not report it -- then again, if it didn't prevent transmission, perhaps it's best to ignore it. It may just depend on the error.

All in all, I don't really have an influencing perspective here since I'll be cherry-picking and tacking on my own changes in my project's fork (I need the non-blocking behavior in some cases). The .ok() pattern to ignore an error seems strange to me since it looks more like a mistake, but if this is the behavior that works, I can't say much against it!

Thanks for fixing this! 😄

[therealprof]

LGTM, thanks for bearing with me.

bors r+

[newAM]

LGTM, thanks for bearing with me.

Thanks for bearing with me as well 👍

Happy to help!

[bors]

Build succeeded:

• ci (stable)
• Rustfmt