Overhauled API design

[Rahix]

This PR serves to track progress of the new implementation of shared-bus. See #14 for the goals that this archieves to implement.

This version is also available for tesing on crates.io as 0.2.0-alpha.1. The documentation is here: https://docs.rs/shared-bus/0.2.0-alpha.1/shared_bus/

I'd be grateful for feedback whether this new design works well.

[Rahix]

I need feedback on this from people who are using shared-bus already!

@ryankurte: Re #13, can you try using the new_std!() macro like shown in the test:

https://github.com/Rahix/shared-bus/blob/a0e52c3b3dc30ee790680dbe366b932dff9ef257/tests/i2c.rs#L94-L117

Does this work for you?

[Rahix]

Documentation is still missing in this version, but as an example, this is how a bus could be shared between an interrupt and thread mode:

#![no_std]
#![no_main]

// use panic_halt as _;
use panic_semihosting as _;

use stm32f3_discovery::stm32f3xx_hal;
use stm32f3_discovery::stm32f3xx_hal::interrupt;
use stm32f3_discovery::stm32f3xx_hal::prelude::*;
use stm32f3_discovery::stm32f3xx_hal::stm32;

use stm32f3_discovery::button;

use cortex_m_semihosting::hprintln;

type I2cBus = stm32f3xx_hal::i2c::I2c<
    stm32::I2C1,
    (
        stm32f3xx_hal::gpio::gpiob::PB6<stm32f3xx_hal::gpio::AF4>,
        stm32f3xx_hal::gpio::gpiob::PB7<stm32f3xx_hal::gpio::AF4>,
    ),
>;

static mut RIGHT: core::mem::MaybeUninit<
    pcf857x::Pcf8574a<shared_bus::I2cProxy<shared_bus::CortexMMutex<I2cBus>>>,
> = core::mem::MaybeUninit::uninit();

#[cortex_m_rt::entry]
fn main() -> ! {
    let device_periphs = stm32::Peripherals::take().unwrap();
    let mut reset_and_clock_control = device_periphs.RCC.constrain();

    let core_periphs = cortex_m::Peripherals::take().unwrap();
    let mut flash = device_periphs.FLASH.constrain();
    let clocks = reset_and_clock_control.cfgr.freeze(&mut flash.acr);

    hprintln!("Initializing ...").unwrap();

    // initialize I2C
    let mut gpiob = device_periphs.GPIOB.split(&mut reset_and_clock_control.ahb);
    let scl = gpiob.pb6.into_af4(&mut gpiob.moder, &mut gpiob.afrl);
    let sda = gpiob.pb7.into_af4(&mut gpiob.moder, &mut gpiob.afrl);
    let i2c = stm32f3xx_hal::i2c::I2c::i2c1(
        device_periphs.I2C1,
        (scl, sda),
        100.khz(),
        clocks,
        &mut reset_and_clock_control.apb1,
    );

    // let bus = shared_bus::BusManagerSimple::new(i2c);
    let bus = shared_bus::new_cortexm!(I2cBus = i2c).unwrap();

    let mut lsm303dhlc = stm32f3_discovery::lsm303dlhc::Lsm303dlhc::new(bus.acquire_i2c()).unwrap();

    let mut left = pcf857x::Pcf8574a::new(
        bus.acquire_i2c(),
        pcf857x::SlaveAddr::Alternative(false, false, true),
    );
    let mut right = pcf857x::Pcf8574a::new(bus.acquire_i2c(), pcf857x::SlaveAddr::Default);

    left.set(0xff).unwrap();
    right.set(0xff).unwrap();

    unsafe {
        RIGHT = core::mem::MaybeUninit::new(right);
    }

    cortex_m::asm::dmb();

    button::interrupt::enable(
        &device_periphs.EXTI,
        &device_periphs.SYSCFG,
        button::interrupt::TriggerMode::Rising,
    );

    hprintln!("Done!").unwrap();

    loop {
        let accel = lsm303dhlc.accel().unwrap();

        left.set((accel.z >> 8) as u8).unwrap();
    }
}

#[interrupt]
fn EXTI0() {
    static mut COUNTER: u8 = 0;
    //If we don't clear the interrupt to signal it's been serviced, it will continue to fire.
    button::interrupt::clear();

    let right = unsafe { &mut *RIGHT.as_mut_ptr() };

    right.set(!*COUNTER).unwrap();
    *COUNTER = COUNTER.wrapping_add(1);
}

(the unsafe code is only needed for moving the device to interrupt context)

[Rahix]

I've released an alpha version of the current state! The documentation is here: https://docs.rs/shared-bus/0.2.0-alpha.1/shared_bus/

I'd be grateful for feedback whether this new design works well.

[therealprof]

I've tested shared_bus::BusManagerSimple with my https://github.com/therealprof/MCUmeter-software and it works a treat, even saving a few byte over the previous version. 👍

[ryankurte]

hey looks good! i'll try have a go with it but, not entirely sure when this might be.

is it worth documenting the mutex interactions for the listed example? i think because we're always within a cortex_m::interrupt_free there is no possibility of deadlock, though this is a property of the cortex-m implementation rather than the mutex which may be good to describe?

[ryan-summers]

