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.

[JamesHuard]

I want to also address:

It seems like ODP loses the ability to provide common logic and OEMs would have to copy/paste everything together.

Head on.

This really isn't true-- ODP doesn't lose the ability to provide common logic for known, supported hardware designs. It simply needs to put this in an appropriately domain-specific location.

The PSU logic given should be completely ignorant of the hardware particulars (and fall out, i.e. synchronization methodology etc.).

If ODP then has a reference design, or let's say class of supported reference design variants, then go and make some repos and crates for those. There's nothing stopping us from creating a companion repo + crate set odp-ref-power or similar, where any design that uses Bqxyz and TiABC parts can take that "off the shelf", threading and all.

It just doesn't belong inside the actual power policy logic.

[RobertZ2011]

If ODP then has a reference design, or let's say class of supported reference design variants, then go and make some repos and crates for those. There's nothing stopping us from creating a companion repo + crate set odp-ref-power or similar, where any design that uses Bqxyz and TiABC parts can take that "off the shelf", threading and all.

It sounds like we're roughly on the same page, but I want to clarify a few things. The split essentially becomes something like:

• power-policy-interface: Common traits and types (e.g. PSU trait would live here)
• power-policy-service: ODP implementation of power policy logic (no concept of any sort of locking)
• power-policy-application: ODP provided locking semantics

Naturally, we recommend using power-policy-service and power-policy-application, but you're free to provide your own implementations if those aren't suitable.

Would you be against Lockable being present in at the power-policy-application level? Going back to this example:

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;
    }
}

Let's say the OEM now wants to re-use this same code on a product with a different IMU type. This is where I see Lockable coming into play:

async fn task_user_b(shared_imu: impl Lockable<Inner: IMU>, 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.lock().await;
        generic_host_command_handler(command, imu).await;
    }
}

This is the type of scenario I had in mind when I said:

It seems like ODP loses the ability to provide common logic and OEMs would have to copy/paste everything together.

If we don't have something like Lockable then the above code can't be generic and we're forced to copy/paste (either manually or through macros). But if the design is that Lockable should only be used in application-level code then I don't see any problem with that. This is particularly relevant in the shared case where taking &Mutex<impl IMU> wouldn't be appropriate.

How does this interact with event broadcasting? The power policy broadcasts events for consumer connected/disconnected. This is handled through this trait:

embedded-services/power-policy-interface/src/service/notification.rs

Line 18 in 9cabed8

pub trait Notifier<'device> {

. Currently, this is capable of implementing deferred logic in the sense that you can store the lockable reference somewhere and then call functions on it in a different task. But if this logic isn't aware of lockable then this becomes harder to do. If you need this sort of functionality, is the recommended design to essentially follow what v0.1 did and use per-device IDs that can be looked up in a registry to produce the corresponding lockable?

[JamesHuard]

If ODP then has a reference design, or let's say class of supported reference design variants, then go and make some repos and crates for those. There's nothing stopping us from creating a companion repo + crate set odp-ref-power or similar, where any design that uses Bqxyz and TiABC parts can take that "off the shelf", threading and all.

It sounds like we're roughly on the same page, but I want to clarify a few things. The split essentially becomes something like:

• power-policy-interface: Common traits and types (e.g. PSU trait would live here)
• power-policy-service: ODP implementation of power policy logic (no concept of any sort of locking)
• power-policy-application: ODP provided locking semantics

I think application here might be too broad a word/domain of responsibility. I think once you're at the point of "locking resources", you're no longer in an abstract world. I would recommend something more like:

• power-policy-interface: Common traits and types (e.g. PSU trait would live here)
• power-policy-service: ODP implementation of power policy logic (no concept of any sort of locking)
• power-policy-reference-bsp: ODP provided support for reference hardware
• reference-board: pulls power-policy-reference-bsp and any other hardware-grounded bsp crates in to a centralized crate, ready for "out of the box" support

Would you be against Lockable being present in at the power-policy-application level?

To put it bluntly: no, I don't think this would likely be an appropriate abstraction.

