Model behind SPI trait is incompatible with SPI peripherals on some platforms about embedded-hal HOT 14 CLOSED

@df5602 Thanks for your feedback! I don't think this is actually a problem since in the end the application will always be responsible for proper hardware setup and configuration. I think it is a bad driver design to make assumptions about a particular hardware setup, e.g. requiring that a component implementing another trait is required for correct operation because, as you said, things like SS or RST may or may not be handled in hardware, not be connected at all or tied to a specific level in the hardware design.

OTOH your suggestion with the trait extension will also not work because ideally there should be a way to share a bus between multiple components/drivers, which requires the use of dedicated SS pins per component (that's the whole idea behind the use of SS signals anyway).

Coming to think of it, maybe to proper way to address the sharing and CS implementation would be a new trait allowing to split a bus between multiple drivers while specifying additional hardware specifics allowing to arbitrate the bus...

from embedded-hal.

Fixed by #351 . Such devices can now implement the SpiDevice trait instead of SpiBus. There's no need for a dummy CS pin anymore.

from embedded-hal.

I think it is a bad driver design to make assumptions about a particular hardware setup, e.g. requiring that a component implementing another trait is required for correct operation because, as you said, things like SS or RST may or may not be handled in hardware, not be connected at all or tied to a specific level in the hardware design.

But isn't this basically the case now? The current SPI abstraction seems to assume that the CS is a separate entity that can be controlled independently from the SPI. (Worse, the current examples [0] around SPIs seem to suggest you take something that implements OutputPin as your chip select, which may or may not be in line with the software programming model of a specific processor.) Making the CS the responsibility of the SPI driver would remove that assumption, since the CS could be basically anything and the platform- and/or board-specific SPI implementation would handle it.
As a consequence, the device driver wouldn't have to worry about the chip select, only about the semantics of the SPI transfer (e.g. should the CS line toggle between words or not..)

OTOH your suggestion with the trait extension will also not work because ideally there should be a way to share a bus between multiple components/drivers, which requires the use of dedicated SS pins per component (that's the whole idea behind the use of SS signals anyway).

I don't see how this objection follows from my suggestion. You could e.g. have an instance of an SPI driver per device. Whether the constructor of the SPI driver takes an output pin to use as a chip select or e.g. an enum that specifies which of the available chip selects to use would then be an implementation detail of the platform- and/or board-specific SPI driver.
You would of course have to ensure mutual exclusion between multiple users of the SPI bus, but that's something you have to worry about regardless of how you structure your driver..

[0] see e.g. http://blog.japaric.io/brave-new-io/#hal-traits

from embedded-hal.

But isn't this basically the case now? The current SPI abstraction seems to assume that the CS is a separate entity that can be controlled independently from the SPI.

No, not really. By passing it to the driver you're still controlling what happens when it is used. If your CS is hardwired (or handled differently in hardware) you might as well just pass in a no-op implementation of the OutputPin trait. The driver does not need to know anything about the hardware it is being used on.

Whether the driver author is handling CS internally or not is personal decision. The author might as well keep it completely out of the driver.

Making the CS the responsibility of the SPI driver would remove that assumption, since the CS could be basically anything and the platform- and/or board-specific SPI implementation would handle it.

No, the issue here is that the SPI HAL implementation is chip and not board specific. So the designer and/or user of the application decides which and how peripherals are used, not the author of the HAL.

I don't see how this objection follows from my suggestion. You could e.g. have an instance of an SPI driver per device.

No, you can't. Only one driver can "own" a peripheral in Rust, the only thing one could (and probably should) do is create a multiplexing driver that hands out instances implementing the SPI traits while managing a single access to the real hardware.

from embedded-hal.

I've ran into this problem recently as well.

SpiDev (e.g. linux on raspberry) manages Chip Select (CS) pins automatically, so one could pass a no-op pin as chip select to the driver.

However, lot of peripheral supports "BURST" SPI read/write, where you address the first register in the first byte and then just reading/writing the rest of the data, without changing CS.

So driver implementations will not work for the noop-pin cases in such cases, for example:
(source)

I'll decorate it with comments:

// enable CS, but it is a noop, so does nothing
self.ncs.set_low();
// write the address. Internally:
// CS will be selected, the byte will be written to the bus, CS will be deselected
self.spi.write(&[Instruction::WBM.opcode()])?; 
// write the buffer. Internally:
// CS will be selected, the buffer will be written to the bus, CS will be deselected
// Target peripheral will interpret the first byte as address, and the rest: as burst data.
// Who knows registers it will overwrite?
self.spi.write(buffer)?;
// disable CS, but it is a noop
self.ncs.set_high();

Workarounds:
A. transmit bytes one-by-one (doubles the traffic, makes two times slower)
B. do not use chip select pin, but use a dedicated IO pin (one wasted pin, could be a blocker on small devices)
C. clone the buffer into a bigger one that is prepended with the address (memory overhead - you need fixed size buffer. In the case of the Ethernet driver it could mean 8K buffer...).

(I'll go with "A" for now, because my driver will be used both on RPi and stm32... but it is ugly)

The current SPI trait encourages such buggy implementations, so how could we avoid these situations? The problem is that some hardware/platform does not allow the control of CS pin at all.

Let me know if I got something wrong, or if you have better solution.

UPDATE: I might go with "B", the bit banged CS variant, because the hardware CS might be overriden to be a GPIO, but needs a PoC.

from embedded-hal.

@susu You can create your own dummy GPIO by implementing the required traits as nops.

from embedded-hal.

That is what I'm already doing, the bug arises when I use the aformentioned "no-op CS" pin with burst SPI transfer. It will not be a single burst transfer but a single byte and then a garbage burst transfer that could even overwrite config registers.

Maybe my comment was not clear enough. It should be interpreted in the context of this "no-op CS" / "dummy GPIO" / "nop pin" / whatever we call it.

from embedded-hal.

If you're using hardware magic then you'll have to stick to the rules imposed by it, the driver should not have to care about such implementation specific details. The only two options I see are changing the HAL to either disable the hardware CS and going for a software managed CS, or create a special "OutputPin" that will tell the SpiDev when to assert/release the CS.

from embedded-hal.

Thanks!

Checked some SPI driver in the linux-kernel, they are doing so much bit-banged CS... I thought hardware-driven CS is much more common. I'll go forward with software managed CS.

from embedded-hal.

Plenty of MCUs also have the option of doing some protocol specific handling (even for simple protocols) in hardware but it's very often the best to just disable it and do it in software since the performance improvement is negligible and there're typically very nasty corner cases and bugs which require nasty workarounds or special consideration... Of course the situation changes a bit if the feature in question is a rather heavyweight operation like hashing or checksumming.

from embedded-hal.

FWIW, just bringing up my experiences with SPI in Linux for pscontroller:

1. I did rely originally on the hardware CS. Device timing may also require driver control as there are per-byte ACKs in my case, and the NTC CHIP did mean things like cutting off the CS pin early.
2. Adding an input pin to the driver wasn't difficult, and is required for more than one device as stated by @therealprof earlier.

from embedded-hal.

I've been buried in SPI drivers recently and had an experience that I think might be relevant.
As is mentioned above, with variable length SPI transfers, you typically want to:

1. Assert CS
2. Write (fixed length) command
3. Write or Read (variable length) argument
4. De-assert CS

If the SPI driver controls CS automatically, you have to join / copy / manage the transfer as one chunk, which is useful for DMA but also requires a buffer array of whatever maximum size. With manual control of CS, you don't need to manipulate arrays at all, leading to something like this:

impl<E, SPI, OUTPUT, INPUT> Driver<SPI, OUTPUT, INPUT>
where
    SPI: spi::Transfer<u8, Error = E> + spi::Write<u8, Error = E>,
    OUTPUT: OutputPin,
    INPUT: InputPin,
{

/// Create new driver inst
pub fn new(spi: SPI, sdn: OUTPUT, cs: OUTPUT, gpio: [Option<INPUT>; 4]) -> Result<Self, E> {
    ...
}

/// Read data from a specified register address
/// This consumes the provided input data array and returns a reference to this on success
fn reg_read<'a>(&mut self, reg: Registers, data: &'a mut [u8]) -> Result<&'a [u8], E> {
    // Setup read command
    let out_buf: [u8; 2] = [SpiCommand::Read as u8, reg as u8];
    // Assert CS
    self.cs.set_low();
    // Write command
    match self.spi.write(&out_buf) {
        Ok(_r) => (),
        Err(e) => {
            self.cs.set_high();
            return Err(e);
        }
    };
    // Transfer data
    let res = match self.spi.transfer(data) {
        Ok(r) => r,
        Err(e) => {
            self.cs.set_high();
            return Err(e);
        }
    };
    // Clear CS
    self.cs.set_high();
    // Return result (contains returned data)
    Ok(res)
}
}

I'm sure there are simpler ways of expressing it, but it seems to work quite well.

Using a no-op with automagic CS implementation with this approach would result in incorrect behavior, so I don't think that's a particularly good solution / one we should recommend.

I personally lean towards the CS should always be explicit camp, it'll double the transactions required (if you're using this command structure) but remove the need for copy buffers, and if someone needs it down the line we could introduce an separate trait for SPIAutoCS.

