machine: Add nchanges param to time_pulse_us func.#16160
Conversation
|
Code size report: |
7a9b45d to
f59833b
Compare
|
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.. |
f59833b to
e29b760
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #16160 +/- ##
==========================================
- Coverage 98.46% 98.45% -0.01%
==========================================
Files 176 176
Lines 22811 22813 +2
==========================================
+ Hits 22460 22461 +1
- Misses 351 352 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
f25ae18 to
b9d6cf0
Compare
b9d6cf0 to
aa1cba8
Compare
90f267b to
28906bc
Compare
dd7b953 to
b6bcf72
Compare
|
Remove additionnal synchronization time_pulse_us() from tests/extmod_hardware/machine_pwm.py |
b6bcf72 to
e2a1c3a
Compare
e2a1c3a to
27486f2
Compare
|
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 |
It's not (yet) visible in PR 17346. |
|
Now that #17346 is merged, this PR can be updated on that. |
27486f2 to
694ddcd
Compare
Signed-off-by: IhorNehrutsa <Ihor.Nehrutsa@gmail.com> Co-Authored-By: Robert Hammelrath <12476868+robert-hh@users.noreply.github.com>
1cdca1b to
c3b0cfc
Compare
| The function returns -3 if there was timeout waiting for condition marked (***) above. | ||
| The function will return -2 if there was timeout waiting for condition marked | ||
| (*) above, and -1 if there was timeout during the main measurement, marked (**) | ||
| (**) above, and -1 if there was timeout during the main measurement, marked (*) |
There was a problem hiding this comment.
It's quite tricky to document the nchanges parameter.
I think the original description should remain unchanged. And then add a paragraph at the end stating something like "The above behaviour is for when nchanges=2. That behaviour can be generalised by passing in a larger value for nchanges which is the number of times the function waits for the pin to change. On error the return value can be down to -nchanges and indicates how many edges were missed." Or something like that.
| nchanges = mp_obj_get_int(args[3]); | ||
| } | ||
| mp_uint_t us = machine_time_pulse_us(pin, level, timeout_us, nchanges); | ||
| // May return -1 or -2 or -3 in case of timeout |
There was a problem hiding this comment.
This comment needs adjusting because the return value can be between -nchanges and -1 (on timeout).
| time_pulse_us(pulse_in, level, timeout) | ||
| for _ in range(n_averaging): | ||
| t += time_pulse_us(pulse_in, level, timeout) | ||
| t += time_pulse_us(pulse_in, level, timeout, 3) |
There was a problem hiding this comment.
I think this code should stay as it was, because inside this loop you don't need to wait for an extra change.
time_pulse_us(pin, pulse_level, timeout_us=1000000, nchanges=2, /)
If
nchangesis 3, if the pin is initially equal to pulse_level then firstwaits 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 #16147 (comment)
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.