Code size report:

Reference:  rp2/modules/rp2.py: Don't corrupt globals on asm_pio() exception. [c3ca843]
Comparison: tests/extmod_hardware/machine_counter.py: Separate the connection test. [merge of 2322d37]
  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: +5892 +1.564% 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.42%. Comparing base (c3ca843) to head (2322d37).
⚠️ Report is 6 commits behind head on master.

@@           Coverage Diff           @@
##           master   #12347   +/-   ##
=======================================
  Coverage   98.42%   98.42%           
=======================================
  Files         174      174           
  Lines       22329    22329           
=======================================
  Hits        21978    21978           
  Misses        351      351           

☔ 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.

Thank you for the thorough review. I made most of the suggested changes, except one. You suggested a note that the cycles counter is reset when value is set to 0. That is actually not the case.

quickref.rst is updated.

Updated. The documentation is hard to get perfect. Every time one looks at it there are spots to improve. Of course, that holds for code too.

The documentation is hard to get perfect.

It doesn't need to be perfect 😄

Thank you.

@robert-hh both ADAFRUIT_METRO_M7 and MAKERDIARY_RT1011_NANO_KIT fail to build with this PR, something about XBARA1 undefined. Are you able to fix that?

Will do.

The files mpconfigboard.h for ADAFRUIT_METRO_M7 and MAKERDIARY_RT1011_NANO_KIT are updated. The firmware builds and runs. To be sure, I built the binaries for all MIMXRT boards, and all build fine. The change is published as PR #18908.

For history: Since the MIMXRT1010 does not support Encoder, these boards were not tested with machine.Encoder, and being introduced later that the first versions of the PR12347, they were not any more considered as relevant. So building all boards is a required test step.

For history: Since the MIMXRT1010 does not support Encoder, these boards were not tested with machine.Encoder, and being introduced later that the first versions of the PR12347, they were not any more considered as relevant. So building all boards is a required test step.

OK, thanks for the info. It's hard to get everything right when so many things are supported, and with all the moving pieces. And anyway it's an easy problem to fix.