Skip to content
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: Fix PWM not allowing frequencies < 611 Hz (16 bit resolution). #8210

Closed
wants to merge 5 commits into from

Conversation

IhorNehrutsa
Copy link
Contributor

@IhorNehrutsa IhorNehrutsa commented Jan 25, 2022

This is another version of
esp32: Fix PWM not allowing frequencies < 611 Hz. #8200

It uses a maximum of 16bit PWM resolution.

Tested on ESP32

@IhorNehrutsa
Copy link
Contributor Author

ESP-IDF doesn't like changing the PWM resolution.

# pwm_test_freq.py

from utime import sleep
from machine import Pin, PWM

F_MIN = 1
F_MAX = 10 #20_000_000

f = F_MIN
delta_f = 1

pin = Pin(26)

p = PWM(pin, f, duty_u16=2**16//2)
while True:
    p.freq(f)
    print(p)
    
    sleep(10)
    
    f += delta_f
    if (f >= F_MAX) or (f <= F_MIN):
        delta_f = -delta_f
        
        break

These tests are executed with uncommented lines

/*
// Bug: Sometimes duty is not set right now.
// Not a bug. It's a feature. The duty is applied at the beginning of the next signal period.
// Bug: It has been experimentally established that the duty is setted during 2 signal periods, but 1 period is expected.
// See https://github.com/espressif/esp-idf/issues/7288
if (duty != get_duty_u16(self)) {
PWM_DBG("set_duty_u16(%u), get_duty_u16():%u, channel_duty:%d, duty_resolution:%d, freq_hz:%d", duty, get_duty_u16(self), channel_duty, timer.duty_resolution, timer.freq_hz);
ets_delay_us(2 * 1000000 / timer.freq_hz);
if (duty != get_duty_u16(self)) {
PWM_DBG("set_duty_u16(%u), get_duty_u16():%u, channel_duty:%d, duty_resolution:%d, freq_hz:%d", duty, get_duty_u16(self), channel_duty, timer.duty_resolution, timer.freq_hz);
}
}
*/

In this PR

PWM(Pin(26), freq=1, duty_u16=32768, resolution=16, (duty=50.00%, resolution=0.002%), mode=0, channel=0, timer=0)
PWM(Pin(26), freq=2, duty_u16=32768, resolution=16, (duty=50.00%, resolution=0.002%), mode=0, channel=0, timer=0)
PWM(Pin(26), freq=3, duty_u16=32768, resolution=16, (duty=50.00%, resolution=0.002%), mode=0, channel=0, timer=0)
PWM(Pin(26), freq=4, duty_u16=32768, resolution=16, (duty=50.00%, resolution=0.002%), mode=0, channel=0, timer=0)
PWM(Pin(26), freq=5, duty_u16=32768, resolution=16, (duty=50.00%, resolution=0.002%), mode=0, channel=0, timer=0)
PWM(Pin(26), freq=6, duty_u16=32768, resolution=16, (duty=50.00%, resolution=0.002%), mode=0, channel=0, timer=0)
PWM(Pin(26), freq=7, duty_u16=32768, resolution=16, (duty=50.00%, resolution=0.002%), mode=0, channel=0, timer=0)
PWM(Pin(26), freq=8, duty_u16=32768, resolution=16, (duty=50.00%, resolution=0.002%), mode=0, channel=0, timer=0)
PWM(Pin(26), freq=9, duty_u16=32768, resolution=16, (duty=50.00%, resolution=0.002%), mode=0, channel=0, timer=0)

In #8200

set_duty_u16(32768), get_duty_u16():65536, channel_duty:262144, duty_resolution:19, freq_hz:1
PWM(Pin(26), freq=1, duty_u16=32768, resolution=19, (duty=50.00%, resolution=0.000%), mode=0, channel=0, timer=0)
set_duty_u16(32768), get_duty_u16():65536, channel_duty:131072, duty_resolution:18, freq_hz:2
PWM(Pin(26), freq=2, duty_u16=32768, resolution=18, (duty=50.00%, resolution=0.000%), mode=0, channel=0, timer=0)
PWM(Pin(26), freq=3, duty_u16=32768, resolution=18, (duty=50.00%, resolution=0.000%), mode=0, channel=0, timer=0)
set_duty_u16(32768), get_duty_u16():65536, channel_duty:65536, duty_resolution:17, freq_hz:4
PWM(Pin(26), freq=4, duty_u16=32768, resolution=17, (duty=50.00%, resolution=0.001%), mode=0, channel=0, timer=0)
PWM(Pin(26), freq=5, duty_u16=32768, resolution=17, (duty=50.00%, resolution=0.001%), mode=0, channel=0, timer=0)
PWM(Pin(26), freq=6, duty_u16=32768, resolution=17, (duty=50.00%, resolution=0.001%), mode=0, channel=0, timer=0)
PWM(Pin(26), freq=7, duty_u16=32768, resolution=17, (duty=50.00%, resolution=0.001%), mode=0, channel=0, timer=0)
set_duty_u16(32768), get_duty_u16():65536, channel_duty:32768, duty_resolution:16, freq_hz:8
PWM(Pin(26), freq=8, duty_u16=32768, resolution=16, (duty=50.00%, resolution=0.002%), mode=0, channel=0, timer=0)
PWM(Pin(26), freq=9, duty_u16=32768, resolution=16, (duty=50.00%, resolution=0.002%), mode=0, channel=0, timer=0)

@robert-hh
Copy link
Contributor

That's what I get if I run #8200 with your script. Board ESP32_GENERIC_SPIRAM. Everything fine, and on the oscilloscope it looks right as well.

PWM(Pin(21), freq=1, duty_u16=32768, resolution=19, (duty=50.00%, resolution=0.000%), mode=0, channel=0, timer=0)
PWM(Pin(21), freq=2, duty_u16=32768, resolution=18, (duty=50.00%, resolution=0.000%), mode=0, channel=0, timer=0)
PWM(Pin(21), freq=3, duty_u16=32768, resolution=18, (duty=50.00%, resolution=0.000%), mode=0, channel=0, timer=0)
PWM(Pin(21), freq=4, duty_u16=32768, resolution=17, (duty=50.00%, resolution=0.001%), mode=0, channel=0, timer=0)
PWM(Pin(21), freq=5, duty_u16=32768, resolution=17, (duty=50.00%, resolution=0.001%), mode=0, channel=0, timer=0)
PWM(Pin(21), freq=6, duty_u16=32768, resolution=17, (duty=50.00%, resolution=0.001%), mode=0, channel=0, timer=0)
PWM(Pin(21), freq=7, duty_u16=32768, resolution=17, (duty=50.00%, resolution=0.001%), mode=0, channel=0, timer=0)
PWM(Pin(21), freq=8, duty_u16=32768, resolution=16, (duty=50.00%, resolution=0.002%), mode=0, channel=0, timer=0)
PWM(Pin(21), freq=9, duty_u16=32768, resolution=16, (duty=50.00%, resolution=0.002%), mode=0, channel=0, timer=0)

@robert-hh
Copy link
Contributor

Same result with UM_FEATHERS2 and GENERIC_C3. Only for the C3 I had to use the 10 <= f < 20 range.

@robert-hh
Copy link
Contributor

Note: I have a glitch in my build system, in that changes to machine_pwm.c do not cause a re-compile of it. I did not look for the reason yet, but remove machine_pwm.c.obj manually instead to force the re-compile.

@robert-hh
Copy link
Contributor

Second note: Sometimes, after a a power cycle. I see the phenomenon that the PWM does start extremely slow at the first frequency set with your test code, whatever it is, 1 Hz or 10 Hz. The next setting then is fine. So there seems to be an uninitialized state somewhere. When starting with 1Hz, the duty_u16 shown is 65538, when starting with 9Hz the duty shown is 262144(2**18). But the PWM output is fine. It runs at the proper frequency and duty cycle.

@robert-hh
Copy link
Contributor

I catched an extended cycle at the transition from 3 to 5 Hz, resolution 18 to 17. It shows an extended cycle. On the left side, it's 3 Hz, on the right it's 4 Hz. And sometimes the values are set completely odd. Most likely a problem in the esp-idf.

pwm_3_to_4_Hz

@IhorNehrutsa
Copy link
Contributor Author

IhorNehrutsa commented Jan 25, 2022

That's what I get if I run #8200 with your script. Board ESP32_GENERIC_SPIRAM. Everything fine, and on the oscilloscope it looks right as well.

Have you uncommented PWM_DBG?

// #define PWM_DBG(...)
#define PWM_DBG(...) mp_printf(&mp_plat_print, __VA_ARGS__); mp_printf(&mp_plat_print, "\n");

P.S. Sorry for the delay and excuse me for spending your time.

@robert-hh
Copy link
Contributor

robert-hh commented Jan 26, 2022

I uncommented the block in set_duty_u16(). Whatever, I have seen this glitch in the PWM trace itself: there are disturbed clock cycles when the resolution changes. Here is another example when switching between 1220 and 1221 Hz (resolution 16->15). The same happens when switching between resolution 15<->14, and so on.

pwm_1220_to_1221_Hz

@IhorNehrutsa
Copy link
Contributor Author

Most likely a problem in the esp-idf.

Yes, most likely in

check_esp_err(ledc_set_duty(self->mode, self->channel, channel_duty));
check_esp_err(ledc_update_duty(self->mode, self->channel));

@robert-hh
Copy link
Contributor

I've seen also states where the PWM does not start at all without a resolution change happening. That's even more strange.

@IhorNehrutsa
Copy link
Contributor Author

It seems that issue is resolved 3 hours ago (but I don't see a commit with fixes):
espressif/esp-idf#7288

@espressif-bot espressif-bot added Resolution: NA Status: Done Resolution: Done and removed Status: In Progress Resolution: NA labels 3 hours ago

@IhorNehrutsa
Copy link
Contributor Author

May I rename
PWFREQ to PWM_FREQ
PWRES to PWM_RES_10_BIT
?

@robert-hh
Copy link
Contributor

Sure. But we should make only one PR for that issue. Do you want to keep yours?

@IhorNehrutsa
Copy link
Contributor Author

Yes if you not against.
This PR has a smaller changing of resolution.

@robert-hh
Copy link
Contributor

This PR has a smaller changing of resolution.

OK. So I'll close mine then. Tell me, if you are finished, Then I can make a few tests,

@IhorNehrutsa
Copy link
Contributor Author

Ready

@robert-hh
Copy link
Contributor

Did a test series with GENERIC_SPIRAM, GENERIC_C3 and UM_FEATHERS2
GENERIC_SPIRAM works down to 1 Hz.
GENERIC_C3 works down to 10Hz
UM_FEATHERS2 works down to 1 Hz, with a glitch: When switching from 10..50Hz to 1Hz it takes a few seconds until the PWM output starts again at 1 Hz. Maybe the known esp-idf glitch, maybe another. Switching from 2..9Hz to 1Hz or >59Hz to 1 Hz is no problem (except for the one below).

The glitch when changing between resolutions still exists.It must be fixed in the esp-idf. The UM_FEATHERS2 has a glitch on EVERY frequency change, sometimes for several signal periods, when the resolution changes as well.

According to that test, the machine_pwm.c code works, but bugs in the esp-idf still cause glitches. But that should have existed already before and is identified by espressif.

@dpgeorge
Copy link
Member

dpgeorge commented Feb 1, 2022

Thanks for this, rebased and merged in a5e64c2 and 15e65b7 (the last commit contains the 4 clean-up commits).

@dpgeorge dpgeorge closed this Feb 1, 2022
@robert-hh
Copy link
Contributor

robert-hh commented Feb 13, 2022

@IhorNehrutsa There is another tiny hiccup. For frequencies < 611, the duty rate cannot be set with the constructor after hard reset. It will always be 512. See this post in the forum: https://forum.micropython.org/viewtopic.php?f=18&t=11975&p=65175#p65083

@IhorNehrutsa IhorNehrutsa deleted the pwm611_res16 branch July 10, 2023 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants