ports/PWM: Reduce inconsistencies between the ports. #10850

robert-hh · 2023-02-24T20:11:08Z

This is the first set of commits, consisting of:

mimxrt: Start PWM only if freq and duty are set. And straighten the duty handling.
samd: Add the init() method to PWM. This is merely restructuring existing code, at some costs for flash size and RAM usage. init() without argument restarts a PWM channel which had been stopped with deinit().
rp2: Enable keyword arguments with the PWM constructor and add the init() method. init() restarts a PWM channel which had been stopped with deinit(). Added code to stop PWM on soft reset.
Adapt the quickref examples to set freq and duty_u16 using keyword arguments in the constructor.
ESP8266: Add the methods duty_u16() and duty_ns().
rp2: Edd a invert=True|False keyword argument to init() and the constructor to invert channel. In order to use not more memory, some flags of the object are put into a bit field.

So Rp2, ESP8266, MIMXRT and SAMD support now keyword arguments with the constructor and an init() method with optional argumnents. All four do no start PWM until freq and duty are set. The behavior at deinit() is different. The state of the pins during set-up and after deinit() varies. For ESP32 there is a PR enforcing freq and duty settings with the constructor or init().

The STM32 and Renesas-RA ports do not have a machine.PWM class, but it could be added. The nrf port's PWM modules behaves completely different and requires a full rework to get compatible.

robert-hh · 2023-03-04T21:17:52Z

I could not stand to leave the nrf port aside and so I started to rewrite the machine.PWM implementation. hard PWM looks fine so far, soft PWM has to be adapted, and more testing is due. But I'll make a separate PR for it.

robert-hh · 2023-03-13T21:14:37Z

This PR is good for now as a first step of reducing the difference between ports. I have prepared another PR for adding the methods start() and stop() to PWM, but the semantics must be still discussed.
The ESP32 change is still going on in a separate PR.

pdg137 · 2023-04-28T19:59:46Z

requesting freq and duty rate to be set with the constructor results is the most consistent option, but is a breaking change.

Requiring frequency to be set the way you are doing it now is also a breaking change. For example it breaks the first part of the first example in the PWM docs:

pwm = PWM(pin)          # create a PWM object on a pin
pwm.duty_u16(32768)     # set duty to 50%

Also, did I understand correctly that you're planning to require freq/duty in the constructor for the ESP32 port? If so, shouldn't rp2 also require those arguments for consistency? At least it's not likely to be a silent/intermittent error.

I still to not understand your concern about the duty rate of 65535.

Is this in reference to my other PR #11336? It seems like it's going to get addressed one way or another here so I'm not concerned anymore.

robert-hh · 2023-04-28T20:22:28Z

Requiring frequency to be set the way you are doing it now is also a breaking change.

I consider the actual behavior of the RP2 port a bug, in starting at an arbitrary frequency (1907.35 Hz) if only the duty rate is set. PWM must only start if both freq and duty rate are set.

Also, did I understand correctly that you're planning to require freq/duty in the constructor for the ESP32 port?

No. I do not plan that. That was just an option which you suggested. It could be required, and one aim of this PR is to allow for all ports setting both frequency and duty rate with the constructor. The actual ESP32 port (and the PR for the update) behaves different, in that it starts PWM with 5KHz/50% duty if the constructor is called with just the Pin argument. That is different to all other ports and should be changed.

DavidEGrayson · 2023-04-28T22:18:19Z

ports/rp2/machine_pwm.c

