Skip to content

Refactor PWM for simpler generic use #256

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from
Closed

Refactor PWM for simpler generic use #256

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
468141e
Refactor PWM for simpler generic use
ryankurte Oct 15, 2020
9183f60
run fmt
ryankurte Oct 15, 2020
abe693b
split pwm traits, example with markers and split/join
ryankurte Oct 17, 2020
f10ce7c
remove associated type bounds for pwm functions
ryankurte Oct 17, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
156 changes: 91 additions & 65 deletions src/pwm.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
//! Pulse Width Modulation

use core::time::Duration;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have not used this so far. How good does it play with embedded-time?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no idea, but, it avoids an external dependency / seems like it should be viable to have embedded-time play well with this?


/// Pulse Width Modulation
///
/// # Examples
Expand All @@ -9,107 +11,133 @@
/// ```
/// extern crate embedded_hal as hal;
///
/// use hal::prelude::*;
/// use core::time::Duration;
/// # use core::marker::PhantomData;
/// use hal::pwm::*;
///
/// fn main() {
/// let mut pwm: Pwm1 = {
/// let mut pwm = {
/// // ..
/// # Pwm1
/// # Pwm1(HasPins)
/// };
///
/// pwm.try_set_period(1.khz()).unwrap();
///
/// let max_duty = pwm.try_get_max_duty().unwrap();
/// let max_duty = Pwm1::<HasPins>::MAX_DUTY;
///
/// // brightest LED
/// pwm.try_set_duty(Channel::_1, max_duty).unwrap();
/// pwm.try_set_duty(0, max_duty).unwrap();
///
/// // dimmer LED
/// pwm.try_set_duty(Channel::_2, max_duty / 4).unwrap();
/// pwm.try_set_duty(1, max_duty / 4).unwrap();
///
/// let (pwm, [mut ch1, mut ch2]) = pwm.split();
///
/// ch1.try_set_duty(max_duty / 5).unwrap();
/// ch2.try_set_duty(max_duty / 5).unwrap();
///
/// }
///
/// # use core::convert::Infallible;
/// # struct KiloHertz(u32);
/// # trait U32Ext { fn khz(self) -> KiloHertz; }
/// # impl U32Ext for u32 { fn khz(self) -> KiloHertz { KiloHertz(self) } }
/// # enum Channel { _1, _2 }
/// # struct Pwm1;
/// # impl hal::pwm::Pwm for Pwm1 {
/// # trait U32Ext { fn khz(self) -> Duration; }
/// # impl U32Ext for u32 { fn khz(self) -> Duration { Duration::from_nanos( 1_000_000u64 / self as u64 ) } }
/// // Note: possibly you could genericise this over [PwmPinImpl; CHANNELS], which would be great for accessing channels internally
/// // except, these are probably not all going to be the same type, so instead we're using marker traits to specify whether it has been split or not
/// # struct Pwm1<S>(S);
/// # struct Pwm1Pin;
/// # struct HasPins;
/// # impl Pwm1<HasPins> {
/// # pub fn split(self) -> (Pwm1<()>, [Pwm1Pin; 2]) {
/// # (Pwm1(()), [Pwm1Pin, Pwm1Pin])
/// # }
/// # }
/// # impl Pwm1<()> {
/// # pub fn join(self, pins: [Pwm1Pin; 2]) -> Pwm1<HasPins> {
/// # Pwm1(HasPins)
/// # }
/// # }
/// #
/// # impl <S> hal::pwm::Pwm<u16> for Pwm1<S> {
/// # const CHANNELS: usize = 4;
/// # const MAX_DUTY: u16 = 1024;
/// # type Error = Infallible;
/// #
/// # fn try_disable(&mut self, _: usize) -> Result<(), Self::Error> { unimplemented!() }
/// # fn try_enable(&mut self, _: usize) -> Result<(), Self::Error> { unimplemented!() }
/// # fn try_get_period(&self) -> Result<Duration, Self::Error> { unimplemented!() }
/// # fn try_set_period(&mut self, _: Duration) -> Result<(), Self::Error> { Ok(()) }
/// # }
/// #
/// # impl hal::pwm::PwmDuty<u16> for Pwm1<HasPins> {
/// # fn try_get_duty(&self, _: usize) -> Result<u16, Self::Error> { unimplemented!() }
/// # fn try_set_duty(&mut self, _: usize, _: u16) -> Result<(), Self::Error> { Ok(()) }
/// # }
/// # impl hal::pwm::PwmPin<u16> for Pwm1Pin {
/// # const MAX_DUTY: u16 = 1024;
/// # type Error = Infallible;
/// # type Channel = Channel;
/// # type Time = KiloHertz;
/// # type Duty = u16;
/// # fn try_disable(&mut self, _: Channel) -> Result<(), Self::Error> { unimplemented!() }
/// # fn try_enable(&mut self, _: Channel) -> Result<(), Self::Error> { unimplemented!() }
/// # fn try_get_duty(&self, _: Channel) -> Result<u16, Self::Error> { unimplemented!() }
/// # fn try_get_max_duty(&self) -> Result<u16, Self::Error> { Ok(0) }
/// # fn try_set_duty(&mut self, _: Channel, _: u16) -> Result<(), Self::Error> { Ok(()) }
/// # fn try_get_period(&self) -> Result<KiloHertz, Self::Error> { unimplemented!() }
/// # fn try_set_period<T>(&mut self, _: T) -> Result<(), Self::Error> where T: Into<KiloHertz> { Ok(()) }
/// # fn try_disable(&mut self) -> Result<(), Self::Error> { unimplemented!() }
/// # fn try_enable(&mut self) -> Result<(), Self::Error> { unimplemented!() }
/// # fn try_get_duty(&self) -> Result<u16, Self::Error> { unimplemented!() }
/// # fn try_set_duty(&mut self, _: u16) -> Result<(), Self::Error> { Ok(()) }
/// # }
/// ```
// unproven reason: pre-singletons API. The `PwmPin` trait seems more useful because it models independent
// PWM channels. Here a certain number of channels are multiplexed in a single implementer.
pub trait Pwm {
/// Enumeration of `Pwm` errors
type Error;

/// Enumeration of channels that can be used with this `Pwm` interface
///
/// If your `Pwm` interface has no channels you can use the type `()`
/// here
type Channel;
pub trait Pwm<Duty> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering about this change. In my experience, type parameters are much more annoying to abstract over than associated types. It makes sense, if we expect HALs to implement this trait multiple times per timer, for u8, u16, f32, etc., but I don't know if that makes sense.

Copy link
Contributor Author

@ryankurte ryankurte Oct 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is consistent with the manner we have implemented spi. Duty could be a primitive type reflecting the word-size of the underlying timer, implemented once for each HAL (addressing #226). abstract consumers would require more complex bounds to support arbitrary duty types, but, this is somewhat inevitable for any abstraction and somewhat simplified by the use of a concrete type parameter vs. associated type w/ bounds ime.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is consistent with the manner we have implemented spi. Duty could be a primitive type reflecting the word-size of the underlying timer, implemented once for each HAL (addressing #226).

If it's implemented once per HAL, then it should be an associated type. SPI (and serial) traits have this design, because it's reasonable to implement them multiple times per peripheral, as peripherals generally support multiple word sizes (I've done this in several HALs).

If there's one implementation-defined value of Duty, it should be an associated type. Associated types are less cumbersome to abstract over, in my experience, as they don't have to show up as a type parameter on the methods or impl blocks of the driver.

/// Number of available channels
const CHANNELS: usize;

/// A time unit that can be converted into a human time unit (e.g. seconds)
type Time;
/// Maximum duty cycle value
const MAX_DUTY: Duty;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering how much good that will do. Any driver built on top of this still won't know what Duty actually is, so what can it do with this constant? Maybe we should have a duty Duty trait that is implemented for any type that provides certain operations (division, multiplication)? Then it would be pretty straight-forward to do something like set_duty(MAX_DUTY / 2).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a driver built on this has to know what duty is (or have a bound supporting conversion to duty) or it can't call any of the methods?

it would be possible for drivers to impose num_traits bounds on Duty if required, or to directly implement over <u16> and <u32> if those are all the duty types we see in practice. is there a case in which it's useful to a HAL to implement multiple Duty instances or could this be simplified by assuming Duty is a timer-defined primitive?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a driver built on this has to know what duty is (or have a bound supporting conversion to duty) or it can't call any of the methods?

I can imagine cases where all duty values are passed into the driver by the consumer, but even if that is a real-word use case, it would be limited.

it would be possible for drivers to impose num_traits bounds on Duty if required,

I agree that this is totally reasonable. This is another point where I'm concerned about stabilization though. If it turned out that most drivers require similar bounds, and HALs can provide them (in the end, duty is just a number after all, or isn't it?), we could later add those bounds to embedded-hal, to improve interoperability.

With 1.0 out, we could lock ourselves into a non-optimal situation, where most drivers require some bounds, but not always the same ones, and not all HALs provide them (if the duty type were a newtype, and not u16/u32 directly, for example).

or to directly implement over <u16> and <u32> if those are all the duty types we see in practice.

I think this would be an unfortunate situation. It's fine for SPI/Serial, as there's a standard type that every HAL provides in practice (u8), but I don't see such a standard type here (as timers have different resolutions). Not sure what the best solution here is.

is there a case in which it's useful to a HAL to implement multiple Duty instances or could this be simplified by assuming Duty is a timer-defined primitive?

I don't know. If stabilization weren't a concern (and it wouldn't be, if we had embedded-pwm), I'd suggest to assume that Duty is timer-defined, make it an associated type, and wait for the complaints to roll in.


/// Type for the `duty` methods
///
/// The implementer is free to choose a float / percentage representation
/// (e.g. `0.0 .. 1.0`) or an integer representation (e.g. `0 .. 65535`)
type Duty;
/// Enumeration of `Pwm` errors
type Error;

/// Disables a PWM `channel`
fn try_disable(&mut self, channel: Self::Channel) -> Result<(), Self::Error>;
fn try_disable(&mut self, channel: usize) -> Result<(), Self::Error>;
Copy link
Member

@eldruin eldruin Oct 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this is simpler, it does away with having names for channels, which may be more descriptive when reading the code.
Also, this introduces the necessity for a new runtime error: WrongChannel (or similar), which is unnecessary when the channel is an enum or so. It would be nicer if this could be enforced at compile time.

Copy link
Contributor Author

@ryankurte ryankurte Oct 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, the idea of this is that an abstract Something<T: Pwm> can drive N channels using an arbitrary PWM driver, which is somewhere between difficult and impossible using the current system because the abstract thing does not (and should not) know about the associated Channel type. with this approach a Pwm consumer can check the number of channels and address them as required, independent of implementation.


/// Enables a PWM `channel`
fn try_enable(&mut self, channel: Self::Channel) -> Result<(), Self::Error>;
fn try_enable(&mut self, channel: usize) -> Result<(), Self::Error>;

// Note: should you be able to _set_ the period once the channels have been split?
// my feeling is, probably not
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this question is misplaced here. The answer is "whatever makes sense for the target hardware", meaning it is for HAL authors to answer. embedded-hal should provide the building blocks for HAL authors, but not stand in their way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i can see this on either side, agreed the key focus is usability but both for hal authors and consumers. imo it'd be pretty unexpected if you split your PWM to use in two separate places and one of the altered the operation of other, and should you for some reason have two pins from two PWMs passed into a driver the operation of set_period is going to be fairly non-obvious.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that there are potential problems, and I haven't thought this through yet. I don't know what the right solution would be, and whether there are valid use cases for changing the period of split channels.

I'm saying that embedded-hal should be careful about pre-deciding such questions. Defining these interfaces is already hard enough (because there always seems to be some use case or hardware capability/constraint that wasn't considered). I can totally see adding such rules later, as experience with implementing and using these traits grows (which would be a breaking change, strictly speaking; another reason why I'm concerned about the current push for 1.0).


/// Returns the current PWM period
fn try_get_period(&self) -> Result<Self::Time, Self::Error>;
fn try_get_period(&self) -> Result<Duration, Self::Error>;

/// Sets a new PWM period
fn try_set_period(&mut self, period: Duration) -> Result<(), Self::Error>;

// Note: technically there could be a `channel` or `split` method here but this is
// rather extremely difficult prior to const-generics landing (and CHANNELS would need to be a const generic)
Copy link
Member

@hannobraun hannobraun Oct 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we shouldn't concern ourselves with split here. Initialization and configuration have always been out of scope for embedded-hal, and I think that's for the better. Just provide I/O interfaces that can be provided by HALs and consumed by drivers. Setting those up is the user's responsibility, and probably too device-specific to be abstracted over in a zero-cost/unopinionated way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah this is fair, we do have to communicate the expectations around split'ing to hal implementers tho, so if there's any opportunity for us to smooth that experience it seemed worth a look? can't do till const-generics and mostly agreed about not being opinionated about this just, exploring options.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see a future where the design space is generally much better understood, and where adding such constraints makes sense. I don't think we're there yet.

+1 to exploring options though!

}

/// PwmDuty trait allows PWM pins duty-cycles to be set per-channel
///
/// This should be implemented for a `Pwm` type that currently holds channel references
pub trait PwmDuty<Duty>: Pwm<Duty> {

/// Returns the current duty cycle
///
/// While the pin is transitioning to the new duty cycle after a `try_set_duty` call, this may
/// return the old or the new duty cycle depending on the implementation.
fn try_get_duty(&self, channel: Self::Channel) -> Result<Self::Duty, Self::Error>;

/// Returns the maximum duty cycle value
fn try_get_max_duty(&self) -> Result<Self::Duty, Self::Error>;
fn try_get_duty(&self, channel: usize) -> Result<Duty, Self::Error>;

/// Sets a new duty cycle
fn try_set_duty(&mut self, channel: Self::Channel, duty: Self::Duty)
-> Result<(), Self::Error>;

/// Sets a new PWM period
fn try_set_period<P>(&mut self, period: P) -> Result<(), Self::Error>
where
P: Into<Self::Time>;
fn try_set_duty(&mut self, channel: usize, duty: Duty) -> Result<(), Self::Error>;
}

/// A single PWM channel / pin
///
/// See `Pwm` for details
pub trait PwmPin {
/// This trait should be implemented over split PWM channels, see `Pwm` for details
pub trait PwmPin<Duty> {

/// Maximum duty cycle value
const MAX_DUTY: Duty;

/// Enumeration of `PwmPin` errors
type Error;

/// Type for the `duty` methods
///
/// The implementer is free to choose a float / percentage representation
/// (e.g. `0.0 .. 1.0`) or an integer representation (e.g. `0 .. 65535`)
type Duty;

/// Disables a PWM `channel`
fn try_disable(&mut self) -> Result<(), Self::Error>;

Expand All @@ -120,11 +148,9 @@ pub trait PwmPin {
///
/// While the pin is transitioning to the new duty cycle after a `try_set_duty` call, this may
/// return the old or the new duty cycle depending on the implementation.
fn try_get_duty(&self) -> Result<Self::Duty, Self::Error>;

/// Returns the maximum duty cycle value
fn try_get_max_duty(&self) -> Result<Self::Duty, Self::Error>;
fn try_get_duty(&self) -> Result<Duty, Self::Error>;

/// Sets a new duty cycle
fn try_set_duty(&mut self, duty: Self::Duty) -> Result<(), Self::Error>;
fn try_set_duty(&mut self, duty: Duty) -> Result<(), Self::Error>;
}