reversing direction skips position update in rotaryio.IncrementalEncoder

[nmorse]

encoder.position does not change (first tick is not recorded) when reversing direction.

I have found encoder.position to accurately report while consistently incrementing or decrementing (clockwise or counterclockwise), but it does not report the first tick when changing direction.

Similarly, if the user toggles back and forth between two adjacent (tick) positions, no change is ever reported by encoder.position.

Hardware setup: this guide
MPU Feather nRF52840 Express (running circuitpython 6.0.0)

[tannewt]

I don't think we'll have a chance to look at this any time soon. The relevant code is here if you'd like to take a look: https://github.com/adafruit/circuitpython/blob/main/ports/nrf/common-hal/rotaryio/IncrementalEncoder.c

[nmorse]

@dhalbert You had suggested that I look at the nrf and the atmel-samd versions. Fun to read and understand both, my informal analysis is: They look to be functionally equivalent, but use resources differently. The nrf interrupt handler may use more CPU, where the atmel-samd probably uses less cpu-cycles, but could have a slightly larger memory footprint. Classic trade-off. What would you suspect to be the more precious resources at runtime?

then I have another small quandary : there could be encoders out there that are not exactly like mine 🔢
The encoder I have is open at rest (in the detent position). Pins a and b are pulled high at rest (detent is a=1, b=1)

The fix is easy if all encoders are configured as (detent a=1, b=1), but if encoders are closed at rest or pins are pulled low (detent a=0, b=0), then the code solution that I am cooking up will work but continue to exhibit this bug.
To solve in general, a new parameter will need to be passed to the constructor (to declare the detent pin levels a=1, b=1 or a=0, b=0).

Is adding a new constructor param a breaking change or can we make a new parameter with a default value? As a Dev/User I see circuitpython has default params, but I don't know how to set that up in C code.

[tannewt]

@nmorse What encoder are you using? It seems incorrect for there to be a single value at rest.

It is possible to add a keyword argument without breaking the API because they are optional. The underlying C just gets the default value passed in.

Argument parsing is done here: https://github.com/adafruit/circuitpython/blob/main/shared-bindings/rotaryio/IncrementalEncoder.c#L67

[nmorse]

encoder is this one adafruit.com/product/377

It seems incorrect for there to be a single value at rest.

^ my fault for using a short-hand (binary 11 or 00) to represent two pin levels in one value.

Pins a and b are pulled high at rest (detent is binary 11)

I have edited my 11's to be in a more readable form (a=1, b=1)

thanks for the pointer on argument passing.

[tannewt]

We shouldn't need to add anything here. The detent state shouldn't matter to the gray code.

[nmorse]

I know the team is busy with a release, but when you get back to this bug, try out this simulated interrupt handler (python) script for Rapid prototyping and switching between several handler algorithms

[dhalbert]

I looked at this more, and did some instrumentation of the current code and your PR #3967. I instrumented the interrupt handler and noticed it is losing interrupts sometimes (though that may be due to my print statement). A couple of observations on the base algorithm:

• If interrupts are missed, then there will be missed counts, because the count-to-4 logic won't count to 4 by the next detent, and will get out of sync.
• If we can determine the quiescent state (at a detent, usually 11), then we can check for the quiescent state and increment the count only when we get to that state, assuming we can still figure out the direction, even with missed counts. We could read the state at contruction and remember the detent state. (Maybe this is wrong due to bounces??)

I also looked at other algorithms. This very interesting and simple algorithm by David Johnson-Davies uses only one interrupt, and has less trouble with bouncing and misses: http://www.technoblogy.com/show?1YHJ. And here is a quite complete analysis of it: https://wiki.fryktoria.com/doku.php?id=arduino:rotary-encoder. It notes that Johnson-Davies' algorithm inherently drops one count on direction change, which is unfortunately the problem you were trying to solve. However, it is very good in many other ways.