SimpleHalSpiDevice does not execute all operations in a single transaction

[whatisbyandby]

Hi,

I'm working on a project with a Raspberry Pi 5, and I'm trying to interface with a RFM69 radio chip using this library

https://github.com/almusil/rfm69

When using the SimpleHalSpiDevice the SPI operations are not all sent within the same transaction.

I'm able to reproduce the issue with this snippet

use embedded_hal::spi::{Operation, SpiDevice};
use rppal::spi::{Bus, Mode, Segment, SimpleHalSpiDevice, SlaveSelect, Spi};

fn main() {
    let spi = Spi::new(Bus::Spi0, SlaveSelect::Ss0, 1_000_000, Mode::Mode1).unwrap();

    let mut read_buff: [u8; 1] = [0x00];
    let write_buff:[u8; 1] = [0x01];

    // This works as I would expect, where all segments are transfered in a single transaction
    let mut segments = [Segment::with_write(&write_buff), Segment::with_read(&mut read_buff)];
    spi.transfer_segments(&mut segments).unwrap();

    let mut spi_device = SimpleHalSpiDevice::new(spi);

    // This doesn't work as I would expect, and the write, and read operations are executed in separate transactions
    let mut operations = [Operation::Write(&write_buff), Operation::Read(&mut read_buff)];
    spi_device.transaction(&mut operations).unwrap();

    println!("{:?}", read_buff);

}

Note that Channel 2 (purple) is the Chip Select line, and Channel 1 (yellow) is on the MOSI line.

Expected Result: Both transfers look the same where the CS line is held low for both the write and read operations

Actual Result: When using the SimpleHalSpiDevice the CS line goes high between each operation

[whatisbyandby]

I think the solution could be here, where the fn transaction is implemented for SpiDevice.

It seems like if instead of executing the operations directly, it could instead map the operations to segments, that could then be executed all together. I'll be happy to do this, and open a PR, but just wanted to make sure I'm on the right track first.

 fn transaction(
        &mut self,
        operations: &mut [embedded_hal::spi::Operation<'_, u8>],
    ) -> Result<(), Error> {
        for op in operations {
            match op {
                embedded_hal::spi::Operation::Read(read) => {
                    self.bus.read(read)?;
                }
                embedded_hal::spi::Operation::Write(write) => {
                    self.bus.write(write)?;
                }
                embedded_hal::spi::Operation::Transfer(read, write) => {
                    self.bus.transfer(read, write)?;
                }
                embedded_hal::spi::Operation::TransferInPlace(words) => {
                    self.bus.transfer_in_place(words)?;
                }
                embedded_hal::spi::Operation::DelayNs(us) => {
                    embedded_hal::delay::DelayNs::delay_us(&mut Delay::new(), *us);
                }
            }
        }
        Ok(())
    }

[golemparts]

Thanks for reporting this issue @whatisbyandby. It looks like you're correct, and the SimpleHalSpiDevice isn't actually performing a transaction, but rather executing the operations separately.

Mapping this to segments so transfer_segments can be used directly seems like the best approach to get this fixed. Thanks for the offer to submit a PR. I'd be happy to accept it.

[whatisbyandby]

Great! I'll get started, Thanks for the feedback.