Rahix / shared-bus

Crate for sharing buses between multiple devices
Apache License 2.0
129 stars 34 forks source link

Is it possible to use this library with RTIC? #4

Closed capnfabs closed 9 months ago

capnfabs commented 5 years ago

Hi there,

I'm a visitor from https://github.com/rust-embedded/embedded-hal/issues/35 :) I'm currently trying to use shared-bus in an RTFM project and am trying to late-initialise peripherals. I can't quite get it to work when using shared-bus, however, because the initial BusManager either goes out of scope at the end of the init method, or gets moved into init::LateResources, and therefore I can't figure out how to annotate the lifetime of the BusProxy. Here's a small-ish example:

//! examples/init.rs

#![deny(unsafe_code)]
#![no_main]
#![no_std]

extern crate panic_semihosting;

use stm32f0::stm32f0x0::I2C1;

use stm32f0xx_hal::{
    i2c::I2c,
    prelude::*,
    gpio,
    gpio::{
        Alternate,
        gpiob,
    },
};

use rtfm::app;

use shared_bus::BusManager;
use ssd1306::Builder;
use ssd1306::prelude::*;
use embedded_graphics::prelude::*;

type I2C1Bus = I2c<I2C1, gpiob::PB8<Alternate<gpio::AF1>>, gpiob::PB9<Alternate<gpio::AF1>>>;

#[app(device = stm32f0::stm32f0x0)]
const APP: () = {
    static mut I2C_BUS: BusManager<
        cortex_m::interrupt::Mutex<core::cell::RefCell<I2C1Bus>>, I2C1Bus> = ();
    static mut DISPLAY: ssd1306::mode::graphics::GraphicsMode<
        ssd1306::interface::i2c::I2cInterface<
            shared_bus::proxy::BusProxy<
                '_,  // this lifetime parameter is the one I can't figure out.
                cortex_m::interrupt::Mutex<core::cell::RefCell<I2C1Bus>>,
                I2C1Bus>>> = ();

    #[init]
    fn init() -> init::LateResources {
        let dp = device;

        let mut flash = dp.FLASH;
        let mut rcc = dp.RCC.configure().freeze(&mut flash);
        let gpiob = dp.GPIOB.split(&mut rcc);

        // Configure i2C pins
        let (scl, sda) = cortex_m::interrupt::free(|token| {
            (
                gpiob.pb8.into_alternate_af1(token),
                gpiob.pb9.into_alternate_af1(token),
            )
        });
        let i2c = I2c::i2c1(dp.I2C1, (scl, sda), 400.khz(), &mut rcc);
        let bus = BusManager::<cortex_m::interrupt::Mutex<_>, _>::new(i2c);

        let mut disp: GraphicsMode<_> = Builder::new().connect_i2c(bus.acquire()).into();

        init::LateResources {
            I2C_BUS: bus,
            DISPLAY: disp,
        }
    }
};

My questions are:

Also, I can't figure out if this is more an issue on the RTFM side or on the shared-bus side - I suspect that it's a limitation intrinsic to the way resources are initialised with RTFM. I'm still new to rust; please let me know if this seems like something I should be raising over there instead.

Thanks in advance.

Rahix commented 5 years ago

Hey!

Thanks for your interest! To be quite honest, I have never actually used RTFM so far, so the following might not be 100% correct ...

First of all, something unrelated to your issue but which is still very relevant: The cortex_m::interrupt::Mutex is a very bad idea when using RTFM, because it masks all interrupts and thus also disables higher priority tasks. Instead, RTFM should implement its own Mutex type which properly handles priorities. I made some effort over in rust-embedded/embedded-hal#132 to standardize the Mutex interface but this is still in the works (mostly blocked on me not having the time to continue right now ...). Once that is done, RTFM could (AFAIK) implement this trait and allow shared-bus to correctly handle priorities.

Now onto your actual issue: From your code I unfortunately think this is not trivially possible right now. The problem is that rust does not allow you to store a value and a reference to that value in the same struct (See this StackOverflow post).

