Embedded-hal: Non-destructive SPI transfer() call

[RandomInsano]

Hey all,

Currently the linux-embedded-hal implements its SPI transfer by overwriting the original buffer which saves memory allocations which is in my books.

I hit a problem though as I dabble with writing a driver for PlayStation controllers (taking a little break from the axp209 for a bit).

The PlayStation often sends the same commands to the controllers and so I'm finding myself doing a lot of data copying I shouldn't need to do:

    const CMD_POLL: &[u8] = &[0x01, 0x42, 0x00];

...

    // Needed because `copy_from_slice` doesn't work on different sized arrays for some silly reason
    fn byte_copy(from: &[u8], to: &mut [u8]) {
        assert!(from.len() <= to.len());

        for i in 0 .. from.len() {
            to[i] = from[i];
        }
    }

    pub fn read_buttons(&mut self) -> GamepadButtons {
        let mut buffer = [0u8; 21];
        let mut data = [0u8; 6];

        // here's the copy I'd like to avoid:
        Self::byte_copy(CMD_POLL, &mut buffer);
        self.dev.transfer(command)?;

        data.copy_from_slice(&buffer[3 .. 9]);

        let controller = ControllerData { data: data };

        unsafe {
            return controller.ds.buttons;
        }
    }

Ideally, I'd waste some extra bytes padding out my command to be the same as the RX buffer and then I wouldn't need the byte_copy() function.

Is there enough of a demand to create a non-destructive version that could re-use the output buffer?

Maybe something with this function definition?:

        fn transfer<'w>(&mut self, send: & [W], receive: 'w mut & [W]) -> Result<(), S::Error> {

Adding to the API isn't something to be taken lightly since there are more and more HAL implementations coming out, but I wonder what the demand is here. Are other people jumping through hoops because there isn't a function that maintains the output buffer?

[therealprof]

@RandomInsano You can avoid the byte_copy() function by sub-slicing your data to an equal length slice before calling copy_from_slice().

[RandomInsano]

There’s thinking... thanks!

[RandomInsano]

Heh. Now, how about avoiding the copy altogether?

[therealprof]

I'm not really a SPI aficionado so I don't have any real world based opinion about whether that would generally be useful or more addressing of a corner case.

[RandomInsano]

Oh, yeah. No problem there @therealprof. I just re-asked to address the larger audience. If I hadn’t, people who skim usually assume this issue is addressed and move on.

A bit of detail here is that the Linux-embedded-hal is using Spidev from the kernel which itself can does save the buffer, the embedded-hal API doesn’t usually support that.

I think some data diving is required here. I’ll go skim the other crates to see who’s fought with this.

[austinglaser]

I think one of the main advantages from a non-destructive variant would be to allow directly passing constant data for the write. On some architectures (ARM Cortex-M, at least), the this would allow write data to be read directly out of program memory. This won't actually save memory -- since a mutable buffer of the same length would still have to be allocated -- but it might have an ergonomic benefit, and possible a correctness one (allowing more use of non-mutable data is always a win, in my book).

It would avoid the necessity of two buffer-length copies per transaction (for repeated transfers, i.e. polling). I'm hesitant to assert that this would be a large gain in performance, though, since accessing memory is nearly always much faster than the serial transaction itself.

I think it would be a reasonable addition, and simple enough to add a default implementation for as with the current blocking transfer. I may take some time to throw a proof-of-concept together and open a pull request, so we can see how we like it in practice.

[RandomInsano]

I ended up abstracting away the ugliness, and really you’re right that memory isn’t going to be the bottleneck.

I’ll close it out if I ages out past 30 days with no real interest here.

[RandomInsano]

Closing in favour of https://github.com/rust-embedded/embedded-hal/issues/94