mimxrt/machine_encoder: Fix the numeric values of UP and DOWN.#19268
mimxrt/machine_encoder: Fix the numeric values of UP and DOWN.#19268robert-hh wants to merge 1 commit into
Conversation
The values should be 0 for UP and 1 for DOWN to match the documentation. Instead they were -2 and -3, causing a test with a count direction set by a Pin to fail, when the symbols were used instead of numbers.. Tested with the machine_encoder.py and machine_counter.py script and the extended machine_counter.py script from PR micropython#19239. Signed-off-by: robert-hh <robert@hammelrath.com>
1396b12 to
2d80d7a
Compare
|
Code size report: |
projectgus
left a comment
There was a problem hiding this comment.
This change seems fine to me, but as far as I can tell the docs don't specify integer values for these at all.
The only other place they're defined is ports/esp32/modules/machine.py which maps them to the PCNT.INCREMENT and PCNT.DECREMENT values which are set to constants in ESP-IDF.
If we want this to stay consistent in MicroPython (and I think we do) then the way to be sure would be:
- Add a header
extmod/machine_counter.hand defineMACHINE_COUNTER_UPandMACHINE_COUNTER_DOWN. - Use that header in
ports/mimxrt/machine_encoder.c - Also include that header into
esp32_pcnt.cand static assert thatMACHINE_COUNTER_UP == PCNT_CNT_INCandMACHINE_COUNTER_DOWN == PCNT_CNT_DECwith a comment that this is for themachine.Counterimplementation found inports/esp32/machine.py
Otherwise it could break at any time due to an ESP-IDF update, or a new machine.Counter implementation could be added which also breaks it.
This is a lot of extra work compared to changing two constants though, so please feel free to also ignore this suggestion and we can merge this as-is.
The values of MACHINE_COUNTER_UP and MACHINE_COUNTER_DOWN have to be port-specific, because they depend on the hardware. So they cannot be the same for all ports. I stubled over that when testing parts of PR #19239, where the numeric values for counting up and down for ESP23 are opposite to the ones needed for MIMXRT. If the count direction is directly controlled by an input pin, the level cannot be arbitrarily defined. The actual PR only ensures, that the symbols Counter.UP and Counter.DOWN behave identical for software- and pin-controlled counting. |
OK, I see. I guess we could always map the Python-facing values into the hardware-specific internal values to make this consistent, but that's probably overkill.
Ah, I understand now. OK. |
Summary
The values should be 0 for UP and 1 for DOWN to match the documentation. Instead they were -2 and -3, causing a test with a count direction set by a Pin to fail, when the symbols were used instead of numbers..
Testing
Tested with the machine_encoder.py and machine_counter.py script and the extended machine_counter.py script from PR #19239.
Generative AI
I did not use generative AI tools when creating this PR.