avr-hal icon indicating copy to clipboard operation
avr-hal copied to clipboard

the SPI implementation does not support channel select pins other than pin 10 (PB2)

Open mutantbob opened this issue 3 years ago • 2 comments

I have a project with an Arduino Uno, an ethernet shield (v1), and an OV5642 SPI/I2C. There are actually 3 SPI peripherals in this setup, because the ethernet shield includes an SD card reader. The CS pins in use are 10: W5100 ethernet 4: sdcard reader 3: OV5642 (I could use something else since it is connected with jumper wires).

The current SPI implementation does not easily allow for use of SPI devices using a channel select pin other than 10.

The Spi type provided by https://github.com/Rahix/avr-hal/blob/e897783816437a677aa577ddfdaa34e9a1e86d96/mcu/atmega-hal/src/spi.rs#L25 only uses PB2 (pin 10). If a user attempts to create their own type

avr_hal_generic::impl_spi! {
    hal: atmega_hal::Atmega,
    peripheral: atmega_hal::pac::SPI,
    sclk: arduino_hal::hal::port::PB5,
    mosi: arduino_hal::hal::port::PB3,
    miso: arduino_hal::hal::port::PB4,
    cs: arduino_hal::hal::port::PB1,
}

The following error occurs:

error[E0117]: only traits defined in the current crate can be implemented for arbitrary types
   --> src/main.rs:273:1
    |
273 | / avr_hal_generic::impl_spi! {
274 | |     hal: atmega_hal::Atmega,
275 | |     peripheral:
276 | |     //avr_hal_generic::pac::SPI,
277 | |     atmega_hal::pac::SPI,
    | |     -------------------- `SPI` is not defined in the current crate
...   |
282 | |     cs: arduino_hal::hal::port::PB1,
283 | | }
    | | ^
    | | |
    | | impl doesn't use only types from inside the current crate
    | | `Atmega` is not defined in the current crate
    | | `PB5` is not defined in the current crate
    | | `PB3` is not defined in the current crate
    | |_`PB4` is not defined in the current crate
    |   `PB1` is not defined in the current crate
    |
    = note: define and implement a trait or new type instead
    = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)

How can I use the SPI functionality for multiple devices?

mutantbob avatar Mar 18 '22 13:03 mutantbob

I have prototyped an alternate SPI implementation by modifying the code that already exists to allow the channel-select and SCLK/MOSI/MISO pins to exist separately until it is time to transmit/receive data.

https://github.com/mutantbob/arduino-spi

I look forward to folks proposing improvements, but the old SPI implementation is definitely not usable.

mutantbob avatar Mar 21 '22 13:03 mutantbob

I think you've misunderstood how the Spi bus driver is meant to be used.

First of all, the chip select pin you pass to the constructor does not need to be used as a CS pin. In fact, CS management is meant to be handled entirely outside the Spi bus driver. The reason you have to pass the CS pin to the constructor (and then receive it back) is that the hardware requires the pin to be an output. If it is not, the spi peripheral does not actually work - it will switch to slave mode instead of operating as bus master. You can read up more on this in issue #27. Again, this is the only reason for passing this pin to the constructor at all.

Now, to actually do chip select operation, peripheral drivers currently need to contain their own manual implementation. The pattern is to consume a Spi bus + a GPIO pin to be used as the chip select line. So something along the lines of

pub struct MyPeripheralDriver<SPI, CSPIN> {
    bus: SPI,
    cs: CSPIN,
}

impl<SPI, CSPIN> MyPeripheralDriver<SPI, CSPIN>
where
    SPI: blocking::spi::Write<u8>, CSPIN: OutputPin,
{
    pub fn do_something(&mut self) {
        self.cs.set_low();
        self.bus.write(&[0xc0, 0xff, 0xee]);
        self.cs.set_high();
    }
}

This of course consumes the Spi bus so by itself it prevents sharing the bus between multiple peripherals. For this purpose, the shared-bus crate exists. It allows you to hand out multiple bus references to drivers of the above design:

let spi = Spi::new(...);
let spibus = shared_bus::BusManagerSimple::new(spi);

let cs1 = pins.d10.into_output();
let driver1 = Driver1::new(spibus.acquire_spi(), cs1);

let cs2 = pins.d6.into_output();
let driver2 = Driver2::new(spibus.acquire_spi(), cs2);

That said, this pattern has a number of issues related to concurrency. That's why there is a move towards a new design, as proposed upstream in https://github.com/rust-embedded/embedded-hal/pull/351. With this, CS management is finally moved out of the peripheral driver and will be handled at the layer where shared-bus sits in the code above. Once the new design has settled, avr-hal will also move to doing things this way of course.

Rahix avatar Mar 21 '22 21:03 Rahix