@@ -87,16 +139,40 @@ STATIC mp_obj_t mp_machine_pwm_make_new(const mp_obj_type_t *type, size_t n_args
    uint slice = pwm_gpio_to_slice_num(gpio);
    uint8_t channel = pwm_gpio_to_channel(gpio);
    machine_pwm_obj_t *self = &machine_pwm_obj[slice * 2 + channel];
+    self->invert = 0;
    self->duty_type = DUTY_NOT_SET;

    // Select PWM function for given GPIO.
    gpio_set_function(gpio, GPIO_FUNC_PWM);


This line that calls gpio_set_function should be removed since it causes glitches like I was describing, and you already copied it to the end of the function.

This was fixed.

And also fix/improve the following: - Simplify the duty handling a little bit. - Allow duty_u16(65536), which sets the output high. - Rename machine_pwm_start() to mp_machine_pwm_start(), in preparation for a possible start/stop method pair.

The PWM.init() method has been added. Calling init() without arguments restarts a PWM channel stopped with deinit(). Otherwise single parameters except for "device=n" can be changed again. The device can only be specified once, either in the constructor or the first init() call. Also simplify get_pwm_config() and get_adc_config(), and shrink the PWM object.

This adds support for freq/duty_u16/duty_ns keyword arguments in the PWM constructor, and adds the PWM.init() method. Using init() without arguments enables a previously deinit-ed PWM again. Further changes in this commit: - Do not start PWM output if only duty was set. - Stop all PWM slices on soft-reset. - Fix a bug when changing the freq on a channel pair with duty_ns set.

This adds the freq and duty_u16 keyword settings to the constructor, and sometimes other details in the PWM section. For mimxrt a clarification regarding the PWM invert argument was added, and for rp2 a few words were spent on PWM output pairs of a channel/slice.

Also adds these keyword arguments to the constructor and init method.

Using the invert=True|False keyword option with the constructor or init().

Changes in this commit: - Limit duty_u16() to 65535 and duty_ns() to the period duration. - Return 0 for pwm.freq() if the frequency has not been set yet. - Return 0 for pwm.duty_us16() and duty_ns() unless both frequency and duty cycle have been set. - Initialize the pin to PWM at the very end of the constructor, to avoid possible glitches on the pin when setting up the PWM.

All ports that enable MICROPY_PY_MACHINE_PWM now enable these two sub-options, so remove these sub-options altogether to force consistency in new ports that implement machine.PWM. Signed-off-by: Damien George <damien@micropython.org>

This keeps up with the changed Pin naming scheme.

dpgeorge · 2023-05-04T03:23:40Z

This is looking good, a good improvement to over consistency between ports PWM behavoiur. Thank you very much @robert-hh !

I have cleaned up the commits a bit (splitting out the non-PWM docs changes to a separate commit), and added a commit that removes MICROPY_PY_MACHINE_PWM_INIT and MICROPY_PY_MACHINE_PWM_DUTY_U16_NS because they are now always enabled by all ports.

I also removed the commented-out code in rp2/machine_pwm.c:

--- a/ports/rp2/machine_pwm.c
+++ b/ports/rp2/machine_pwm.c
@@ -158,15 +158,7 @@ void machine_pwm_deinit_all(void) {
     for (int i = 0; i < 8; i++) {
         slice_freq_set[i] = false;
         pwm_set_enabled(machine_pwm_obj[i].slice, false);
-        // pwm_set_chan_level(i, 0, 0);
-        // pwm_set_chan_level(i, 1, 0);
     }
-    // Clean out the table
-    // for (int i = 0; i < 16; i++) {
-    //     machine_pwm_obj[i].invert = 0;
-    //     machine_pwm_obj[i].duty = 0;
-    //     machine_pwm_obj[i].duty_type = DUTY_NOT_SET;
-    // }
 }
 
 STATIC void mp_machine_pwm_deinit(machine_pwm_obj_t *self) {

github-actions · 2023-05-04T03:28:35Z

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% standard
      stm32:    +0 +0.000% PYBV10
        rp2:  +512 +0.157% PICO[incl +8(bss)]

codecov-commenter · 2023-05-04T03:43:11Z

Codecov Report

Merging #10850 (786013d) into master (0264465) will not change coverage.
The diff coverage is n/a.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@           Coverage Diff           @@
##           master   #10850   +/-   ##
=======================================
  Coverage   98.39%   98.39%           
=======================================
  Files         156      156           
  Lines       20603    20603           
=======================================
  Hits        20272    20272           
  Misses        331      331

robert-hh · 2023-05-04T06:06:54Z

Thank you for merging. It's now only the ESP32 port which behaves a little bit different in starting with a default frequency/duty when these are not set at instantiation. The author insists on the way it behaves.

IhorNehrutsa · 2023-05-04T09:42:38Z

@robert-hh @dpgeorge ESP32 port will be changed in #10854

This is required due to a recent change to MicroPython: micropython/micropython#10850 Without this, the PWM output doesn't actually get initialized.

Tested that output is high at duty_u16(65565) and low at duty_u16(0). Also verified that at duty_u16(32768) the high and low pulse have the same length. Partially reverting PR micropython#10850. Signed-off-by: robert-hh <robert@hammelrath.com>

The following ports used 65536 as the upper value (100% duty cycle) and are changed in this commit to use 65535: esp8266, mimxrt, nrf, samd. Tested that output is high at `duty_u16(65535)` and low at `duty_u16(0)`. Also verified that at `duty_u16(32768)` the high and low pulse have the same length. Partially reverts micropython#10850, commits 9c7ad68 and 2ac643c. Signed-off-by: robert-hh <robert@hammelrath.com>

robert-hh marked this pull request as draft February 24, 2023 20:11

robert-hh force-pushed the ports_pwm branch 3 times, most recently from 53591e0 to b8111bf Compare February 26, 2023 15:43

robert-hh marked this pull request as ready for review February 26, 2023 16:36

robert-hh force-pushed the ports_pwm branch 4 times, most recently from 6a346eb to e46e866 Compare February 27, 2023 18:46

robert-hh mentioned this pull request Feb 27, 2023

PWM: Reduce inconsitencies between ports. #10817

Closed

robert-hh force-pushed the ports_pwm branch 8 times, most recently from b16d70a to 9eea19b Compare March 3, 2023 10:49

robert-hh marked this pull request as draft March 12, 2023 20:35

robert-hh added ports Relates to multiple ports, or a new/proposed port docs labels Mar 12, 2023

robert-hh force-pushed the ports_pwm branch 3 times, most recently from dfff382 to d901148 Compare March 13, 2023 20:56

robert-hh marked this pull request as ready for review March 13, 2023 21:14

robert-hh force-pushed the ports_pwm branch 2 times, most recently from b38132b to ca5f2dd Compare March 15, 2023 07:56

robert-hh mentioned this pull request Mar 23, 2023

samd: A set of small changes and bug fixes. #11106

Merged

DavidEGrayson reviewed Apr 28, 2023

View reviewed changes

robert-hh force-pushed the ports_pwm branch from 37ef0c1 to c76aab0 Compare April 29, 2023 06:47

robert-hh and others added 9 commits May 4, 2023 13:09

esp8266/machine_pwm: Implement duty_u16() and duty_ns() for consistency.

2ac643c

Also adds these keyword arguments to the constructor and init method.

rp2/machine_pwm: Add support for inverting a PWM channel output.

0b3b508

Using the invert=True|False keyword option with the constructor or init().

docs/samd: Make use of pin names more consistent in examples.

786013d

This keeps up with the changed Pin naming scheme.

dpgeorge force-pushed the ports_pwm branch from c76aab0 to 786013d Compare May 4, 2023 03:21

dpgeorge merged commit 786013d into micropython:master May 4, 2023

robert-hh deleted the ports_pwm branch June 2, 2023 08:52

ricksorensen mentioned this pull request Feb 5, 2024

PWM behavior has changed #13600

Closed

robert-hh mentioned this pull request Nov 4, 2024

ports/machine_pwm: Set the PWM duty_u16() back to 65535. #16153

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ports/PWM: Reduce inconsistencies between the ports. #10850

ports/PWM: Reduce inconsistencies between the ports. #10850

robert-hh commented Feb 24, 2023 •

edited

Loading

robert-hh commented Mar 4, 2023

robert-hh commented Mar 13, 2023

pdg137 commented Apr 28, 2023

robert-hh commented Apr 28, 2023

DavidEGrayson Apr 28, 2023 •

edited

Loading

dpgeorge May 4, 2023

dpgeorge commented May 4, 2023

github-actions bot commented May 4, 2023

codecov-commenter commented May 4, 2023

robert-hh commented May 4, 2023

IhorNehrutsa commented May 4, 2023

ports/PWM: Reduce inconsistencies between the ports. #10850

ports/PWM: Reduce inconsistencies between the ports. #10850

Conversation

robert-hh commented Feb 24, 2023 • edited Loading

robert-hh commented Mar 4, 2023

robert-hh commented Mar 13, 2023

pdg137 commented Apr 28, 2023

robert-hh commented Apr 28, 2023

DavidEGrayson Apr 28, 2023 • edited Loading

Choose a reason for hiding this comment

dpgeorge May 4, 2023

Choose a reason for hiding this comment

dpgeorge commented May 4, 2023

github-actions bot commented May 4, 2023

codecov-commenter commented May 4, 2023

Codecov Report

robert-hh commented May 4, 2023

IhorNehrutsa commented May 4, 2023

robert-hh commented Feb 24, 2023 •

edited

Loading

DavidEGrayson Apr 28, 2023 •

edited

Loading