Code size report:

Reference:  tests/float/complex1.py: Fix CPython 3.14 deprecation. [b14d129]
Comparison: micropython/docs: Update the Encoder/Counter documentation. [merge of a1179e8]
  mpy-cross:    +0 +0.000% 
   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% standard
      stm32:    +0 +0.000% PYBV10
      esp32:    +0 +0.000% ESP32_GENERIC
     mimxrt: +5836 +1.552% TEENSY40
        rp2:    +0 +0.000% RPI_PICO_W
       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS
  qemu rv32:    +0 +0.000% VIRT_RV32

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.38%. Comparing base (230bbbb) to head (a1179e8).
⚠️ Report is 3 commits behind head on master.

@@           Coverage Diff           @@
##           master   #12347   +/-   ##
=======================================
  Coverage   98.38%   98.38%           
=======================================
  Files         171      171           
  Lines       22298    22298           
=======================================
  Hits        21937    21937           
  Misses        361      361           

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

@dpgeorge @projectgus Now that the Encoder/Counter classes are available for ESP32, I have rebased and retested the implementation for MIMXRT. Since the Encoder/Counter hardware differs between ESP32 and MIMXRT, the ESP32 oriented test in extmod_hardware show quite a few deviations. Major reasons:

• The MIMXRT Encoder always counts every transition. So count numbers are by a factor of 4 higher than in the ESP32. But it does not have the ESP32 glitch to need 3 transitions for the first count.
• Encoder/Counter input values cannot be read back using the machine.Pin methods, causing the connection test to fail.
• The MIMXRT Counter always counts the rising edge. The active edge cannot be changed.

