Added transactional SPI interface

[ryankurte]

This PR adds a transactional interface for SPI devices (#94), compatible with linux spidev.

Split from #178 as I believe this is complete and useful, but that there is more experimentation required before (if?) the I2C component is landed, check there for previous reviews / discussion.

Demonstrated in:

• Linux embedded hal: Add transactional SPI linux-embedded-hal#35
• STM32F4xx-hal: Added default transactional spi impl stm32-rs/stm32f4xx-hal#167
• embedded-spi driver abstraction (previously provided a polyfill for equivalent transactional functionality) https://github.com/ryankurte/rust-embedded-spi/pull/4/files#diff-74eea42f4e5e15399ac9184c8f2727a9R344
• sx128x radio driver: Adding support for transactional SPI rust-iot/rust-radio-sx128x#5

Notes:

• Operation::Transfer uses one buffer to allow polyfill using the existing Transfer trait (with the convenient side effect of reducing memory requirements)
• W has a static bound as it should only ever be a type with static lifetime (u8, u16 etc., not a reference), and to differentiate this from 'a which is the lifetime of the data in the object and only bound to the function
• exec(.., &mut [Operation]) is chosen over exec<O: AsMut<[Operation]>(..) as the latter imposes limits on generic types using this trait (which i ran into, see E0038)

cc. @rust-embedded/hal folks, @eldruin, @RandomInsano, @Rahix, @austinglaser for opinions / review

[rust-highfive]

r? @therealprof

(rust_highfive has picked a reviewer for you, use r? to override)

[ryankurte]

bors try

[bors]

try

Build succeeded

• continuous-integration/travis-ci/push

[eldruin]

Thank you for this! So far looks good to me.

The only point where I am not completely sold is the single array for Operation::Transfer(&'a mut [W]).

This works fine for the Linux kernel (and probably any situation with a kernel with user / kernel space separation) due to forced copying and doing it at different timepoints as we discussed in rust-embedded/linux-embedded-hal#33. Additionally it saves some memory.

However, I also see the appeal of having separate arrays for tx/rx so that the tx array can be immutable.
Imagining some kind of API with a function to send/receive some data I would expect the sent data to be immutable (just my individual expectation).
If I later have a library that consumes that API and sends the data via SPI, Operation::Transfer(&'a mut [W]) would force me to copy the data. With separate arrays I would not need to copy it.

However, this is only a thought and a very particular situation. I have not actually encountered this and it is probable that in this hypothetical situation I cannot just pipe the data through but some kind of manipulation is required and then it wouldn't matter anymore as I would need to copy it.

Can somebody think of such a situation where the data copy would actually be relevant? Something like DMA-based SPI implementation or so?

Anyway, thanks for keeping at it! I look forward to the transaction-based approach. It is already a lot of fun to use in embedded-hal-mock :)

[ryankurte]

thanks for the feedback!

Imagining some kind of API with a function to send/receive some data I would expect the sent data to be immutable (just my individual expectation).

• ime this usually tends to be an Operation::Write followed by an Operation::Transfer anyway? in which case your Write section can be immutable
• arguably the cost of setting copying your immutable output whatever into the transfer buffer should be reasonably low, and with a DMA based approach you're going to have to copy your immutable library data into an appropriately aligned DMA buffer anyway.
• this (single buffer) approach is reasonably implementable on spidev and using the existing Transfer trait (as well as consistent with this) where the split buffer approach is difficult / maybe impossible without allocation.
• using split buffers also makes writing a higher level API that takes an arbitrary slice difficult, as you then need to somehow allocate an additional arbitrarily sized [0u8; N] to go along with your transaction, which may not be viable without alloc since we don't have dynamically sized stack objects (like C99 with int whatever[N] = {0};)

[eldruin]

• Operation::Write + Operation::Transfer seems interesting but that would send data twice, right?
• I have also had a fair amount of pain due to unknown/generic buffer sizes and allocations. Any mitigation would be great.

[ryankurte]

Operation::Write + Operation::Transfer seems interesting but that would send data twice, right?

if you did it twice, yeah, but most spi interactions tend to be command then response / data, for which [Operation::Write(command), Operation::Transfer(data)] works nicely. you can see examples of how to implement this on the proposed trait here

I have also had a fair amount of pain due to unknown/generic buffer sizes and allocations. Any mitigation would be great.

afaik (and i have used it reasonably extensively) the approach used in this PR does not cause any of these issues.

[eldruin]

Operation::Write + Operation::Transfer seems interesting but that would send data twice, right?

if you did it twice, yeah, but most spi interactions tend to be command then response / data, for which [Operation::Write(command), Operation::Transfer(data)] works nicely. you can see examples of how to implement this on the proposed trait here

Is it? Maybe for async-style interactions but from drivers I have written, for example DS3234 and BMI160 expose a simple write/read interface, similar to I2C's so this would not be an option.
Anyway the point is whether immutable tx data is somehow more preferable. i.e. Transfer(&[W], &mut [W]).

I have also had a fair amount of pain due to unknown/generic buffer sizes and allocations. Any mitigation would be great.

afaik (and i have used it reasonably extensively) the approach used in this PR does not cause any of these issues.

Yes, I was trying to highlight a benefit of this new approach in contrast to the current fn transfer<'w>(&mut self, words: &'w mut [W]) -> Result<&'w [W], Self::Error>; where a buffer is returned. Apologies for not being clear enough.

[ryankurte]

Is it? Maybe for async-style interactions but from drivers I have written, for example DS3234 and BMI160 expose a simple write/read interface, similar to I2C's so this would not be an option.

AFAIK your interface methods here and here are functionally equivalent to the transactional version here, except the transactional version supports read/write over slices (without allocation) as well as single bytes (and I manage CS elsewhere).

Anyway the point is whether immutable tx data is somehow more preferable. i.e. Transfer(&[W], &mut [W]).

Where the transfer is simplex, the same thing can be achieved using multiple operations as illustrated in the previous paragraph, and the immutable tx data creates a whole pile of almost insurmountable problems so, I do not think it preferable (or even viable) at this stage.

I was trying to highlight a benefit of this new approach in contrast to the current fn transfer<'w>(&mut self, words: &'w mut [W]) -> Result<&'w [W], Self::Error>; where a buffer is returned

