Refactor PWM for simpler generic use

[ryankurte]

this is just a draft / proof of concept for discussion

related to complaints about existing PWM trait usability, this attempts to improve the Pwm trait for use as a generic in higher level applications / logic. the key consideration here is that associated types that vary by implementation should be avoided as inputs to minimize integration complexity, and this applies to roughly all Channel based traits.

• Removes Time associated type, replaced with core::time::Duration
• Removes Channel associated type, replaced with associated const CHANNELS and usize indices
  • This means the driver does need an error for channel out of bounds rather than allowing type-system enforcement
  • Another approach would be a TryFrom<usize> bound, but, we're getting more complicated again

Questions:

• Is there a good / useful way to replace the Duty associated type?
  • One option would be to add a couple of From bounds for useful intermediates such as f32 so consumers could leave Duty unbound but consistently pass in f32 or other useful arguments.
  • This approach does not work for the &dyn case, which is useful but maybe not critical
• Is this a worthwhile goal?
  • I think so, and i believe we have achieved this reasonably well with the i2c/spi bus traits

[rust-highfive]

r? @eldruin

(rust_highfive has picked a reviewer for you, use r? to override)

[eldruin]

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

[ryankurte]

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

[eldruin]

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.

[ryankurte]

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.

[hannobraun]

Thank you @ryankurte for working on this! I appreciate that you're making a concrete proposal here. That said, I don't like it :-)

I do have the following problems:

• While this makes Pwm easier to use, it doesn't improve the core problems with the module as a whole. PwmPin is still here. Now I can move the PwmPin implementation somewhere else, but still change that channel's duty cycle from somewhere else.
• As @eldruin noted, this adds a runtime error, which I'd like to avoid.
• I think this is too opinionated and not flexible enough. As a HAL author, I'd like to implement something that is more flexible (provide the ability to move a single channel to another context, for example) and more solid (no needless runtime errors). As a driver author, I might only need a single channel (i.e. the ability to set duty cycle of a signal, not change frequency as a whole). With this proposal, I'd either build my driver on PwmPin, which makes it possible to mess with my operation by using Pwm from somewhere else, or I'd use the whole Pwm implementation, blocking resources I don't need.

(Side note: The conflict between Pwm and PwmPin and related problems can be solved by some runtime management of channels. The Pwm implementation could keep track for which channels a PwmPin instance exists for, and hand out runtime errors accordingly. I'd again argue that this is too opinionated. As a HAL author, I wouldn't want to write runtime code to manage something that could just as well be solved at compile-time using move semantics.)

Aside from these big-ticket criticisms, I'm wondering if embedded-time would be better than core::Duration. I've just started using it, so I can't say with certainty that it would be a good idea, but it seems it's specifically designed for cases like this.

Regarding the replacement of Duty, I see the following problems:

• If you use a type that's too small (e.g. u8), you might give up precisions on hardware that, for example, has 32-bit timers.
• If you use a type that's not suited for the target hardware (e.g. u32 on an AVR, or f32 on a Cortex-M0), you make embedded-hal unattractive for that target hardware.

I think this is pretty obvious, but I wanted to spell it out. All those type parameters and associated types exist for a reason. Yes, they are a pain to build abstractions on top of, and we pay a price in complexity, but the alternative might be to make embedded-hal less useful as a general abstraction. (And as a side note, personally I wouldn't mind if all number types were u32 or i32, as that would fit my use case perfectly. I'm just saying, if we want to be general, we might need to pay for that in complexity.)

---

I'd like to provide a counter-proposal that I think is more flexible and more robust. It would completely replace the current traits.

This is also only a draft, and a quick one at that. I haven't thought all aspects through (we might want to support type state for enabled/disabled channels, for example, which this proposal does not). And I haven't had a chance to try this in real life yet.

trait EnableChannel {
    type Error;

    fn try_enable(&mut self) -> Result<(), Self::Error>;
    fn try_disable(&mut self) -> Result<(), Self::Error>;

First, a separate trait for enabling/disabling channels. A HAL that wants to support one type per channel, can implement this directly on the channel type (channel.try_enable()). If this is not possible or desired, it can be implemented on an intermediate type and integrated into the timer API (timer.channel1().try_enable()).

trait Period {
    type Period; // or embedded-time, or `core::Duration`, I don't know

