micropython icon indicating copy to clipboard operation
micropython copied to clipboard

machine: Add wait_opposite param to time_pulse_us func.

Open IhorNehrutsa opened this issue 1 year ago • 7 comments

time_pulse_us(pin, pulse_level, timeout_us=1000000, wait_opposite=false, /)

If wait_opposite is true, if the pin is initially equal to pulse_level then first waits until the pin input becomes different from pulse_level. Then if the current input value of the pin is different to pulse_level, the function first waits until the pin input becomes equal to pulse_level, then times the duration that the pin is equal to pulse_level.

The advantage is that there is no need for additional synchronization before measuring the pulse duration. A little bit longer, but with higher accuracy.

The jitter of the pulse measurement process is about 5-15 μs on the ESP32 board.

Idia from @robert-hh https://github.com/micropython/micropython/pull/16147#issuecomment-2454287302

This function is used in DRAFT PR for tests PWM from 10845 and time_hardware_pulse_us() and test from 16147. #16161 The tests results show the functionality of the function over the entire ESP32 PWM frequency range from 1Hz to 40MHz.

IhorNehrutsa avatar Nov 05 '24 15:11 IhorNehrutsa

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:  +104 +0.012% standard
      stm32:   +68 +0.017% PYBV10
     mimxrt:   +72 +0.020% TEENSY40
        rp2:   +56 +0.006% RPI_PICO_W
       samd:   +68 +0.025% ADAFRUIT_ITSYBITSY_M4_EXPRESS
  qemu rv32:    +0 +0.000% VIRT_RV32

github-actions[bot] avatar Nov 05 '24 15:11 github-actions[bot]

Thanks for mentioning me. I considered creating that feature, but as optional argument to time_pulse_us() instead of a new method. That looks like a smaller change, even if the additional code size might be the same..

robert-hh avatar Nov 05 '24 15:11 robert-hh

Codecov Report

Attention: Patch coverage is 71.42857% with 2 lines in your changes missing coverage. Please review.

Project coverage is 98.53%. Comparing base (2264340) to head (27486f2). Report is 317 commits behind head on master.

Files with missing lines Patch % Lines
extmod/machine_pulse.c 71.42% 2 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #16160      +/-   ##
==========================================
- Coverage   98.54%   98.53%   -0.01%     
==========================================
  Files         169      169              
  Lines       21877    21882       +5     
==========================================
+ Hits        21558    21561       +3     
- Misses        319      321       +2     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codecov[bot] avatar Nov 05 '24 22:11 codecov[bot]

Remove additionnal synchronization time_pulse_us() from tests/extmod_hardware/machine_pwm.py

IhorNehrutsa avatar Dec 19 '24 13:12 IhorNehrutsa

After discussing with @projectgus , we feel that this is a useful enhancement but maybe not worth the cost in code size.

I think it would be possible to make this PR a lot simpler and less code size. I opened #17346 as a basis for that.

If #17346 is merged, then the additional argument added here could be generalised to a counter, eg:

machine.time_pulse_us(pin, pulse_level, timeout_us=1000000, num_levels=2)

With num_levels=2 it's the same behaviour as before. With num_levels=3 it waits for 3 level changes and that implements the behaviour in this PR. Higher values are possible, which allow more synchronisation.

dpgeorge avatar May 23 '25 03:05 dpgeorge

then the additional argument added here

It's not (yet) visible in PR 17346.

robert-hh avatar May 23 '25 10:05 robert-hh

Now that #17346 is merged, this PR can be updated on that.

dpgeorge avatar Jun 13 '25 06:06 dpgeorge