Code size report:

Reference:  esp32: Make a generic sdkconfig.flash_qio_80m config snippet. [4fd7295]
Comparison: machine_encoder.c [merge of f44c864]
  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: -1184 -0.068% ESP32_GENERIC[incl -1648(data) +24(bss)]
     mimxrt:    +0 +0.000% 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.47%. Comparing base (1c63211) to head (f44c864).
⚠️ Report is 72 commits behind head on master.

@@           Coverage Diff           @@
##           master   #19239   +/-   ##
=======================================
  Coverage   98.47%   98.47%           
=======================================
  Files         176      176           
  Lines       22845    22845           
=======================================
  Hits        22497    22497           
  Misses        348      348           

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

@robert-hh
May you test on the MIMXRT port?
tests/extmod_hardware/machine_counter.py

@IhorNehrutsa
The test has to be changed. Line 142, 143 have to be:

        self.counter.init(in_pin, direction=Counter.UP, match=100)
        self.counter.irq(handler=callback, trigger=Counter.IRQ_MATCH)

@robert-hh

Does irq.flags() mean the Counter.status() and Encoder.status()?

Does irq().flags() mean the Counter.status() and Encoder.status()

There is no Counter.status() or Encoder.status() method. irq().flags() returns the flag which caused the IRQ to happen. And that may be different for Counter and Encoder.