    fn try_set_period(&mut self, period: impl Into<Self::Period>) -> Result<(), Self::Error>;
    fn try_get_period(&self) -> Result<Self::Time, Self::Error>;
}

On the hardware I've seen, the period can be set once per timer and affects all channels of that timer (which is represented in the current traits). So most likely, HALs would implement this on timers directly (timer.try_set_period()), but on more exotic hardware, having it as a separate trait provides flexibility.

trait Duty {
    type Error;
    type Duty;

    fn try_set_duty(&mut self,  duty: impl Into<Self::Duty>) -> Result<(), Self::Error>;
    fn try_get_duty(&self) -> Result<Self::Duty, Self::Error>;
    fn try_get_max_duty(&self) -> Result<Self::Duty, Self::Error>;
}

Like the EnableChannel trait, this can be implemented on a channel type (channel.try_set_duty(duty)), or through an intermediate type for the timer (timer.channel1().try_set_duty()).

---

So much for the traits themselves. Now my thoughts on how to abstract over them. I'll use drivers for motor controllers as an example, as that's what I've been thinking about recently.

Example 1: DC Motor Driver

I think for a DC motor, you just need to control the duty cycle to control speed. So the driver would just take ownership of a Duty implementation, which on typical hardware corresponds to a channel. You can control multiple motors using the same timer.

timer.try_set_period(some_appropriate_value)?;

let motor1 = DcDriver::new(channel1);
let motor2 = DcDriver::new(channel2);

If, for some reason, you can't separate timers and channels (my timer.channel1().try_set_duty(duty) example from above), you can either give the intermediate type to DcDriver, which would likely have a reference to the timer, or you could transform your timer into a new type that implements both Period and Duty for a given channel (timer.into_channel1()). I can't think of a reason why this would be necessary. I'm just saying this model provides options.

Example 2: Stepper Motor Driver

To a stepper motor driver, the duty cycle doesn't matter much. It needs control over the period. One option is for the driver to require an impl Period and an impl Duty:

let motor1 = StepperDriver::new(timer1, channel1_1);
let motor2 = StepperDriver::new(timer2, channel2_1);

Another option is for the driver to require one single type that implements both Duty and Period. Whether to do this is up to the driver author. Either the HAL or the user of the driver and the HAL can easily accommodate this by providing an additional type that wraps the timer and one of its channels.

let timer_and_channel = TimerAndChannel::new(timer, channel1); // API is up for debate
let timer_and_channel = channel1.upgrade(timer); // maybe this instead; doesn't matter to embedded-hal anyway

let motor = StepperDriver::new(timer_and_channel);

---

As I said, this is just a quick draft. I'm going to have need for solid PWM traits in the near-ish future (timeline is a bit unclear; it's a side project and I got lots to do), including traits that abstract over DMA-powered PWM. So unless someone preempts me with another solution, I will likely try to implement my proposal on both the driver and HAL sides.

Elsewhere I've floated the idea of doing this in an embedded-pwm crate, which would take away some burden from embedded-hal (allowing for a 1.0 release that much sooner) and merging the traits back into embedded-hal, once matured.

[ryankurte]

thanks for looking at it, all fair criticisms. the key point to me is to address the use of generic traits by consumers, though i am sure there are many ways to go about this and happy with any that do adequately provide abstraction over Pwm and PwmPin instances.

While this makes Pwm easier to use, it doesn't improve the core problems with the module as a whole. PwmPin is still here. Now I can move the PwmPin implementation somewhere else, but still change that channel's duty cycle from somewhere else.

yeah, i didn't intend to address this initially. splitting the traits as you suggest seems reasonable, though the split methods or similar cannot easily be represented in our traits (at least without const-generics). i have updated the PR with an approach to mitigate this, definitely interested in what you come up with when you get to working on this.

Regarding the replacement of Duty, I see the following problems ...

keeping the Duty type is not so bad as far as things go, especially if we can loose some other associated types and if it is only implemented over primitive / numerics, though with a further point about associated type bounds at the end.

Aside from these big-ticket criticisms, I'm wondering if embedded-time would be better than core::Duration. I've just started using it, so I can't say with certainty that it would be a good idea, but it seems it's specifically designed for cases like this.

