Reach parity with v0.1: enum dispatch to support different device types

[RobertZ2011]

Summary

High-level code needs to be able to interact with multiple different types of hardware (e.g. power code needs to interact with a PD port and a DC jack), but Rust forces us into a single concrete type. v0.1 code supported this, but we need to determine how this is handled in v0.2.

Background

Interacting with a service requires a device driver to implement one or more traits defined by the service. We want services to be able to interact with multiple different types of devices at the same time, i.e. the power policy needs to coordinate with all PSUs on the system even though one may be a DC barrel jack while another is a Type-C port. This is futher complicated by our use of async code, which normally requires boxing futures on the heap to allow for fully generic async code. We decided to address this by using enum dispatch, allowing code to be generic over only the enum dispatch type. However, the enum_dispatch crate doesn't support async and only works with traits and implementations that are in the same crate.

Additionally, hardware devices might need to be shared with mulitple services, or between ODP code and OEM code. ODP provides the Lockable trait as an abstraction over mutex-like objects. This allows service implementations to be generic over Lockable<Inner: SomeTrait>. When code needs to call a trait function, Lockable::lock is called to yield a guard object which implements DerefMut<Target: SomeTrait>, deref_mut is implicitly called to yield the end &mut impl Sometrait reference.

With enum dispatch, the above approach fails at the DerefMut step because we would need to return a reference to the dispatch type, as demonstrated in this example:

enum LockableDispatch<'a> {
    TypeA(&'a Mutex<TypeA>),
    TypeB(&'a Mutex<TypeB>).
}

enum GuardDispatch<'a> {
    TypeA(MutexGuard<TypeA>),
    TypeA(MutexGuard<TypeB>),
}

enum ReferenceDispatch<'a> {
    TypeA(&mut TypeA),
    TypeB(&mut TypeB),
}

impl<'a> DerefMut<Target = Dispatch<'a>> for GuardDispatch<'a> {
    type Target = ReferenceDispatch<'a>;

    fn deref_mut(&mut self) -> &Self::Target {
        // Can't construct a `ReferenceDispatch` instance because we have to return a reference
        // Can't store a `ReferenceDispatch` instance because that won't pass the borrow checker
    }
}

Proposed Solution

Change Lockable so that the guard type is what implements SomeTrait instead of being required to implement DerefMut<Target = SomeTrait>. In the above example, this would move the enum dispatch implementation to the GuardDispatch type instead of needing another layer of indirection through DerefMut. This does lead to a complication when not using enum dispatch, which relies on DerefMut. This would be addressed through blanket impls on embassy guard types like impl<'a> SomeTrait for embassy_sync::mutex::MutexGuard<'a, T> where T: SomeTrait { ... }, or a blanket impl like impl SomeTrait for T where T: DerefMut<Target = SomeTrait> { ... }

Alternatives Considered

Put Dispatch Enum inside of the Lockable

In this approach the dispatch enum would be placed in the Lockable instead. With the following Dispatch type:

enum Dispatch {
    TypeA(TypeA),
    TypeB(TypeB),
}

The flow would then be Lockable::lock -> Guard<Dispatch> -> &mut Dispatch. This avoids the issue with DerefMut because the enum dispatch type is already in the guard. However, this precludes sharing among multiple services because only a single enum dispatch type would have access to the hardware. Similarly, OEM code would no longer have individual access to the hardware, access would have to be done through the aggregate Dispatch type.

Dispatch Before Locking

In this approach the SomeTrait implementation would instead be responsible for locking an inner mutex type and call the appropriate function, like so:

struct TypeA {
    lock: Mutex<TypeAInner>,
}

impl SomeTrait for TypeA {
    async fn some_func(&self) {
        self.lock().await.some_func().await;
    }
}

The downside to this approach is that we lose the ability to compose operations atomically. This could allow for concurrency bugs where task A and B are executing functions, which get interleaved and lead to incorrect behavior.

[RobertZ2011]

This could allow for concurrency bugs where task A and B are executing functions, which get interleaved and lead to incorrect behavior.

I'm particularly wary of this approach because I've dealt with bugs in the past that boiled down to this. I'm also not sure if there's a good, easy way to resolve these types of bugs if they do happen. However, I took a quick look, and it doesn't seem like ODP code is currently susceptible to this.

[jerrysxie]

Just for clarification, the proposed solution would be something like this:

