#3875 proposed fix for dead spot when reversing

[nmorse]

Slight change in logic for updating the position. fixes the dead spot #3875 when reversing direction of the encoder knob.

Fixes #3875 [edit to link issue]

[nmorse]

I have not accounted for the possibility that some encoders might be wired with closed circuit (pins a and b connected to common) when at rest (the detent resting spot). My encoder is open circuit (both a and b pins are OFF - disconnected from the common) in the detent position.

I have simulated what an encoder with closed circuit (a and b pins ON - connected to common) at rest. In this case, this PR does not fixed this dead spot, although it continues to function as before. If what I am calling a closed circuit encoder exists and needs this fix, an added configuration parameter will be needed, since this is my first PR I am keeping it simple and not adding this parameter, unless it is required.

[dhalbert]

Is your encoder the same as https://www.adafruit.com/product/377 ? If not, could you point to its datasheet?

[nmorse]

that is the one!

[nmorse]

@dhalbert I have only (board) tested the nrf common-hal side of things (ports/atmel-samd/common-hal/rotaryio/IncrementalEncoder.c is untested)

[nmorse]

if a different encoder (with detent logic levels of pin_a=0 and pin_b=0) needs to be supported, this line of code would effectively need to check new_state == detent_decimal_gray_code where detent_decimal_gray_code = 0. not a hard coded '2'

for now this PR is not accommodating configuration of a detent_decimal_gray_code 😊

[tannewt]

I don't believe this is the correct fix. The code shouldn't depend on last state.

[nmorse]

re: ports/atmel-samd/common-hal/rotaryio/IncrementalEncoder.c line 164

The property last_state is a meaningful name before line 153, but after that point, it is set to the current (newest) state. That line of code uses last_state (shamelessly), because it was readily available (not a great excuse), handy (all with no additional stack space used for a local var). But the meaning of name 'last_state' certainty does not shine after assignment.

@tannewt Admittedly, the PR's use of property last_state is miss-leading, but given that it is assigned the current state just prior, it may be functionally correct. Could a property renaming to be corrective? Or, are you pointing toward some other method of coding a 'correct fix' for this bug?

ideas for possible remedies:

• Add a local var called current_state (and use that to test for position update and assign to last_state)
• rename last_state, remove the past tense from the property name (just call it state , or to me the name gray_code_index is possibly more descriptive of the values stored)

thanks, tell me what you are thinking.

[dhalbert]

I am going to look at this in more detail but am preoccupied with getting bugs out of the current upcoming release. I can look at this but maybe not for a few days, sorry.

[dhalbert]

still on my list, but I need to do a more thorough look at the algorithm.

[dhalbert]

I am not sure there is a simple algorithm that both works reliably and does not have a dead spot when reversing. @nmorse, how do you think we should proceed on this and #3875?

[nmorse]

@dhalbert thanks for waking up this thread. From my standpoint, I am happy with my local build that fixes the skip. Code in this PR works well in my application, as it solves the skip when fine tuning a setting, up and down. I would address the 'request for change', but it is non-specific on what acceptance criteria could be. Maybe only the name of the variable last_state is the issue -- (sorry to reiterate & deep dive here) to save space that var gets mutated in place, in effect becoming the current state.
Any which way, this PR may be too old, but the Idea of it was to settle the state to a quadrature count of 0 when both pins are low.

I have thought on (but not implemented) that other technique that only interrupts on one pin. It is kind of cool, minimal, creative. It could be best for slower or taxed processors. I need to find time to combine that with the concept of settling the state to 0 on the detent (both pins low) as that is the only way (that I know / have succeeded) to fix reverse skipping :)

Let's give ourselves some credit and acknowledge that there will always be noise coming from these (physical contacts, non-optical) encoders and let's face it, they are "relative" not absolute encoders, so what we think of as impossible transitions may well appear to really happen!

[nmorse]

I defer to core team members to choose: this PR or go for a different tack. I'll try to help if you give me some direction.

[nmorse]

afterthought: the state machine that we are trying to wrangle here, is required to debounce those physical contacts. That combined with the possibility of missed interrupts, necessitates a really hardened, all possibilities handled, solution.
And looking forward to seeing a rp2040 PIO solution 😸

[dhalbert]

I'm going to close this now without prejudice., since this is still under discussion and the approach may change. We can come back to this.