is it possible to initialise i2c devices using SharedBus in the #[init] method of an RTFM application?

As explained in the link above, this can never work like you tried because you move the LateResources which would invalidate the references to BusManager. All hope is not lost, however (Unless RTFM does some magic that would disallow it ...): You can keep the BusManager at a stable address outside the RTFM resources by using a global. This is sound from a safety perspective because you never mutate the bus manager. You will however need unsafe to initialize the manager as there is no equivalent to once_cell for no_std right now.

In code, this would roughly look like this (untested):

// Note that I still used the cortex-m mutex here
// which does bad things to your realtime system
static mut I2C_BUS: Option<shared_bus::CortexMBusManager<...> = None;

fn init() -> init::LateResources {
    let i2c_bus = {
        let i2c = I2c::i2c1(dp.I2C1, (scl, sda), 400.khz(), &mut rcc);
        let bus = shared_bus::CortexMBusManager::<_>::new(i2c);

        unsafe {
            I2C_BUS = Some(bus);
            // This reference is now &'static
            &I2C_BUS.as_ref().unwrap()
        }
    };

    // Initialize all your drivers now
    let mut disp: GraphicsMode<_> = Builder::new().connect_i2c(bus.acquire()).into();

    init::LateResources {
        DISPLAY: disp,
    }
}

if not, is this something that you'd be open to design ideas / pull requests for?

Definitely! The solution above is not pretty ... I think the only real solution would be a once_cell equivalent for no_std, but if you have any other ideas, I would love to hear them!

capnfabs commented 5 years ago

Hi, thanks so much for the comprehensive writeup!

I tried what you suggested in this example, and it works excellently - thank you!

The cortex_m::interrupt::Mutex is a very bad idea when using RTFM, because it masks all interrupts and thus also disables higher priority tasks

Yep, I'm aware of this :) Thanks for making the effort to standardise the Mutex trait! I also don't have much experience with RTFM, but I'll have a play around with this in the coming days / weeks schedule-permitting and see if I can answer your question with regards to handling priorities correctly.

Thanks again for the help.

capnfabs commented 5 years ago

I guess I'll leave this issue open because it's still not entirely clear if this works with RTFM, due to the fact that it's unclear whether there's a good mutex solution available for RTFM.

mpasternacki commented 4 years ago

I'm having the same problem here. The issue, as I understand it, is that RTFM implements its own locks, with ceiling analysis etc, to ensure safe access – and shared-bus would want to have its own locks. I'm not sure it's possible without using cortex_m::interrupt::Mutex, disabling interrupts completely, and throwing RTFM preemption / priorities / real-time promises under the bus (pun not intended).

The "official" way in RTFM would be to use a shared resource with the item being locked; problem is that multiple devices are sharing a bus that needs to be locked safely. My initial idea was to ditch shared-bus and just pass the original bus in context, and making it a required parameter in calls to the individual devices' APIs. But then I can't use any existing drivers – crates with drivers for I²C devices consume whole bus for one device. I need either shared-bus, or my own drivers.

For my project, I figured I can have my cake and eat it (I think – if there's any obvious error in my approach, I'd appreciate the feedback). I have a struct that holds BusManager and all I²C devices I'm using (and performs a bit of lifetime trickery to keep the compiler happy). This struct is a single shared resource, RTFM manages access / locking to it, so every task that got it can safely access the bus (if it's a lower priority task, it has to use RTFM's lock on the whole bus+devices struct). This way, no locking is needed in the shared-bus (it's only used to be able to have more than one device) – I included a no-op implementation of BusMutex.

My current code is here: https://gist.github.com/mpasternacki/93c73d2f94630fbc356e28436e7f70ac

mpasternacki commented 4 years ago

Now I wonder if I even need shared-bus with this mechanism – wouldn't it be the same (and still safe) if I just unsafely cloned the controller in the "bus+devices" struct constructor?

