Spi api changes

[pietgeursen]

There are few things that I discovered I need from the SPI.

This PR is a bit of conversation starter. I'm happy to split it up / close it if it's not useful. But I thought I should at least try and get my work back upstream :)

Spi::set_baudrate is slow

Slow is relative of course, but it takes ~100us at runtime on rp2040. That's a deal breaker for me because I have multiple devices on a single SPI bus and I need to be able to reconfigure the SPI and write to it it every 50us.

I extracted the guts of the calculation into const fn calc_spi_clock_divider_settings_for_baudrate. This returns a SpiClockDividerSettings which can then be passed to pub fn set_baudrate_from_settings(&mut self, settings: &SpiClockDividerSettings) which is what actually writes registers.

This enables you to only do that calculation when you actually need to.

set_baudrate is refactored to use these functions internally.

Spi::set_mode(&mut self, mode: Mode)

I think this is safe to do. It's just a convenience function to update the Modeon an enabled Spiwithout having to disable it and create a new one.

Spi::pub fn complete_transfer(&mut self) -> Result<(), nb::Error<Infallible>>

I found a footgun with the embedded hal spi trait

The rp2040 has fifo of width 8. Imagine you do this:

chip_select.set_low().unwrap();
nb::block!(spi.send(byte1)).unwrap();
nb::block!(spi.send(byte2)).unwrap();
nb::block!(spi.send(byte3)).unwrap();
chip_select.set_high().unwrap();

Assuming the fifo was empty to start with, those sends will never block. The implementation only returns WouldBlock when the transmit buffer is full, not when the transfer is complete. That's arguably the correct thing to do. But my chip select will go high in the middle of the sends.

Now you can do this:

chip_select.set_low().unwrap();
nb::block!(spi.send(byte1)).unwrap();
nb::block!(spi.send(byte2)).unwrap();
nb::block!(spi.send(byte3)).unwrap();
nb::block!(spi.complete_transfer()).unwrap()
chip_select.set_high().unwrap();

[jannic]

Another remark: The future of nb is a little bit unclear. It might get replaced by an async API.
While I'd say that we should continue supporting the nb traits of embedded-hal for the foreseeable future, I wouldn't add methods using it to the the rp2040-hal impls.

So this method should have the same signature as https://docs.rs/embedded-hal/1.0.0-alpha.9/embedded_hal/spi/trait.SpiBusFlush.html#tymethod.flush, fn flush(&mut self) -> Result<(), Self::Error>.

[ptpaterson]

Self::Error is currently being implemented as Infallable, so the proposed signature change would mean flush would have to be blocking, right?

    fn flush(&self) -> Result<(), Self::Error> {
        while self.device.sspsr.read().bsy().bit() {}
        Ok(())
    }

If that's the case, why not make the inherent impl return () and allow an eh alpha SpiBusFlush implementation to use that. See Fallability discussion in proposed migration guide.

    fn flush(&self) {
        while self.device.sspsr.read().bsy().bit() {}
    }

Or for that matter, if it is helpful to have an inherent non-blocking implementation (I honestly don't know if that's helpful) why not let the inherent impl be non-blocking (keep the current signature with Result<(), nb::Error<Infallible>>) to be consistent with the other inherent impls and the SpiBusFlush trait impl loop over it to make it blocking.

[ptpaterson]

Just realized this PR is based on a relatively old commit -- this needs a rebase.

So if rebased, Spi<Enabled, D, DS> now has an is_busy method which can be used to implement flush. I think it makes sense to only implement flush for enabled SPI anyway.

impl<D: SpiDevice, const DS: u8> Spi<Enabled, D, DS> {
    /* ... */
    fn flush(&self) {
        while self.is_busy {}
    }
}

which would allow later:

    #[cfg(feature = "eh1_0_alpha")]
    impl<D: SpiDevice> eh1::SpiBusFlush for Spi<Enabled, D, $nr> {
        fn flush(&mut self) -> Result<(), Self::Error> {
            self.flush();
            Ok(())
        }
    }

[jannic]

So if rebased, Spi<Enabled, D, DS> now has an is_busy method which can be used to implement flush.

Exactly, and is_busy is functionally equivalent to the non-blocking version of flush, so it's sufficient to have the blocking flush method.

[ithinuel]

Is there anything else to do on this PR? (rebase/reach compromise/clone?)

[ithinuel]

Making fields public is a strong commitment (see rust's future proofing guidelines).

Also, I'm not even sure allowing users to set them to arbitrary values is a good idea since they'd need to be validated before set in the registers (eg only even values are allowed in CPSDVSR).

[ithinuel]

I think this would make a nice inherent method to SpiClockDividerSettings like

impl SpiClockDividerSettings {
    pub const fn new(peri_frequency: HertzU32, baudrate: HertzU32) -> Self {
        todo!()
    }
}

[ithinuel]

This method should most likely derive:

• Copy, Clone
• PartialEq, Eq
• Debug
• defmt::Format (when the feature is enabled)