enum GuardDispatch<'a> {
    TypeA(MutexGuard<'a, TypeA>),
    TypeB(MutexGuard<'a, TypeB>),
}

impl<'a> SomeTrait for GuardDispatch<'a> {
    async fn some_func(&mut self) {
        match self {
            GuardDispatch::TypeA(g) => g.some_func().await,
            GuardDispatch::TypeB(g) => g.some_func().await,
        }
    }
}

[RobertZ2011]

Just for clarification, the proposed solution would be something like this:

enum GuardDispatch<'a> {
    TypeA(MutexGuard<'a, TypeA>),
    TypeB(MutexGuard<'a, TypeB>),
}

impl<'a> SomeTrait for GuardDispatch<'a> {
    async fn some_func(&mut self) {
        match self {
            GuardDispatch::TypeA(g) => g.some_func().await,
            GuardDispatch::TypeB(g) => g.some_func().await,
        }
    }
}

Correct, though in practice this code would be automatically generated through procedural macros. The enum_dispatch crate already provides an implementation of such macros, but it doesn't work with async, and it only works with traits and implementations that are in the same crate.

[JamesHuard]

Interacting with a service requires a device driver to implement one or more traits defined by the service.

This first sentence is actually where I'd argue we've made a design misstep.

A device driver should have absolutely no-knowledge of the upper software constructs around it. A device driver's role is purely hardware-mechanics: never high-level mission logic. That's a mixing of responsibilities that results in non-composable, rigidly hierarchical systems.

If we're able to abstract an interface around a common set of similar device classes, then we have a trait with meaningful abstraction. The simplest example I can think of here is an accelerometer:

pub trait Accel {
    // this isn't an accel trait proposal, just an example
    type Data: Into<AccelMeasurement>;
    type Error: MaybeDefmt;

    fn read(&mut self) -> Result<Self::Data, Self::Error>;
}

A service that utilizes an accel hardware component directly could utilize this. Note that, however, a service is far more likely to operate at the conceptual level of a full IMU solution: which may or may not be spread across multiple parts. Here's a hypothetical composition for a MotionFuser service-interface class:

// some MotionFuser interface declaration
pub trait MotionFuser {
    fn Tick(&mut self) -> Result<Quaternion, Self::Error>;
}

// some IMU-backed service impl library:
pub struct ImuMotionFusion<IMU: Accel + Gyro + Mag> {
    imu: IMU,
    kalman: Kalman,
    // so on ..
}

impl MotionFuser for ImuMotionFusion { .. }

Note that as a hardware composer, i.e. the product that actually uses the service collections, I get to define dispatch as is appropriate for my building block composition:

// case 1: 1 to 1 hardware map
    let some_service_that_consumes_motion_fuser = OrientationService::new(
        ImuMotionFusion::new(
            Some3InOneChip::new( ..i2c ) // THIS implements Accel + Gyro + Mag
    ));

// case 2: 1 to 2 hardware map (very common)
    struct MyProductIMU {
        i2c: Mutex<I2c>, // in this example these share a bus
        driver_accelgyro: AccelGyroPart,
        driver_mag: MagPart
    }
    impl Accel for MyProductIMU { .. }
    impl Gyro for MyProductIMU { .. }
    impl Mag for MyProductIMU { .. }
    
    let orientation = OrientationService::new(
        ImuMotionFusion::new(
            MyProductIMU::new(i2c)));

// case 3: 2 map, mag is shared across tasks
    struct MyProductIMU {
        driver_accelgyro: AccelGyroPart,
        driver_mag: &'static Mutex<MagPart>,
    }
    // as before..
    
   static I2C_BUS: StaticCell<Mutex<I2c>> = StaticCell::new();
   static MAG: StaticCell<Mutex<MagPart<&'static Mutex<I2c>>> = StaticCell::new();
   // wiring and device config 
   ..
   let shared_mag = ..; // can use a new-type, or any mechanism really to impl Mag over the Mutex type

   // service integration:
   let dumb_compass = DumbCompass::new(shared_mag.clone());
   let orientation = Orientation::new(ImuMotionFusion::new(MyProductIMU::new(accel_gyro, shared_mag)));

Here, dispatch is entirely defined by the composition level; that's the only place with enough horizontal context to decide the right model.

If we're trying to extend behaviors across single software devices beyond their domain of responsibility, we inevitably end up with an unscalable enum "God object" that must have a composition condition for every single potential product combination of parts that anyone uses, ever. At that point, there's no reason to even use traits-- it's a frozen design that serves specifically and only the cases that it was prototyped on.

In your example, the issue here is that you're trying to abstract alongside a dispatch-mechanism level, but that mechanism is actually top-level contextually defined. There is no "one size fits all", because to get there you'd have to construct a virtual "God object" that knows every potential hardware and software system combination a priori.

Basically, we should not have a concept of Lockable at all. It's forcing a dispatch design decision into a context where it's irrelevant. In my example above, it's entirely inappropriate for Orientation and DumbCompass to prescribe (via a Lockable trait) the method of dispatch between them. That is, fundamentally, a non-composable design because it restricts its use cases to exactly that mechanism, which as demonstrated is only 1 of 3 examples (and you can imagine spreading this out infinitely into the future).

Design the composite parts (the "services") such that they have no knowledge of dispatch. Dispatch methodology can only be meaningfully decided from the outer most "bounding box" of any given system.

If you find there's current designs where "Device Drivers" are implementing traits shared across multiple "domains of responsibility", the immediate question should be: are these traits actually hardware-level traits, or do they define a "software concept" that should be abstracted on its own compositional level (i.e., it's own struct and crate or mod). I suspect many such cases, especially in the type-c/pd domain today, fall into the second class (meaning, we likely are doing "too much" at the hardware interface level, causing rigid hierarchy/anti-compositional patterns to bubble upwards).

[RobertZ2011]

Interacting with a service requires a device driver to implement one or more traits defined by the service.

This first sentence is actually where I'd argue we've made a design misstep.

A device driver should have absolutely no-knowledge of the upper software constructs around it. A device driver's role is purely hardware-mechanics: never high-level mission logic. That's a mixing of responsibilities that results in non-composable, rigidly hierarchical systems.

If we're able to abstract an interface around a common set of similar device classes, then we have a trait with meaningful abstraction. The simplest example I can think of here is an accelerometer:

pub trait Accel {
// this isn't an accel trait proposal, just an example
type Data: Into;
type Error: MaybeDefmt;

fn read(&mut self) -> Result<Self::Data, Self::Error>;

}
A service that utilizes an accel hardware component directly could utilize this. Note that, however, a service is far more likely to operate at the conceptual level of a full IMU solution: which may or may not be spread across multiple parts. Here's a hypothetical composition for a MotionFuser service-interface class:

// some MotionFuser interface declaration
pub trait MotionFuser {
fn Tick(&mut self) -> Result<Quaternion, Self::Error>;
}

// some IMU-backed service impl library:
pub struct ImuMotionFusion<IMU: Accel + Gyro + Mag> {
imu: IMU,
kalman: Kalman,
// so on ..
}

impl MotionFuser for ImuMotionFusion { .. }
Note that as a hardware composer, i.e. the product that actually uses the service collections, I get to define dispatch as is appropriate for my building block composition:

// case 1: 1 to 1 hardware map
let some_service_that_consumes_motion_fuser = OrientationService::new(
ImuMotionFusion::new(
Some3InOneChip::new( ..i2c ) // THIS implements Accel + Gyro + Mag
));

// case 2: 1 to 2 hardware map (very common)
struct MyProductIMU {
i2c: Mutex, // in this example these share a bus
driver_accelgyro: AccelGyroPart,
driver_mag: MagPart
}
impl Accel for MyProductIMU { .. }
impl Gyro for MyProductIMU { .. }
impl Mag for MyProductIMU { .. }

let orientation = OrientationService::new(
    ImuMotionFusion::new(
        MyProductIMU::new(i2c)));

// case 3: 2 map, mag is shared across tasks
struct MyProductIMU {
driver_accelgyro: AccelGyroPart,
driver_mag: &'static Mutex,
}
// as before..

static I2C_BUS: StaticCell<Mutex> = StaticCell::new();
static MAG: StaticCell<Mutex<MagPart<&'static Mutex>> = StaticCell::new();
// wiring and device config
..
let shared_mag = ..; // can use a new-type, or any mechanism really to impl Mag over the Mutex type

// service integration:
let dumb_compass = DumbCompass::new(shared_mag.clone());
let orientation = Orientation::new(ImuMotionFusion::new(MyProductIMU::new(accel_gyro, shared_mag)));
Here, dispatch is entirely defined by the composition level; that's the only place with enough horizontal context to decide the right model.

Unless I'm misunderstanding, this is what we're currently doing. The PD traits define common operations, and any higher-level functionality is implemented on top of the traits. The only logic present in the trait implementations is the local logic required to implement the trait. The other big difference is that PD traits use composition to represent optional functionality. E.g. MaxSinkVoltage is optional functionality on top of the minimum required in Pd, but this is distinct from your example where you're implementing what I would call a composite device by combining unrelated traits.

If we're trying to extend behaviors across single software devices beyond their domain of responsibility, we inevitably end up with an unscalable enum "God object" that must have a composition condition for every single potential product combination of parts that anyone uses, ever. At that point, there's no reason to even use traits-- it's a frozen design that serves specifically and only the cases that it was prototyped on.

In your example, the issue here is that you're trying to abstract alongside a dispatch-mechanism level, but that mechanism is actually top-level contextually defined. There is no "one size fits all", because to get there you'd have to construct a virtual "God object" that knows every potential hardware and software system combination a priori.

I disagree. There would not be a single "God object" that contains everything. You would only need to create dispatch enums for the traits and hardware you actually use.

E.g.

// All our hardware that implements the `Psu` trait
enum Psu {
   PdPort(PdPort),
   DcJack(DcJack),
}

// All our sensors that implement `TempSensor`
enum Temp {
   SensorA(SensorA),
   SensorB(SensorB),
}

// All our hardware that implements `FwUpdate`
enum FwUpdate {
   PdController(PdController),
   // Really fancy temperature sensor
   SensorA(SensorA),
}

The dispatch enums are just the types that implement a specific trait. I would argue that this is just manually implemented dynamic dispatch. If we weren't using async then we'd be working with &dyn Psu, but &dyn Psu is essentially a compiler generated &Psu.

We might also be talking past each other a bit. Your example shows how to create a composite device from multiple real hardware devices, but that's not what I mean when I say dispatch. I'm talking about how to re-implement the functionality that would normally be provided by &dyn SomeTrait when we cant use &dyn Psu` due to async. But I'm glad you brought it up because it is something that we need to consider in our design.

Basically, we should not have a concept of Lockable at all. It's forcing a dispatch design decision into a context where it's irrelevant. In my example above, it's entirely inappropriate for Orientation and DumbCompass to prescribe (via a Lockable trait) the method of dispatch between them. That is, fundamentally, a non-composable design because it restricts its use cases to exactly that mechanism, which as demonstrated is only 1 of 3 examples (and you can imagine spreading this out infinitely into the future).

Getting rid of Lockable would cause us to lose the ability to compose operations atomically and open us to hard to fix concurrency bugs. Let's say we have an IMU task containing this code:

let accel = imu.read_accel();
let rot = imu.read_rotation();
let mag = imu.read_mag();

// Sensor fusion algorithm

and a host communication task containing:

match HostCommand::ResetImu {
    // Resets all IMU values to zero
    imu.reset();
}

Without Lockable or something similar, we cannot prevent the operations in these tasks from interleaving and interfering like

• IMU Task: imu.read_accel() -> correct accel value
• Host Task: imu.reset()
• IMU Task: imu.read_rotation() -> zero
• IMU Task: imu.read_mag() -> zero
• IMU Task: Incorrect value

This is a bug that I could 100% see an OEM encountering. I think that there are ways to mitigate the risk here through careful design of traits, but I don't think we could completely remove it. With Lockable the IMU code could be atomic and the interleaving couldn't happen. This would work with your MyProductIMU struct. It would just need to own the hardware that composes it and be contained in a mutex. However, this may be where your "God Object" might come into play. If everything in MyProductIMU can accept a FW update then MyProductIMU would need to implement the functionality to dispatch the FW update to the correct device. There are situations where I can imagine this becoming unwieldy. It seems like we're running into a trade-off between ease of composition and ability to atomically compose operations.

Design the composite parts (the "services") such that they have no knowledge of dispatch. Dispatch methodology can only be meaningfully decided from the outer most "bounding box" of any given system.

This should already be true. In an environment with an allocator, you should be able to drop in a Mutex<Box<dyn SomeTrait>> and everything should just work. Dispatch is already an implementation detail; the issue is really between Lockable and dispatch.

If you find there's current designs where "Device Drivers" are implementing traits shared across multiple "domains of responsibility", the immediate question should be: are these traits actually hardware-level traits, or do they define a "software concept" that should be abstracted on its own compositional level (i.e., it's own struct and crate or mod). I suspect many such cases, especially in the type-c/pd domain today, fall into the second class (meaning, we likely are doing "too much" at the hardware interface level, causing rigid hierarchy/anti-compositional patterns to bubble upwards).

We can dive in deeper, to be sure we're on the same page, but this should be generally true. There's very little code that isn't concerned with managing or controlling hardware state. This is something that was worse before the refactor. For example, the PD controller code used to contain a CFU implementation, but now the CFU-specific logic is contained in its own struct which interacts with the PD controller through a FwUpdate trait.

[asasine]

I haven't been able to fully respond to all points here but I do want to respond to one.

Getting rid Lockable would cause us to lose the ability to compose operations atomically and open us to hard to fix concurrency bugs.

This isn't true. Any operation which requires atomicity should always be defined as a singular method [on a trait]. The method's signature, fn -> impl Future, is conveying exactly what we want: a single asynchronous state machine, with exactly two visible states (pending and done), with an output when done. For the IMU read + reset example, reading all three should be a single fn read_imu() -> impl Future<Output = ImuMeasurement>. If the duration of the method needs atomicity, it's an implementation detail, not part of the public API.

[JamesHuard]

I disagree. There would not be a single "God object" that contains everything. You would only need to create dispatch enums for the traits and hardware you actually use.

E.g.

// All our hardware that implements the Psu trait
enum Psu {
PdPort(PdPort),
DcJack(DcJack),
}

// All our sensors that implement TempSensor
enum Temp {
SensorA(SensorA),
SensorB(SensorB),
}

// All our hardware that implements FwUpdate
enum FwUpdate {
PdController(PdController),
// Really fancy temperature sensor
SensorA(SensorA),
}
The dispatch enums are just the types that implement a specific trait. I would argue that this is just manually implemented dynamic dispatch. If we weren't using async then we'd be working with &dyn Psu, but &dyn Psu is essentially a compiler generated &Psu.

When I said "god object" here, what I'm trying to say is that when we make this decision in the shared code itself, we're inherently limiting support to specifically the set of tested hardware compositions that are reflected by said enum. enum Temp { A/B } shouldn't exist in any service or shared code, because that creates a limitation on how that shared code can be used. "God object" is what results if you follow this process to its extreme: everything in ODP could effectively be wrapped in a giant SupportedPlatforms enum, and "dispatched" from that. The composition in your example isn't really "composition" at the interface level, it's a heirarchy of supported hardware platforms implicitly linked together through these enums across the shared code.

Another way to look at this is to say the enum is creating a mixing of domain responsibilities: the enum itself is simply a stand in for a variant thing that does A. Writing a shared library that consumes said enum then becomes not a "library generic over A", but rather a library that "multiplexes over supported hardware variants, manages those variants behaviors, and also includes generic logic over A". Here we have two clearly delineated lines of responsibility being served by the same compositional convergence point: the crate is moving from a "compositional building block in a large toolbox" into "a fully implemented feature A, that may or may not be included as is in your project". These are importantly but subtly different designs. The former, what I'm referring to as a "compositional architecture", can scale to anyone's quirky hardware platform. The latter is inherently bound to only those explicit hardware platforms implemented and supported by ODP. It's not my mandate to say which is the right strategy for ODP, but from where I'm standing as a consumer of ODP, the former is the only option that realistically works for our product portfolio. We realistically can't survive more than one or two product cycles in the confines of the latter -- it's too rigidly hierarchical, and our hardware designs move too fast to support it.

We might also be talking past each other a bit. Your example shows how to create a composite device from multiple real hardware devices, but that's not what I mean when I say dispatch. I'm talking about how to re-implement the functionality that would normally be provided by &dyn SomeTrait when we cant use &dyn Psu` due to async. But I'm glad you brought it up because it is something that we need to consider in our design.

I'm suggesting that we shouldn't have &dyn SomeTrait as a fundamental compositional tool for interfacing with our "services". What follows:

Basically, we should not have a concept of Lockable at all. It's forcing a dispatch design decision into a context where it's irrelevant. In my example above, it's entirely inappropriate for Orientation and DumbCompass to prescribe (via a Lockable trait) the method of dispatch between them. That is, fundamentally, a non-composable design because it restricts its use cases to exactly that mechanism, which as demonstrated is only 1 of 3 examples (and you can imagine spreading this out infinitely into the future).

Getting rid of Lockable would cause us to lose the ability to compose operations atomically and open us to hard to fix concurrency bugs. Let's say we have an IMU task containing this code:

let accel = imu.read_accel();
let rot = imu.read_rotation();
let mag = imu.read_mag();

// Sensor fusion algorithm
and a host communication task containing:

match HostCommand::ResetImu {
// Resets all IMU values to zero
imu.reset();
}
Without Lockable or something similar, we cannot prevent the operations in these tasks from interleaving and interfering like

• IMU Task: imu.read_accel() -> correct accel value
• Host Task: imu.reset()
• IMU Task: imu.read_rotation() -> zero
• IMU Task: imu.read_mag() -> zero
• IMU Task: Incorrect value

This is a bug that I could 100% see an OEM encountering. I think that there are ways to mitigate the risk here through careful design of traits, but I don't think we could completely remove it. With Lockable the IMU code could be atomic and the interleaving couldn't happen. This would work with your MyProductIMU struct. It would just need to own the hardware that composes it and be contained in a mutex. However, this may be where your "God Object" might come into play. If everything in MyProductIMU can accept a FW update then MyProductIMU would need to implement the functionality to dispatch the FW update to the correct device. There are situations where I can imagine this becoming unwieldy. It seems like we're running into a trade-off between ease of composition and ability to atomically compose operations.

is exactly an example of where I think domains of responsibility are getting coerced together and becoming a hierarchical model instead of a compositional one.

Your example is right on-- it's a perfect example where I think we've implicitly designed ourselves into a corner by smooshing responsibility boundaries into one singular "super object". Let's work with the imu example further, because the mangling of state you described happens as an artifact of "too much API" in a trait.

Have you noticed how there's virtually no traits for init when it comes to things like embedded-io? I don't believe this is an accident. How you configure something is inherently part specific. The IMU is a great example of this, because even within the same supplier and chip family you can have dramatic differences in hardware interface and operational sequencing (register behaviors and the like).

Take the accel trait above as an example:

pub trait Accel {
    // type magic
    fn read(&mut self) -> impl Future<Output = AccelMeasurement>;
}

When a clever developer looks at this, they immediately think: "I've got an excellent interface to read to the accel, but I can do even more with my beautiful interface! Every chip needs some kind of startup sequence, and those that don't just make it empty. Let me expand it thusly:"

pub trait Accel {
    fn init(resources: Self::Resources) -> Result<Self, Self::InitError>;
    
   // as before ..
}

However, we've now just made a critical misstep in interface design. We've now declared that anyone who wants to use an Accel must necessarily manage init sequencing. That's a huge mix-up of domain responsibilities. Initialization is by necessity board specific -- you really can't generalize it meaningfully without imposing strict hierarchies on what sort of devices you can tolerate in your generic startup-system. Think through some circular dependencies to see how nasty this gets (what if I need to power on one part to use another, but in another system the opposite is true).

What that means is we've just cut off any class of customer (or product hardware choice and design) who doesn't conform to our rigidly defined hierarchy of components in ODP from actually using any of our impl Accel objects in ODP. And, when you follow this line of thinking further, you'll see it becomes a tangling factor between even large-scoped domains: suddenly your PDController interface change ripples up into a impl PowerPolicyDevice functional change, despite those supposedly being separate crates that can compile independent of one another. That's not a compositional model (as I'm using the terms), that's a hierarchical or layered on.

The runtime question is actually implicitly answered in there, but let's explicitly work through that concept map too:

What if I have a device with state that's "sticky"? Well, that actually is easily solved by following the compositional rules for splitting interfaces above.

Let's say I have IMUUserA and IMUUserB:

// separate crates, separate git repos:
pub struct IMUUserA { .. }

impl IMUUserA {
    pub async fn tick(&mut self) -> Result<..> {
        let accel_now = self.accel.read().await;
    }
}

// .. 
pub struct IMUUserB {}

impl IMUUserB {
    pub async fn host_request(&mut self, command: HostCommand) -> Result<..> {
        match command {
            HostCommand::Configure(settings) => { .. },
            HostCommand::Reset => { .. },
            HostCommand::Read => { .. },
            HostCommand::Compute => { .. },
        }
    }
}

Now let's look at what their domain of responsibility is:

• IMUUserA is a simple data consumer of a generic Accelerometer measurement. It's domain of responsibility begins after the sample arrives and ends after it's performed some kind of transformation or fusion on it.
• IMUUserB is responsible for managing device configuration, destructive device-state, as well as some runtime behaviors like IMUUserA

IMUUserA is basically fine. IMUUserB, however, is the violating our compositional design principles here, and it's actually violating in multiple ways:

1. It's domain of responsibilities crosses between direct hardware management and logical deliverables. In practical terms, we've defined a "god-approaching object" that is responsible for both managing the functional state of a part, and processing its outputs. It's not a block in a block diagram, it's closer to a Venn diagram between a hardware and a software block.
2. It's consuming either a single over-stepping trait interface PlatformTypeAccelVariant (which starts to look/smell a lot like the enums posted before) or it's composed one out of an equally-over-stepping trait interface PlatformOperationsTypeAccelVariant + Accel.

There's no real fully valid way to retain a toolbox like compositional model while including things like PlatformType___ constructs. Those are by their nature hardware specific. It's not that we can't have these things, it's that they have to live in the appropriate place: the product/product-family that uses them.

So what happens if we fix these design violations? Well, funnily enough, it not only makes the code more re-usable, but it also eliminates the runtime race condition.

IMUUserA can stay as it is. It's cleanly composable.

IMUUserB has to be broken up. It's doing two main things: servicing HostCommand requests, and interfacing to a set of platform board-style configurations on a particular Accel/Accels that look sufficiently similar (hopefully you can see the issue here with how difficult it is to even capture accurately in a single sentence).

So what do we do? Surely someone has to have the "top level knowledge" because their implicit to the HostCommand request domain. You could argue maybe that set of requests is invalid, but I don't think we need to explore that route, because it is valid to say "[this crate] supports requests of HostCommand type".

Let's just blindly apply our "one domain" rule:

Thus, we get something like:

pub trait HostCommandServicer {
    pub fn reset(&mut self) -> impl Future..;
    pub fn configure(&mut self, HostCommandSettings /* note: Self::Settings would need some extra thought */) -> impl Future..;
    pub fn read(&mut self) -> impl Future..;
    pub fn compute(&mut self) -> impl Future..;
}

// Note: I'm _not_ adding this for the reasons stated above, this is a device/platform specific decision, not something we can meaningfully abstract in a way that respects flat composition
//pub trait GenericPlatformAccelOperator {
//    fn reset(&mut self) -> ..;
//    fn configure(...) -> ..;
//}

// and an example consumer:
// note you could implement this almost any way that structurally makes sense, what matters is the separation of the interface contract from the implementation, and the roles of responsibilities there-in
async fn generic_host_command_handler(command: HostCommand, servicer: &mut HostCommandServicer) {
    match command { .. }
}

But how do we make sure we don't hit an invalid runtime config now? How have we eliminated the issue? By the borrow checker :) --

For a system where we need to both process HostCommands and IMUUserA, these are actually not trivially related: when the chip is reset, the device is no longer an Accel (measurement gatherer, ready to be read from). We're in software land now, so we can break this up however we want! Let's not get hung up on making singular objects. Instead, let's compose them based on their explicit relationships:

// note, I'm using type-state here but there's a bunch of ways to solve it, I think this is just the most clear way to demonstrate in this case
trait Sealed {}
struct Ready;
struct Reset;
impl Sealed for Ready {}
impl Sealed for Reset {}
pub struct ManagedAccelXYZ<S: Sealed> { .. }

impl Accel for ManagedXYZ<Ready> { .. }
impl ManagedXYZ<Ready> {
    fn reset(self) -> ManagedXYZ<Reset> {
        // XYZ here denotes a specific board/chipset/combination. This might be writing to a pin, sending a command, both, neither -- it all is highly situationally specific, thus we only implement it for this explicitly supported combo. There's a reasonable chance this simply exists in the product itself! (Or perhaps some kind of "like-minded" design adapter support crates)
    }
}

impl ManagedAccelXYZ<Reset> {
    fn configure(self) -> ManagedXYZ<Ready> { 
        // same considerations here as reset; what configuration we use is board, part, scenario, maybe even runtime decision specific. Don't try to capture this behavior generically, it's not generic.
    }
}

// this could even be an enum. You can use any mechanism here you want
struct ProductIMUOrchestrator {
  // essentially just an either
  managed: Either<ManagedAccelXYZ<Reset>, ManagedAccelXYZ<Ready>>
}

// here's a newtype over a mutex for this example. Again, not prescriptive, just illustrative
struct SharedProductIMU(&Mutex<ProductIMUOrchestrator>);

async fn task_user_a(shared_imu: SharedProductIMU) {
    loop {
        let _ = sample_timer.next().await;
        // yes, unnecessary layering here, can be more elegant, etc.
        match shared_imu.0.lock().await.managed {
            Either::Left(reset) => { .. /* can't sample, it's off! You can turn it back on, or do anything else as appropriate */ },
            Either::Right(ready) => {
                let computer = IMUUserA::new(ready);
                report_a(computer.tick().await).await;
            }
        }
    }
}

async fn task_user_b(shared_imu: SharedProductIMU, host_commander: HostCommandReceiver) {
    // I think this shouldn't need expansion to be clear enough (it just twiddles Left/Right as appropriate), let me know if otherwise:
    impl HostCommandServicer for ProductIMUOrchestrator { .. }
    
    loop {
        let command = host_commander.recv().await;
        let imu = shared_imu.0.lock().await;
        generic_host_command_handler(command, imu).await;
    }
}

What Lockable is attempting to do is draw a boundary of abstraction between task_user_a and task_user_b. This is an aesthetically pleasing goal; but unfortunately, it's not scalable to different hardware/product configurations and designs outside whatever "mold" that particular hierarchy has captured. To capture the same level of dynamics as the above, you'd be generating some kind of "God object" that represents every conceivably supported set of hardware variants your "generic" systems operate on. With that model, there's not really much reason to even use traits -- just have some top level "Board variant" enums, and call direct connective tissues that do their "board operations" in place as is defined by that particular configuration.

Let's look at this really specifically, in case the above doesn't land clearly:

As an anti-example, let's run through a version with prescribed dispatch methodology via the shared Lockable trait. With that design, I can no longer actually support the same runtime scenarios: I've locked myself out of any software compositions that resolve up differently and have forced all of my "shared" code to now be conscious of this front-loaded dispatch decision. The exact case where this fork breaks down is when we're trying to compose at a smaller rung on the design "layering", rather than a higher one. In other words, it hurts us on two fronts: Any resources that might have shared contention (devices that can't be singularly "Locked" at a top level) can't be fed into these shared crates, and because I've now forced my shared crates to design around the concept of a shared resource, they can't compose with interfaces that don't have that restriction baked in.

Here's code that demonstrates these two pitfalls:

//// example: product nesting nightmares
async fn shared_lockable_dep<T: Lockable + Lockable::Guard<Other>>(t: T) {
    let res = t.lock().await.do_other_sync(); // note this isn't async, we can't hide another lock guard in here
    // ...
}

// in my product, I have an Other that's composed of a couple of different resources, which themselves are shared:
struct ProductOther {
  other_a: OtherA,
  other_b: OtherB
}

struct OtherComposer { .. Mutex<ProductOther> }

impl Lockable for OtherComposer {}

struct OtherAComposer {..Mutex<OtherA>}
struct OtherBComposer { ..Mutex<OtherB>}

impl Lockable for OtherAComposer { .. }
impl Lockable for OtherBComposer { .. }

// but wait, now how do we lock these mutexes if we Lock Other?
// The quick + dirty answer: you're lockable is actually a Lockable of Lockables...
// BUT WAIT: now our Guard type doesn't resolve to trait A/B/Other... 
// Let's just push it down a layer!
// BUT NOW -> what happens when:

struct OtherBComposer{ ..Mutex< (OtherB, OtherC) > }
// Now we're one layer deeper: do we just keep impl T for Lockable::Guard<Lockable::Guard<T>> ?
// No, this just gets pushed to the products responsibility, because only the product can really know how these things are composed meaningfully anyway
// So wait, why do we have Lockable? It's irrelevant to the contract that consumes Other, and it's just forcing our product to design along a nest of Mutex/Lockable implementers, which may or may not be a great choice for that product

///// example: incompatibility/deadlock or race condition when using non-Lockable APIs
async fn my_lockable_aware_crate<T: Lockable.. MyTrait>>(t: &mut T) {
    let locked = t.lock().await;
    let x = untethered_x_crate(locked).await;
}

async fn untethered_x_crate(x: &mut impl MyTrait) -> X  { //// POSTING NOW FOR CONTEXT, NOTE: INCOMPLETE

Design the composite parts (the "services") such that they have no knowledge of dispatch. Dispatch methodology can only be meaningfully decided from the outer most "bounding box" of any given system.

This should already be true. In an environment with an allocator, you should be able to drop in a Mutex<Box<dyn SomeTrait>> and everything should just work. Dispatch is already an implementation detail; the issue is really between Lockable and dispatch.

Are you arguing that a Lockable is not inherently a dispatch model? I'm struggling to understand what the distinction you're making is here, as I see the domain of responsibility for Lockable as explicitly covering the expectation for how information is shared across contexts. That to me is synonymous with dispatch.

If you find there's current designs where "Device Drivers" are implementing traits shared across multiple "domains of responsibility", the immediate question should be: are these traits actually hardware-level traits, or do they define a "software concept" that should be abstracted on its own compositional level (i.e., it's own struct and crate or mod). I suspect many such cases, especially in the type-c/pd domain today, fall into the second class (meaning, we likely are doing "too much" at the hardware interface level, causing rigid hierarchy/anti-compositional patterns to bubble upwards).

We can dive in deeper, to be sure we're on the same page, but this should be generally true. There's very little code that isn't concerned with managing or controlling hardware state.

It's not so much about the volume of code manipulating hardware state, but rather the way we've partitioned responsibility for that.

This is something that was worse before the refactor. For example, the PD controller code used to contain a CFU implementation, but now the CFU-specific logic is contained in its own struct which interacts with the PD controller through a FwUpdate trait.

I don't at all doubt that ODPv2 made improvements on what we had previously. But I want to make sure it's clear that we haven't actually moved away from a hierarchical model. This isn't "composition" in a tool-box sense.

[asasine]

@jerrysxie for what it's worth, I think a Discussion for proposals like this would be more conducive to parallel discussion like we're having, since you can have distinct replies to the main post, with each reply being its own thread with further replies. They're not enabled for this repo.