Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.57%. Comparing base (94343e2) to head (bdda91f).
Report is 1 commits behind head on master.

@@           Coverage Diff           @@
##           master   #16147   +/-   ##
=======================================
  Coverage   98.57%   98.57%           
=======================================
  Files         164      164           
  Lines       21349    21349           
=======================================
  Hits        21045    21045           
  Misses        304      304           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dpgeorge What is the problem with MIMXRT and constant high setting. At least PWM is creating a constant high signal?
For SAM21 i understand, that the device is too slow to time higher frequencies.

Edit: I see that the test for 100% calculates the duty_u16 as 100 * 65535 // 100, resulting in 65535. That will indeed still create a PWM pulse, not only at the MIMXRT devices. duty_u16(65536) is constant high.

time_pulse_us() is not good at timing repeating pulses. If the level at start is identical to the requested level, then it will return the remaining duration of that pulse. Changing the code to wait for the opposite level first improves it.

MP_WEAK mp_uint_t machine_time_pulse_us(mp_hal_pin_obj_t pin, int pulse_level, mp_uint_t timeout_us) {
    mp_uint_t start = mp_hal_ticks_us();
    while (mp_hal_pin_read(pin) == pulse_level) {
        if ((mp_uint_t)(mp_hal_ticks_us() - start) >= timeout_us) {
            return (mp_uint_t)-1;
        }
    }
    while (mp_hal_pin_read(pin) != pulse_level) {
        if ((mp_uint_t)(mp_hal_ticks_us() - start) >= timeout_us) {
            return (mp_uint_t)-2;
        }
    }
    start = mp_hal_ticks_us();
    while (mp_hal_pin_read(pin) == pulse_level) {
        if ((mp_uint_t)(mp_hal_ticks_us() - start) >= timeout_us) {
            return (mp_uint_t)-1;
        }
    }
    return mp_hal_ticks_us() - start;
}

time_pulse_us() is not good at timing repeating pulses. If the level at start is identical to the requested level, then it will return the remaining duration of that pulse. Changing the code to wait for the opposite level first improves it.

I don't think we should change time_pulse_us(). It works well and as specified in the docs.

The test here uses a dummy call to time_pulse_us() at to synchronise with the PWM pulses. And then times each low pulse 10 times in a row. Then it times 10 high pulses in a row. That should be accurate.

I was not suggesting changing time_pulse_us(). And the test script seems to implement that behavior.

Would it makes sense to extract the pins configuration to a seperate file/input?

I dont want to over-engineer, but having a common, perhaps shared, config will make running these HIL against different MCU more efficient

Would it makes sense to extract the pins configuration to a seperate file/input?

That would be nice. But I think it's quite valuable to have a self contained test that's easy to run, and also all the requirements/settings for the test are all within that test. Eg even if the pin config was in a special board_test_setup.py file, the tests still need to tune various timing margins for different ports, so still need logic at the top of the test to do this (and may as well then set the pins).

Well, maybe we can improve this in the future. For now I want to encode the behaviour of hardware peripherals in the tests to make it easier to test ports and get them all behaving the same.

I dont want to over-engineer, but having a common, perhaps shared, config will make running these HIL against different MCU more efficient

FWIW I'd also like to see this eventually too (split out the hardware config and the test). I don't think it has to be a complex architecture, but I'd appreciate a way to add a new test case without either appending to an existing file or copy-pasting the config into a new file.

However, as damien says, the test environment is currently simple because very few tests rely on additional files. It's a non-trivial amount of work to change this.

ESP32, ESP32C3 passed this test in #10854.

Please see some of the improvements of tests/extmod_hardware/machine_pwm.py in #16161

@dpgeorge and all interested persons.

I want to express my opinion about the range of duty PWM.
I believe that the range 0-65356 is more promising.

First, many ports already have a range of 0-65356.
I'm not sure: mimxrt, same, esp8266, nrf, esp32

Second, ports that do not have the physical ability to accept the value 65356 can easily replace it with 65355 internally.

#define MAX_DUTY_UI16 65356
void set_duty_u16(machine_pwm_obj_t *self, int duty) {
    if ((duty < 0) || (duty > MAX_DUTY_UI16)) {
        mp_raise_msg_varg(&mp_type_ValueError, MP_ERROR_TEXT("duty_u16 must be from 0 to %d"), MAX_DUTY_UI16);
    }
    if (duty > 65355)) {
        duty = 65355;
    }
    ...
}

