Feature/spi builder

[har7an]

Adds a builder interface for SPI peripherals. Takes required SPI parameters in the builders constructor as arguments (clocks, sck line, ...) and allows the user to specify the rest as needed. This allows for fine-grained control about which SPI lines to use or omit, and reduces the length of the argument list in the call to new.

The Builder is added as new struct to maintain compatibility with the previous, direct instantiation method.

Feedback is very welcome!

[har7an]

I also adapted the spi_eh1_loopback example for the esp32 so you can see what the new interface looks like.

[bjoernQ]

Thanks for working on this!

I wouldn't agree the clock rate and mode should be optional. While Mode0 might be common I don't think there is a common frequency

But generally, I'm afraid the builder pattern doesn't match well here because of the generics - i.e. leaving out one of the optional pins makes the compiler yell on the user and needing to write out the type of the builder pretty much makes it almost useless .... but just my opinion maybe there are other opinions

[jessebraham]

Thank you @har7an, I will review this shortly! I think as @bjoernQ has mentioned we need to think a bit about what is required and what is optional, but we'll get there :)

@bjoernQ I'm not sure what we were doing wrong before, but based on some quick testing this seems to be building even when I omit the CS and MISO pins from the builder, so I think it's probably okay.

[bjoernQ]

If it compiles that would be awesome - however that is how things look like for me leaving out CS and MISO:

error[E0282]: type annotations needed
  --> examples\spi_eh1_loopback.rs:58:19
   |
58 |       let mut spi = SpiBuilder::new(
   |                     -^^^^^^^^^^^^^^
   |                     |
   |  ___________________cannot infer type for type parameter `MISO`
   | |
59 | |         peripherals.SPI2,
60 | |         &mut system.peripheral_clock_control,
61 | |         &clocks,
62 | |         sclk,
63 | |     )
64 | |     .frequency(1000u32.kHz())
   | |_____________________________- this method call resolves to `SpiBuilder<'a, T, SCK, MISO, MOSI, CS>`

or with a more recent rustc

error[E0282]: type annotations needed
  --> examples\spi_eh1_loopback.rs:58:19
   |
58 |     let mut spi = SpiBuilder::new(
   |                   ^^^^^^^^^^^^^^^ cannot infer type of the type parameter `MISO` declared on the struct `SpiBuilder`
   |
help: consider specifying the generic arguments
   |
58 |     let mut spi = SpiBuilder::<esp32_hal::esp32::SPI2, Gpio19<esp32_hal::gpio_types::Unknown>, MISO, Gpio23<esp32_hal::gpio_types::Unknown>, CS>(
   |                             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

[jessebraham]

Sorry you're right, I was building in the wrong directory (need more coffee).

[har7an]

Right, I guess I should have tried leaving out an IO as well... It seems I cannot get around this with some magic "Mock" GPIO either.

One thing we could do, since we already have the spi Instance when creating the Builder, is to directly associate the individual GPIOs with their SPI signals when calling the mosi/miso/cs functions. Then we can drop the IOs from the struct and don't need to carry around the additional generics.

This violates the RAII pattern and would have unwanted side-effects, e.g. when the user creates the builder, adds the IOs and then doesn't build but leaves it hanging around. But in that case the IOs passed to the builder become inaccessible to the user in any case due to the borrow checker correctly identifying that the IOs have been moved (and do not implement Copy). What do you think about that?

I don't mean to push this against your will, I'm just trying to think of a way to make the list of parameters to the constructor shorter. It looks very unwieldy IMO... If you say you have discussed this feature internally already and have decided against it, feel free to close this PR.

Edit: You can look at the code, I implemented it there already.

[bjoernQ]

Interesting idea - I didn't think of that (while the constructors do it the exact same way 🤦‍♂️ ) .... since this is an optional addition, I'm probably fine with it

IF we want to keep mode and frequency optional the defaults should at least be mentioned in the docs. Still not convinced that a default frequency makes sense but not a big deal

Just FYI - no action required on your side: we are considering to change the API (for all peripherals) to take a &mut instead of moving the peripheral into the driver - also the pins. Not that soon and maybe not at all. Just considering.

That would eliminate the need for a free/release function (which in this case doesn't free the pins ... but that's another story) since just dropping the driver would make the peripheral and the pins available again - if / when we do that we need to come up with another idea since we need to keep the mutable reference to avoid the user messing up things - but your builder shares that problem with the existing constructors so we need a solution to that no matter if we have the builder or not ... as said: just FYI

[har7an]

IF we want to keep mode and frequency optional the defaults should at least be mentioned in the docs. Still not convinced that a default frequency makes sense but not a big deal

I don't insist on having the frequency optional, but this way it's another parameter that's not part of the default constructor. So far I have mostly used HALs instead of contributing, so I have no idea how many users would run the risk of complaining "Why is my SPI so slow/fast/wrong".

Do you have any preference where in the docs this should be mentioned? It's already part of the mode and frequency function docs. I could add it to the SpiBuilder docs, maybe like so:

/// Builder to create an SPI interface.
///
/// # Default parameters
///
/// By default, this builder will create an SPI interface with the following default parameters:
/// - **mode**: Mode 0
/// - **frequency**: 100 kHz
///
/// To override these, use the [`mode`] and [`frequency`] methods, respectively.
pub struct SpiBuilder<'a, T> {

if / when we do that we need to come up with another idea since we need to keep the mutable reference to avoid the user messing up things

I'm not an expert on this sort of thing, but isn't that what PhantomData could be used for? I see your point though.

Also there's another small flaw with the current builder: Since it doesn't track which functions have been called, I can add arbitrary amounts of each I/O line. Is that something that should be taken care of? Or do we just tell the user and trust they don't do anything funny with it?

[bjoernQ]

Yes, that is the kind of comment I was looking for 👍

I'm not an expert on this sort of thing, but isn't that what PhantomData could be used for? I see your point though.

PhantomData won't help here ... but anyway, it's nothing to worry about today

Also there's another small flaw with the current builder: Since it doesn't track which functions have been called, I can add arbitrary amounts of each I/O line. Is that something that should be taken care of? Or do we just tell the user and trust they don't do anything funny with it?

Ideally the correct usage would be enforced by type state but that's quite some effort manually and I don't think we should add a dependency for that - but I wouldn't stop you from looking into that 😄

[har7an]

Tried implementing type state manually, but it doesn't seem to work the way I'd expect it to... :/

Do you happen to have a good resource on implementing type state? I took the one from the embedded-rust book as template.

[bjoernQ]

Do you happen to have a good resource on implementing type state? I took the one from the embedded-rust book as template.

There are quite some articles on it - e.g https://www.greyblake.com/blog/builder-with-typestate-in-rust/ (not sure it's good - just a random Google result - best to google for "rust type state safe builder pattern" and you should get some good resources)

[jessebraham]

I've converted this to a draft just because it's not ready to merge in its current form. Feel free to change it back or close it at your leisure, whichever is the appropriate choice.