Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
mutantbob opened this issue Mar 18, 2022 · 2 comments
Labels
question Further information is requested

Comments

@mutantbob
Copy link
Contributor

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

pub type Spi = avr_hal_generic::spi::Spi<
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
Copy link
Contributor Author

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.

@Rahix
Copy link
Owner

Rahix commented Mar 21, 2022

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 rust-embedded/embedded-hal#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 Rahix added the question Further information is requested label Jul 4, 2022
Repository owner locked and limited conversation to collaborators Sep 2, 2022
@Rahix Rahix converted this issue into discussion #328 Sep 2, 2022

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants