-
-
Notifications
You must be signed in to change notification settings - Fork 8.4k
tests/machine_encoder.py: Fix esp32 Encoder: only 3 pulses to count first rotation. #17914
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
tests/machine_encoder.py: Fix esp32 Encoder: only 3 pulses to count first rotation. #17914
Conversation
d153043
to
80ddee0
Compare
Add pin initial level. Add test Encoder phases x2 and x4. Signed-off-by: Ihor Nehrutsa <Ihor.Nehrutsa@gmail.com>
80ddee0
to
cf658f0
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #17914 +/- ##
=======================================
Coverage 98.38% 98.38%
=======================================
Files 171 171
Lines 22295 22295
=======================================
Hits 21936 21936
Misses 359 359 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @IhorNehrutsa for the explanation and the updated test. This looks good to me.
@robert-hh do you have any concerns about these changes?
@@ -13,12 +13,22 @@ | |||
import sys | |||
from machine import Pin | |||
|
|||
PRINT = False | |||
PIN_INIT_VALUE = 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test fails if this is set to any other value, yes? If so, I think it's OK to hard-code it at line 46
I have, indeed. It works with ESP32. It fails with MIMXRT. The previous test version worked. The reason is about what should be the result of the encoder value with different settings of phases. Looking at a few test lines:
The tests sets up three instances with phases 1, 2, and 4. After one pulse, the test expects all three counter to advance by one. So the expected position is (1,1,1). The way the phases emulation is implemented in MIMXRT, the result is (0,0,1). MIMXRT counts in the positive range at phases=2 at the last one of every 2 transitions, and likewise for phases=1 in the last of every 4 transitions.ESP32 seems to advance the counter at the first of every 2 resp 4 transition block. The phases emulation can be changed, that's not a problem. but what it the proper behavior. Counting before an event is finished like the ESP32 does seems wrong to me. But it may be the behavior defined in a standard. Do you know any reference? P.S. And yes, setting the |
Best way to think about the current ESP32 logic is to divide the diagram into four edges, t1 to t4, from left-to-right. In the clockwise direction, those edges are passed through t1 > t2 > t3 > t4. In the anti-clockwise direction, they are passed through t4 > t3 > t2 > t1. The 1-phase logic is to always count on t1. That means the start of the step in the clockwise direction and the end of the step in the anti-clockwise direction. The 2-phase logic is to count on t1 and t3 – so a similar difference depending on which direction you're turning. This is because the logic counts only on edges of the A signal. For 4-phase logic, a second counter channel is used to count on both edges of the B signal as well. I guess this sort of makes sense if you think about the counting occurring at a place rather than a time: if I very carefully turn the encoder in one direction and then back again, the increment and decrement will happen at exactly the same physical position. Staring at the setup, I think it would be possible to count on the later edge in either direction for 1- and 2-phase logic, but it would require using both channels and a more complicated configuration. I have no idea whether there is any "right" answer to which edges one should count on with 1- and 2-phase logic. I can find no particular reference. Perhaps because anyone who cares actually uses 4-phase logic… |
@jonathanhogg I do of yourse understand the diagram at the first post. My question is, whether this behavior is a kind of standard and all implementations must match that, or is it the ESP32 implementation and other vendors may implement it differently. The MIMXRT hardware implements solely phase=4. So the emulation would divide the internal counter value by 2 or 4 to emulate phases=2 or phases=1 And I added now offsets to meet the expected behavior of this specific test script. @projectgus Speaking of a connection check. The MIMXRT encoder hardware allows to get the actual state of the input signals. That could be uses as a means to check the connection, if these values are exposed with an API. |
Add pin initial level.
Add test Encoder phases x2 and x4.
Explanation to the #12346 (comment)
Output waveform of industrial quadrature encoders:

Output waveform of rotary encoders is slightly different:

They have several fixed positions per revolution with channel A and channel B output levels equal to 1.
I suspect that @jonathanhogg, @jimmo used exactly this kind of encoder.
He has high levels of input channels and increment the Encoder on the falling edge of channel A.
micropython/ports/esp32/modules/machine.py
Line 161 in 744270a
So, the esp32 test requires initial high levels of channels before starting the count.
Also added mimxrt configuration from the #12347
@robert-hh You can cherry-pick from here too.