SPI errors will be persistent

[austinglaser]

My reading of the current error detection code in the SPI module seems to suggest that if an error occurs it will never be cleared. The code in question:

                fn read(&mut self) -> nb::Result<u8, 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.rxne().bit_is_set() {
                        // NOTE(read_volatile) read only 1 byte (the svd2rust API only allows
                        // reading a half-word)
                        return Ok(unsafe {
                            ptr::read_volatile(&self.spi.dr as *const _ as *const u8)
                        });
                    } else {
                        nb::Error::WouldBlock
                    })
                }

                fn send(&mut self, byte: u8) -> 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() {
                        // NOTE(write_volatile) see note above
                        unsafe { ptr::write_volatile(&self.spi.dr as *const _ as *mut u8, byte) }
                        return Ok(());
                    } else {
                        nb::Error::WouldBlock
                    })
                }

The reference manual (section 30.5.11, pp 974) states of the overrun flag:

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

Of the mode fault flag:

Use the following software sequence to clear the MODF bit:

1. Make a read or write access to the SPIx_CR1 register while the MODF bit is set.
2. Then write to the SPIx_CR1 register.

The procedure for clearing a CRC error is not explictly listed in the SPI section, but it's listed as an rc_w0 bit. Section 2.1, pp 46, says:

Software can read as well as clear this bit by writing 0. Writing '1' has no effect on the bit value.

Because of the way the module is coded, I suspect (though I haven't tested my assumption) that if one of these errors is set, the SPI driver will become completely nonfunctional -- the bit will never be cleared, and so the SPIx_DR register will never again be accessed. Furthermore, the programmer cannot even re-initialize the driver, since the constructor consumes the SPI control block and pins.

The question then becomes how to expose error-clearing functionality -- especially in a platform independent way. I see several options, each with some tradeoffs:

1. Clear errors automatically on detection.
   • This is probably the most ergonomic and presents the fewest barriers to portability.
   • On the other hand, it provides the programmer the least flexibility
2. Provide a function for manually clearing all errors
   • Still pretty portable, since it doesn't leak details of what errors you're clearing to the programmer -- could be added to the SPI trait, or could be exposed as a separate trait
   • A possible stumbling block for users
3. Provide functions for clearing each error specifically
   • Less portable, less ergonomic, more room for programmer error

Of these, I think I favor option 1 or 2. The only advantage I could see to 2 over 1 would be if there's additional action the user of the SPI driver needs to take between detecting an error and clearing it in the SPI driver -- for instance, resetting an external device. This use case is a bit of a stretch.

I'd love to take a crack at implementing this myself -- but I'd like to get some feedback as to what you'd accept in a pull request before I do any actual work.