once embedded-time has stabilised and been adopted here this may be a good option, however, for now this means we avoid an additional (unstable) dependency and it'll just work (tm) with std::time::Duration for linux users. nothing precludes using core::time::Duration as the interface between e-h and e-t to achieve (i believe) the same outcome.

Elsewhere I've floated the idea of doing this in an embedded-pwm crate, which would take away some burden from embedded-hal (allowing for a 1.0 release that much sooner) and merging the traits back into embedded-hal, once matured.

i have wondered about this too, we could remove all the unfinished / blocked traits from the 1.0.0 release and work towards re-implementing them later. this does however seem pretty annoying for hal users who will have to pay the porting penalty twice (and work out what to do with missing functionality in between) :-/ maybe something that could be raised with the larger working group.

I think this is pretty obvious, but I wanted to spell it out. All those type parameters and associated types exist for a reason. Yes, they are a pain to build abstractions on top of, and we pay a price in complexity, but the alternative might be to make embedded-hal less useful as a general abstraction.

absolutely, the problem is that every associated type that isn't usefully generic breaks the abstraction, and any additional complexity either in or due to our e-h traits is something that the entire ecosystem has to content with, it's tradeoffs all the way down.

As it stands, it is currently impossible to write a Something<P: Pwm> implementation crate without a concrete implementation for each possible Pwm instance (and associated types) you may wish to use, which imo isn't a useful generic abstraction.
due to compiler constraints it's also impossible to box (or &dyn) a trait using associated type bounds (you must to specify the concrete underlying type), so while adding conversion bounds can smooth the associated type interoperability pains in some situations, it only addresses function inputs, and breaks in frustrating and irreconcilable ways in others. we have to work out what the midpoint is but, i am sure we can improve the existing trait on all fronts ^_^

[hannobraun]

Thank you, I've taken a look at the new commits and left some comments.

Overall, I think this new version concerns itself too much with matters that have always been out of scope of embedded-hal, namely initialization and HAL architecture. Other modules just provide I/O traits. How to provide those traits is up to the HAL, and I think that's for the better. I believe my own proposal is very much in the spirit of that.

Providing all these traits that have redundant methods, and telling HAL authors when to implement each, is just muddying the waters, in my opinion.

[hannobraun]

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.

[ryankurte]

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.

[hannobraun]

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).

[hannobraun]

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.

[ryankurte]

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.

[hannobraun]

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.

[hannobraun]

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).

[ryankurte]

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?

[hannobraun]

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.

[hannobraun]

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.

[ryankurte]

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.

[hannobraun]

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!

[hannobraun]

yeah, i didn't intend to address this initially. splitting the traits as you suggest seems reasonable, though the split methods or similar cannot easily be represented in our traits (at least without const-generics). i have updated the PR with an approach to mitigate this, definitely interested in what you come up with when you get to working on this.

I don't think a split method (or any other architecture that a HAL might choose) needs to be represented, or even should be. We don't do this for any of the other modules. Why do it here?

once embedded-time has stabilised and been adopted here this may be a good option, however, for now this means we avoid an additional (unstable) dependency and it'll just work (tm) with std::time::Duration for linux users. nothing precludes using core::time::Duration as the interface between e-h and e-t to achieve (i believe) the same outcome.

embedded-time can work with core::time::Duration, but as that link states, "Due to the inner types used by core::time::Duration, a lot of code bloat occurs when it is used.". I haven't checked how significant that is, but one of the stated goals of embedded-hal is to be zero-cost, which a forced conversion from u32 to core::time::Duration, back to u32, for example, definitely wouldn't be.

You said "for now", and I totally agree that core::time::Duration is good enough for now. I don't think it would be good enough to release with 1.0, unless a very clear and informed decision has been made, that other options (like embedded-time) are not acceptable.

i have wondered about this too, we could remove all the unfinished / blocked traits from the 1.0.0 release and work towards re-implementing them later. this does however seem pretty annoying for hal users who will have to pay the porting penalty twice (and work out what to do with missing functionality in between) :-/ maybe something that could be raised with the larger working group.

