spi: make SpiDevice transaction take an operation slice instead of a closure.

[Dirbaio]

As discussed in today's WG meeting.

• Fixes SPI implementation doesn't follow embedded-hal requirements for CS linux-embedded-hal#87
• Gets rid of the cursed borrow issues in async.
• Makes the trait more consistent with I2C.

[eldruin]

I think this is an interesting idea, thank you!
However, I do not have everything discussed in issues like #35, #350, #351, #245 and #299 in my mind.
Those should be considered for evaluation.

TODO:

• Update changelog

[ryankurte]

hey seems reasonable to me! a couple of questions:

• how do we represent busses with actual hardware CS, pass through a NullCS of some sort?
• can we test this with a PR against linux-embedded-hal as well as another hal and a driver?

[Dirbaio]

how do we represent busses with actual hardware CS, pass through a NullCS of some sort?

implementations with hardware CS would directly implement SpiDevice, not SpiBus. You dont take a "SpiBus with hardware CS" and then wrap it on an ExclusiveDevice with NullCS.

[Dirbaio]

can we test this with a PR against linux-embedded-hal as well as another hal and a driver?

• esp-idf-hal updated here Embedded-Hal PR - incoorparate e-hal master changes in spi / i2c / gpio / delay  esp-rs/esp-idf-hal#224 (thanks @Vollbrecht !)
• embassy-nrf updated here Update embedded-hal embassy-rs/embassy#1306
• nrf70 driver updated here wip: update embedded-hal. embassy-rs/nrf70#1 (WIP because waiting for embedded-hal and embassy, the SPI stuff is already working). Note that it's much nicer especially with async, it gets rid of the unsafe.
• I'm not familiar with the linux SPI API, hopefully someone else can do linux-embedded-hal.

@ryankurte @eldruin @therealprof could you please take a look? I'd really like to get this unblocked :)

[eldruin]

@Vollbrecht made an interesting comment about read_transaction but it seems only a mild HAL-impl-side inconvenience:

fn read_transaction(&mut self, operations: &mut [&mut [Word]]) -> Result<(), Self::Error>;

Its a lot of duplication here from transaction. The problem is the transaction takes an &mut [Operation<'_, u8>] but this read_transaction is &mut [&mut [u8]]. We would need something that converts the &mut [u8] in an Operation::Read

It seems to me like this should be fine to implement on linux-embedded-hal but we should check on it.

[ryankurte]

aight thanks for all your work / bearing with me / lgtm!

note that one consequence of the move from closures is that it is no longer possible to represent devices that need to do things within as CS assertion using SpiDevice, where this behaviour is required drivers should instead bind SpiBus with an OutputPin (ie. implement software/userspace CS control themselves)

It seems to me like this should be fine to implement on linux-embedded-hal but we should check on it.

worst case we can duplicate the spidev call which has no problems with a list of arrays (in fact, i believe we already have to throw around some vecs to map from operations to the actual ioctl)

[Dirbaio]

@eldruin: @Vollbrecht made an interesting comment about read_transaction but it seems only a mild HAL-impl-side inconvenience:

Yes, it's somewhat annoying but I think it's better than taking a &[Operation] and then asserting at runtime that it has only reads (or only writes). And I can't think of any other solution...

The duplication also shows up in #444 . It won't affect most HALs though, as they'll likely implement only SpiBus and let the user use embedded-hal-bus to get an SpiDevice. The only reasons a HAL would implement SpiDevice is for hardware CS (which is very annoying and the perf benefits are questionable in MCUs), or when the kernel handles CS for you (like in esp-idf or Linux).

@eldruin: It seems to me like this should be fine to implement on linux-embedded-hal but we should check on it.

Yeah I think so too. It has to convert from either &[Operation] or &[&[u8]] to the linux ioctl struct anyway...

[eldruin]

We discussed this in this week's meeting.
As @ryankurte also mentioned, there are some things that are not possible anymore.
@GrantM11235 Came up with two interesting examples of these:

// with the slices approach we would need to create copies of the data
fn send_data(&mut self, data: &[u16]) {
    self.spi.transaction( |bus|
        for chunk in data.chunks(32) {
            let mut buffer = [0_u16; 32];
            let mut buffer = &mut buffer[0..chunk.len()];
            buffer.copy_from_slice(chunk);

            for word in buffer {
                *word = word.to_be();
            }

            let buffer = transmute_u16_to_u8(buffer);

            bus.send(buffer);
        }
    )
}

// this version is more efficient than what would be possible with slices
fn send_packet(&mut self, packet: &[u8]) {
    let mut control_flow = Continue;
    while control_flow == Continue {
        self.spi.transaction( |bus| {
            let mut first_byte = [COMMAND_SEND];
            bus.transfer_inplace(&mut first_byte);
            let [status_byte] = first_byte;

            if check_can_send(status_byte) {
                bus.write(packet);
                control_flow = Break;
            }
        });
        // maybe add a delay here
    }
}

The MAX3421E driver might benefit from the closure-based approach.

On balance, it is clear that the slice-based approach is simpler to understand, to implement and to use while probably covering > 90% of the use cases as well as resolving the problem with the linux implementation.
As such, let's take the slices-based approach for 1.0 and let's discuss re-adding a closure-based approach in a future release.

Thank you everybody for your work, insights and the fruitful discussions.

bors r+

[bors]

Build succeeded:

• clippy
• fmt
• test (1.59.0, thumbv6m-none-eabi)
• test (1.59.0, thumbv7m-none-eabi)
• test (1.59.0, x86_64-unknown-linux-gnu)
• test (nightly, thumbv6m-none-eabi)
• test (nightly, thumbv7m-none-eabi)
• test (nightly, x86_64-unknown-linux-gnu)
• test (stable, thumbv6m-none-eabi)
• test (stable, thumbv7m-none-eabi)
• test (stable, x86_64-unknown-linux-gnu)