@robert-hh
Oh, thanks, I was watching the old version. :(
May you give a small example of using irq().flags() instead of xxx.status() in tests\extmod_hardware\machine_counter.py?

Try to call Counter.irq().flags() instead of Counter.status().

@robert-hh
It seems better to set the handler after disabling interrupts

@robert-hh
In file
tests/extmod_hardware/machine_counter.py
added print counter.irq().flags() in callback function.
May you test it on the MIMXRT port?
Thanks

>>> %run -c $EDITOR_CONTENT
test_connections (__main__.TestConnections) ... ok
test_count_rising_UP (__main__.TestCounter) ... ok
test_count_rising_DOWN (__main__.TestCounter) ... ok
test_count_falling_UP_DOWN (__main__.TestCounter) ... ok
test_count_pin_direction (__main__.TestCounter) ...
 callback counter.value():      5000, counter.irq().flags():  4, flags.IRQ_MATCH:True

 callback counter.value():      5000, counter.irq().flags():  4, flags.IRQ_MATCH:True
 ok
test_irq (__main__.TestCounter) ...
 callback counter.value():     50000, counter.irq().flags():  4, flags.IRQ_MATCH:True

 callback counter.value():    -50000, counter.irq().flags():  4, flags.IRQ_MATCH:True
 ok
----------------------------------------------------------------------
Ran 6 tests

OK
>>> 

Tested with Teensy 4.0: works.

~/Temp$ mpr run machine_counter.py 
test_connections (__main__.TestConnections) ... ok
test_count_rising_UP (__main__.TestCounter) ... ok
test_count_rising_DOWN (__main__.TestCounter) ... ok
test_count_falling_UP_DOWN (__main__.TestCounter) ... skipped: FALLING edge not supported
test_count_pin_direction (__main__.TestCounter) ... skipped: Pin as a count direction is not supported
test_irq (__main__.TestCounter) ...
 callback counter.value():     50000, counter.irq().flags():136, flags.IRQ_MATCH:True

 callback counter.value():    -50000, counter.irq().flags(): 72, flags.IRQ_MATCH:True
 ok
----------------------------------------------------------------------
Ran 6 tests

OK (skipped=2)

I shall note that setting the Counting direction with a Pin works as well with the MIMXRT port, with 0 for counting up and 1 for counting down. So the polarity is inverted, and using the constants counter.UP and counter.DOWN fails at MIMXRT because they are defined with the wrong values. I'll have to fix that. The test script should use self.counter.UP and self.counter.DOWN.

@robert-hh
Another question:
Is it possible not to reset the counter to 0 during Counter.init()?
Due to this feature, this code is not possible, but I think it is more natural for a user:

    def test_irq(self):
        self.counter.init(in_pin, direction=Counter.UP, match=50_000)
        self.counter.irq(handler=callback, trigger=Counter.IRQ_MATCH)
        toggle(60_000)
        self.assertCounter(60_000)
        self.assertEqual(callback_counter_value, 50_000)

        self.counter.init(in_pin, match=100_000)  # try to count UP, with new match value, but calling init() ZEROES the counter
        toggle(60_000)
        self.assertCounter(120_000)
        self.assertEqual(callback_counter_value, 100_000)

Technically it may be possible, but it breaks existing behavior. At the moment, a call to init() will reset the counter to the min value, whatever it is. One can of course set the counter to any value after a call to init(),

Technically it may be possible, but it breaks existing behavior.

Yes, I understand. But was this behavior discussed, or did it just happen on its own?

At the moment, a call to init() will reset the counter to the min value, whatever it is. One can of course set the counter to any value after a call to init(),

counter = Counter(id, in_pin, direction=Counter.UP, match=50_000)
toggle(60_000)
assert counter.value() == 60_000
...
save_counter_value = counter.value()
counter.init(match=50_000)  # reset counter to 0, but save direction and other params
counter.value(save_counter_value)  # restore to saved
toggle(60_000)
assert counter.value() == 120_000
...

This is not the same behavior: init() modifies only explicitly provided parameters and preserves all omitted parameters, but zeroes the counter.

If init() saves the counter code is simpler.

counter = Counter(id, in_pin, direction=Counter.UP, match=50_000)
toggle(60_000)
assert counter.value() == 60_000
...
counter.init(match=50_000)  # save counter and save direction and other params
toggle(60_000)
assert counter.value() == 120_000
...

If the user needs to reset the counter it can do it explicitly.

counter = Counter(id, in_pin, direction=Counter.UP, match=50_000)
toggle(60_000)
assert counter.value() == 60_000
...
counter.init(match=50_000)  # save counter and save direction and other params
counter.value(0)  # zeroes couner explicitly
toggle(60_000)
assert counter.value() == 60_000
...

The Zen of Python:
Explicit is better than implicit: Don't hide what your code is doing.

@robert-hh
I propose to add the last mimxrt port updates from tests/extmod_hardware/machine_counter.py to #19268.
I will be waiting for approval #19268

I did not plan to make any changes to the test scripts. I'll leave that to you.The existing test scripts work, since they to not test counting direction.
But if you are changing the test scripts for Encoder and Counter, you could address an inconsistency, The way, the pin configuration is defined, is different for Counter and Encoder. Encoder uses the target_wiring scheme to define Pin and ID, while for Counter these are coded in the test script itself. The new method is using target_wiring.

I'm afraid this PR won't be approved as #16743 ;)

I'm afraid this PR won't be approved as #16743 ;)

In general I think we'd really like to replace esp32/machine.py with a C implementation, so please request a review on this PR once it is ready. 👍

If you can keep the behaviour the same as the existing machine.Counter and machine.Encoder then this will make it a lot easier to approve.

If you can keep the behaviour the same as the existing machine.Counter and machine.Encoder then this will make it a lot easier to approve.

I don't like that init() resets the counter to zero. What are your thoughts on this?
See #19239 (comment) and
#19239 (comment)

@robert-hh
Are the min and max values ​related to ROLL_UNDER and ROLL_OVER?
Do ROLL_UNDER_MIN and ROLL_OVER_MAX occur during the counting?

@projectgus

If you can keep the behaviour the same as the existing machine.Counter and machine.Encoder then this will make it a lot easier to approve.

Which existing behaviour must I use?
IRQ_ROLL_UNDER and IRQ_ROLL_OVER

With respect to min and max counting, the ESP32 and MIMXRT hardware behaves different. The ESP32 creates and IRQ when the counter is AT min (or max), The MIMXRT creates an IRQ when the counter rolls under from min to max (or over from max to min). So for ESP32, use IRQ_MIN and IRQ_MAX.

@robert-hh
Thanks for clarifying.
I'm not fully sure, but it seems like if
in ESP32 set PCNT_EVT_H_LIM to max+1 this will be the same as MIMXRT kENC_PositionRollOverFlag
Or for the IRQ_MAX effect in MIMXRT max_count may be 1 less.

EDITED:
https://gemini.google.com/share/8a935a1eaf41

I don't like that init() resets the counter to zero. What are your thoughts on this? See #19239 (comment) and #19239 (comment)

I don't feel strongly about this, but I think it is better to keep the current behaviour if possible. Apart from test code, is there a use case where this behaviour causes a major problem?

@projectgus

I don't feel strongly about this, but I think it is better to keep the current behaviour if possible. Apart from test code, is there a use case where this behaviour causes a major problem?

I'm keeping the current behaviour (init() zeros the counter) now, but I think it's better for the end user to have a flat/straight counter scale ranging from MIN to MAX with the ability to set MATCH to a new value without resetting the counter scale.

I'm not fully sure, but it seems like if
in ESP32 set PCNT_EVT_H_LIM to max+1 this will be the same as MIMXRT kENC_PositionRollOverFlag

min and max at MIMXRT define a cyclic counting range. If for instance the counter is at max, the next count up event will set it to the value of min. I just tested a generic ESP32 with max set to 100 (using counter._pcnt,init(max=100)). Th next up-count event then has set the counter to 32000, which seems odd. Maybe an unintended behavior.

I'm keeping the current behaviour (init() zeros the counter) now,

At MIMXRT, init() sets the Counter (and Encoder) to the value defined as min. So changing that behavior to keep the existing counter value has to consider as well, if the counter is still within the min/max range and change either change the counter value appropriately, violating the "does not change" rule, or not to change it, violating the "counter in always in minx/max range" rule.