Thanks @robert-hh for updating and noting these differences. (I think it's a good example of the potential for tests to find these hardware-specific parts as well.)

The MIMXRT Encoder always counts every transition. So count numbers are by a factor of 4 higher than in the ESP32. But it does not have the ESP32 glitch to need 3 transitions for the first count.

The ESP32 version of this class has a documented port-specific phases parameter, that defaults to 1 but it can also count each transition by setting it 4. So we could change the default value of phases, but another option would be to emulate the phases parameter in software on the mimxrt version. What do you think?

Encoder/Counter input values cannot be read back using the machine.Pin methods, causing the connection test to fail.
The MIMXRT Counter always counts the rising edge. The active edge cannot be changed.

I think the solution for these will probably be @unittest.skipIf() for the respective tests. Possibly in the top block we can set variables like COUNT_FALLING = True and then evaluate those in the skip clauses.

I am a bit surprised that machine.Pin() doesn't work for the input values, though. Is that because there are pins which can never be configured as GPIO, or is that a limitation that once the port muxes them to be counter inputs it doesn't/can't mux them back?

Good day @projectgus, thank you for the swift reply.
Emulating Phase would be possible to a certain extend by dividing the MIMXRT counter by 2 or 4. That would however not cater a possible situation, that ESP32 increases the count with a specific transition. That seems to be the case and may be the reason for the first count in the test needing only 3 transitions. Maybe that could be solved by finding the proper start conditions of the inputs. Another indication of changing the count at a specific transition is the fact, that for ESP32 a single transition down from 0 decreases the counter to -1. A simply division would not cater for that.

Obviously one can skip the test for the FALLING edge of the counter.

I did not look in detail why the Pin value() cannot read back when the Pin was configured for Encoder/Counter. Probably it's because for Encoder/Counter a PIN is configured for a different ALT mode. So maybe the test has to be changed to configure the Pin for machine.Pin mode first, make the connectivity test and then configure it as Encoder/Counter. I wondered why there is the connectivity test at all. The other tests to not have it.

So I added an emulation for phases and adapted the test scripts, taking into account the not-supported options and the fact, that different to ESP32 the MIMXRT expects 4 transitions to count from 0 to 1. For ESP32, setting the initial value of pulses to -1 fixes that.

Sorry I didn't reply last week, @robert-hh - the updated PR looks great to me, thanks for accommodating the limits in the API and tests! I don't think I have any mimxrt board set up to do any local tests on, unfortunately (there might be a Teensy 4 hiding somewhere, will look later).

I wondered why there is the connectivity test at all. The other tests to not have it.

I borrowed this from Ihor's test PR, but it's something I support building into tests - especially hardware-in-loop tests. The idea is to have a quick and user-friendly way to differentiate "the test environment is broken" from "the driver is broken". More than once I've had some unusual pattern of failures and spent too long debugging them before realising a wire is loose, or some equivalent prerequisite is missing (i.e with MicroPython, forgetting to do mpremote rtc --set after hard reset has gotten me more than once).

As you observe, MicroPython's tests don't have much of this kind of thing (yet) - but I think that's to the project's detriment. It costs time even for core developers (like me) who run the test suite a lot, but for developers who aren't already very familiar with the project then it's even more limiting. We get a lot of PRs from new(ish) contributors who are competent developers, but the relevant tests either haven't been run, or they ran and failed and the developer hasn't dug in to figure out why.

(Sorry for dropping in this somewhat tangential rant. cc @hmaerki and @dpgeorge as I know they've been thinking about tests an awful lot, lately.)

In the particular case of the connections test for Counter/Encoder, I think skipping them on mimxrt - as you've done here - is fine for now. Long term, putting them in a separate TestCase at the top of the same file (so they're run without the setUp step that inits the Encoder and muxes the pins) will probably allow them to pass on more platforms, but I can look at that in a follow-up if you like.

More than once I've had some unusual pattern of failures and spent too long debugging them before realising a wire is loose, or some equivalent prerequisite is missing (i.e with MicroPython, forgetting to do mpremote rtc --set after hard reset has gotten me more than once).

That's what #16112 is trying to address, although it's gotten pretty lost among all the other PRs. Might be nice to get that merged, I use it all the time now.

The idea is to have a quick and user-friendly way to differentiate "the test environment is broken" from "the driver is broken

We are definitely missing a simple Pin test that toggles IO and checks it works. That could run before any of these other more sophisticated tests to verify the hardware is wired up correctly.

But also, I think we need to break out all this board wiring configuration into separate files, one for each board, that can be reused in all hardware-related tests. That's something I'm working on now.

putting them in a separate TestCase at the top of the same file (so they're run without the setUp step that inits the Encoder and muxes the pins) will probably allow them to pass on more platforms, , but I can look at that in a follow-up if you like.

I'm not familiar with the mechanics of unittest, so I do not object if you look at that later.

@dpgeorge I have rearranged the commits a little bit, and more squashing can be done to the test-related commits and the two about documentation.

Thanks @robert-hh for updating the docs. It's now much clearer how the mimxrt code matches the existing docs (and esp32 implementation) and what's been added.

So there's now two things to do here:

1. Review the mimxrt implementation to see that the existing documented functionality works as expected. It seems that is indeed the case because the tests pass.
2. Review and discuss the additions to the Counter and Encoder classes. I can see you've been quite thorough there, adding support for the full mimxrt behaviour (or most of it).

Ideally (1) and (2) would be separate PRs, because (1) is much easier to review and merge, whereas (2) may take some back and forth to finalise the enhanced API. But I can understand that might be more effort than you have time for. So it's up to you how to proceed, ether split this PR into two where the first just implements existing documented behaviour, or keep it as one PR and it'll take a bit more time to review.

Thank you for the comment. The decision is hard to make, because the underlying hardware between MIMXRT and ESP32 is different. Of course I can reduce behavior and the set of options and methods to being as close as possible to the ESP's PCNT/Encoder/Counter triple. But doing that by removing code and putting it back in later seems error prone. I would prefer leaving the extra code in and just disabling it with a compile switch.
So what would it be:

1. features not available in the ESP32 version.

• defining the pins for the index, home and match signals. That is kind of independent and easily disabled and enabled. Maybe one of index or home can be dropped totally.
• the signed/unsigned option. We could go for signed only.
• the counter/cycles model. I could hide the cycles property and just set the count range to 32 bit at phase=4. Since the hardware always counts 4 phases, at a phases=1 setting the range would drop to 30 bit.
• the id() method. That one may be obsolete, once the irq().flags() method is available.

2. Feature that are slightly different.

• the edge option for ESP32's counter, related to PCNT's rising/falling options. That is not supported for MIMXRT, and I did not find a way to implement it. Even if the chip has a configurable logic block, that can be set as inverter, there seems to be no way to route the output of that block to a Encoder/Counter input,.
• a Pin defining the count direction. That feature is available on MIMXRT, and available through the mode_pin option of the ESP32 PCNT class. Only the way to specify it is different.
• The IRQ events. The only common event is a counter match, which is called IRQ_MATCH for MIMXRT and IRQ_THRESHOLD0 (or 1) for ESP32. I could call it IRQ_THRESHOLD0 at the MIMXRT port. But the other events are different and I see no way to emulate e.g. ESP32's events properly in the MIMXRT port or the opposite.
• Encoder. status(), Counter..status() vs irq.flags(). That can be harmonized to provide the irq()'s flag method.
• Reading the MIMXRT data sheets again, it seems that the min and max options of the ESP32.PCNT class could be enabled.

P.S.: Being a pensioner, I should have every time of the world. But since everyone assumes this (and asks me to do this or that), it's way less. Anyhow, time is not a problem.

the underlying hardware between MIMXRT and ESP32 is different

Yes, but you already did a great job to get the existing tests passing on mimxrt, so the API matches pretty well already.

So what would it be:

1. features not available in the ESP32 version.
   ...
2. Feature that are slightly different.
   ...

OK, thanks for the detailed list of differences.

Don't change anything for now and leave it with me. I'll do a more thorough review and we can take it from there.

I made a minor fix to dealing with the phases argument. Besides that all is as it was. Looking though the code again I noticed, that the cpc=b option acts like the max=n option of the PCNT class. Renaming it and adding min=b is then easy, since the required mechanism is used anyhow.

I could change the implementation of the irq method to make it more compliant to newer code. That would not change it's properties.

I noticed, that the cpc=b option acts like the max=n option of the PCNT class. Renaming it and adding min=b is then easy

OK, that sounds like a good change. Having min/max arguments seem a lot more general (to other ports) than cpc.

I could change the implementation of the irq method to make it more compliant to newer code.

Yes please. The suggested changes for that would be:

1. Drop the id() method, it shouldn't be needed (the IRQ callback is passed the Encoder/Counter instance already).
2. Move the status() method from the Encoder/Counter class to the IRQ object, using the general mp_irq_methods_t.info slot (see how it's done in machine_uart.c).
3. Use mp_irq_new() to create the IRQ instance.
4. Remove the value argument from .irq() method, then use the common mp_irq_init_args struct for the argument parsing of the .irq() method.
5. Add a new method to Encoder/Counter to get/set the IRQ matching value (since it was removed in step 4 above).

I made the suggested changes.

1. cpc was renamed to max, and a min option is added. Note that these behave slightly different to ESP32, in that they define the lower and upper limit for a modulus style counting. While ESP32 resets the counter to 0, MIMXRT rolls over or under. In any case, the event can cause an interrupt.
2. The iqr() implementation is changed into using the common code from runtime/mpirq. As suggested, I added a encoder.match() method to set the counter match value. The ESP32 implementation of PCNT uses a keyword option of init() instead. That can be done here as well. The id() and status() methods are removed.

The names of the trigger events are still not changed. They could change to:

IRQ_MATCH into IRQ_THRESHOLD0
IRQ_ROLL_UNDER into IRQ_MIN
IRQ_ROLL_OVER into IRQ_MAX.

Another change is the behavior of the index and home pins. They do now different things. The index pin transition now just increments or decrements the cycles counter. The home pin transition just resets the pulse counter.

The documentation is not yet updated. That will be done when the code is settled.

The documentation is updated, and the signed keyword option removed.