Reconfigure Spi new functions for better type inference

[fu5ha]

Does what it says on the tin :) unfortunately a breaking change but it means we can get rid of the Spi::<_, _, _, 8>:: paradigm.

[jannic]

Seems to be a good tradeoff. In many cases, the word size will be 8 bits anyway, so calling the new API gets simpler. And for other word sizes, the change to Spi::new_with_data_size is easy.

[jannic]

I wonder if this method should be moved down to the impl<D: SpiDevice, P: ValidSpiPinout<D>, const DS: u8> Spi<Disabled, D, P, DS> block? It's a little bit strange to have the constructor for the bit-generic version in the 8-bit impl.

[fu5ha]

Done. The only awkward thing is that now there is already DS const generic from the impl block in scope, so I had to rename it on the function level parameter. But not that big of a deal.

[jannic]

I don't see any update on the branch on github. Did you perhaps forget to push it?

Regarding the function level parameter: I think you don't need it at all, you can use the parameter from the impl block:

impl<D: SpiDevice, P: ValidSpiPinout<D>, const DS: u8> Spi<Disabled, D, P, DS> {
  [...]
  pub fn new_with_data_size(device: D, pins: P) -> Spi<Disabled, D, P, DS> {
    [...]
  }
  [...]
}

Which makes me wonder: Is the new_with_data_size name confusing, if it's often possible to call it without specifying the data size at the call location? It's enough if the compiler can somehow infer the correct value for DS, eg. if you later store the value in some struct where the type is specified.

[fu5ha]

True, but I feel like storing this in a wider struct or somewhere it could be inferred would be relatively uncommon? Maybe I'm mistaken though.

[ithinuel]

It seems to me that choosing 8bits for the fn new a bit arbitrary.
I understand 8 bits were used as the default for the type before, but I don't think it makes it more correct 😛

[fu5ha]

To be fair I don't have that much embedded experience, but it seems like 8 bits as word size is extremely common in general and would at least not be particularly surprising

[jannic]

That was my assumption as well, but to be honest my experience with embedded peripherals is also quite limited.

[jannic]

I had a look at embassy-rp to see how they handle the word width. They don't. It's hard coded to 8 bits. 😄
https://github.com/embassy-rs/embassy/blob/main/embassy-rp/src/spi.rs#L87

[fu5ha]

Maybe a way to solve both comments @ithinuel and @jannic would be to just make fn new<const DS: usize> so you always have to specify DS on the function (if it can't be inferred), but at least now you wouldn't have to _ the other three type parameters. Thoughts?

[ithinuel]

I need to try a few thing, to make sure I'm not missing some inference subtleties.

EDIT: I made this: ithinuel@90c6c82
I only managed to get rid of the turbofish by introducing a sorta builder type.

Let me know your thoughts.

[jannic]

EDIT: I made this: ithinuel@90c6c82 I only managed to get rid of the turbofish by introducing a sorta builder type.

Let me know your thoughts.

I'm not convinced.
This is all about discoverability and ease of writing code. The current incantation, let spi = hal::spi::Spi::<_, _, _, 8>::new(pac.SPI0, (spi_mosi, spi_miso, spi_sclk));, isn't unreadable. It's just difficult to get right if you have to write it.

This is not really getting better with let spi = hal::spi::SpiBuilder::build::<8>(pac.SPI0, (spi_mosi, spi_miso, spi_sclk));.
In some cases (like in the on target tests) it even gets worse, as previously the parameters were inferred from the struct State definition, so the actual code was just let spi = hal::spi::Spi::new(pac.SPI0, (spi_mosi, spi_miso, spi_sclk));

I wonder if we could remove the DS parameter entirely? Sure, we'd lose a little bit of type safety. But not much, I don't think this prevents actual errors. However, removing it would prevent us from easily implementing the DMA traits with the correct word width.

[jannic]

Another option could be to make DS a conventional generic parameter, with different types for the available bit sizes. This would have the additional advantage that it would become impossible to set invalid bit sizes. The current API would allow to configure a device with a bit size of 0 or more than 16 bits. (That device would be unusable though, as the read and write methods are only defined for valid bit sizes.)

[ithinuel]

I'm not too attached to that parameter TBH. On a setup with multiple spi peripheral with the rp2040 as one controller on the bus, you may need to change the word size between different peripheral and this generic parameter does not allow for an easy implementation of that usecase.

[fu5ha]

I will close this for now, it may still be relevant but not so much work to revive or redo if we want it...