Let's dig into the actual domain of responsibility for Lockable. Lockable is an abstraction for a mechanism that locks. That doesn't describe a hardware resource: that describes an OS synchronization concept. A "lockable device" doesn't make sense as a statement, but "a lockable mutex over a shared device" does. I'll tie this back to my original example of where Lockable might actually be an appropriate abstraction.

Let's say you had a crate layout like the above proposed. With our current code base and embassy-only support, there isn't really a place for Lockable, because it abstracts around a domain where there isn't any need for multiple implementers. There aren't two competing "Mutexes" that we pull from embassy natively, and if there were, embassy would likely provide a Lockable over them for exactly this reason (ignore the mechanics underneath the async embassy mutex logic where they do have something like this, that's more for handling the different threading modes they support, it's not a domain interface abstraction in the same sense).

So what if we had a desire to support a different backend than embassy all together? Suddenly Lockable now has contextual purpose. You would probably want some kind of generic os-synchronization interface crate/collection, where Lockable would look quite natural:

pub trait Mutex /* Lockable */ {
    // something that looks suspiciously like embassy's current Mutex def, but as a trait
}

pub trait Channel { .. }
pub trait Task { .. }
// so on

Then, your reference-board crate becomes OS agonistic, depending on this os-synchronization crate entirely. Then you have two final fully composed crates, reference-board-embassy, reference-board-other-thing`. Lockable allows us to abstract the highly specific and indivisible domain of mutually exclusive synchronization, which today is just embassy mutex.

Note that even in this non-existent-today case, the logic of the "things that are being locked" remains completely ignorant of the "mechanism by which they are locked" (or even the fact that they are locked at all).

Going back to this example:

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;
    }
}

Let's say the OEM now wants to re-use this same code on a product with a different IMU type. This is where I see Lockable coming into play:

async fn task_user_b(shared_imu: impl Lockable<Inner: IMU>, 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.lock().await;
        generic_host_command_handler(command, imu).await;
    }
}

This is the type of scenario I had in mind when I said:

It seems like ODP loses the ability to provide common logic and OEMs would have to copy/paste everything together.

If we don't have something like Lockable then the above code can't be generic and we're forced to copy/paste (either manually or through macros). But if the design is that Lockable should only be used in application-level code then I don't see any problem with that. This is particularly relevant in the shared case where taking &Mutex<impl IMU> wouldn't be appropriate.

I'll come back to the same statement I made on this earlier: it's not generic. That code isn't re-usable. It's entirely, wholly specific to the parts and composition of features that your reference-board features.

I think I've also given you an alternative path to this above, by introducing a "shared lib style BSP" for your particular combination of hardware variants. It's still grounded in the actual deciding factors -- how these known parts interact in this particular layout -- but if someone "takes" that combo into their board, then they can take it "as-is".

How does this interact with event broadcasting? The power policy broadcasts events for consumer connected/disconnected. This is handled through this trait:

embedded-services/power-policy-interface/src/service/notification.rs

Line 18 in 9cabed8

pub trait Notifier<'device> {

. Currently, this is capable of implementing deferred logic in the sense that you can store the lockable reference somewhere and then call functions on it in a different task. But if this logic isn't aware of lockable then this becomes harder to do. If you need this sort of functionality, is the recommended design to essentially follow what v0.1 did and use per-device IDs that can be looked up in a registry to produce the corresponding lockable?

I think you might be going in a loop on this last piece here. Look at the domain and what its singular responsibility is: informing an observer of a state change. It's not the mechanism by which that inform takes place. That's a crucial, perhaps the critical distinction in role.

The trait you have there has a minimal transform (though you may end up finding you can simplify further after removing the crossed/overly complex domain responsibilities):

pub trait Notifier<'device> {
    type Psu: crate::psu::Psu + 'device;

    /// Notify that a consumer has disconnected
    fn notify_consumer_disconnected(
        &mut self,
        psu: &'device Self::Psu,
        flags: ConsumerDisconnect,
    ) -> impl Future<Output = Result<(), Error>>;
    /// Notify that a consumer has been connected
    fn notify_consumer_connected(
        &mut self,
        psu: &'device Self::Psu,
        capability: ConsumerPowerCapability,
    ) -> impl Future<Output = Result<(), Error>>;
    /// Notify that a provider has disconnected
    fn notify_provider_disconnected(&mut self, psu: &'device Self::Psu) -> impl Future<Output = Result<(), Error>>;
    /// Notify that a provider has connected
    fn notify_provider_connected(
        &mut self,
        psu: &'device Self::Psu,
        capability: ProviderPowerCapability,
    ) -> impl Future<Output = Result<(), Error>>;
    /// Notify that the unconstrained state has changed
    fn notify_unconstrained(&mut self, unconstrained: UnconstrainedState) -> impl Future<Output = Result<(), Error>>;
}

If there's synchronization requirements at the device ownership level, those necessarily are defined, like in our examples above, by the allocator of tasks and devices. No one else has sufficient information to make that decision. This might be a trivial transform:

struct ProductPsu(&'static Mutex<PsuDevice>);
impl Psu for ProductPsu { .. }

but it might be far more complicated: you don't know, and shouldn't know, at the interface declaration. That's taking a one-time problem of synchronization and making it a partial decision at every place in the software stack, forcing components to decide "how" synchronization should be done without those components having the faintest idea of the system's final synchronization requirements. Lockable in this trait today is the reason (in a design pattern sense, not exclusively in the literal one) our type-c/pd services are so brittle.

I will note that we should also be careful about how we handle mutability in these interfaces. One might be inclined to keep the references as immutable-- but what that is saying is that "you, notifier, CAN NOT modify the PSU state on notification reception". If that statement is true, part of the interface contract, then great. Leave it as is. But the moment we need to do an actual stateful modification of Psu (which I suspect might be implied by the use of Lockable in the original) this must now be a &mut Self::Psu reference. Otherwise, you're forcing interior mutability as an interface design assumption, which broadly is an anti-pattern and not something we should build our fundamental interface contracts around.

IFF that interface actually does an immutable operation on PSU to any Notifiers, then it's highly likely that the product can use a RwLock instead of a Mutex to greatly simplify their ownership and locking semantics behind the scenes.

[RobertZ2011]

There aren't two competing "Mutexes" that we pull from embassy natively,

There are though. Embassy's Mutex is generic over a raw mutex type so there are already multiple mutex types in the form of Mutex<NoopRawMutex, T>, Mutex<ThreadModeRawMutex, T>, etc. Likewise, going back to this example:

enum ProductIMU {
    SkuVar1(Imu1),
    SkuVar2(Imu2),
}

impl Imu for ProductIMU { .. }

In the shared case this would be something like:

enum ProductIMU {
    SkuVar1(&Mutex<Imu1>),
    SkuVar2(&Mutex<Imu2>),
}

impl Imu for ProductIMU { .. }

This is a Lockable. Without Lockable this would be:

async fn task_user_b(shared_imu: ProductIMU, 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;
        // This is just <ProductIMU as Lockable>::lock()
        let imu = match shared_imu {
            ProductIMU::SkuVar1(a) => ProductIMUGuard::SkuVar1(a.lock().await),
            ProductIMU::SkuVar2(b) => ProductIMUGuard::SkuVar2(b.lock().await),
        };
        generic_host_command_handler(command, imu).await;
    }
}

Not having something like Lockable makes me worry about maintainability. Instead of maintaining a single implementation that's generic over a Lockable, an OEM would have to maintain N copies and ensure that any bug fix reaches all those copies. An OEM might decide to standardize on certain sharing/ownership patterns not just among SKUs of a single product, but among various product families. I think it's a good idea to support a broad range of design decisions, but I fear we might be going too far in the other direction in assuming that there will never be common code at this level.

this must now be a &mut Self::Psu reference.

Agreed, but this is my point. In the case where we are using a shared device Self::Psu would have to be some sort of mutex guard (since locking would be handled by something like find_best_consumer). But Embassy's MutexGuard doesn't natively allow a conversion back to the original mutex type. So we either use a mutex guard type that also contains a reference to the original mutex or we do something like passing around some form of device ID that can be converted to actual device access. It is ultimately application-specific, but I also want to be able to provide specific recommendations to OEMs if something like this comes up.

[RobertZ2011]

Jimi and I talked in a 1:1 and he answered my questions. Next steps here are for me to do a vertical slice demo so we can see how everything looks from top to bottom as well as make sure we don't run into any unexpected issues.

[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.

[asasine]

I think it's worth clarifying what "enum dispatch" is (the technique/pattern) and what its first-order benefits are. The title of this discussion is about "enum dispatch to support different device types," but the original post didn't clearly establish this, and most of the discussion so far has been around software design, through which enum dispatch has shown up in some derived form or served as a strawman for related points rather than as a tool in the engineer's toolbox for "[supporting] different device types."

Different device types

Starting with the premise, "[supporting] different device types," I think there are three possible interpretations:

1. The OEM is multi-sourcing different physical parts on their boards for the same logical feature. The part may be
   1. known at compilation time, or
   2. only determined at runtime.
2. ODP provides out-of-the-box application code that supports a predetermined list of physical parts.
3. ODP provides library code that works with different implementations.

(1) is a closed set: the OEM knows exactly what hardware it will populate onto its board. Even if limitations (e.g., can only provide one binary to manufacturing), and determining which part is a runtime operation (1.ii, using e.g., board markers), the possible set of hardware is still finite and known. (2) is also a closed set: application code¹ offerred by ODP to OEMs that comes with ODP support for that code (e.g., tested on hardware, known good behavior) can reasonably limit itself to a fixed set of hardware. (3) is an open set: library code² that ODP offers to OEMs for integration into their own applications, with OEM's own hardware selection that's not known/available to ODP for support (complexity, trade secret/NDA, price, etc.), must be open to unknown implementations. Note that (3) applies to ODP itself: (2) is a form of (1), where the specific part provided to ODP when (2) is instantiated is decided by the OEM, either during compilation (1.i), at runtime (1.ii), or statically "at runtime" (OEM hardcodes a specific part into its ODP config, which appears "at runtime" to ODP code but is itself static in the application binary). Because of this, I'll group (1) and (2) as "closed set" and (3) as the "open set" going forward.

Of note: an open set is a superset of a closed set (infinite > finite). Corrollary: anything that works with an open set also works with a closed set.

Rust feature support

Rust provides some built-in features for supporting open/closed set functionality:

1. Generics parameters³, possibly with trait constraints: <T: SomeTrait>, impl SomeTrait, and where T: SomeTrait
2. Trait objects dyn SomeTrait, behind some pointer type &dyn SomeTrait, Box<dyn SomeTrait>, or others.

Both generic items (functions, structs, enums, implementations, etc.) and items that accept a trait object are an open set: they work with any type that implements the trait.

Method tables

In compiled code, both of these are backed by a symbol table:

1. Generics create a separate symbol per concrete type in the final program at each generic site, known as monomorphization. This becomes a static symbol table, "looked up" during comptime, with no runtime lookup.
2. Trait objects are two pointers, one to the trait object's instance data (the concrete type that implements the trait), and another to the virtual method table ("vtable") for the instance type's method implementations for this trait.

Drawbacks

These aren't without their drawbacks:

1. Large generic methods, or even small generic methods with many concrete types, may generate a lot of code. In embedded software, where flash is a premium, this can be problematic. Compiler optimizations might deduplicate some of these, but do not rely on this if binary size is critically important.
2. A trait must be dyn-compatible. Most of our async traits are not dyn-compatible due to opaque return types. That is, a trait method returning impl Future (Return Position Impl Trait In Trait or "RPITIT"), including async fn (which desugar to -> impl Future).

Countering the drawbacks

1. For large codegen cost, the name of the game is to reduce the number of monomorphized versions of a generic item.
2. For trait objects, you can:
   1. Make your trait dyn-compatible. For async traits, this is usually done by making your return types concrete by returning Box<impl Future> instead.
   2. Avoid trait objects by using generics.

But these are circular! Generics have codegen cost, so we want trait objects, but async traits aren't dyn-compatible, so we must box, but we don't do that in embedded because heap bad, so we must use generics, but they have codgen cost...

Enum dispatch, the pattern

Enter: enum dispatch.

"Enum dispatch" is first and foremost a code pattern that bakes the vtable into the code, using enums, rather than letting the compiler generate one for you (an "evtable" perhaps?). Iff you have a closed set, you can encode that closed set into an enum of your own choosing, baking the vtable lookup into the trait methods' implementations. This can counter both drawbacks of compiler-generated method tables:

1. A single monomorphized version of a generic item has minimized codegen.
2. You don't need trait objects if you don't have the codgen cost of multiple monomorphized versions of a generic item.

The way it works is simple. First, you have your trait and the generic item:

// Note: not dyn-compatible because of an opaque return type
trait Animal {
    // could also be written as async fn something(&self);
    fn speak(&self) -> impl Future<Output = ()>;
}

// could also be a generic struct SomeStruct<T: SomeTrait>(T) or anything else generic
async fn speak_forever<T: Animal>(animal: T) -> ! {
    loop {
        animal.speak().await;
    }
}

Then, you have your various types that implement the trait

struct Human;
impl Animal for Human {
    async fn speak(&self) {
        eprintln!("Hello");
    }
}

struct Dog;
impl Animal for Dog {
    async fn speak(&self) {
        eprintln!("woof");
    }
}

struct Cat;
impl Animal for Cat {
    async fn speak(&self) {
        eprintln!("meow");
    }
}

async fn main() -> ! {
    let h = Human;
    let d = Dog;
    let c = Cat;
    join!(
        speak_forever(h),
        speak_forever(d),
        speak_forever(c),
    ).await.0
}

This generates three versions of speak_forever for each type that implements Animal. Using the enum dispatch pattern, we can collapse this to one:

enum Animals {
    Human(Human),
    Dog(Dog),
    Cat(Cat),
}

impl Animal for Animals {
    async fn speak(&self) {
        match self {
            Self::Human(h) => h.speak().await,
            Self::Dog(d) => d.speak().await,
            Self::Cat(c) => c.speak().await,
        }
    }
}

async fn main() -> ! {
    let h = Animals::Human(Human);
    let d = Animals::Dog(Dog);
    let c = Animals::Cat(Cat);
    join!(
        speak_forever(h),
        speak_forever(d),
        speak_forever(c),
    ).await.0
}

That's it. That's all it is. That's the only thing that it provides on its own.

There's now one monomorphized version of speak_forever for the Animals type, and 1+N implementations of the Animal trait. The vtable the compiler would have generated for a dyn Animal now sits in the explicit match self { _ } in the impl Animal for Animals. At runtime, the correct type will be looked up based on the enum discriminant, the correct trait method will be retrieved and invoked, and the compiler will generate an impl Future that encompasses all N inner impls of Animal::speak.

---

Footnotes

1. "Application code" here does not refer to Rust library vs. binary crates. ODP may very likely provide "application code" to OEMs that is a Rust library, as well as prebuilt binaries, from a binary crate, that can be consumed directly on supported hardware/boards. In this context, "application code" refers to how OEMs consume the code while integrating with their product: minimally configured and used as-is (application), or in an application unforeseen to ODP (library).
2. On the other hand, "library code" probably does refer to a Rust library crate, as binary crates don't offer symbols meant for consumption in other binaries.
3. Includes const generics.

[asasine]

Now, for how this simple pattern pertains to specific points in the original post:

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.

Rust does not force us into a single concrete type. Embassy does by nature of #[embassy_executor::task] requiring all concrete types.

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.

Said another way, there's a service's library code, targetting an open set, and a service's application code, targetting a closed set.

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.

Async code does not require boxing futures to allow for fully generic async code. Quite the opposite, async code is generic code (RPITIT). Boxing is only necessary to make an async trait dyn-compatible. You only need this if you need trait objects, and as I highlighted above, you don't need trait objects; just use generics. The cost of monomorphization is manageable with the enum dispatch pattern, employed at the level that has a closed set (the application).

We decided to address this by using enum dispatch, allowing code to be generic over only the enum dispatch type.

This is not "generic." It makes the library code a closed set, which is a constraint that should only be applied by application code. The solution for "our library traits are not dyn-compatible" is not to avoid traits, it's to separate the code that needs to be generic (library) from that which is concrete (application). The library code does not require dyn compatibility to be correct, so it should not include such design decisions.

However, the enum_dispatch crate doesn't support async and only works with traits and implementations that are in the same crate.

This is true. However, "enum dispatch" (the pattern) is simple enough to implement by hand, so enum_dispatch (the crate) is not wholly necessary. You can trivially apply the pattern to external traits and types (e.g., using newtypes) when hand-implementing.

Additionally, hardware devices might need to be shared with mulitple services, or between ODP code and OEM code.

This is true, but it has nothing to do with enum dispatch. Enum dispatch is only an optimization technique to minimize monomorphization (the same reason you'd use a trait object).

Additionally, sharing a device between library code (ODP library code but also OEM library code) and application code (OEM application code but also ODP application code) is exclusively an application decision. Library code should remain generic, and the application decides to share the resource across library code. It does this via impl<T: Trait> Trait for &MutexNewtype<T> and giving &mutex to library code that's generic over impl Trait. No enum dispatch in sight (for the library). Enum dispatch only applies if your optimizing for the aforementioned drawbacks of generic code, and that's exclusively an application decision (OEM and ODP), because it requires knowledge of the closed set.

Sharing between ODP application and OEM application is contradicting: you can only have one application. An application shares with multiple libraries, but applications do not share.

---

The rest of the original post is about the Lockable trait, and how you'd implement the enum dispatch pattern for it.

Iff the library code is designed around runtime acquisition of mutual exclusivity from a shared reference (&self -> &mut self), it's valid for that library code to be generic with &Mutex<impl Trait> rather than impl Trait. If that library code wants to further abstract the mutex for various reasons, it can be generic over impl Lockable<impl Trait> instead. Both are valid, and both have clear semantics: "I take a shared reference to some object that I can asynchronously lock at runtime to get some &mut impl Trait that I am generic over."

However, that is a very different semantic from "I am generic over impl Trait"! &Mutex<impl Trait> requires being a shared resource, impl Trait does not require it but it does allow for it (by way of impl<T: Trait> Trait for &MutexNewtype<T>). Generic library code should only bake in the mutex API if it requires that exact semantic (mutex sharing). If it does not, impl Trait is correct.
A library of libraries might have good reason to be generic over a type that affords the mutex semantic &self -> &mut self over another type it's also generic over, for instance to combine different PSU types (a library) into a power policy. However, the power policy library still fully generic and open set: no enum dispatch in sight. It cannot know what hardware exists, only the application can, therefore only the application is able to bake in the closed set of types for the generic library code.

Nowhere in the process of deciding whether your library code should be impl Trait or impl Lockable<impl Trait> does enum dispatch come up. Enum dispatch is exclusively a set-closing technique to reduce monomorphization and avoid the need for trait objects, and it can only be done by the application. It's not that you can't implement an enum-dispatch-like pattern on top of Lockable, it's that the library wouldn't; only the application knows the closed set and therefore the application is the one to implement the pattern. The library takes impl Lockable<impl Trait> and that's that; the library remains generic over the open set of lockable objects that yield Trait.

If the application wishes to use enum dispatch to reduce monomorphization, they may choose to do so; only the application knows the closed set and therefore must be the one that applies the pattern. In practice, if your library is generic over impl Lockable<impl Trait>, the application must apply the pattern twice because there are two generics: the impl Lockable and the inner impl Trait. As a form of documentation for how to reliably implement the pattern for a Lockable trait, the provided example suffices.

As a design pattern for generic libraries to "support different device types," enum dispatch is the wrong tool for the job.