Ahh, I completely missed that it was the return that was significant here. I am not opposed to passing the &mut [Operations] object through as a return, but, I do not think either way offers any particular benefit?

[eldruin]

Is it? Maybe for async-style interactions but from drivers I have written, for example DS3234 and BMI160 expose a simple write/read interface, similar to I2C's so this would not be an option.

AFAIK your interface methods here and here are functionally equivalent to the transactional version here, except the transactional version supports read/write over slices (without allocation) as well as single bytes (and I manage CS elsewhere).

I am sorry I think we are having a misunderstanding. I do not understand how this would be equivalent. I call transfer which sends [register, 0], then read the output data with result[1]. I understand that this would be the equivalent of:

let ops = [spi::Operation::Transfer(data)]; // send and read data

This would totally work fine for me.
I do not understand how this would be equivalent to:

let ops = [spi::Operation::Write(prefix), // send prefix
           spi::Operation::Transfer(data)]; // send and read data

Since some data is sent twice. What am I missing?

Anyway the point is whether immutable tx data is somehow more preferable. i.e. Transfer(&[W], &mut [W]).

Where the transfer is simplex, the same thing can be achieved using multiple operations as illustrated in the previous paragraph, and the immutable tx data creates a whole pile of almost insurmountable problems so, I do not think it preferable (or even viable) at this stage.

Ok, if immutable tx data would cause problems then I am fine with the current approach.

I was trying to highlight a benefit of this new approach in contrast to the current fn transfer<'w>(&mut self, words: &'w mut [W]) -> Result<&'w [W], Self::Error>; where a buffer is returned

Ahh, I completely missed that it was the return that was significant here. I am not opposed to passing the &mut [Operations] object through as a return, but, I do not think either way offers any particular benefit?

Again just a misunderstanding. I wanted to highlight a benefit from the transactional approach (this PR) as-is. I was referring to the transactional approach when I said "this new approach", sorry I was not clear enough.

[ryankurte]

Again just a misunderstanding. I wanted to highlight a benefit from the transactional approach (this PR) as-is. I was referring to the transactional approach when I said "this new approach", sorry I was not clear enough.

Aha, thank you for the clarification and for baring with me there! I appreciate your effort and had totally misunderstood 🙃

I do not understand how this would be equivalent ...

At the moment you have something like:

fn read_reg(&mut self, reg: u8) -> Result<u8, Error> {
    let mut data: [u8; 2] = { reg, 0 };
    let ops = [spi::Operation::Transfer(&mut data)]; // Send reg, read value
    ...
    Ok(data[1])
}

However, you're basically writing the register, throwing away the first byte response, then reading the next byte, which could also be represented as something like:

fn read_reg(&mut self, reg: u8) -> Result<u8, Error> {
    let mut data = [0u8; 1];
    let ops = [
        spi::Operation::Write(&[reg]), // Send reg
        spi::Operation::Transfer(&mut data), // Read value
    ]
    ...
    Ok(data[0])
}

In this instance the prefix is only one byte, and you're only returning one byte, so the [u8; 2] approach works and there's no clear advantage to the transactional approach.

As a more complex example, with 16-bit LE addresses and multi-byte reads I tend to use something like:

fn read_reg(&mut self, reg: u16, values: &mut[u8]) -> Result<(), Error> {
    let prefix = [
        reg as u8,
        (reg >> 8) as u8,
    ];
    let ops = [
        spi::Operation::Write(&prefix), // Send reg
        spi::Operation::Transfer(values), // Read value(s)
    ]
    ...
    Ok(data[0])
}

Which can be extended to support arbitrary length immutable prefixes if it's useful, and afaik all of these approaches are equivalent, does that help clarify?

[eldruin]

Aah! It does. I got it now, thank you. I'm all for the merge! :)

[ryankurte]

how long did it take me to notice the rebase had added try_ prefixes? too long... need to update demonstration PRs to match, but, now we're really out of sync against existing HALs so this is an ever increasing nightmare.

[therealprof]

Time to land the breaking changes then?

[eldruin]

Other than my comment, looks good to me. thanks!

[eldruin]

Looks good to me, thanks!
@therealprof would you also agree to merging this?

[therealprof]

LGTM, too. Let's land this!

[therealprof]

bors r+

[bors]

Build succeeded:

• ci-linux (1.35.0, x86_64-unknown-linux-gnu)
• ci-linux (stable, thumbv6m-none-eabi)
• ci-linux (stable, thumbv7m-none-eabi)
• ci-linux (stable, x86_64-unknown-linux-gnu)

[therealprof]

@ryankurte And as usual the CHANGELOG entry was forgotten (and also as usual that was not noticed until after the fact 😅). Would you mind adding one?