Rahix commented 4 years ago

(and performs a bit of lifetime trickery to keep the compiler happy)

Hm, that looks like a very hacky solution ... It also would not work for any bus peripheral which is not zero-sized.

if I just unsafely cloned the controller in the "bus+devices" struct constructor?

This would lead to issues if the bus driver has some internal state (which would then be duplicated).

mpasternacki commented 4 years ago

(and performs a bit of lifetime trickery to keep the compiler happy)

Hm, that looks like a very hacky solution ...

It's hacky and it's a big tradeoff (the whole bus is locked per task, so it might not be useful for all solutions), but it seems to cooperate with RTFM rather than work against it or need to reimplement much of RTFM's logic.

But yeah, this might be more of a workaround than solution. Still, this issue is open since June, so I figured it might be useful to write it down.

It also would not work for any bus peripheral which is not zero-sized.

Why wouldn't it? The transmute calls only manually modify lifetimes, is there a copy operation I'm missing? Is it about Send for Bus<T> trait?

The only part I see that's zero-sized-dependent is the helper method at the bottom to unsafely get the raw device (pub unsafe fn lpi2c1 etc), but this is to work around another issue altogether and is clearly unsafe. This part shouldn't be in the example snippet, I've removed it.

if I just unsafely cloned the controller in the "bus+devices" struct constructor?

This would lead to issues if the bus driver has some internal state (which would then be duplicated).

True, I'd need to use some kind of cell here to actually share the instance.

Thanks for the feedback – and for the library! It's really useful, as each device driver seems to want to consume whole bus driver.

ryan-summers commented 4 years ago

Hey guys, I know this may be a little bit late, but I was also having this issue. What I discovered is that there is a safe API to do this without modifying shared-bus.

When RTIC (previously known as RTFM) finalizes resources in init(), it then moves them to 'static lifetimes. Essentially, this means that after init(), you can freely acquire references to the manager (e.g. you are free to use manager.acquire()).

An implementation employing this essentially stores the BusManager as an RTFM resource. Any devices that require the bus cannot be constructed until after the init() function completes, so they should instead be wrapped in an Option() and be set to None during init().

Once you first enter idle, the BusManager is now static and we are free to hand out references, so we can replace the Options that we made earlier with the actual devices.

type MutexInner = core::cell::RefCell<I2c>;
type Mutex = cortex_m::interrupt::Mutex<MutexInner>;
type BusManager = shared_bus::CortexMBusManager<MutexInner, I2c>;
type BusProxy = shared_bus::proxy::BusProxy<'static, Mutex, I2c>;

//...
struct Resources {
    manager: BusManager,
    device: Option<MyI2cDevice<BusProxy>>,
}

fn init() -> init::LateResources {
    let i2c = ();
    let manager = BusManager(i2c);

    init::LateResources {
        manager,
        device: None,
    }
}

fn idle(c: idle::Context) -> ! {
    // Finally, construct our I2C device using the manager reference.
    c.resources.device.replace(MyI2cDevice::new(c.resources.manager.acquire()));
}

Of course, as @Rahix noted earlier, the mutex used by shared-bus will block all interrupts, which conflicts with RTFM. A workaround for this would be to implement a "dummy" mutex for shared-bus (where we assume that RTFM will handle all resource requests, so it is safe to ignore resource conflicts at this level).

Edit: Upon analysis later, I realized that the above statement about a dummy mutex is only valid if all devices on the shared-bus are stored as a single RTIC resource. Otherwise, you may have contention with the shared underlying resource.

BlinkyStitt commented 4 years ago

Any suggestions on how to implement a "dummy" mutex?

ryan-summers commented 4 years ago

@WyseNynja https://github.com/ryan-summers/shared-bus-rtic/blob/b33e05009dd92639197bc63174fb51ff5ed1078e/src/lib.rs#L59-L86 I originally used a dummy mutex in shared-bus-rtic, but was encountering issues with sharing the bus across multiple RTIC tasks (due to it not implementing Sync) and have since switched away from this method.