from embedded-hal.

@ryankurte I see where you're coming from, but I definitely don't like CS being fully manually managed. I think your example is actually an excellent example of why that can be a bad thing -- you've had to explicitly include calls to cs.set_high() on each error return path.

Something in the middle could be useful -- perhaps a callback-based interface, where you provide a closure to be called with CS held low and thus can perform multiple operations at once.

I think there's also been some work lately on supporting an iterator-based interface, which could address the same problem orthogonally -- chaining a couple of iterators is more ergonomic (and perhaps more performant) than copying into a shared buffer.

from embedded-hal.

As mentioned, I don't think the poorly written example undermines the utility of being able to control CS explicitly, especially where you might need strobes and things, but I am not skilled enough at rust to express it more elegantly.

A callback-based interface could be interesting, I've been playing with closures and chaining but haven't managed to write one that works (I'm still at the "fighting the borrow checker" stage of learning rust).

I'm not a huge fan of the iterator approach because it seems unclear how that would interact with a DMA based SPI peripheral wrt. copy buffers etc. This seems less magic, doesn't require the copy buffer, and makes your sizes explicit through the API.

My best attempt so far is:

    /// Read a frame from the device
    pub fn read_frame<'a>(&mut self, data: &'a mut [u8]) -> Result<&'a [u8], DeviceError<E>> {
        // Setup read frame command
        let cmd: [u8; 1] = [device::FRAME_READ_FLAG as u8];
        // Assert CS
        self.cs.set_low();
        // Write command and transfer data
        let res = self.spi.write(&cmd).and(spi.transfer(data))
                    .map_err(|e| DeviceError::SPI(e) );
        // Clear CS
        self.cs.set_high();
        // Return result (contains returned data)
        res
    }

Which is not strictly correct because transfer gets called regardless of write success, but, it doesn't require any fighting the borrow checker.

from embedded-hal.