At the same time, they will return the actual achieved duty value 65355.

    pwm.duty_u16(65356)
    pwm.duty_u16()
    >>>65355

This is not a problem because

Third, at high frequencies the resolution decreases and most duty values ​​cannot be achieved. So, they are replaced by the closest possible ones. For example, when freq=20MHz the resolution decreases to 5 available duty value of all range 0-65356.
Available duty value are 0%, 25%, 50%, 75%, 100%, that's all.
All other duty values are converted to these values.
This tested on ESP32 board:

pwm = PWM(2, freq=20_000_000)
DUTY_MAX = 65536
DUTY_LEVELS = 8
for duty in range(0, DUTY_LEVELS + 1):
    duty_u16 = DUTY_MAX * duty // DUTY_LEVELS
    pwm.duty_u16(duty_u16)
    print(pwm)

output is

>>>PWM(Pin(2), freq=20000000, duty_u16=0)     # duty_u16 = 0     =   0.00% Ok!
                                              #                               not achieved
>>>PWM(Pin(2), freq=20000000, duty_u16=0)     # duty_u16 = 8192  =  12.50%    not achieved
                                              #                               not achieved
>>>PWM(Pin(2), freq=20000000, duty_u16=16384) # duty_u16 = 16384 =  25.00% Ok!
                                              #                               not achieved
>>>PWM(Pin(2), freq=20000000, duty_u16=16384) # duty_u16 = 24576 =  37.50%    not achieved
                                              #                               not achieved
>>>PWM(Pin(2), freq=20000000, duty_u16=32768) # duty_u16 = 32768 =  50.00% Ok!
                                              #                               not achieved
>>>PWM(Pin(2), freq=20000000, duty_u16=32768) #  duty_u16 = 40960 =  62.50%   not achieved
                                              #                               not achieved
>>>PWM(Pin(2), freq=20000000, duty_u16=49152) #  duty_u16 = 49152 =  75.00% Ok!
                                              #                               not achieved
>>>PWM(Pin(2), freq=20000000, duty_u16=49152) #  duty_u16 = 57344 =  87.50%   not achieved
                                              #                               not achieved
>>>PWM(Pin(2), freq=20000000, duty_u16=65536) #  duty_u16 = 65536 = 100.00% Ok!

So replacing 65356 with 65355 in the second paragraph works the same like here.

Fourthly, 65356 is a power of two and is more beautiful than 65355 purely aesthetically. 😄

Sorry for the many words.
Thanks.

@IhorNehrutsa the method is named duty_u16() which means it takes a 16-bit unsigned integer as the argument. So that means the duty must be in the range 0-65535.

For example you may want to store duty values in 2 bytes (eg in array.array("H")), and in that case you can't store 65536. So the maximum needs to be 65535.

Once #16216 is merged, this test will be refactored to use unittest. That will allow it to run and pass on esp8266, and also have more diagnostics output.

Teensy pins to be updated to test 3 output types, per #16153 (comment) :

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

This has been refactored to use unittest, and it's a lot cleaner and can now skip higher frequencies on esp8266.

I also added support for more than one set of pins, and when running on a Teensy board it uses the three sets suggested by @robert-hh .

Now that it's using unittest it could print out the pins being used? Perhaps just on failure?

Updated to skip higher frequencies on SAMD21 (which is too slow to measure the short duty duration). Retested on all supported ports.

Now that it's using unittest it could print out the pins being used?

I have made a test class for each set of supported pins, which is automatically generated based on the list of pins provided by the port. Eg on rp2 the test class is called Test_GPIO0_GPIO1 (and the methods are test_freq_50 etc).

And at the start of the test it now prints out the pins that are configured for that run, eg:

set up pins: GPIO0 GPIO1
test_freq_50 (__main__.TestBase) ...
freq=50    duty_u16=0     : freq=50 freq_er=0 duty=0 duty_er=0 :
freq=50    duty_u16=6553  : freq=50 freq_er=0 duty=6553 duty_er=0 : level=0 timing_er=1 level=1 timing_er=0
...