embedded-hal icon indicating copy to clipboard operation
embedded-hal copied to clipboard

Tracking issue for PWM traits

Open eldruin opened this issue 2 years ago • 5 comments

The PWM traits available on the 0.2.x versions have been removed ahead of the 1.0.0 release. See: #357 This issue servers as a tracking issue until we add them back. The open (breaking) problems should be reasonably-well solved and all associated types should have appropriate bounds that enable generic code use.

PWM traits as of the 0.2.7 release:

As noted below, the PwmPin only has a Duty associated type and does not need a settlement on the Time type as Pwm does.

Relevant issues/PRs:

  • #352
  • #256
  • #226
  • #182
  • Your issue here!

Please feel free to participate, highlight your current use cases, problems and provide solutions.

eldruin avatar Feb 13 '22 21:02 eldruin

As PwmPin doesn't have Time and only Duty, I think it is not so big issue. We just need to decide what type to use. Float is bad idea as timers are discrete. And could always be evaluated as (get_duty() as f32)/(get_max_duty() as f32). So u16 or u32? I think in most cases u16 is enough. But 32-bit timers can support both u16 and u32.

What if we move duty from associated type to generic and make u16 default type (like with I2c address type)?

pub trait PwmPin<Duty=u16> {
    fn disable(&mut self);
    fn enable(&mut self);
    fn get_duty(&self) -> Duty;
    fn get_max_duty(&self) -> Duty;
    fn set_duty(&mut self, duty: Duty);
}

In this case we "say" that u16 is required and other types are optional.

Alternative: return u32 always (like with Delay):

pub trait PwmPin {
    fn disable(&mut self);
    fn enable(&mut self);
    fn get_duty(&self) -> u32;
    fn get_max_duty(&self) -> u32;
    fn set_duty(&mut self, duty: u32);
}

But in this case we have potential problems with set_duty on 16-bit timers.

burrbull avatar Feb 14 '22 06:02 burrbull

My gut instinct favors a generic Duty:

  • Specifying duty cycles in the timer's domain feels more natural to me (maybe because I'm used to it)
  • Using the timer's domain could safe some instruction cycles for down-scaling the duty cycle or its limits
  • Storing precomputed duty sequences like for logarithmically fading an LED could get away with smaller arrays than always using u32 when avoiding to scale
  • And using adatpers for floating point or u32 duty cycles looks still possible with this approach

sirhcel avatar Jul 11 '22 21:07 sirhcel

i just ran into this (cf. https://github.com/dbrgn/embedded-hal-mock/pull/52#issuecomment-1336983674) as i so far use e-h 0.x which still has PwmPin. i understand the time-related issues and that this doesn't affect PwmPin itself and also see that there are solution suggestions here by @burrbull. is there anything specific still blocking re-adding PwmPin to e-h 1 with a generic Duty?

this affects e.g. drivers for typical h-bridge motor drivers, see e.g. the l298n crate or my own tb6612fng crate (both based on e-h 0.x). i had to hardcode the duty in my crate for the moment (it isn't released yet and i'm just using it on a single board), having the duty as a generic would mean that i could just let my consumer define it - my crate doesn't care about the exact type.

rursprung avatar Dec 05 '22 10:12 rursprung

We discussed this at some length in today's meeting, and the result is #430.

adamgreig avatar Dec 20 '22 20:12 adamgreig

How about async version PWM trait?

overheat avatar Apr 20 '23 08:04 overheat