Skip to content
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.

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

Tracking issue for PWM traits #358

Open
eldruin opened this issue Feb 13, 2022 · 5 comments
Open

Tracking issue for PWM traits #358

eldruin opened this issue Feb 13, 2022 · 5 comments

Comments

@eldruin
Copy link
Member

eldruin commented Feb 13, 2022

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:

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

@burrbull
Copy link
Member

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.

@sirhcel
Copy link

sirhcel commented Jul 11, 2022

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

@rursprung
Copy link
Contributor

i just ran into this (cf. dbrgn/embedded-hal-mock#52 (comment)) 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.

@adamgreig
Copy link
Member

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

dbrgn pushed a commit to dbrgn/embedded-hal-mock that referenced this issue Jan 6, 2023
this extends the `Pin::Mock` to also cover the [`PwmPin`] trait.

some observations on this commit:
* i took the liberty of just hardcoding the `PwmPin::Duty` to be an
  `u16` (wrapped in the `PwmDuty` type because it's used in other
  places here in the module as well). i don't see an easy way to make
  this configurable because i'd have to add it as a generic attribute to
  `Pin` and then it'd have to be defined by everyone using it
  (unnecessary for all non-PWM use-cases). but i think this should be
  acceptable as `u16` probably covers most/all use-cases.
* the current code features quite some code duplication (essentially the
  method check implementations are all the same for all setters / all
  getters). i've continued this for now but it might be worth a
  refactoring in the future (i haven't touched it because i'm not sure
  if there's some strategic decision behind this?)
* there's a `TransactionKind::is_get` API for which i don't really see
  the reason (why not just check it directly?). i've decided to not copy
  that approach for the others as it'd IMHO just bloat the code and i'd
  instead suggest to remove `TransactionKind::is_get` at a later point.
* `PwmPin` only exists on `embedded-hal` 0.x, it is currently missing
  from the planned 1.0 release. see rust-embedded/embedded-hal#358 for
  the discussion on this. once it has (hopefully) been re-added there a
  corresponding mock can be provided here also for 1.x.

[`PwmPin`]: https://docs.rs/embedded-hal/0.2.7/embedded_hal/trait.PwmPin.html
bors bot added a commit that referenced this issue Mar 14, 2023
430: Add PWM SetDuty trait. r=therealprof a=Dirbaio

This adds back a version of the 0.2 trait `PwmPin`. 

cc #358

WG meeting [chatlog](https://matrix.to/#/!BHcierreUuwCMxVqOf:matrix.org/$_4DVnptWkGRjvW7-5Y6qCQZvj8oi9t4tbt8v4hXxJak?via=matrix.org&via=psion.agg.io&via=tchncs.de)

Differences to 0.2:

- `enable()` and `disable()` are gone. I think they were underspecified (for example, is the pin high or low when disabled?), and not very useful in practice. Disabling can be achieved already by setting duty to 0% or 100%. If the HAL cares about power consumption, it can transparently disable the timer and set the pin to fixed high/low when duty is 0% or 100%.
- Duty is no longer an unconstrained associated type. `u16` has been chosen because it gives enough dynamic range without being too big. `u8` might give too little dynamic range for some applications, `u32` might be annoyingly big for smaller archs like AVR/MSP430.
- Range is `0..u16::MAX` instead of `0..get_max_duty()`. This makes the HAL instead of the user responsible for the scaling, which makes using the trait easier. Also, if the HAL wants to optimize for performance, it can set the hardware period to be a power of 2, so scaling is just a bit shift.
- Name is `SetDuty` instead of `PwmPin`, because we might have a `SetFrequency` or similar in the future.
- I haven't included `get_duty()`, because I think in practice most drivers don't need it, and implementing it in HALs is annoying. They either have to read it back from hardware and unscaling it (losing precision), or storing the unscaled value in `self` (wasting RAM). We could add a `GetDuty` or `StatefulSetDuty` in the future if it turns out this is wanted, but I hope it won't be.

Co-authored-by: Dario Nieuwenhuis <dirbaio@dirbaio.net>
Co-authored-by: Grant Miller <GrantM11235@gmail.com>
@overheat
Copy link

How about async version PWM trait?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants