-
-
Notifications
You must be signed in to change notification settings - Fork 8.2k
rp2/machine_pwm: Fix disallowed default TOP value of 65535 by changing it to 65534. #11336
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
Conversation
27df56f
to
2e76d57
Compare
One intended side effect for setting the Duty cycle to TOP is to have a permanent high output., Likewise setting it to 0 would result in a permanent low output. |
I'm not sure what you're saying. The "permanent high output" is supposed to be obtained by calling And we were already claiming that the maximum allowed value of TOP was 65534. The point of this PR is to make this work by enforcing that maximum value even if the frequency is unset. Some alternative approaches could be to choose a sensible default frequency other than the random 1907 Hz we get now, or to fix the duty cycle at 0% until both frequency and duty cycle have been set. But the current behavior where |
When I test it, I get a 16.4 ns low pulse for a setting of duty_ns(65534), and a constant high level for duty_ns(65535). That seems reasonable, being the opposite of duty_u16(0). Besides that, please have a look at PR #10850, which attempts to reduce inconsistencies between ports. With that PR, no signal is created until both freq and duty cycle are set. |
Thanks for testing and I'm glad you think it looks reasonable. I tried your branch, and while it is nice that it doesn't actually activate the PWM until the frequency is set, it could still use my change to avoid overflow effects that are visible to the user: MicroPython v1.19.1-999-g7b8c85311 on 2023-04-26; Raspberry Pi Pico with RP2040
Type "help()" for more information.
>>> from machine import *
pwm = PWM(Pin(25, Pin.OUT))
>>> pwm.duty_u16(65534)
>>> pwm.duty_u16()
65534
>>> pwm.duty_u16(65535)
>>> pwm.duty_u16()
0
>>> pwm.freq(1000)
>>> pwm.duty_u16()
65535 Also, none of these changes address the completely nonsensical values we can get with multiple pwm objects: MicroPython v1.19.1-999-g7b8c85311 on 2023-04-26; Raspberry Pi Pico with RP2040
Type "help()" for more information.
>>> from machine import *
pwm = PWM(Pin(25, Pin.OUT))
pwm.duty_u16(65534)
pwm2 = PWM(Pin(25, Pin.OUT))
pwm2.freq(1000)
>>> pwm.duty_u16()
66570
>>> |
The behavior of the first example results from the fact, that the frequency was not set yet. Then, the duty rate cannot be determined. The code could raise an error in that case instead. The behavior of the second example occurs, when two python objects use the same slice & channel. Instantiating the second object will set the duty type to unknown. Possible way to avoid this result from a strange coding style:
|
"Set the duty to 0" sounds like a good option for your branch; it would be very surprising for the accessor method Is that branch likely to get merged soon? If not I recommend at least doing the minor fix in this PR first. |
Actually I went for raising an error, if duty_xx() or freq() is called before both duty rate and/or freq have been set using the same python object. The way machine.PWM is implemented in the rp2 port require for duty() both values to be set and valid, before a correct value for the duty rate can be returned. Given that, the fix that you suggest is wrong, and it addresses arbitrarily only the overflow situation. But the values returned for other settings are likewise wrong. e.g.: >>> from machine import PWM, Pin
>>> p=PWM(Pin(2))
>>> p.duty_u16(32768)
>>> p2=PWM(Pin(2))
>>> p2.freq(1000)
>>> p.duty_u16()
33287
>>> p2.duty_u16()
33287 As you can see, there is no overflow, but the value is wrong. as soon as you set the duty again, the proper value is returned. |
Sounds good. The change that I added was raising an error instead of returning a wrong value. But that only for the rp2 port. Other ports may have similar behavior, in returning wrong duty values when not completely configured. |
Like I said, this PR was not even attempting to address the problems created when the constructor is called again. The duty cycle you get with this version, when you read it back after initializing a single object is just fine: MicroPython v1.19.1-1020-g2e76d5715 on 2023-04-28; Raspberry Pi Pico with RP2040
Type "help()" for more information.
>>> from machine import PWM, Pin
p=PWM(Pin(25))
p.duty_u16(32768)
p.duty_u16()
32768 So I don't agree that there's anything wrong with any other duty cycles in the same way that 65535 was wrong. Anyway I'll close this and go comment on the other PR since it is now going to address this issue! |
Before this fix, the following code would surprisingly set the duty cycle to 0%:
Since TOP has a hardware default of 65535, our duty_u16 calculation was trying to set CC to 65536, which overflows to zero.
With this change, when setting the duty cycle, we set TOP back to 65534 in this case.