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

robert-hh · 2024-11-04T15:45:50Z

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.

Tested with ESP8266, MIMXRT, NRF and SAMD, which are affected by this change. Other ports did not require a change.

Partially reverting PR #10850.

github-actions · 2024-11-04T15:56:04Z

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% standard
      stm32:    +0 +0.000% PYBV10
     mimxrt:  +168 +0.046% TEENSY40
        rp2:    +0 +0.000% RPI_PICO_W
       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS
  qemu rv32:    +0 +0.000% VIRT_RV32

dpgeorge · 2024-11-04T22:59:54Z

ports/mimxrt/hal/pwm_backport.c

@@ -36,15 +36,15 @@ void PWM_UpdatePwmDutycycle_u16(

    // Setup the PWM dutycycle of channel A or B
    if (pwmSignal == kPWM_PwmA) {
-        if (dutyCycle >= 65536) {
+        if (dutyCycle >= PWM_FULL_SCALE) {


This constant is still defined as 65536 in pwm_backport.h. Does it need changing there?

Updated. Behavior still fine. At duty_u16(32768) the difference between low and high phases with a Teensy 4.x are 700ps (flexpwm) and 400ps (QTMR).

Can the hardware have even better symmetry with 50% duty cycle, if you choose the right upper limit for the counter? If so, it may be possible to get this working with machine.PWM by using 65536 for the hardware full scale, but 65535 for the duty_u16 max, and then scaling it via x * 65536 / 65535. I think this is how the rp2 port works.

The difference between the high/low phase is 10% of the PWM clock or about 50% of the CPU clock. It hardly can get much better.

Right... picoseconds are very small! Great work testing it to that degree.

projectgus

This looks good to me, thanks @robert-hh

mattytrentini · 2024-11-05T07:58:35Z

Thanks for taking care of all of these important details folks!

robert-hh · 2024-11-05T20:55:19Z

Running the PWM test on this updated MIMXRT/Teensy 4.x, I made a few "observations". The MIMXRT port has three different mechanisms for PWM, which I call : FLEXPWM_AB, FLEXPWM_X and QTMR. All work fine with usual PWM signal, but with duty_u16(0) and duty_u16(65535) there are some problems.

QTMR: No problem.

FLEXPWM_X: duty_u16(0) is implemented by switching the output off. Otherwise there are still pulses at the output. If the pulse_pin for the test was previously switched to IN mode, it takes very long for the input to drift from High to Low. Edit: fixed

FLEX_PWM_AB: There are problems of switching from duty_u16(65535) to duty_u16(0). It takes longer than a full frequency period. If there is a different duty value enabled between duty_u16(65535) and duty_u16(0), then the transition is fast. I do not know yet what the reason is. Simple test programs seem to work. In any case switching the PWM settings require a full previous period to expire. Edit: fixed

The pins used by the machine_pwm.py test are D0 and D1. D0 has a FLEXPWM_X type PWM. Enabling PULL_DOWN on pulse_pin is sufficient to make the transition fast and pass the test. But I'll check if the output can be set to Pin mode low instead of switching it off.

robert-hh · 2024-11-06T09:01:24Z

A suitable pin pair on Teensy 4.x wich uses QTMR and has a UART is D14/D15 and UART3.
The PWM test passes with FLEXPWM_AB pins if duty and freq are set in a single call using pwm.init() and not separate. That looks at first suprising, since all methods of setting PWM parameters use the same function for actually configuring the hardware and the sequence should not matter. I tested that initially, but not with the sequence duty_u16(65535) -> freq(x) -> duty_u16(0). That's the one which make trouble.

Edit: The reason for issue with the FLEX_PWM_AB channels is double loading of PWM parameters within one PWM cycle. The second load is ignored. Fixed by clearing a pending load before loading new paramerts.

robert-hh · 2024-11-07T09:22:58Z

The issues with the FLEXPM channels and duty_u16(0) are fixed. The changes are pushed. Tested with MIMXRT 1011, -1015, -1020, -1052, -1062 (Teensy 4.x) and -1176.

dpgeorge · 2024-11-07T09:59:43Z

Excellent work @robert-hh ! We should probably make more of these hardware tests, they are really teasing out the bugs.

The MIMXRT port has three different mechanisms for PWM

I think we should make the PWM test test all three of these mechanisms (for mimxrt boards). Can you suggest three pairs of pins that can be used for this? Maybe one pair can also have MISO/MOSI to test loopback SPI.

robert-hh · 2024-11-07T10:44:54Z

Can you suggest three pairs of pins that can be used for this? Maybe one pair can also have MISO/MOSI to test loopback SPI.

Sure. With Teensy 4.x:
Pin D0/D1: FLEXPMW X and UART 1
Pin D2/D3: FLEXPWM A/B
Pin D11/D12: QTMR and MOSI/MISO of SPI 0

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>

Changes in this commit: - When setting PWM parameters of a FLEXPWM AB channel twice within a PWM cycle, the second setting was ignored. Now the second setting persists. - With `duty_u16(0)` a FLEXPWM X channel was set to high impedance. Now it is set to low impedance with value 0 for `invert=False`, and 1 for `invert=True`. - The align parameter requires a duty rate and frequency to be set. Align will now be ignored if freq or duty are missing. Signed-off-by: robert-hh <robert@hammelrath.com>

dpgeorge · 2024-11-18T05:27:48Z

Merged. Thanks again @robert-hh for your efforts here.

robert-hh · 2024-11-18T07:50:44Z

Thank you for setting up the test, which proves to be very useful.

robert-hh mentioned this pull request Nov 4, 2024

tests/extmod_hardware: Add a test for machine.PWM freq and duty. #16147

Merged

dpgeorge reviewed Nov 4, 2024

View reviewed changes

dpgeorge added port-esp8266 port-nrf port-samd port-mimxrt labels Nov 4, 2024

projectgus mentioned this pull request Nov 4, 2024

esp32/PWM: Reduce inconsitencies between ports. #10854

Open

projectgus reviewed Nov 4, 2024

View reviewed changes

robert-hh force-pushed the ports_pwm_range branch from 099562a to b48cc26 Compare November 5, 2024 07:42

robert-hh force-pushed the ports_pwm_range branch 2 times, most recently from c92dbd1 to 1e26fda Compare November 7, 2024 08:18

robert-hh added 2 commits November 18, 2024 16:05

dpgeorge force-pushed the ports_pwm_range branch from 1e26fda to e2532e0 Compare November 18, 2024 05:13

dpgeorge approved these changes Nov 18, 2024

View reviewed changes

dpgeorge merged commit e2532e0 into micropython:master Nov 18, 2024
12 checks passed

robert-hh deleted the ports_pwm_range branch November 19, 2024 07:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

robert-hh commented Nov 4, 2024

github-actions bot commented Nov 4, 2024 •

edited

Loading

dpgeorge Nov 4, 2024

robert-hh Nov 5, 2024

robert-hh Nov 5, 2024

dpgeorge Nov 5, 2024

robert-hh Nov 5, 2024

dpgeorge Nov 5, 2024

projectgus left a comment

mattytrentini commented Nov 5, 2024

robert-hh commented Nov 5, 2024 •

edited

Loading

robert-hh commented Nov 6, 2024 •

edited

Loading

robert-hh commented Nov 7, 2024

dpgeorge commented Nov 7, 2024

robert-hh commented Nov 7, 2024 •

edited

Loading

dpgeorge commented Nov 18, 2024

robert-hh commented Nov 18, 2024

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

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

Conversation

robert-hh commented Nov 4, 2024

github-actions bot commented Nov 4, 2024 • edited Loading

dpgeorge Nov 4, 2024

Choose a reason for hiding this comment

robert-hh Nov 5, 2024

Choose a reason for hiding this comment

robert-hh Nov 5, 2024

Choose a reason for hiding this comment

dpgeorge Nov 5, 2024

Choose a reason for hiding this comment

robert-hh Nov 5, 2024

Choose a reason for hiding this comment

dpgeorge Nov 5, 2024

Choose a reason for hiding this comment

projectgus left a comment

Choose a reason for hiding this comment

mattytrentini commented Nov 5, 2024

robert-hh commented Nov 5, 2024 • edited Loading

robert-hh commented Nov 6, 2024 • edited Loading

robert-hh commented Nov 7, 2024

dpgeorge commented Nov 7, 2024

robert-hh commented Nov 7, 2024 • edited Loading

dpgeorge commented Nov 18, 2024

robert-hh commented Nov 18, 2024

github-actions bot commented Nov 4, 2024 •

edited

Loading

robert-hh commented Nov 5, 2024 •

edited

Loading

robert-hh commented Nov 6, 2024 •

edited

Loading

robert-hh commented Nov 7, 2024 •

edited

Loading