@Rahix I really like this redesign - I think it's going to fit all of my use cases quite well, thanks for investing the time into this! I think the send+sync usage is going to help make this a lot safer, and it was a really great idea to leverage them.

I would also propose one additional type of mutex that sits in between the "NullMutex" and a full cirtical-section "CortexM Mutex" - specifically, in cases in RTIC where multiple peripherals on the same bus are contained in the same object (e.g. a containing struct) such that RTIC manages resource contention inherently, it would be nice to have a SimpleMutex (or similar) type that will panic if the bus is attempted to be used simultaneously across threads (since in a correct impl, RTIC would guarantee that this does not occur).

This is just a thought for inclusion into the library and I would be happy to PR it separately as well (or keep it entirely external and just use it in my internal projects, since we can impl custom external mutexes), but thought it would be worth mentioning.

In these cases, we can rely on RTIC to do all the managing work and just want shared-bus to manage references and enforce usage constraints.

Edit: I'm going to be incorporating the alpha release into a current project and will report back on success.

Edit-2: I just tried out the cortex-m mutex and it worked gloriously. Will attempt working with my own custom mutex next.

Edit-3: I have now tested using a custom mutex imp and it's suiting my needs as well. This is what I describe abovel:

pub struct AtomicCheckMutex<BUS> {
    bus: core::cell::UnsafeCell<BUS>,
    busy: core::sync::atomic::AtomicBool,
}

impl<BUS> shared_bus::BusMutex for AtomicCheckMutex<BUS> {
    type Bus = BUS;

    fn create(v: BUS) -> Self {
        Self {
            bus: core::cell::UnsafeCell::new(v),
            busy: core::sync::atomic::AtomicBool::from(false)
        }
    }

    fn lock<R, F: FnOnce(&mut Self::Bus) -> R>(&self, f: F) -> R {
        self.busy.compare_exchange(
                false,
                true,
                core::sync::atomic::Ordering::SeqCst,
                core::sync::atomic::Ordering::SeqCst)
            .expect("Bus conflict");
        let result = f(unsafe { &mut *self.bus.get() });

        self.busy.store(false, core::sync::atomic::Ordering::SeqCst);

        result
    }
}

unsafe impl<BUS> Sync for AtomicCheckMutex<BUS> {}

type AtomicCheckManager<T> = shared_bus::BusManager<AtomicCheckMutex<T>>;

macro_rules! new_atomic_check_manager {
    ($bus_type:ty = $bus:expr) => {{
        let m: Option<&'static mut _> = cortex_m::singleton!(
            : AtomicCheckManager<$bus_type> =
                AtomicCheckManager::new($bus)
        );

        m
    }};
}

[Rahix]

Thanks a lot for the feedback everyone!

is it worth documenting the mutex interactions for the listed example? i think because we're always within a cortex_m::interrupt::free() there is no possibility of deadlock, though this is a property of the cortex-m implementation rather than the mutex which may be good to describe?

@ryankurte: I'm not quite sure what you mean? The mutex will only dead-lock when the I2C implementation locks up, so it's neither better nor worse than without shared-bus. Or am I missing something?

I would also propose one additional type of mutex that sits in between the "NullMutex" and a full cirtical-section "CortexM Mutex" - specifically, in cases in RTIC where multiple peripherals on the same bus are contained in the same object (e.g. a containing struct) such that RTIC manages resource contention inherently, it would be nice to have a SimpleMutex (or similar) type that will panic if the bus is attempted to be used simultaneously across threads (since in a correct impl, RTIC would guarantee that this does not occur).

@ryan-summers: We can surely add such a mutex type as well! I'm all in for compatibility with the widest possible range of use-cases in upstream. Ideally, no one should need to maintain mutex-impls outside of shared-bus ...

[ryankurte]

@ryankurte: I'm not quite sure what you mean? The mutex will only dead-lock when the I2C implementation locks up, so it's neither better nor worse than without shared-bus. Or am I missing something?

it might be me missing the point too tbh, and this is somewhat irrelevant because this PR is a net improvement / shouldn't block this at all, but, what is the behaviour if the mutex is taken in the main task, then an interrupt occurs and the interrupt attempts to take the mutex? this cannot happen with the current implementation within interrupt_free but, this seems to me to be a property of the cortex_m implementation rather than that of the mutex?

for example, the std::sync::Mutex does not operate in this manner, however, it's also not an issue because in this case we have threads that can yeild while waiting for the mutex. i haven't dug into the cortex_m::Interrupt impl a lot but, as far as i know on a single core it would be equally valid to only disable interrupts while taking / returning the mutex, not while executing it, which would be another break case?

[ryan-summers]

it might be me missing the point too tbh, and this is somewhat irrelevant because this PR is a net improvement / shouldn't block this at all, but, what is the behaviour if the mutex is taken in the main task, then an interrupt occurs and the interrupt attempts to take the mutex? this cannot happen with the current implementation within interrupt_free but, this seems to me to be a property of the cortex_m implementation rather than that of the mutex?

I think the problem here is that "mutex" in this case is a bit of a misnomer. As opposed to a true mutex that is acquired and released to denote ownership of a resource, the "mutexes" in this crate simply ensure that pre-emption of the task using the bus is not possible by another task that is using the bus. That's why there's no formal acquire/release mechanism.