Add PWM SetDuty trait.

[Dirbaio]

This adds back a version of the 0.2 trait PwmPin.

cc #358

WG meeting chatlog

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.

[ryankurte]

seems broadly good to me!

Duty is no longer an unconstrained associated type. u16 has been chosen because it gives enough dynamic range without being too big.

if we do need more (or less) resolution in the future it should be non-breaking to swap to a default associated type + markers (à la vec) and const MAX... i'd probably prefer this from the get-go, (and maybe there's some overlap with timer widths) but am okay either way.

[GrantM11235]

This is the alternative that I mentioned during the meeting. It brings back the get_max_duty method and adds some handy convenience methods. I think that this makes it easier to use and it also should result in faster math with less rounding errors

pub trait Pwm {
    // Required methods
    fn get_max_duty(&self) -> u16;
    fn set_duty(&mut self, duty: u16);

    // Provided methods
    fn set_off(&mut self) {
        self.set_duty(0)
    }
    fn set_on(&mut self) {
        self.set_duty(self.get_max_duty())
    }
    fn set_fraction(&mut self, num: u16, denom: u16) {
        let duty = num as u32 * self.get_max_duty() as u32 / denom as u32;
        self.set_duty(duty as u16)
    }
    fn set_percent(&mut self, percent: u8) {
        self.set_fraction(percent as u16, 100)
    }
}

[rursprung]

i like the set_percent feature from @GrantM11235, that'd cover nearly all my existing use-cases.
the only other use-case i have seen is when mapping angles (in degrees) to a PWM signal when driving a servo - obviously going from 0 - 180° (or even 0 - 359°) has a higher resolution than going from 0 - 100% (see the arduino API for servos). but that would of course be covered by the set_fraction method which can be used for this use-case.

[burrbull]

@GrantM11235 All autoimplemented methods should be marked with #[inline].

related: https://rust.godbolt.org/z/44bn77Msx

[GrantM11235]

Some more notes in no particular order:

• It's important to stress that setting the duty to the max value means "on 100% of the time", not just 99.99%.
• My previous post was a simplified version that didn't contain all the necessary result/error stuff. In the real version, all methods should return a Result except for get_max_duty which should be infallible.
• If set_duty is called with a value greater than the max duty, the implementation is allowed do just about anything, as long as it isn't UB. Including but not limited to:
  • Clamp to max duty
  • Panic
  • Return an error
  • Set the duty to some other value (due to integer wrapping)
  • That's all I can think of
• The same thing applies to calling set_fraction with num > denom and calling set_percent with a value > 100.
• Calling set_fraction with a denom of zero will panic.

[rursprung]

• the implementation is allowed do just about anything, as long as it isn't UB

for drivers this might be an issue if they're passing on input - now they have to do the sanity check of the input values and the HAL might well do it again. i think it'd be good if this were specified.

