esp32/machine_pwm: handle multiple timers. #6276

p1ng0o · 2020-07-22T21:52:31Z

This commit allow to use all the available timer for the ledc subsystem,
without affecting the API.

If a new frequency is set, first it check if an other timer is using
the same freq.
If yes, then it use this timer, otherwise it create a new one.

If all timers are used, the user should set an already used frequency,
or de-init a channel.

This PR is related to the work done in #3608, but without the API changes.

Tested on esp32-wrover.

dpgeorge · 2020-08-26T14:10:28Z

Thanks for the contribution.

I understand that PWM on esp32 is lacking because it is fixed to LEDC_TIMER_1 and cannot have independent frequencies on different pins, so an improvement like this is welcome. Compared to #3608 it does not change the API, as described above.

Ideally I want to work towards a machine.PWM API which is consistent across all ports, and this PR is compatible with that goal (at least more compatible than #3608) because it doesn't change the existing API. So I'd be happy to merge it once a few things are fixed with it.

dpgeorge · 2020-08-26T14:11:14Z

ports/esp32/machine_pwm.c

+    // Find a free PWM Timer using the same freq
+    for (timer = 0; timer < LEDC_TIMER_MAX; ++timer) {
+        if (timers[timer].freq_hz == freq) {
+            break;


could just return timer here

dpgeorge · 2020-08-26T14:11:31Z

ports/esp32/machine_pwm.c

+            break;
+        }
+        if ((timer_found == -1) && (timers[timer].freq_hz == -1)) {
+            timer_found = timer;


could just return timer here

dpgeorge · 2020-08-26T14:11:47Z

ports/esp32/machine_pwm.c

+            timer_found = timer;
+        }
+    }
+    if (timer >= LEDC_TIMER_MAX) {


could just return -1 here if above suggestions are used

dpgeorge · 2020-08-26T14:17:51Z

ports/esp32/machine_pwm.c

+    int freq = args[ARG_freq].u_int;
+
+    if (freq == -1) {
+        freq = PWFREQ;


If freq is not passed in as an argument (so it's -1) then the frequency should not be reconfigured (that's the existing behaviour of this function). Eg:

pwm = machine.PWM(machine.Pin(0), freq=500) pwm.init(duty=200) # this should retain the existing freq setting

I think this can be fixed by:

if (chan_timer[self->channel] == -1) { freq = PWFREQ; } else { freq = timers[chan_timer[self->channel]].freq_hz; }

p1ng0o · 2020-08-26T14:43:14Z

Hi @dpgeorge, thanks for the review.
Regarding the found timer algorith, I will submit an update. But we have to use an existing channel freq prior looking for an available channel.

Regarding the init, I think I kept the behavior you described.

It freq is not passed in as an argument, I just set the freq with the default value. And that was the case before. (channel struct init).

The main difference, is that not reuse the "global" frequencie.

Not sure about this one. Let me know.

dpgeorge · 2020-08-27T02:59:35Z

Before this patch the behaviour was:

MicroPython v1.12-683-g5fb276de3-dirty on 2020-08-26; TinyPICO with ESP32-PICO-D4
Type "help()" for more information.
>>> import machine
>>> pwm = machine.PWM(machine.Pin(0), freq=500)
>>> pwm
PWM(0, freq=500, duty=512)
>>> pwm.init(duty=200)
>>> pwm
PWM(0, freq=500, duty=200)

With the patch running the same code gives:

MicroPython v1.12-689-g7351f537e on 2020-08-27; TinyPICO with ESP32-PICO-D4
Type "help()" for more information.
>>> import machine
>>> pwm = machine.PWM(machine.Pin(0), freq=500)
>>> pwm
PWM(0, freq=500, duty=512)
>>> pwm.init(duty=200)
>>> pwm
PWM(0, freq=5000, duty=200)

Note the last line: the freq changed back to 5000 for this patch.

p1ng0o · 2020-08-27T15:26:33Z

Understood. I will work on it. Thanks.

dpgeorge · 2020-08-28T00:40:52Z

Thanks for updating, it looks good now, is this ready to merge? (the CI failures can be ignored)

dpgeorge · 2020-08-28T02:58:26Z

Actually, doing some tests, it seems that, even with only 1 active PWM, changing the frequency while it's active does not work. Eg:

pwm = machine.PWM(machine.Pin(4))
pwm.freq(4000)

The pin stops outputing a PWM signal.

dpgeorge · 2020-08-28T04:20:10Z

ports/esp32/machine_pwm.c

    }

    // set
    int tval = mp_obj_get_int(args[1]);
-    if (!set_freq(tval)) {
+    cleanup_timer(self->channel, chan_timer[self->channel]);


I think it's important to make it so that there is a smooth transition to another frequency, if the same timer that's currently connected to the pin/channel can be reused. Eg, when freq changes, if the timer on this pin is not used by any other PWM channel, and there is no other timer with the requested freq, then just call set_freq() on the current timer.

p1ng0o · 2020-08-28T13:34:07Z

I think all you valuable review has been taken in account. I made some tests with the oscilloscope today. It seems pretty ok. But It would be better if we have some tests suite around that. Whats will be the best way to do that ? (it could be an other PR)

dpgeorge · 2020-08-30T06:06:29Z

With the latest changes it seems that there is an error when trying to increase the frequency on a pin. Eg:

>>> import machine
>>> p4 = machine.PWM(machine.Pin(4))
>>> p4.freq(10000)
E (326020) ledc: ledc_bind_channel_timer(174): timer_select argument is invalid
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: Failed to bind timer to channel

IhorNehrutsa · 2020-11-19T17:29:19Z

Are any problems still exist with this PR?
I have tested it with 4 different frequencies at 8 different channels with different duties.
Frequencies and duties are right on proper pins.

p1ng0o · 2020-12-05T13:50:33Z

@dpgeorge, I think this PR is mature now. Can I do something to move forward on it ?

IhorNehrutsa · 2020-12-05T14:35:11Z

@p1ng0o I made a little fix to your work in #6654, but unfortunately, I lost your authority in commits, sorry.
It will be right to approve this PR and then #6654(with #6638).

IhorNehrutsa · 2020-12-05T17:05:22Z

@p1ng0o, Could you test the #6654 and #6639? And what you think about #6638? Thanks. Sorry for the offtopic.

ports/esp32/machine_pwm.c

stinos · 2020-12-10T13:50:50Z

ports/esp32/machine_pwm.c

+            return true;
+        }
+    }
+


Newline not really needed

Not sure about that one. Consistency is not matched in this file.

That's fair, but I just don't see why it's needed :)

ports/esp32/machine_pwm.c

stinos · 2020-12-10T13:56:06Z

The commit messages should start with a capital after the colon, see other commits in the repository. So for the first commit that would be for instance:

esp32/machine_pwm: Re-use existing timer if possible.

In order to smooth the pwm transition.  If the timer is only used by
one channel just set the frequency.

This commit allow to use all the available timer for the ledc subsystem, without affecting the API. If a new frequency is set, first it check if an other timer is using the same freq. If yes, then it use this timer, otherwise it create a new one. If all timers are used, the user should set an already used frequency, or de-init a channel.

stinos · 2020-12-10T21:02:03Z

ports/esp32/machine_pwm.c

+    // Find a free PWM Timer using the same freq
+    for (timer = 0; timer < LEDC_TIMER_MAX; ++timer) {
+        if (timers[timer].freq_hz == freq) {
+            // A timer already use the same freq. Use it now.


'already use' -> 'already uses' (sorry missed this one earlier)

In order to smooth the pwm transition. If the timer is only use by one channel just set the frequency.

p1ng0o · 2021-03-10T10:16:27Z

@dpgeorge how can i move forward on this work ?

IhorNehrutsa · 2021-03-10T11:44:10Z

I think this PR is completely ready for approval and I see no reason to further postpone its implementation.
All that's left is to click the merge button.
:(
But we are in a https://en.wikipedia.org/wiki/Slow_television

tve

I see two issues: one is that the error code on esp-idf calls are not checked (all it takes is wrapping the call in check_esp_err) and that some initialization routine needs to be tied into the soft reset in main.c so PWM isn't in a limbo state after a soft reset.

tve · 2021-03-13T15:32:27Z

ports/esp32/machine_pwm.c

        return MP_OBJ_NEW_SMALL_INT(duty);
    }

    // set
    duty = mp_obj_get_int(args[1]);
    duty &= ((1 << PWRES) - 1);
-    duty >>= PWRES - timer_cfg.duty_resolution;
+    duty >>= PWRES - timers[chan_timer[self->channel]].duty_resolution;
    ledc_set_duty(PWMODE, self->channel, duty);
    ledc_update_duty(PWMODE, self->channel);


Why do you no longer check the error code on these two calls?

IhorNehrutsa · 2021-09-18T22:22:29Z

This pull request is obsolete since
af64c2d#diff-71ce230991b3c3b3b928d13bd84430a2e5e5db6148c2da5497fba7280d6731a2
Please close it.
#7817 is based on this PR.
Thanks, @p1ng0o

dpgeorge · 2021-09-19T12:43:34Z

Aah, sorry for not getting back to this!

@IhorNehrutsa thanks for working on it. Can you please explain (briefly) how your #7817 is different to this one? Is it just that you updated it to work with latest master and the new extmod/machine_pwm.c bindings?

IhorNehrutsa · 2021-09-19T13:19:02Z

@dpgeorge This PR supports only LEDC_HIGH_SPEED_MODE

#define PWMODE (LEDC_HIGH_SPEED_MODE)

So it can operate only 4 timers and 8 channels.
This PR supports only the ESP32 generic target.

#7817 supports up to 8 timers and 16 channels ESP32, ESP32_S2, ESP32_C2 chips.

dpgeorge · 2021-09-19T13:22:18Z

Ok, thanks for the info! I'll close this then in favour of #7817. Thanks @p1ng0o for the initial work here.

This commit allows using all the available PWM timers (up to 8) and channels (up to 16), without affecting the PWM API. If a new frequency is set, first it checks if another timer is using the same frequency. If yes, then it uses this timer, otherwise, it creates a new one. If all timers are used, the user should set an already used frequency, or de-init a channel. This work is based on #6276 and #3608.

p1ng0o force-pushed the esp32port_pwm_multi_timers branch from d1e7ce0 to 23ca3d4 Compare July 22, 2020 22:14

p1ng0o force-pushed the esp32port_pwm_multi_timers branch from 23ca3d4 to edfbe35 Compare August 26, 2020 11:58

dpgeorge added the port-esp32 label Aug 26, 2020

dpgeorge reviewed Aug 26, 2020

View reviewed changes

p1ng0o force-pushed the esp32port_pwm_multi_timers branch from edfbe35 to 7bf1bc8 Compare August 26, 2020 14:45

p1ng0o force-pushed the esp32port_pwm_multi_timers branch 2 times, most recently from 3237d03 to 3913bd6 Compare August 27, 2020 17:10

dpgeorge reviewed Aug 28, 2020

View reviewed changes

p1ng0o force-pushed the esp32port_pwm_multi_timers branch from 6e87272 to 62b53fa Compare August 30, 2020 07:58

p1ng0o force-pushed the esp32port_pwm_multi_timers branch from 62b53fa to f629bc2 Compare October 18, 2020 09:05

p1ng0o force-pushed the esp32port_pwm_multi_timers branch from f629bc2 to 4bf8c33 Compare November 9, 2020 20:50

IhorNehrutsa mentioned this pull request Nov 27, 2020

esp32/machine_pwm.c: Handle multiple timers: Bugfix PWM.deinit() critical error and fix pwm_print() #6654

Closed

IhorNehrutsa reviewed Dec 9, 2020

View reviewed changes

ports/esp32/machine_pwm.c Show resolved Hide resolved

p1ng0o force-pushed the esp32port_pwm_multi_timers branch 2 times, most recently from a7024c0 to 91a0744 Compare December 9, 2020 11:17