-
-
Notifications
You must be signed in to change notification settings - Fork 8k
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
esp32/PWM: Reduce inconsitencies between ports. #10854
base: master
Are you sure you want to change the base?
esp32/PWM: Reduce inconsitencies between ports. #10854
Conversation
Code size report:
|
Wouldn't it be better to put DEBUG printing into a separate PR? |
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #10854 +/- ##
=======================================
Coverage 98.36% 98.36%
=======================================
Files 159 159
Lines 21090 21090
=======================================
Hits 20745 20745
Misses 345 345 ☔ View full report in Codecov by Sentry. |
V30303-161405.mp4 |
I fetched the PR, compiled & loaded it. What I found somewhat unexpected was the error message after the sequence:
Which said: What has to be discussed beside that is the method duty(). The range is different for the ports which support it. 1024 for ESP32, 256 for ESP8266, 100 for nrf and 10000 for CC3200. The duty() method was intentionally not implemented any more at the newer ports. And i may be dropped in a release later than 1.20 (nickname Godot). |
output is
Earlier in a discussion, someone said that 1023 is too small for his application case, so duty=100 in nrf port is small too. 0..10000 is attractive for noobs
output is
I vote for the duty [0..1023] range. |
pwm.duty() is kept for legacy only in the ports that had it before duty_us() was introduced. It will be dropped some day. For instance with the announced version 2.x, which will have breaking changes. So no reason to change it at the moment. |
V30309-134257.mp4 |
V30309-141247.mp4 |
983b093
to
eb33120
Compare
7d821bb
to
265b82e
Compare
@IhorNehrutsa I have a question regarding the PWM module. Reading the various ESP32 reference manuals I learned, the the ESP32 has 3 to 8 timers, which can be assigned to 6 to 16 PWM channels, which then can be assigned to GPIO pins. Pretty flexible. Since the timer and channel to be used cannot be selected in the constructor, the selection of timer an channel is kind of automatic. What is the policy you have implemented? Is there some way to control this assignment? |
Try to find a timer with the same frequency in the current mode, otherwise in the next mode.
Output is::
See https://github.com/IhorNehrutsa/micropython/blob/pwm_reduce_inconsist/docs/esp32/tutorial/pwm.rst |
Thank you for the explanation and the example code. It is very convenient. The strategy looks fine for an automatic assignment. It may as well be helpful to specify timer and channel in the constructor replacing the automatic search, such that one can have synchronous channels at a timer. |
@IhorNehrutsa In order to make
You could consider whether you skip the test for changes and just configure the channel always when you call init(). |
265b82e
to
490b8ea
Compare
The esp32 board build fails with esp-idf v4.0.2 because of this include in machine_pwm.c: |
Please restrict the changes in this PR to the machine.PWM module and do not add unrelated code like for debug printing. These affect all other ports as well. I you consider that useful for everyone, please open a separate PR for it. |
MP_PRN() will removed in release |
At the moment I can only use ESP IDF 5.0.2. Switching back to 4.4.2 is not possible. Since you PR fails to build with IDF 5.0.2, I cannot buils & test it. |
Indeed. With Edit: Or |
I would be happy to help you to test it, but how can rebranch on this code (not very confident with git...) ? |
Good start. Then you have to checkout the branch.
|
Actually, I wanted to suggest |
I have run a basic test that create all available channel with or wthout lightsleep enbale, and I don't get same result (especially when there is clock conflict). This is the output of the test programe (to compare with the trace in the #16102 (comment) :
We can see that the exception is not rise, and less infos are printed out... |
@yoann-darche |
According to the: micropython#10817. Signed-off-by: Ihor Nehrutsa <[email protected]>
1ab1a47
to
d17f4f0
Compare
Updated version of
|
d17f4f0
to
117fedc
Compare
Update
to get
|
|
Ok understand, but from my point of view, these information are not errors, but can be usefull for electronic developers. Shall we keep it as normal reporting ? |
At one of the iterations of the two-year code review, one of the reviewers suggested just such a solution. (I am not sure.) Users use compiled bin files, and developers increase the MICROPY_ERROR_REPORTING and compile the code as they need. |
I did not make that comment, but as general rule the information printed for an object should be able to be fed into a init() method. Needless to say that there is much legacy stuff that does not meet this rule. |
I've a use case that fails due to the mixture of Clocks : from machine import PWM, Pin, lightsleep
a=PWM(Pin(4), freq=3000, duty=256, lightsleep=True)
print(a)
f=PWM(Pin(2), freq=2000, duty=64)
print(f)
lightsleep(20*1000) # Led goes off The excepted functionality is the Pin(4) should be set in order to continue to run under lighsleep(), and the pin(2) should to stop under lightsleep(). but I got this error du too the miss selecting of the active clock:
The thing that I've proposed : if an timer is already set with RC_FAST_CLK clock, all other time need to use it either if lightsleep is true or not, except if the SOC support High speed mode where each group can use a separate clock source. EDIT: After several tests, this case happend when the frequency differs so when it is need to create a new timer source. |
And even more
|
|
Perfect now it is working as expected on ESP32-S3, I've also check the output signal with a scope, and it is working. @IhorNehrutsa great job ! |
Thank you very much @yoann-darche @robert-hh |
esp32/machine_pwm.c: Reduce PWM inconsistencies between ports. 1. duty_u16() high value is 2**16-1 == 65535 2. Invert PWM wave with invert=1 parameter 3. Enable PWM in light sleep mode 4. Allows to output PWM and read pulse input simultaneously on the same Pin() 5. Code refactoring Co-Authored-By: Angus Gratton <[email protected]> Co-Authored-By: robert-hh <[email protected]> Co-Authored-By: Andrew Leech <[email protected]> Co-Authored-By: Yoann Darche <[email protected]> Signed-off-by: Ihor Nehrutsa <[email protected]>
Debugging Note: Increase the MICROPY_ERROR_REPORTING level to view _FUNCTION__, __LINE__, __FILE__ in check_esp_err() exceptions. Signed-off-by: Ihor Nehrutsa <[email protected]>
c07ec0e
to
10e53b5
Compare
Test on ESP32-WROVER-E:
I've also test the lightsleep mode and again it is working as expected 👍. Please note the last test (#4) that exploit the double clock source available on ESP32... nice ! |
IMHO. This PR is ready for merging. |
Thanks @IhorNehrutsa. This is marked for the 1.26 release. The 1.25 release is being finalised now, and we won't merge such a big change so late in the release. However, expect to see some movement on this after 1.25 is out. |
The 2nd anniversary since the opening of PR! Congratulations! |
This PR is developed according to the: PWM: Reduce inconsistencies between ports. #10817
duty_u16() high value is 2**16-1 == 65535
Invert PWM wave with
invert=1
parameterEnable PWM in light sleep mode like in
esp32/machine_pwm: Enhancement of PWM: Add features light_sleep_enable. #16102
Test code 1
Test code 2
Allows PWM output and the pulse-input is the same pin.