The fact is, there are interfaces in embedded-hal which are not ready (otherwise we wouldn't be able to have this discussion), and the goal of the working group (as far as I know) is to release embedded-hal 1.0. These two points are in conflict, and there is some price we have to pay to resolve this conflict:

1. Remove stuff from embedded-hal, make it more difficult for users of that stuff.
2. Delay the 1.0 release of embedded-hal, until said stuff has matured.
3. Release un-matured traits with embedded-hal 1.0.

It's not my call to make, but out of these I think option 3 is the only unacceptable solution. My fear is that the desire to avoid option 2 will lead to option 3, which is why I favor option 1.

[ryankurte]

hey thanks for all the reviewing! again i don't really intend to land this just, think it useful to explore the problem space. probably be good to do some impls / drivers to prove any approach but, doesn't seem worth the time without more direction / consensus.

embedded-time can work with core::time::Duration, but as that link states, "Due to the inner types used by core::time::Duration, a lot of code bloat occurs when it is used.

ohhhh, i always forget there's a u64 in there :-/

I don't think it would be good enough to release with 1.0, unless a very clear and informed decision has been made, that other options (like embedded-time) are not acceptable.

relates to #211, it seems to me this would also block e-h on the stabilisation and adoption of e-t into the wg. wrt the rest of your points this may need more wg-wide discussion but, i agree that option 3 is the least desirable outcome / would aim to keep chugging away with -alpha releases until we get it all polished.

[hannobraun]

hey thanks for all the reviewing! again i don't really intend to land this just, think it useful to explore the problem space. probably be good to do some impls / drivers to prove any approach but, doesn't seem worth the time without more direction / consensus.

Thank you again for the proposal and the discussion! Totally understood that this isn't intended to be the final solution. I hope I don't come across as too stand-offish. Like you, I'm interested in exploring the problem space and pointing to potential problems where I see them.

I don't think it would be good enough to release with 1.0, unless a very clear and informed decision has been made, that other options (like embedded-time) are not acceptable.

relates to #211, it seems to me this would also block e-h on the stabilisation and adoption of e-t into the wg. wrt the rest of your points this may need more wg-wide discussion but, i agree that option 3 is the least desirable outcome / would aim to keep chugging away with -alpha releases until we get it all polished.

Yes, it would. No idea how pratical that would be in the near-term. Personally, I'd be fine chugging away with alpha release for the next few years, if that's what it takes. Given the overall push for 1.0 though, I'd imagine that would be a problem for some people. Hence my idea of taking problematic areas off the table, via an embedded-pwm and maybe other crates along those lines.

[hannobraun]

After this whole discussion and some thinking, I looked at the current traits with fresh eyes again, and I think I've been wrong about most things I said about them:

1. The biggest misunderstanding I've been hung up on is that the channel argument in Pwm and the type implementing PwmPin are somehow supposed to be the same type. This can't work (which was part of my criticism), but I now believe that it would be fundamentally wrong to try and do it, or somehow enable it by modifying the traits.
2. My other misunderstanding (which I based a criticism on) is that the existence of Pwm and PwmPin are in conflict. I no longer think they are, and the right way to implement it seems quite obvious in retrospect (and probably has been obvious to everyone else the whole time): Pwm is what exists initially. PwmPin must either invalidate Pwm once it comes into existence (via split()), or PwmPin must be a field of Pwm, which would make Pwm impossible to use once a PwmPin is used separately (i.e. moved somewhere else).
3. My own proposal is wrong and should be discarded. It doesn't support the use case of needing to set the period and duty cycle for multiple channels. It only allows for either setting the duty cycle of multiple channels or the period and duty cycle for one.

With these new insights, I think that the current traits capture two different use cases and need to be kept. At least I can't think of a way to substantially improve upon them (modulo the details, like embedded-time or bounds on Duty, as discussed above).

From my perspective, this leaves one thing wrong with the current traits: That abstracting over them requires a Copy or Clone bound on Pwm::Channel (which @ryankurte addresses in #246).

---

Based on my new understanding, I'm thinking that the changes made in this pull request are a step in the wrong direction:

1. I'm still doubtful that converting Duty into a generic argument is a good idea, for the reasons expressed above.
2. I believe that using usize for the channel argument is a step in the wrong direction.

I'd like to expand on that second point a bit:

• Unless we assume that HALs provide separate APIs for generic use (i.e. embedded-hal trait impls) and concrete use (i.e. the API of that specific HAL), which is the opposite of what I've seen in the wild, we also have to consider how changes to embedded-hal affect the concrete use case. Here, the switch to usize is a clear loss in type safety.
• It also wouldn't be zero-cost, as the usize argument will have to be checked for validity in all methods that accept it. While an enum, on the other hand, can be relied upon to only provide valid values.
• I don't even think it would be a win from the driver perspective (although this has yet to be proven by experience; I have not yet tried to write a driver on top of the current traits):
  • The driver would need an additional check in its constructor (e.g. if Timer::MAX_CHANNELS < 2 { return Err(...); }). This incurs another runtime cost that wouldn't be needed otherwise (not zero-cost) and it might complicate the driver API (might require a Result where none would be needed otherwise, and adding a source of panics is never good either).
  • With the current traits, the driver could just take the channels that are relevant to it as arguments to its constructor, which would be zero-cost (e.g. fn new<Timer>(motor1: Timer::Channel, motor2: Timer::Channel) or even fn new<Timer>(motors: &[Timer::Channel])).

[ryankurte]

replying to lots of comments here because, we're a bit everywhere 😂

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).

yep, gotta be careful of all this eh. with i2c we've sealed the traits so Addr can only be a u8 (7-bit) or u16 (10-bit), could do something like that if required i guess (and extending the allowed types is non-breaking).

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.

the resolution thing is interesting but, as a fundamental a timer has some count register width and as a low level abstraction it seems reasonable to me to bound duty to the appropriate width (like u16) with a MAX_DUTY constant representing the top value? on your classic ARM platform it might even always be a u32 for register width, but with const MAX_DUTY: u32 = 2^12 or whatever.

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.

oh, the other reason to do this is i believe (though may have missed something) on cannot have a const MAX_DUTY: Self::Duty. not critical as we can also express this with fn max_duty() -> Result<Self::Duty, ..> as the trait currently is. i suspect this might not be quite as compiler optimizable but, i'm also not sure what the implications of associated constants on type bounds are either so...

With the current traits, the driver could just take the channels that are relevant to it as arguments to its constructor

yeah passing a Pwm and an array of channel objects (or, an array of PwmPins, tho that may require some fun dyn tricks) isn't at all unreasonable / is how i intend to mitigate this with my radio.Channel trait. and switching to by reference per 246 makes this possible, totally onboard with this.

[hannobraun]

the resolution thing is interesting but, as a fundamental a timer has some count register width and as a low level abstraction it seems reasonable to me to bound duty to the appropriate width (like u16) with a MAX_DUTY constant representing the top value? on your classic ARM platform it might even always be a u32 for register width, but with const MAX_DUTY: u32 = 2^12 or whatever.

I think I said somewhere before, as far as I'm concerned, we could make it u32 everywhere and be done with it. But my use case is not the only one, and we have to consider the goals of embedded-hal. I've seen lots of timers with resolutions of 31 or 32 bits, so u16 is the wrong choice (not general enough). Using u32 seems like it wouldn't be zero-cost for lower-resolution timers, especially on 16-bit or 8-bit platforms (although I don't have a good enough intuition about how the compiler might optimize certain cases to really be sure).

