Drop the peripheral_trait! macro

[rrbutani]

what

Background

I.e. this macro:

core/traits/src/peripherals/mod.rs

Lines 196 to 358 in 5e0f0eb

	#[doc(hidden)]
	#[macro_export]
	macro_rules! peripheral_trait {
	($nom:ident, $(#[$attr:meta])* pub trait $trait:ident $(<$lifetime:lifetime>)? $(: $bound:ident )? { $($rest:tt)* }) => {
	$(#[$attr])*
	pub trait $trait $(<$lifetime>)? where Self: $($bound)? { $($rest)* }
	// $crate::deref_impl!($trait$(<$lifetime>)? $(| $lifetime |)?, { $($rest)* });
	// $crate::borrow_impl!($trait$(<$lifetime>)? $(| $lifetime |)?, { $($rest)* });
	$crate::peripheral_set_impl!($trait$(<$lifetime>)? $(| $lifetime |)?, { $crate::func_sig!($nom, $($rest)*); });
	// $crate::peripheral_deref_set_impl!($trait$(<$lifetime>)? $(| $lifetime |)?, { $crate::func_sig!($nom, $($rest)*); });
	};
	}
	#[doc(hidden)]
	#[macro_export]
	macro_rules! deref_impl {
	($trait:path $(| $lifetime:lifetime |)?, { $($rest:tt)* }) => {
	#[allow(unnecessary_qualification)]
	impl<$($lifetime,)? I, T: Default + core::ops::Deref<Target = I> + core::ops::DerefMut> $trait for T
	where
	I: $trait,
	{ $crate::func_sig!(+(*), $($rest)*); }
	};
	}
	#[doc(hidden)]
	#[macro_export]
	macro_rules! borrow_impl {
	($trait:path $(| $lifetime:lifetime |)?, { $($rest:tt)* }) => {
	#[allow(unnecessary_qualification)]
	impl<$($lifetime,)? I, T: Default + core::ops::Deref<Target = I> + core::ops::DerefMut + core::borrow::Borrow<I> + core::borrow::BorrowMut<I>> $trait for T
	where
	I: $trait,
	{ $crate::func_sig!(%(borrow, borrow_mut), $($rest)*); }
	};
	}
	#[doc(hidden)]
	#[macro_export]
	macro_rules! peripheral_set_impl {
	($trait:ty $(| $lifetime:lifetime |)?, { $($rest:tt)* }) => {
	impl<$($lifetime,)? 'p, G, A, P, T, C, I, O> $trait for $crate::peripherals::PeripheralSet<'p, G, A, P, T, C, I, O/*, G, A, P, T, C, I, O*/>
	where
	$($lifetime: 'p,)?
	G: $crate::peripherals::gpio::Gpio<'p>,
	A: $crate::peripherals::adc::Adc,
	P: $crate::peripherals::pwm::Pwm,
	T: $crate::peripherals::timers::Timers<'p>,
	C: $crate::peripherals::clock::Clock,
	I: $crate::peripherals::input::Input<'p>,
	O: $crate::peripherals::output::Output<'p>,
	{ $($rest)* }
	};
	}
	// #[doc(hidden)]
	// #[macro_export]
	// macro_rules! peripheral_deref_set_impl {
	// ($trait:ty $(| $lifetime:lifetime |)?, { $($rest:tt)* }) => {
	// impl<$($lifetime,)? 'p, G, A, P, T, C, I, O, GInner, AInner, PInner, TInner, CInner, IInner, OInner> $trait for $crate::peripherals::PeripheralSet<'p, G, A, P, T, C, I, O>
	// where
	// $($lifetime: 'p,)?
	// G: 'p + $crate::peripherals::gpio::Gpio<'p>,
	// A: 'p + $crate::peripherals::adc::Adc,
	// P: 'p + $crate::peripherals::pwm::Pwm,
	// T: 'p + $crate::peripherals::timers::Timers<'p>,
	// C: 'p + $crate::peripherals::clock::Clock,
	// I: 'p + $crate::peripherals::input::Input<'p>,
	// O: 'p + $crate::peripherals::output::Output<'p>,
	// GInner: 'p + Deref<
	// { $($rest)* }
	// };
	// }
	#[doc(hidden)]
	#[macro_export]
	macro_rules! func_sig {
	// [No block + Ret] Our ideal form: specified return type, no block:
	// (none)
	($(+($indir:tt))? $(%($i_im:ident, $i_mut:ident))? $($nom:ident)?, $(#[$attr:meta])* fn $fn_name:ident($($idents:ident : $types:ty),*) -> $ret:ty; $($rest:tt)*) => {
	#[inline] fn $fn_name($($idents : $types),*) -> $ret { compile_error!("trait functions not supported yet!") }
	$crate::func_sig!($(+($indir))? $(%($i_im, $i_mut))? $($nom)?, $($rest)*);
	};
	// (self)
	($(+($indir:tt))? $(%($i_im:ident, $i_mut:ident))? $($nom:ident)?, $(#[$attr:meta])* fn $fn_name:ident(self$(,)? $($idents:ident : $types:ty),*) -> $ret:ty; $($rest:tt)*) => {
	#[inline] fn $fn_name(self, $($idents : $types),*) -> $ret { ($($indir$indir)?self)$(.$nom)?$(.$i_im())?.$fn_name($($idents),*) }
	$crate::func_sig!($(+($indir))? $(%($i_im, $i_mut))? $($nom)?, $($rest)*);
	};
	// (mut self)
	($(+($indir:tt))? $(%($i_im:ident, $i_mut:ident))? $($nom:ident)?, $(#[$attr:meta])* fn $fn_name:ident(mut self$(,)? $($idents:ident : $types:ty),*) -> $ret:ty; $($rest:tt)*) => {
	#[inline] fn $fn_name(mut self, $($idents : $types),*) -> $ret { ($($indir$indir)?self)$(.$nom)?$(.$i_mut())?.$fn_name($($idents),*) }
	$crate::func_sig!($(+($indir))? $(%($i_im, $i_mut))? $($nom)?, $($rest)*);
	};
	// (&self)
	($(+($indir:tt))? $(%($i_im:ident, $i_mut:ident))? $($nom:ident)?, $(#[$attr:meta])* fn $fn_name:ident(&self$(,)? $($idents:ident : $types:ty),*) -> $ret:ty; $($rest:tt)*) => {
	#[inline] fn $fn_name(&self, $($idents : $types),*) -> $ret { ($($indir$indir)?self)$(.$nom)?$(.$i_im())?.$fn_name($($idents),*) }
	$crate::func_sig!($(+($indir))? $(%($i_im, $i_mut))? $($nom)?, $($rest)*);
	};
	// (&mut self)
	($(+($indir:tt))? $(%($i_im:ident, $i_mut:ident))? $($nom:ident)?, $(#[$attr:meta])* fn $fn_name:ident(&mut self$(,)? $($idents:ident : $types:ty),*) -> $ret:ty; $($rest:tt)*) => {
	#[inline] fn $fn_name(&mut self, $($idents : $types),*) -> $ret { ($($indir$indir)?self)$(.$nom)?$(.$i_mut())?.$fn_name($($idents),*) }
	$crate::func_sig!($(+($indir))? $(%($i_im, $i_mut))? $($nom)?, $($rest)*);
	};
	// [Block + Ret] Ditch blocks if you've got them:
	// (none)
	($(+($indir:tt))? $(%($i_im:ident, $i_mut:ident))? $($nom:ident)?, $(#[$attr:meta])* fn $fn_name:ident($($idents:ident : $types:ty),*) -> $ret:ty $block:block $($rest:tt)*) => {
	$crate::func_sig!($(+($indir))? $(%($i_im, $i_mut))? $($nom)?, fn $fn_name($($idents : $types),*) -> $ret; $($rest)*); };
	// (self)
	($(+($indir:tt))? $(%($i_im:ident, $i_mut:ident))? $($nom:ident)?, $(#[$attr:meta])* fn $fn_name:ident(self$(,)? $($self:expr,)? $($idents:ident : $types:ty),*) -> $ret:ty $block:block $($rest:tt)*) => {
	$crate::func_sig!($(+($indir))? $(%($i_im, $i_mut))? $($nom)?, fn $fn_name(self, $($idents : $types),*) -> $ret; $($rest)*); };
	// (mut self)
	($(+($indir:tt))? $(%($i_im:ident, $i_mut:ident))? $($nom:ident)?, $(#[$attr:meta])* fn $fn_name:ident(mut self$(,)? $($idents:ident : $types:ty),*) -> $ret:ty $block:block $($rest:tt)*) => {
	$crate::func_sig!($(+($indir))? $(%($i_im, $i_mut))? $($nom)?, fn $fn_name(mut self, $($idents : $types),*) -> $ret; $($rest)*); };
	// (&self)
	($(+($indir:tt))? $(%($i_im:ident, $i_mut:ident))? $($nom:ident)?, $(#[$attr:meta])* fn $fn_name:ident(&self$(,)? $($idents:ident : $types:ty),*) -> $ret:ty $block:block $($rest:tt)*) => {
	$crate::func_sig!($(+($indir))? $(%($i_im, $i_mut))? $($nom)?, fn $fn_name(&self, $($idents : $types),*) -> $ret; $($rest)*); };
	// (&mut self)
	($(+($indir:tt))? $(%($i_im:ident, $i_mut:ident))? $($nom:ident)?, $(#[$attr:meta])* fn $fn_name:ident(&mut self$(,)? $($idents:ident : $types:ty),*) -> $ret:ty $block:block $($rest:tt)*) => {
	$crate::func_sig!($(+($indir))? $(%($i_im, $i_mut))? $($nom)?, fn $fn_name(&mut self, $($idents : $types),*) -> $ret; $($rest)*); };
	// [No Block + No Ret] Add in return types if they're not specified:
	// (none)
	($(+($indir:tt))? $(%($i_im:ident, $i_mut:ident))? $($nom:ident)?, $(#[$attr:meta])* fn $fn_name:ident($($idents:ident : $types:ty),*); $($rest:tt)*) => {
	$crate::func_sig!($(+($indir))? $(%($i_im, $i_mut))? $($nom)?, fn $fn_name($($idents : $types),*) -> (); $($rest)*); };
	// (self)
	($(+($indir:tt))? $(%($i_im:ident, $i_mut:ident))? $($nom:ident)?, $(#[$attr:meta])* fn $fn_name:ident(self$(,)? $($idents:ident : $types:ty),*); $($rest:tt)*) => {
	$crate::func_sig!($(+($indir))? $(%($i_im, $i_mut))? $($nom)?, fn $fn_name(self, $($idents : $types),*) -> (); $($rest)*); };
	// (mut self)
	($(+($indir:tt))? $(%($i_im:ident, $i_mut:ident))? $($nom:ident)?, $(#[$attr:meta])* fn $fn_name:ident(mut self$(,)? $($idents:ident : $types:ty),*); $($rest:tt)*) => {
	$crate::func_sig!($(+($indir))? $(%($i_im, $i_mut))? $($nom)?, fn $fn_name(mut self, $($idents : $types),*) -> (); $($rest)*); };
	// (&self)
	($(+($indir:tt))? $(%($i_im:ident, $i_mut:ident))? $($nom:ident)?, $(#[$attr:meta])* fn $fn_name:ident(&self$(,)? $($idents:ident : $types:ty),*); $($rest:tt)*) => {
	$crate::func_sig!($(+($indir))? $(%($i_im, $i_mut))? $($nom)?, fn $fn_name(&self, $($idents : $types),*) -> (); $($rest)*); };
	// (&mut self)
	($(+($indir:tt))? $(%($i_im:ident, $i_mut:ident))? $($nom:ident)?, $(#[$attr:meta])* fn $fn_name:ident(&mut self$(,)? $($idents:ident : $types:ty),*); $($rest:tt)*) => {
	$crate::func_sig!($(+($indir))? $(%($i_im, $i_mut))? $($nom)?, fn $fn_name(&mut self, $($idents : $types),*) -> (); $($rest)*); };
	// [Block + No Ret] Strip blocks + add in return types:
	// (none)
	($(+($indir:tt))? $(%($i_im:ident, $i_mut:ident))? $($nom:ident)?, $(#[$attr:meta])* fn $fn_name:ident( $($idents:ident : $types:ty),*) $block:block $($rest:tt)*) => {
	$crate::func_sig!($(+($indir))? $(%($i_im, $i_mut))? $($nom)?, fn $fn_name($($idents : $types),*) -> (); $($rest)*); };
	// (self)
	($(+($indir:tt))? $(%($i_im:ident, $i_mut:ident))? $($nom:ident)?, $(#[$attr:meta])* fn $fn_name:ident(self$(,)? $($idents:ident : $types:ty),*) $block:block $($rest:tt)*) => {
	$crate::func_sig!($(+($indir))? $(%($i_im, $i_mut))? $($nom)?, fn $fn_name(self, $($idents : $types),*) -> (); $($rest)*); };
	// (mut self)
	($(+($indir:tt))? $(%($i_im:ident, $i_mut:ident))? $($nom:ident)?, $(#[$attr:meta])* fn $fn_name:ident(mut self$(,)? $($idents:ident : $types:ty),*) $block:block $($rest:tt)*) => {
	$crate::func_sig!($(+($indir))? $(%($i_im, $i_mut))? $($nom)?, fn $fn_name(mut self, $($idents : $types),*) -> (); $($rest)*); };
	// (&self)
	($(+($indir:tt))? $(%($i_im:ident, $i_mut:ident))? $($nom:ident)?, $(#[$attr:meta])* fn $fn_name:ident(&self$(,)? $($idents:ident : $types:ty),*) $block:block $($rest:tt)*) => {
	$crate::func_sig!($(+($indir))? $(%($i_im, $i_mut))? $($nom)?, fn $fn_name(&self $($idents : $types),*) -> (); $($rest)*); };
	// (&mut self)
	($(+($indir:tt))? $(%($i_im:ident, $i_mut:ident))? $($nom:ident)?, $(#[$attr:meta])* fn $fn_name:ident(&mut self$(,)? $($idents:ident : $types:ty),*) $block:block $($rest:tt)*) => {
	$crate::func_sig!($(+($indir))? $(%($i_im, $i_mut))? $($nom)?, fn $fn_name(&mut self, $($idents : $types),*) -> (); $($rest)*); };
	// And, finally, the end:
	($(+($indir:tt))? $(%($i_im:ident, $i_mut:ident))? $($nom:ident)?, ) => {};
	}

which is invoked on all the peripheral trait definitions:

core/traits/src/peripherals/gpio.rs

Line 141 in 5e0f0eb

peripheral_trait! {gpio,

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:

• under the "obvious" paradigm of having peripheral-trait-users be generic over the individual peripheral traits, you end up with users of the traits having types that look like the PeripheralsSet:

  struct MyNeatType<I: Input, O: Output, ...> {
    input: I,
    output: O,
    ...
  }

• the issue with the above is that it requires input and output to be separate instances; if we want one type (i.e. LoopbackIo) and one instance of that type to provide both our Input and Output implementations we need to resort to "external" state sharing mechanisms like Arc<Mutex<_>> to produce two instances that ultimately reference the same instance or some shared data
• having a Peripherals supertrait on the other hand lets us sidestep this:
  • the Peripherals implementor can choose to shell out to another type for a particular peripheral trait
  • but nothing requires it to shell out to separate types (or instances) for each peripheral trait that it is a super trait of

For the above reasons, we made Peripherals a supertrait:

core/traits/src/peripherals/mod.rs

Line 37 in 5e0f0eb

Gpio<'int> + Adc + Pwm + Timers<'int> + Clock + Input<'int> + Output<'int>

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:

• we created PeripheralSet which held instances of each peripheral and was generic over the 7 peripherals
• we had PeripheralSet implement each of the peripheral traits by shelling out to the peripheral instance contained within (this is what the peripheral_trait! macro did)

Problems

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

• it's super brittle! the peripheral_trait macro contains an ad-hoc redefinition of the Rust grammar's definition of what constitutes legal syntax for trait definitions: it's both very hard to read and subtly wrong in many ways
• bad developer experience: rust-analyzer and other tooling (i.e. rustfmt) don't interact well with items declared inside declarative macros like this. errors stemming from mistakes in the declarative macro's definition of trait definition syntax are extremely hard to diagnose for developers not very familiar with Rust declarative macros
• it doesn't compose well; we cannot, for example, do what's described in #... with peripheral_trait! because we can't have a declarative macro like it run on the output of a macro like #[async_trait]

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:

• create a struct holding your LoopbackIo device and whatever other peripherals you want to use
• implement Peripherals on that struct, exposing the same type and instance for Input and Output and their getters

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

• revise the Peripherals trait
• drop the peripheral_trait! macro
• introduce the ambassador provided delegated impls

where

branch: imp/peripheral-trait-reform

open questions