That dummy mutex does panic if you attempt to use the bus at the same time in two different contexts (e.g. a device on the bus pre-empts another device on the bus) due to an atomic bool check.

The underlying problem was that the reference that shared-bus gives out is inherently thread unsafe I believe (because a reference can't be shared between threads or something along those lines - I don't recall exactly).

ryan-summers commented 4 years ago

@Rahix I believe it is now safe to close this issue after the release of 0.2.0 - I'm successfully using this crate as-is with RTIC. I'll look into pulling shared-bus-rtic from crates.io now that shared-bus supports this use case.

Rahix commented 4 years ago

That's cool! Can you share some code? Should we add/change something to make it even easier? Also, would it make sense to add an example and documentation for this usecase?

ryan-summers commented 4 years ago

That's cool! Can you share some code? Should we add/change something to make it even easier? Also, would it make sense to add an example and documentation for this usecase?

Right now, my use-case is unfortunately closed source (although the intent is to open-source it in the near future). However, I'm able to contribute the Mutex implementation back to shared-bus for future users of shared-bus and RTIC - I was just trying to get a PR together :)

dbrgn commented 3 years ago

For reference, the PR by @ryan-summers is at #17 :slightly_smiling_face: :tada:

antonok-edm commented 3 years ago

I just gave @ryan-summers's approach a try for sharing SPI in RTIC. Looks like it's still not possible because acquire_spi is only defined for BusManager<NullMutex<T>>. Is that limitation still relevant? If so, perhaps it's still worth keeping this issue open.

ryan-summers commented 3 years ago

I just gave @ryan-summers's approach a try for sharing SPI in RTIC. Looks like it's still not possible because acquire_spi is only defined for BusManager<NullMutex<T>>. Is that limitation still relevant? If so, perhaps it's still worth keeping this issue open.

This is by-design. You cannot safely share a SPI bus + CS pins using this library alone, as the embedded-hal does not support CS within the FullDuplex SPI trait. As such, there's always a possible race condition between asserting CSn and using the SPI bus. You can use shared-bus-rtic for SPI currently if you observe the restrictions outlined in the usage documentation (notably, all shared bus resources must lock the bus before asserting CSn, which can be done inherently by RTIC).

Rahix commented 3 years ago

With #17 merged and 0.2.2 released, I think this issue is finally one step closer to being solved :)

Still missing:

mbrieske commented 2 years ago

Hi, I have been playing around with the shared-bus library on RTIC and I am a bit confused by all the examples I find - would it be too much to ask if anyone would put together a minimal example on how to initialize the shared bus to use some I2C device in a RTIC task (on cortex-m)? The examples all seem to refer to older versions of this library, or at least your comments suggest that since the examples have been published some pull requests were merged that changed things.

Thanks!

dbrgn commented 2 years ago

Does https://github.com/dbrgn/chicken-door/blob/main/firmware/src/main.rs help? It's still on RTIC 0.5, but the code should be quite easy to adapt if you're already on 1.0.

mbrieske commented 2 years ago

Exactly what I was looking for, thanks a lot! :)

Edit: This example is using shared-bus-rtic instead of shared-bus. Is this still the preferred lib? @ryan-summers comment https://github.com/Rahix/shared-bus/issues/4#issuecomment-683639172 lead me to believe that his library was deprecated and shared-bus would work with RTIC now.

Rahix commented 2 years ago

@mbrieske, try https://github.com/ryan-summers/shared-bus-example I think.

dbrgn commented 2 years ago

@mbrieske good point, I missed that. I should probably upgrade to plain shared-bus 🙂

ryan-summers commented 2 years ago

shared-bus-rtic's only current use is to share a SPI bus for RTIC applications, since shared-bus can't quite do that yet due to some soundness issues around CS ownership

nullobject commented 9 months ago

This issue should be closed, with recent versions of shared-bus it is possible to use it with RTIC.