Spi with 16bit framesize

[unpaid-bill]

This is an attempt to implement issue #256.

This adds a PhantomData tag to the Spi objects, allowing them to be typed as u8 or u16 to indicate their data frame size. The existing constructor creates a Spi<..., u8> object with no change in syntax to not break the existing API. To convert between dataframe modes you must call frame_size_8bit() or frame_size_16bit() which consumes the old type and hands back the new one.

Example usage:

    let spi8 = spi::Spi::spi2(dp.SPI2, (sclk, miso, mosi), spi_mode, 100.khz(), clocks, &mut rcc.apb1);
    let spi16 = spi.frame_size_16bit();

So far there is no way to initialise directly into 16bit data frame mode. I couldn't think of an elegant way without breaking the current API, and didn't like the idea of just having spi::Spi::spi2_16b_mode(dp.SPI1..., so I'd love suggestions.

I'd love some feedback on my implementation as I am quite new to rust!

[TheZoq2]

Thanks for putting the work into doing this! I think we could reduce the duplication of code by the suggestion I left as a review comment.

Also, it would be great if you could add an entry to changelog.md about this :)

[TheZoq2]

What is the point of this impl block!

[unpaid-bill]

So this I'm not 100% sure on, but my understanding is by throwing in a dummy impl for transfer::Default, it somehow auto-implements the ability to use the transfer method if you also have impl ...::spi::FullDuplex. The current master already has this in there, I just duplicated it for <u8> and <u16> version, but am not certain how it works.

[TheZoq2]

Oops, looks like you have to press "Add review comment" for comments to be included 😉

Here is what I meant to write:

There is quite a lot of code that is duplicated between the u16 and u8 versions. Essentially, the only thing that differs is the actual reads and writes: unsafe { ptr::write_volatile(&self.spi.dr as *const _ as *mut u16, *word) } and possibly some configuration parameters. To unduplicate the rest, we could add a trait like this:

trait<T> ReadWrite<T> {
    fn read_data(&mut self) -> T;
    fn write_data(&mut self, data: T);
    fn setup(&mut self);
}

impl ReadWrite<u8> for Spi<..., u8>{
    fn read_data(&mut self) -> T {
        // implementation
    }
    fn write_data(&mut self, data: u8) {
        unsafe { ptr::write_volatile(&self.spi.dr as *const _ as *mut u8, *word) }
    }
    fn setup(&mut self) {
        // implementation
    }
}

with a similar impl for the u16 version.

Then you can replace the register reads with those functions in a generic impl block, and most code will stay unduplicated

I hope that explanation made sense, let me know if you have any questions!

[unpaid-bill]

Great, thanks for the feedback! I'll have another crack

[unpaid-bill]

I've separated out the duplicated code into generic impls where I thought it made sense. The SpiReadWrite trait is really only required to still implement the custom ::blocking::spi::Write implementation from master, otherwise it could be made generic. If I make it ::blocking::spi::Write<FrameSize>, it conflicts the the generic definition already in embedded-hal which I'm not sure how to override (if you even can).

I haven't tested it on hardware yet (I have a STM32F103RB-NUCLEO to test the spi ports in a loopback) so will do that this week.

I also haven't touched DMA yet, so currently the DMA functionality is only implemented for the 8bit dataframe mode. I'm not sure if that should be its own PR.

Also, I saw this note in the code comments:

// NOTE(read_volatile) read only 1 byte (the svd2rust API only allows
        // reading a half-word)
        return unsafe { ptr::read_volatile(&self.spi.dr as *const _ as *const u8) };

However I haven't been able to verify that myself. Reading the dr as *const u16 appeared to work in earlier testing the other week, so I'm not sure if this note is true.

I'll add to the changelog once I've had a chance to test it on hardware.

[TheZoq2]

Looks good to me, nice job! Let me know when you've tested on hardware

[TheZoq2]

Since we don't have a write_slow or similar, I think we could rename this to just write as it is currently, and remove the point in the comment about this being faster than something else.

[unpaid-bill]

I was thinking to differentiate it from the embedded_hal::blocking::spi::Write<T> function fn write(&mut self, words: &[T]), which calls write_fast(). There is a naming conflict if they're both named write. Perhaps spi_write or something similar would be more appropriate?

[TheZoq2]

Oh right, that's a good point. I think spi_write might be a bit clearer.

[TheZoq2]

Also, I saw this note in the code comments:

From what I can tell, that's not relevant anymore. I pressume this used the stm32f1 crate to read the register in the past, but at some point we moved over to the pointer read which has no such imitations and someone just forgot to remove the comment :)

I also haven't touched DMA yet, so currently the DMA functionality is only implemented for the 8bit dataframe mode. I'm not sure if that should be its own PR.

I think we can leave it as a separate PR, better to have one feature now and one later than none now ;)

We should make sure you can't create a DMA version that gives u8 from the u16 version though

[unpaid-bill]

Tested on my Nucleo and successfully sent and received data in a loopback in both 8 and 16 bit modes. I think this is good to go.

[TheZoq2]

Awesome! I think there is one small problem left: as far as I understand, this is a breaking change, so a small edit to the changelog is required :)

[unpaid-bill]

That should do it :)

[TheZoq2]

Lovely, thanks!