PWM get_max_duty() off by one

[jannic]

On the RP2040, the maximum duty cycle is TOP+1, equivalent to 100%:

A CC value of 0 will produce a 0% output, i.e. the output signal is always low. A CC value of TOP + 1 (i.e. equal to the period,
in non-phase-correct mode) will produce a 100% output. For example, if TOP is programmed to 254, the counter will have
a period of 255 cycles, and CC values in the range of 0 to 255 inclusive will produce duty cycles in the range 0% to 100%
inclusive.

However, currently get_max_duty() returns the TOP value.

Fixing this is not as easy as just changing self.regs.read_top() to self.regs.read_top() + 1 in get_max_duty(), as both CC and TOP are 16 bit values. So if TOP is 0xffff, the max CC value is still 0xffff and not 0x10000, and it is not possible to configure full 100% duty cycle.

The most sensible solution out of this conflict could be to just limit the TOP value to 0xfffe in set_top(). (This would also change the default initialization value in the same way.)

Other alternatives are:

• returning 0xffff.min(self.regs.read_top() + 1) from get_max_duty(). Which would mean that we can't implement embedded-hal's max_duty_cycle() fully correctly, because it's defined to return the value equivalent to 100% duty cycle, and that value doesn't exist if top is 0xffff.
• returning 0x10000 as max_duty_cycle() and implement a special case for that value in set_duty_cycle(), overriding the output pin state manually. (This could be difficult and messy.)

[thejpster]

The most sensible solution out of this conflict could be to just limit the TOP value to 0xfffe in set_top()

Makes sense to me

[ithinuel]

Duty is an associated type so would the use of a newtype (and/or an enum) to wrap an integer with range checks be simple enough ? (I'd rather avoid over engineering).

[jannic]

Duty is an associated type so would the use of a newtype (and/or an enum) to wrap an integer with range checks be simple enough ? (I'd rather avoid over engineering).

I don't think this can be fixed by modifying set_duty or it's parameter of type Duty alone: When TOP is set to 0xffff, there's just no register value of CC that would lead to true 100% duty cycle.

[jannic]

Really fixing set_duty in a way that would prevent the caller from setting it to 0xffff would imply adding some error handling: Either the function would have to panic, or return some error value, in case the caller specified 0xffff. (Using some wrapper type with limited range would only shift the same issue to the instantiation of that type.)

As this is not a soundness issue, it should be sufficient to document that 0xffff would prevent the hardware from generating a true 100% duty cycle. However, the default value should be changed to 0xfffe. That's what I implemented in #744.