I'm going off of the stated design goals here. I think it's totally reasonable to want a simple abstraction layer that works well enough on common embedded platforms and is not necessarily zero-cost on all of them. In that case, u32 would be good enough everywhere. It goes against the design goals and established precedent of embedded-hal though.

oh, the other reason to do this is i believe (though may have missed something) on cannot have a const MAX_DUTY: Self::Duty. not critical as we can also express this with fn max_duty() -> Result<Self::Duty, ..> as the trait currently is. i suspect this might not be quite as compiler optimizable but, i'm also not sure what the implications of associated constants on type bounds are either so...

Interesting, I didn't know about that limitation.

Short note: You say duty, but I think you're talking about the period. The maximum duty will depend on the timer period, which is runtime adjustable, while the maximum period is a constant value that depends on the timer resolution.

[ryankurte]

Short note: You say duty, but I think you're talking about the period. The maximum duty will depend on the timer period, which is runtime adjustable, while the maximum period is a constant value that depends on the timer resolution.

yeah, i have conceptually combined period and duty on the basis that they're usually the same width (with the same maximums), which is the justification for a type parameter over this (which can be u32 or u16 or u8 as appropriate). perhaps this would be better termed Width or something.

[hannobraun]

yeah, i have conceptually combined period and duty on the basis that they're usually the same width

Ah, understood, makes sense.

[ryankurte]

closing now PWM has been removed, see #358 for reintroduction