by the way: would the API profit if it'd use something like UpperBounded from num-traits or would that be overkill (or not even possible in this situation)? i have no personal experience with this crate and didn't think it through yet, but have seen it before (probably in some examples, don't remember) and it has 111 million downloads, so i guess it's somewhat mature? 😅

[GrantM11235]

• the implementation is allowed do just about anything, as long as it isn't UB

for drivers this might be an issue if they're passing on input - now they have to do the sanity check of the input values and the HAL might well do it again. i think it'd be good if this were specified.

Some drivers will want to clamp to the max value, some drivers will want to return an error, and some (most?) drivers won't need to do any sanity checking at all if all possible inputs map to a valid duty value. Therefore the most efficient option is for each driver to implement their preferred behavior, and to not require any sanity checking in the HALs

by the way: would the API profit if it'd use something like UpperBounded from num-traits

That trait returns the maximum value that can be represented by a particular type. It doesn't help us in this case because the max duty value is not represented in the type system. If the max duty value was represented in the type system (for example with const generics), that would prevent dynamic configuration/reconfiguration of the timer period

[adamgreig]

It might be nice to not allow panicking in drivers, just because unavoidable panic paths are pretty annoying. If the operation returns a Result anyway, couldn't it just return an error condition (which we could add to ErrorKind) for this situation? We could allow it to silently do something else like clamp or wrap or whatever if it likes.

I expect few implementations will be able to accept all u16 as valid duties, not least because insisting on max being 100% duty is likely to mean HALs will have to special-case that value anyway. I expect it's more likely HALs will do something like accept an 8-bit or 10-bit or whatever value and mask off whatever's supplied as the most-convenient option, or otherwise clamp?

[GrantM11235]

I think that a bad duty value is a "garbage in, garbage out" situation. I think that HALs should be encouraged to use a debug_assert!, and otherwise not worry about it.

I don't think that there should be an ErrorKind for it because that would imply that all implementations need to check it.

Some food for thought, including:

• A pseudo-implementation for stm32 which shows that no special casing is required
• A moderately complex generic driver that uses a precomputed lookup table while also allowing the user to dynamically reconfigure the pwm period.

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=47d88c1da07cfd679f66b6fe0db778ae

[chrysn]

Rather than leaving this to prose, why not use the core type? That way, the compiler will know not to handle a zero-division even when it can't const-propagate the denominator.

Suggested change

	/// The caller is responsible for ensuring that `num` is less than or equal to `denom`,
	/// and that `denom` is not zero.
	#[inline]
	fn set_fraction(&mut self, num: u16, denom: u16) -> Result<(), Self::Error> {
	let duty = num as u32 * self.get_max_duty() as u32 / denom as u32;
	/// The caller is responsible for ensuring that `num` is less than or equal to `denom`,
	/// and that `denom` is not zero.
	#[inline]
	fn set_fraction(&mut self, num: u16, denom: core::num::NonZeroU16) -> Result<(), Self::Error> {
	let duty = num as u32 * self.get_max_duty() as u32 / denom.get() as u32;

[adamgreig]

We discussed this at this week's meeting but didn't reach a consensus. Using NonZero ensures the function can't panic, but makes for a less ergonomic interface, especially since the majority of applications are likely to specify a constant denominator.

[Dirbaio]

Discussed in today's meeting. Last bikeshed item: renamed "duty" to "duty cycle".

I think it should be good to go.

@rust-embedded/hal

[therealprof]

Fine with me.

[therealprof]

bors r+

[Dirbaio]

bors r+

[bors]

Already running a review

[bors]

Build succeeded:

• clippy
• fmt
• test (1.59.0, thumbv6m-none-eabi)
• test (1.59.0, thumbv7m-none-eabi)
• test (1.59.0, x86_64-unknown-linux-gnu)
• test (nightly, thumbv6m-none-eabi)
• test (nightly, thumbv7m-none-eabi)
• test (nightly, x86_64-unknown-linux-gnu)
• test (stable, thumbv6m-none-eabi)
• test (stable, thumbv7m-none-eabi)
• test (stable, x86_64-unknown-linux-gnu)

[usbalbin]

In some cases one PWM timer may be able to adjust frequency on the run and also have multiple output channels with independent duty cycles. In those cases there may be one timer type responsible for the frequency or period and one or more types for the channels. Then the channels does not know what the current period is and thus can not know the max duty value.

With all of the set_* methods supposed to take get_max_duty into account, I believe this might complicate things when the period may be adjusted from another type potentially from another interrupt/thread/context.

Same thing the other way around, say the above was somehow resolved. If we managed to ch1.set_percentage(50), what would happen the next time timer.set_period(old_period * 2) is called? What would that do to the duty? In many cases I would imagine that it would not stay at 50% which I think would be quite surprising

Please correct me if I am wrong but I, atleast in the above case, I believe get_max_duty belongs more with the timer than with the channel. Thus it will have to be up to the user to adjust for that and we are stuck with using raw compare values for setting the duty. While this would put more responsibility on the user, i think it would be less surprising and more straight forward (in that particular case).

Again please let me know if I am missing something :)

[Dirbaio]

The main goal of the traits is to support HAL-independent drivers. For example, a "RGB LED driver" would take 3 impl SetDutys, one for each R/G/B channel.

If the traits did model "PWM peripheral" and "PWM output channel", the RGB LED driver would have to be aware of whether the chip configures max_duty per peripheral or per channel, whether different R/G/B channels are in the same PWM peripheral or not... This makes the trait harder/impossible to use for drivers without HAL compat issues.

(The previous PWM trait in EH0.2 worked similarly to what you mention, it was scrapped due to this reason (and due to unconstrained Time types #324 too, but that's another story))

HALs can still implement the traits for chips where max_duty is per-PWM-peripheral instead of per-channel. There's a few options:

• Simply do not allow changing frequency after creating the PWM.
• Always report max_duty = u16::MAX. On set_duty, save the unscaled (0..u16::MAX) duty, scale it and apply it to the hardware. On freq change, apply max_duty to the hardare, scale all the saved unscaled duties again with the new max_duty, apply them again the hardware. Use Cell/RefCell/Mutex to share that data across the several channels.

The HALs don't necessarily have to use the embedded-hal traits as their main API. They can have a "native" API that exactly matches the hardware and has low level control, and then implement the traits in parallel, only to be used with HAL-independent drivers. So it's OK If the trait API needs some more heavy lifting from the HAL, or is less flexible, to ensure compatibility.