How can we share a bus between multiple drivers? #35
Comments
There are a few ways:
let mut driver_a = DriverA::new(i2c);
driver_a.stuff();
let i2c = driver_a.release();
let mut driver_b = DriverB::new(i2c);
driver_b.other_stuff();
// these blanket implementations should be in embedded-hal
impl<'a, I> blocking::i2c::Write for &'a RefCell<I> where I: blocking::i2c::Write { .. }
// repeat for all the other traitslet shared_i2c = RefCell::new(i2c);
let mut driver_a = DriverA::new(&shared_i2c);
let mut driver_b = DriverB::new(&shared_i2c);
driver_a.stuff();
driver_b.other_stuff();
let mut driver_a = DriverA::new(&mut i2c);
let mut driver_b = DriverB::new(&mut i2c);
driver_a.stuff(&mut i2c);
driver_b.other_stuff(&mut i2c);
I'm not sure if it being app! {
resources: {
statci A: DriverA;
statci B: DriverB;
},
tasks: {
EXTI0: {
path: exti0,
priority: 1,
resources: [A],
},
EXTI1: {
path: exti1,
priority: 2,
resources: [B],
},
},
}
fn init(p: init::Peripherals) -> init::LateResources {
// ..
let i2c_alias: I2c = i2c; // due to `Copy`-ness
init::LateResources { A: DriverA::new(i2c), B: DriverB::new(i2c_alias) }
}
fn exti0(r: &mut EXTI0::Resources) {
r.A.stuff();
}
fn exti1(r: &mut EXTI0::Resources) {
// can preempt `exti0` midway some operation on the I2C bus
r.B.other_stuff();
} |
Sure, but that's quite useless.
That is interesting. I should have a play with that.
I tried but couldn't get it to compile without nasty tricks. This is not easy to implement on chips that have freely selectable pins, at least that's my experience. I also haven't quite figured out how to implement the non-blocking trains; maybe you should try non-STM32 MCUs some time to get a feeling for the different peripheral implementations out there. 😉
Yes, I agree. Copy-able traits in the presence of preemption are probably a bad idea. You really don't want an interrupt handler to take over the bus in a middle of a transfer. |
How will this work with RTFM? fn init(mut p: init::Peripherals, _r: init::Resources) -> init::LateResources {
...
let mut i2c = I2c::i2c1(
p.device.I2C1,
(scl, sda),
&mut afio.mapr,
i2c::Mode::Fast { frequency: 400000, duty_cycle: i2c::DutyCycle::Ratio16to9 },
clocks,
&mut rcc.apb1,
);
let si5351 = SI5351::new(&mut i2c, false, 25_000_000).unwrap();
let _ = ssd1306::init(&mut i2c);
let _ = ssd1306::pos(&mut i2c, 0, 0);
let _ = ssd1306::print_bytes(&mut i2c, b"Send ASCII over serial for some action");'
init::LateResources {
SI5351: si5351,
I2C: i2c
}
}Since the value |
|
@japaric I tried the |
|
@therealprof awesome. I had this on my TODO list as well. My 2 cents why I think this will be the most flexible approach:
This is fine as long as the driver doesn't hold any state. For example: if you need the currently set measurement resolution to correctly interpret the sensor readings. You could probably design the driver to be able to attach/detach to a bus and manage the driver state independently. But this results in extra work for the driver implementer and the user.
I first favored this method but then realized this would probably make hardware interface (I2C, SPI, ...) agnostic sensor traits impossible (I might have overseen sth. here, because of my not so extensive Rust knowledge). The trait methods would also need to pass the bus around and something like this would not work. pub trait Thermometer {
type Error;
fn temp(&mut self) -> Result<f32, Self::Error>;
}The |
|
And also let's not forget that it's quite common to need additional resources instead of just the bus itself. For I2C (and to a lesser extend) SPI there're often separate interrupt pin(s) and for SPI one often needs to worry about a chip select pin as well. The complexity of the solution to such an approach could explode rather quickly. |
…st-embedded#35 Signed-off-by: Daniel Egger <daniel@eggers-club.de>
|
I left a comment in #55 (comment) about the shortcomings of the "put the resource protection (e.g. RefCell) in the driver" approach.
I think this might be the way to go. You design your driver as having two type states: one where it /* embedded-hal */
// imagine this is one of the blocking I2C traits
trait I2c {
/* .. */
}
/* sensor1 */
// Only the sensor state
// The type parameter `I` is mainly to forbid the following scenario: you
// initialize the sensor using I2C1, then you perform some op on it using I2C2
// and then another op using I2C3.
pub struct State<I>
where
I: hal::I2c,
{
// ..
i2c: PhantomData<I>,
}
pub fn init<I>(i2c: &mut I) -> State<I>
where
I: hal::I2c,
{
// ..
}
impl State<I> {
pub fn take<I>(self, i2c: I) -> Owned<I>
where
I: I2c,
{
// ..
}
pub fn borrow<I>(&mut self, i2c: &mut I) -> RefMut<I>
where
I: I2c,
{
// ..
}
}
struct Owned<I>
where
I: hal::I2c,
{
i2c: I,
state: State,
}
impl Owned<I> {
pub fn release<I>(self) -> (State, I)
where
I: I2c,
{
// ..
}
pub fn accel(&mut self) -> u16 {
// defer to RefMut
self.as_ref_mut(|s| s.accel())
}
fn as_ref_mut<R, F>(&mut self, f: F)
where
F: FnOnce(&mut RefMut<I>) -> R,
{
f(&mut RefMut {
i2c: &mut self.i2c,
state: &mut self.state,
})
}
}
pub struct RefMut<'a, I>
where
I: hal::I2c,
{
i2c: &'a mut I,
state: &'a mut State,
}
impl RefMut<'a, I> {
pub fn accel(&mut self) -> u16 {
// ..
}
}
/* app */
fn main() {
let mut i2c: I2c1 = ..;
// neither operation takes ownership of `i2c`
let sensor1: sensor1::State<I2c1> = sensor1::init(&mut i2c);
let sensor2 = sensor2::init(&mut i2c);
// takes ownership of `i2c`
let mut sensor1: sensor1::Owned<I2c1> = sensor1.take(i2c);
let g = sensor1.accel();
let (sensor1, mut i2c) = sensor1.release();
// doesn't take ownership of `i2c`
let m = sensor2.borrow(&mut i2c).mag();
let ar = sensor2.borrow(&mut i2c).gyro();
}The (*) Actually that's another reason why storing a reference to a
If you are going to do blocking operations in your tasks store fn task1(r: EXTI0::Resources) {
let g = r.SENSOR1.borrow(r.I2C1).accel();
}
fn task1(r: EXTI1::Resources) {
let ar = r.SENSOR2.borrow(r.I2C1).gyro();
}If you are going to use the DMA store fn task1(r: EXTI0::Resources) {
let sensor1: sensor1::State<_> = r.SENSOR1.take().unwrap();
let i2c = r.I2C.take().unwrap();
let pending_transfer = sensor1.take(i2c).mag_dma();
// send `pending_transfer` to some other task, etc.
}
The NSS / NCS / interrupt pin can be permanently stored in the |
|
@japaric I'll give your idea a spin.
I would not necessarily say that this is always the case. Pinsharing is not quite as uncommon as it seems and used for all kinds of stuff like measurements triggering, resets and sometimes they're also muxed (though in that case you'd probably need an alternative PIN driver anyway). |
|
@japaric This puts quite a few additional requirements on each individual driver without explicitly defining what they are. I would rather prefer a solution that requires the driver HAL implementation to make sure shared use is safe (by disabling interrupts if necessary) or the user to use a safe mechanism for sharing the bus, same as we already do when sharing resources internally. |
|
@japaric Just to float another idea: How about a generic proxy driver? The proxy would be initialised with some peripheral it would own, e.g. the initialised I2C bus and then you could use this proxy driver to request multiple proxy handles implementing the very same trait(s) which can be passed to a driver. Internally the proxy driver would ensure that only a single transfer is active at a time. |
|
@therealprof That sounds like it would push all the synchronization concerns to the driver crate; meaning that the driver crate would have to be aware of |
|
@japaric Well, the idea of the proxy is that only the proxy needs to be concerned with the locking of the multiplexed resources instead of each individual user of the traits so it can be generic and will only cause the overhead if it is actually used. I'm toying around with an implementation of such a proxy for I2C but I'm not happy with the interface and not sure whether my Rust-Fu is already sufficient to make it work. |
|
I'm giving up. I sunk the better part of 2 days into it but I cannot get it to work due to the freakiness of Rust around mutable sharing. Things I've tried including encapsulating the It's really sad that I'm not to solve such a critical issue with all the drivers being worked on. If we have to change the interfacing and break all drivers to support such an important thing that'd be really a blow to embedded Rust. |
In rust-embedded/embedded-hal#35 one of the proposals for dealing with sharing the buses is to use this pattern. The most appealing aspect of this is that it doesn't require any specific knowledge of synchronization or concurrency used by the embedding application. The `Owned::borrow` method feels a little warty and un-ergonomic. Ideally we'd be able to use `Deref` and `DerefMut` to have the compiler automatically `borrow()` when doing something like `owned.get_bank_a_data()`, but it doesn't appear to be possible to have it meet our lifetime requirements.
|
I took a stab at implementing the type states proposal from #35 (comment) in my simple sx1509 driver (wez/sx1509@a063f2c). A notable difference is that I didn't include the delegation from |
|
Roughly following the example by @wez, in JoshMcguigan/tsl256x@8315f1f I updated a driver I've been working on to borrow the I2C bus, rather than taking exclusive ownership. To keep it simple, rather than implementing two structs (owned and borrowed) to wrap my sensor type, I just modified each method on the sensor struct to take a struct Owned<I2C> {
sensor: Tsl2561<I2C>,
i2c: I2C,
}
let mut i2c = I2c::i2c1(p.I2C1, (scl, sda), 400.khz(), clocks, &mut rcc.apb1);
let mut sensor = Tsl2561::new(&mut i2c).unwrap();
let mut sensor_with_bus = Owned { sensor, i2c };
sensor_with_bus.sensor.power_on(&mut sensor_with_bus.i2c);My goal was to come up with a solution which minimizes the complexity of the driver crates, so as to not discourage people from creating drivers, while also solving the bus sharing problem. Admittedly, my knowledge of how That said, I do think it is important that we try as a community to standardize on a solution here, so that driver users are well as driver developers get a consistent feeling across the various drivers. Any thoughts on this approach vs the approach taken by @wez vs the original suggestion by @japaric? |
|
I think this is a very important issue and an a standardized and ergonomic solution is very much needed. Out of all the proposed solutions, I think @therealprof's proxy idea is the best. I wrote a little implementation to show how I would do it. You can find it here: i2c-proxy-demo The concept consists of 3 components:
|
|
I'm running into a similar issue (and have discussed it in mozilla#rust-embedded). In my current, specific case, multiple peripherals need to rely on a configurable clock, and the peripherals must be invalidated if the clock changes. What doesn't work but I'd like to make work would be something akin to: struct HSI16Clock {
div4: bool,
}
struct LSEClock;
impl HSI16Clock {
fn new(div4: bool) -> Self { HSI16Clock{ div4 } }
}
enum SerialClock<'clk> {
HSI16(&'clk HSI16Clock),
LSE(&'clk LSEClock),
}
impl<'clk> SerialClock<'clk> {
fn release(self) {}
}
struct Serial<'clk> {
clk: &'clk SerialClock<'clk>,
}
impl<'clk> Serial<'clk> {
fn destruct(self) {}
}
fn main() {
let sc = HSI16Clock::new(false);
let c = SerialClock::HSI16(&sc);
let mut s1 = Serial{ clk: &c };
let mut s2 = Serial{ clk: &c };
// time to reconfigure slow down the clock!
s1.destruct();
s2.destruct();
c.release();
let sc = HSI16Clock::new(true);
}This also needs a solution to making the clocks Singletons, which I haven't attempted yet. |
|
I spent some time scratching this itch, and I think I've come up with a decent solution. It is an implementation of the concepts discussed here. This seems like a reasonable place to request feedback. The actual crate itself is still very rough, but you can see an (untested) implementation of the ideas in this repository. The idea is this: when you configure a peripheral, you must operate within the context of that peripheral's configuration. A code example is probably the best way to explain it: fn main() -> ! {
let p = cortex_m::Peripherals::take().unwrap();
let d = stm32l0x1_hal::stm32l0x1::Peripherals::take().unwrap();
let mut syst = p.SYST;
let mut rcc = d.RCC.constrain();
let mut pwr = d.PWR.constrain();
let mut flash = d.FLASH.constrain();
// Configure the Power peripheral to raise the VDD. The VDD range has implications for the
// maximum clock speed one can run (higher voltage = higher MHz). As such, you want to make
// sure this cannot change unless you are prepared to invalidate your clock settings.
pwr.set_vcore_range(stm32l0x1_hal::power::VCoreRange::Range1)
.unwrap();
pwr.power_domain(&mut rcc, |rcc, mut pwr_ctx| {
// Within the closure, we are guaranteed that VDD will not change, as to do so requires
// mutating `pwr`, and Rust's move semantics prevent that!
// The PowerContext `pwr_ctx` contains information we need to make
// decisions based on the VDD and VCore levels available to us. Furthermore, a PowerContext
// can only be obtained within this closure, and is required elsewhere.
// Every time we loop, we will turn the LSE on and off. Use has_lse to track desired state.
let mut has_lse = false;
// Enable the system configuration clock so our changes will take effect.
rcc.apb2.enr().modify(|_, w| w.syscfgen().set_bit());
while !rcc.apb2.enr().read().syscfgen().bit_is_set() {}
// Request that the HSI16 clock be turned on and divided by 4.
rcc.hsi16.enable();
rcc.hsi16.div4();
// Use the prescaled 4 MHz HSI16 clock for SYSCLK (which drives SysTick)
rcc.sysclk_src = stm32l0x1_hal::rcc::clocking::SysClkSource::HSI16;
loop {
if has_lse {
rcc.lse = Some(stm32l0x1_hal::rcc::clocking::LowSpeedExternalOSC::new());
} else {
rcc.lse = None;
}
rcc.clock_domain(&mut flash, &mut pwr_ctx, |mut clk_ctx, _| {
// Within this closure, similarly to `power_domain`, we are assured that the clock
// configuration cannot change. You cannot turn a clock on or off because we've
// already mutably borrowed `rcc`. The `clk_ctx` provides clock speed information
// that would be useful to clocked peripheral busses.
// Configure the SysTick frequency.
syst.set_clock_source(SystClkSource::External);
syst.set_reload(clk_ctx.sysclk().0 / 8); // 1s
syst.clear_current();
syst.enable_counter();
syst.enable_interrupt();
// Here we would construct I2C or Serial peripherals based on `clk_ctx`
// let mut usart2 = Serial::usart2(d.USART2, (tx, rx), Bps(115200), clocking::USARTClkSource::PCLK, ...
loop {
// main "application" loop to handle Serial tx/rx, etc.
asm::wfi();
break;
}
});
has_lse = !has_lse;
}
});
panic!("there is no spoon");
}While I did a clean-sheet rewrite for this exploratory work, I think it wouldn't be too hard to refactor an existing reference crate (stm32f100?). |
|
@thenewwazoo: What you brought up looks very interesting, but I think it deserves a separate issue, because it does not really solve the original problem here: That the current implementation does not allow to share a bus between multiple devices. It is however a very good point that a bus is not the only device that might need to be shared. |
|
@Rahix I wanted to illustrate the approach more than a specific solution, namely that of decoupling the bus itself from operations on the bus, as well as experiment with the ergonomics of using move semantics and closures to provide exclusive access. For something like a shared SPI bus, I'm imagining (but haven't actually built) something like: let gpioa = gpio::A::new(&mut clk_ctx.iop);
let mut dev1_cs = gpioa.PA0.into_output<...>(...); // device 1 chip select
let mut dev2_cs = gpioa.PA1.into_output<...>(...); // device 2 chip select
let mut spi_bus = spi::bus::new(clk_context, ...); // the bus itself
let mut device1 = device::new(device::Options{...});
let mut device2 = device::new(device::Options{...}); // note that the devices do not hold Tx, Rx
spi_bus.domain(&mut dev1_cs, |spi_ctx| {
device1.issue(&mut spi_ctx.tx);
device1.interrogate(&spi_ctx.rx);
}); // the spi bus is responsible for asserting/de-asserting the chip select pin
spi_bus.domain(&mut dev2_cs, |spi_ctx| {
device2.issue(&mut spi_ctx.tx);
device2.interrogate(&mut spi_ctx.rx);
});It occurs to me that this approach may would technically be out of scope for embedded-hal specifically, since the traits here wouldn't need to change. This would just be a pattern used by HAL implementors. |
|
@Rahix This looks fantastic. I definitely have to play with that. Just curious: Any reason why you didn't stick it in it's own library crate for easier reuse? Note to self: pickup idea about |
|
@therealprof I wrote it as a POC, just to see if it is even possible. My original idea was to include it in embedded-hal in the end. This is, because each device-hal crate needs to implement the Before adding to embedded-hal we should however think about something else: As seen in the comments above, a bus is not the only peripheral that might need to be shared. It would be good if an implementation in embedded-hal could be generic enough to allow sharing arbitrary devices. In practice this means, the proxy needs to implement fn lock<R, F: FnOnce(&T) -> R>(&self, f: F) -> R;Which means we can't return a reference from inside the mutex. I tried changing this abstraction, but rust's borrowing rules do not allow a different solution at this time (unless someone who has a much better understanding of this knows of one ... I either got it to work with But I see that I should make the proxy available as a standalone crate asap, I will do so when I get home from work today, hopefully. I have a few ideas for workarounds for the moment. |
|
Ok, I published a crate: For an example, I modified my demo to use the crate, take a look over here: i2c-proxy-demo For now, I only added a mutex implementation for |
|
@Rahix I just had some time to try your I'd love to share your solution with the world, would you mind writing something like a blog article and add it to https://github.com/rust-embedded/awesome-embedded-rust via PR? I just built a little high precision power meter using a Nucleo32-STM32F042, a SSD1306 OLED display and an TI INA260 power meter. If there's interest I could turn this into the first OSS Hardware + Software project using Rust. ;) |
Yes please! I could use one to be honest. |
|
@RandomInsano I'm having a hard time sourcing the INA260 at the moment and I don't really see any good alternatives at the moment. |
|
No worries. I’m assuming it’s a different package than what’s offered on DigiKey? If nothing comes of it, no worries on my side. Producting a product takes a lot of effort even after a prototype is done. |
|
No, that's the one but I prefer different distributors due to shipping costs and neither of them has the INA260 in store at the moment. |
|
Heh. That’s the opposite for me, but I live about two hours from their distribution centre. 😃 |
|
TL;DR: Could transactions solve I just read the
I think this aligns well with what I've been researching for #94. I originally wanted messages to be composed of smaller parts but what if the idea were extended to multiple messages? Something I've been grappling with is whether the code overheard of this is worth the effort: bus_proxy.transfer([
[
bus::write(&MESSAGE),
bus::read(&data)
]
]);
/// Or if you're not into that whole brevity thing:
let mut message = [
bus::write(&MESSAGE),
bus::read(&data),
bus::custom_thing(_, _, _),
];
let transaction = [
message
];
bus_proxy.transfer(&transaction);Bundling reads and writes into transactions allows the driver author to have a guarantee on the grouping of their messages. The Mutex within The My next step is to research how to structure an RFC and outline this. My plan would break a large chunk of the ecosystem of the current device crates, so I'll need help finding holes in my plan. |
|
@RandomInsano: The statement you quoted is not a shared-bus issue, it is referring to the " You do however bring up an issue that I have not yet thought about which is the synchronization of multiple accesses to the same device. If I understand correctly, this is what your transactions are trying to solve?
Hmm, you are right that one proxy would usually only talk to one address. But the current I2C traits do not reflect this so it would need a breaking change.
I'd be very very careful about doing something like this. One of the main reasons for the design of shared-bus is that I do not want to break the existing APIs (see here). Are you sure this is really necessary? |
|
For those not following the other thread it's possible to avoid breaking the ecosystem. I muddied the conversation up a bit by cross-posting. |
We cannot use this right away; see also: rust-embedded/embedded-hal#35
44: Implement transactional I2C interface and add example r=ryankurte a=eldruin I implemented the transactional I2C interface from rust-embedded#223 and added an example with a driver which does a combined Write/Read transaction. This is based on previous work from rust-embedded#33 by @ryankurte, rust-embedded/rust-i2cdev#51 and is similar to rust-embedded#35. Co-authored-by: Diego Barrios Romero <eldruin@gmail.com>
35: Add transactional SPI r=ryankurte a=ryankurte Implementation of transactional SPI - replaces rust-embedded/linux-embedded-hal#33 - blocked on: - ~rust-embedded#191 - ~rust-embedded/linux-embedded-hal#34 ~Important: remove patch and bump hal version before merging~ Co-authored-by: ryan <ryan@kurte.nz> Co-authored-by: Ryan <ryankurte@users.noreply.github.com>
|
Can this be closed now? |
|
Yes, I believe it can. The |
It is quite typical to have multiple I2C or SPI devices hanging on the same bus, but with the current scheme and move semantics the driver is taking possession of the handle, blocking the use of multiple chips at the same bus which is totally legal and (at least) for the blocking APIs should also be safe.
@japaric Should those bus types be made
Copy?The text was updated successfully, but these errors were encountered: