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.

[nmorse]

@dhalbert

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.

Interesting to only interrupt on one signal (not both). Sure that would reduce noise by half and use half the resources. I suspect that the reverse direction drop bug could be fixed . It is an interesting approach of awaking onChange for only one pin, lets try it. I don't understand why the wiki article shows twice as many detent (stable zones), I have never seen two detents per gray code cycle 🤷.

[jepler]

In my recent testing, this is affecting samd21 quite a lot and rp2040 essentially not at all. Given that these both now use "softencoder" to interpret the sequence of encoder A/B values (#4580), it tends to point at a problem with sampling at all the necessary moments. samd21 uses pin change interrupts, while rp2040 uses a PIO state machine to sample at a very high rate (over 1MHz) and report all state changes to C code to interpret.

When giving the Adafruit rotary encoder a good swift twist, it's possible to get "stable times" (where both the A and B phases have settled out) well under 500us, so you'd potentially need to sample at above 2kHz. However, the samd21 can accumulate errors even when being turned well slower than this. How frequently can a pin change interrupt fire? Can error arise because of a different pin value at the "interrupt fired" time and the "pin read" time? ... It would be nice to settle this. but for now I guess I'll just run my rotary encoders from an RP2040.

Meanwhile, contact bounce seems to work itself out within 10 microseconds, at least when turning quickly.

When turning slowly, the contact bounce period is also lengthened

(note inconsistent horizontal scale in the various screenshots; saleae logic was sampling at 50MS/s)

[deshipu]

I think this can still be related to the problems with gamepad and pewpew, where the interrupts were being disabled. While the worst of it has been fixed, there is still a noticeable jitter, so maybe there are more places like that.

[jpconstantineau]

I just tested this on my 4 encoder nRF52 board as I was testing PR #5253 which affected encoders too. I can attest that the dead zone is still there for EC11 encoders.

Looking at the EC11 datasheet they appear to have 2 detents per pulse and have 2 different "detent stability positions" depending on the model. Depending on the model, this may make them appear to have a dead zone when they are reversed. In a nutshell, this may be a "hardware issue", not a software one.

I have another board with a different encoder type and there is no dead zone. It has 100 steps per rotation and it works great. No skips (turned 20 times and counted to 2000) and no dead zones.

[jepler]

Yeah the code we have today is heavily geared to 1 detent per quadrature cycle. Supporting other "granularities", sometimes called X2 and X4, would be a nice addition, except for the problem of making it fit on small boards like the M0s which don't have much code space left.