Skip to content
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

SPI devices on shared bus (embedded-hal-bus) #925

Open
rland93 opened this issue Apr 23, 2024 · 4 comments
Open

SPI devices on shared bus (embedded-hal-bus) #925

rland93 opened this issue Apr 23, 2024 · 4 comments

Comments

@rland93
Copy link

rland93 commented Apr 23, 2024

Is there a canonical way to share SPI devices that are on the same bus using embedded-hal-bus?

All variants of shared SPI devices seem to follow a pattern like this:

  • create a SPI bus
  • wrap the bus into a type which manages its shared-ness
  • create new devices which consume the chip-select pins but take a reference to the bus that is mutable in some way (critical section, mutex, etc)
  • pass those new devices to the drivers for each device on the bus

I have tried RefCellDevice and CriticalSectionDevice (embedded-hal-bus), as well as ArbiterDevice from rtic-sync in this pattern. It looks like this:

mod app {

    // shared, local

    fn init(cx: init::Context) -> (Shared, Local) {
        // rcc, clocks

        // spi3 device setup
        let spi3 = .... 

        let gyro_cs: Pin<'B', 5, Output> = gpiob.pb5.into_push_pull_output();
        let acc_cs: Pin<'D', 2, Output> = gpiod.pd2.into_push_pull_output();

        let timer1: Delay<TIM2, 1000000> = dp.TIM2.delay_us(&clocks);
        let timer2: Delay<TIM10, 1000000> = dp.TIM10.delay_us(&clocks);

        // Wrap the bus in some type which allows it to be shared
        let bus = Arbiter::new(spi3);

        // Separate spi devices each owning their pins but sharing the bus
        let acc_spidev: AccSpiDev = ArbiterDevice::new(&bus, acc_cs, timer1);
        let gyro_spidev: GyroSpiDev = ArbiterDevice::new(&bus, gyro_cs, timer2);

        // acc_spidev and gyro_spidev are then consumed by the driver


        (Shared {}, Local {})
    }

    // task definitions

}

They all run into the same issue: the compiler complains that the borrowed bus does not live long enough. Makes sense: the bus goes out of scope when init() ends. But, that's not what we want! We want the bus to live forever so that the references to it stays valid through the life of the program. I cannot for the life of me figure out how to mark the bus as having a static lifetime or appeasing the borrow checker in some other way. Is there a standard way to do it?

@rland93
Copy link
Author

rland93 commented Apr 24, 2024

Well, just my luck (maybe?)

Looks like embedded-hal-bus came out with another implementation of SpiDevice, this one using an UnsafeCell and AtomicBool for a semaphore.

https://docs.rs/embedded-hal-bus/latest/embedded_hal_bus/spi/struct.AtomicDevice.html

Will try it out and report back here to see if it solves the issues here (seems like maybe it won't since this issue is about ownership, but they do call out rtic specifically so maybe I can get a read on how they intend it to be used)

@rland93
Copy link
Author

rland93 commented Apr 28, 2024

I was able to implement a workaround, like so:

#![no_main]
#![no_std]
#![feature(type_alias_impl_trait)]

use bmi088::Bmi088;
use core::ptr::addr_of;
use defmt_rtt as _;
use embedded_hal_bus::spi::{AtomicDevice, NoDelay};
use embedded_hal_bus::util::AtomicCell;
use panic_probe as _;
use rtic_monotonics::systick::Systick;
use rtic_monotonics::Monotonic;
use stm32f4xx_hal::{
    gpio, pac,
    prelude::*,
    spi::{Mode, Phase, Polarity, Spi, Spi3},
};

type Accel = AtomicDevice<'static, Spi<pac::SPI3>, gpio::Pin<'B', 5, gpio::Output>, NoDelay>;
type Gyro = AtomicDevice<'static, Spi<pac::SPI3>, gpio::Pin<'D', 2, gpio::Output>, NoDelay>;
type Spi3Bus = AtomicCell<Spi<pac::SPI3>>;

static mut SPI3BUS: Option<Spi3Bus> = None;

#[rtic::app(device = stm32f4xx_hal::pac, peripherals = true)]
mod app {

    use super::*;

    #[shared]
    struct Shared {}

    #[local]
    struct Local {
        accel: Accel,
        gyro: Gyro,
        spi3bus: &'static Option<Spi3Bus>,
    }

    #[init]
    fn init(cx: init::Context) -> (Shared, Local) {
        // Setup clocks
        let dp = cx.device;
        let rcc = dp.RCC.constrain();
        let systick_mono_token = rtic_monotonics::create_systick_token!();
        Systick::start(cx.core.SYST, 48_000_000, systick_mono_token);
        let clocks = rcc.cfgr.sysclk(48.MHz()).freeze();

        let gpiob = dp.GPIOB.split();
        let gpioc = dp.GPIOC.split();
        let gpiod = dp.GPIOD.split();

        let spi: Spi3 = Spi3::new(
            dp.SPI3,
            (
                gpioc.pc10.into_alternate(),
                gpioc.pc11.into_alternate(),
                gpioc.pc12.into_alternate(),
            ),
            Mode {
                polarity: Polarity::IdleLow,
                phase: Phase::CaptureOnFirstTransition,
            },
            1.MHz(),
            &clocks,
        );

        let (accel_dev, gyro_dev) = unsafe {
            SPI3BUS = Some(AtomicCell::new(spi));
            let bus = SPI3BUS.as_ref().unwrap();
            (
                AtomicDevice::new_no_delay(&bus, gpiob.pb5.into_push_pull_output()).unwrap(),
                AtomicDevice::new_no_delay(&bus, gpiod.pd2.into_push_pull_output()).unwrap(),
            )
        };

        imu_task::spawn().unwrap();

        (
            Shared {},
            Local {
                spi3bus: unsafe { addr_of!(SPI3BUS).as_ref().unwrap() },
                accel: accel_dev,
                gyro: gyro_dev,
            },
        )
    }

    #[task(local=[accel, gyro])]
    async fn imu_task(cx: imu_task::Context) {
        let mut accel = Bmi088::new_with_spi(cx.local.accel);
        let mut gyro = Bmi088::new_with_spi(cx.local.gyro);

        loop {
            let now = Systick::now();

            let c1 = accel.acc_chipid().unwrap();
            let c2 = gyro.gyro_chipid().unwrap();

            defmt::debug!("acc: {}, gyro: {}", c1, c2);

            Systick::delay_until(now + 200u32.millis()).await;
        }
    }
}

The sensor is a BMI088 which has an accelerometer and gyro which each have their own CS pins. Got around it by keeping a static ref SPI3BUS and passing that to the AtomicDevice constructor. I this should be safe, even if we have concurrent accesses, due to the AtomicDevice.

@korken89
Copy link
Collaborator

Hi, this is now solved in ArbiterDevice for SPI and I2C.
See this PR for the update #955

@pdgilbert
Copy link

@rland93 did you get this to work with ArbiterDevice or are you still using your workaround? (I'm looking for an actual working no_std example of Arbiter using any sensor with any stm32 hal.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants