Improve ENCODER_DEFAULT_POS to recognize lost ticks

[mwyborski]

Modified encoder_update to be more permissive about lost pulses when returning to the ENCODER_DEFAULT_POS to detect if a tick has happened. E.g. on resolution 4 rotary encoder all ticks with only 1, 2 or 3 detected pulses have been discarded in the original code, but when the rotary encoder returns ENCODER_DEFAULT_POS and pulses are present, we can be sure, that a tick has happened. This modification catches all these lost ticks with only 1,2 or 3 pulses, which results in a more reliable performance of the rotary encoder.

Description

Modified the encoder_update method to emit ticks even when not all pulses have been detected but direction is clear and the rotary encoder returned to its ENCODER_DEFAULT_POS.

Types of Changes

• Core
• Bugfix
• New feature
• Enhancement/optimization
• Keyboard (addition or update)
• Keymap/layout/userspace (addition or update)
• Documentation

Issues Fixed or Closed by This PR

Checklist

• My code follows the code style of this project: C, Python
• I have read the PR Checklist document and have made the appropriate changes.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• I have tested the changes and verified that they work and don't break anything (as well as I can manage).

[mwyborski]

I tested it on my Keychron q2 and it makes the rotary encoder perform noticeable better. This enhances the existing ENCODER_DEFAULT_POS feature and i think it is not necessary to change the documentation.

[IBNobody]

This is one potential solution to #16927.

[jurriaan]

Tested on Keychron Q3 👍 seems to work fine!

[FWest98]

For me, this causes the rotary knob on the Keychron Q3 to become too sensitive; every "click" of the knob will result in multiple triggers of the associated key (in my case, volume up).

[mwyborski]

@FWest98 there is only a click when returning to the default position. So it is impossible to receive multiple triggers with one tick. Until you are not using the encoder really quick, there won’t be a difference to standard behaviour. It is about speed not sensitivity.

[FWest98]

I think the issue is that I didn't set the default position, but if that would cause the encoder to be much more sensitive, I think this PR changes default behaviour in a way I am not sure is desirable... 🤔
But based on some tests when debugging it for myself, it seems like for quick turns, the function encoder_read simply isn't called sufficiently often and this solution would still miss some clicks. I think the solution in #16954 is more reliable in this case (but specific to Keychron).

[mwyborski]

@FWest98 as you talk about the keychron q3 i think the rotary encoder is exactly the same like on the Q2. I think you see both solutions in competetion when they are clearly not. The solution #16954 just changes the way how the pulses are detected to allow for insertion in case of interrupts. But during interrupt handling further state changes will still be lost. So the best solution is to use both at the same time. I have it running this way, since i made the Pull Request and it works definetly and testable better.

How did i verify:

1. I assigned the letters "-" and "+" instead of volume to the rotary encoder
2. Opened a empty file in an editor and checked that with the same amount of turning the same amount of symbols appear in the text editor. I verified, that this is the exactly same pulse-series like with the original code by having debug messages in the state changes. Slowly turning will definetly have the exact same ticks and state changes like the original code.
3. Increase the speed for the same amount of tick-travel and check the output. Now you begin to see, that ticks are lost. Very easy to verify with the method above and you don't have to rely on your feeling when checking. Put in the fix from keychron and it gets a little better, but still not a perfect speed for quick turning until you begin to loose ticks. Put in this fix and it get's a little more better.

What bothers me, is that you say this patch would introduce ghosting, when it doesn't and just state this on your feeling. Please elaborate why do you think, that this patch would affect the keychron fix in any way negatively?

[FWest98]

I don't mean to say it introduces ghosting, but without setting ENCODER_DEFAULT_POS the sensitivity is over the top; every physical tick will cause 4-5 keypresses to be sent. To me it is not clear whether this definition is now required, but your code looks like it's not. So I am worried the default behaviour (without setting the DEFAULT_POS) will change in this PR.

As for the difference between the Keychron PR and this one, my observation is based on debug-logging the observed state-values in the encoder_insert_state function and in the encoder_update function. In that testing, on a quick turn of the knob, the insert_state function was called reliably for each change, while the encoder_update function seemed to miss a number of hits, as measured in the code before any of your changes - so that's where my confusion comes from.

When I find some time, I will try to reproduce this test, to make sure it was not a fluke or anything.

[mwyborski]

@FWest98 There are no changes outside of ENCODER_DEFAULT_POS introduced by this patch.
The only differences in other rows is indentation forced by the lint checker. So you are not testing right, because with or without this patch, the code is the same when ENCODER_DEFAULT_POS is not set.

[mwyborski]

@FWest98 please double-check that you have
ENCODER_RESOLUTION 4
in your config.h for q3.