ut-utp / core

Mozilla Public License 2.0
1 stars 0 forks source link

Drop the `peripheral_trait!` macro #179

Open rrbutani opened 2 years ago

rrbutani commented 2 years ago

what

Background

I.e. this macro: https://github.com/ut-utp/core/blob/5e0f0eb43de2fe9e94f124cff6bc7db0821c7737/traits/src/peripherals/mod.rs#L196-L358

which is invoked on all the peripheral trait definitions: https://github.com/ut-utp/core/blob/5e0f0eb43de2fe9e94f124cff6bc7db0821c7737/traits/src/peripherals/gpio.rs#L141

Like #176 this was a well-intentioned but ultimately ill-advised early design decision.

The idea here was to have a Peripherals trait that is a super trait of all of the individual peripheral traits, for a few reasons: 1) so that users of the API could simply ask for a P: Peripherals instance instead of needing to grow 7+ generic parameters 2) so that users of peripheral instances could just operate on a single instance, i.e. Gpio::read(p, ...), Clock::get_milliseconds(p, ...). This is particularly useful in tests 3) so that we don't require peripheral implementations that want to share state with other peripherals (i.e. an Input impl and an Output impl that share state to act as a loopback device) to use external state sharing mechanisms (i.e. Arc<Mutex<_>>, RefCell<_>, etc.)

The 3rd point is a little subtle; here's an example:

For the above reasons, we made Peripherals a supertrait: https://github.com/ut-utp/core/blob/5e0f0eb43de2fe9e94f124cff6bc7db0821c7737/traits/src/peripherals/mod.rs#L37

We still recognized that the common case was definitely going to be separate instances for each peripheral and that it would be unacceptable for users to implement Peripherals for each unique set of peripheral trait implementors they wanted to use together, so:

Problems

This worked but had several downsides, mostly with the peripheral_trait macro:

A Solution

3 years later, with the benefit of hindsight, we can come up with a solution that sidesteps these issues while still fulfilling the design constraints that led us to write peripheral_trait.

Peripherals trait

First: We should have the Peripherals trait have associated types for each of the peripheral traits within instead of being a super trait. Something like:

pub trait Peripherals {
    type Gpio: Gpio;
    type Adc: Adc;
    type Pwm: Pwm;
    type Timers: Timers;
    type Clock: Clock;
    type Input: Input;
    type Output: Output;

    fn get_gpio(&self) -> &Self::Gpio;
    fn get_gpio_mut(&mut self) -> &mut Self::Gpio;

    fn get_adc(&self) -> &Self::Adc;
    fn get_adc_mut(&mut self) -> &mut Self::Adc;

    fn get_pwm(&self) -> &Self::Pwm;
    fn get_pwm_mut(&mut self) -> &mut Self::Pwm;

    fn get_timers(&self) -> &Self::Timers;
    fn get_timers_mut(&mut self) -> &mut Self::Timers;

    fn get_clock(&self) -> &Self::Clock;
    fn get_clock_mut(&mut self) -> &mut Self::Clock;

    fn get_input(&self) -> &Self::Input;
    fn get_input_mut(&mut self) -> &mut Self::Input;

    fn get_output(&self) -> &Self::Output;
    fn get_output_mut(&mut self) -> &mut Self::Output;
}

Our goal was to hide the particular types used for the peripherals instead of having them leak into users of the peripherals as generic parameters. Associated types are a cleaner way of achieving this than supertraits.

As an added bonus it's now easier to selectively have some peripherals be shared; i.e. if you want to have a LoopbackIo device, you only need to:

Under the existing setup you would have had to write delegating trait implementations of all the peripheral traits for your new type (with the Input and Output impls delegating to the same instance).

PeripheralsWrapper

The above addresses the "shared state between peripheral impls" concern (the 3rd point) and manages to make using the Peripherals in your own API not overly cumbersome (you can just be generic over P: Peripherals instead of all 7 individual peripheral traits) but it does not address the 2nd concern: actually using this version of the Peripherals trait is still more cumbersome that the current incarnation because you now need to call get_gpio, etc. before calling your particular peripheral trait's methods.

We can fix this by: providing a type that does have all the peripheral traits implemented on it and delegates to an underlying Peripherals instance.

I think we had kind of the right idea with peripheral_trait! but just bad execution: it's hard to make a declarative macro like this robust and composable but proc macro attributes are a good fit for this task. ambassador is one such proc macro that does exactly what peripheral_trait was looking to do.

We can use ambassador to provide delegated impls of all the peripheral traits on PeripheralsWrapper, where PeripheralsWrapper is a type like:

#[repr(transparent)]
struct PeripheralsWrapper<P: Peripherals>(P);

with delegations like this:

#[ambassador::delegate_to_methods]
#[delegate(Gpio, target_ref = "get_gpio", target_mut = "get_gpio_mut")]
#[delegate(Adc, target_ref = "get_adc", target_mut = "get_adc_mut")]
#[delegate(Pwm, target_ref = "get_pwm", target_mut = "get_pwm_mut")]
#[delegate(Timers, target_ref = "get_timers", target_mut = "get_timers_mut")]
#[delegate(Clock, target_ref = "get_clock", target_mut = "get_clock_mut")]
#[delegate(Input, target_ref = "get_input", target_mut = "get_input_mut")]
#[delegate(Output, target_ref = "get_output", target_mut = "get_output_mut")]
impl<P: Peripherals> PeripheralsWrapper<'_, P> {
    fn get_gpio(&self) -> &P::Gpio { self.0.get_gpio() }
    fn get_gpio_mut(&mut self) -> &mut P::Gpio { self.0.get_gpio_mut() }
    fn get_adc(&self) -> &P::Adc { self.0.get_adc() }
    fn get_adc_mut(&mut self) -> &mut P::Adc { self.0.get_adc_mut() }
    fn get_pwm(&self) -> &P::Pwm { self.0.get_pwm() }
    fn get_pwm_mut(&mut self) -> &mut P::Pwm { self.0.get_pwm_mut() }
    fn get_timers(&self) -> &P::Timers { self.0.get_timers() }
    fn get_timers_mut(&mut self) -> &mut P::Timers { self.0.get_timers_mut() }
    fn get_clock(&self) -> &P::Clock { self.0.get_clock() }
    fn get_clock_mut(&mut self) -> &mut P::Clock { self.0.get_clock_mut() }
    fn get_input(&self) -> &P::Input { self.0.get_input() }
    fn get_input_mut(&mut self) -> &mut P::Input { self.0.get_input_mut() }
    fn get_output(&self) -> &P::Output { self.0.get_output() }
    fn get_output_mut(&mut self) -> &mut P::Output { self.0.get_output_mut() }
}

warning Note:

  • we'd like to just implement, for example <P: Peripherals> Gpio for P but ambassador doesn't have a way to do this so we use the PeripheralsWrapper newtype

We can then offer this extension to actually construct a PeripheralsWrapper without consuming the underlying Peripherals instance:

pub trait PeripheralsExt: Peripherals {
    /// Gets you a wrapper type that impls all the traits.
    fn peripherals_wrapper(&self) -> &'_ PeripheralsWrapper<Self> {
      unsafe { core::mem::transmute(self) } // safe because of `repr(transparent)`
    }
   fn peripherals_wrapper_mut(&mut self) -> &'_ mut PeripheralsWrapper<Self> { ... }
}

impl<P: Peripherals> PeripheralsExt for P { }

And have the Deref/DerefMut impls on InstructionInterpreter leverage the above methods.

We can also provide delegated impls on PeripheralSet to cover the common case.

steps

where

branch: imp/peripheral-trait-reform

open questions