Debounce refactor / API

[alex-ong]

Huge refactor, allowing shared debouncing code between keyboards.

• Debounce algorithm is set using rules.mk.
• Keyboards with a matrix.c file are set to DEBOUNCE_ALGO = manual
• Not setting DEBOUNCE_ALGO means you are using DEBOUNCE_ALGO = sym_g
• Keyboards without matrix.c file have no DEBOUNCE_ALGO set.
• sym_g (Symmetric, global) is the algorithm that is already in use (one global counter reset on any key change, when 5ms has elapsed, push all changes)
• Split keyboards are set to DEBOUNCE_ALGO = manual. Works since the existing split keyboard code debounces inside it's matrix.c
• Keyboards defined with CUSTOM_MATRIX = yes, will be forced to have no debounce algorithm.
• Added a eager per-key algorithm (debounce_eager_pk.c). This sends signal immediately, then restricts new inputs until debounce timer elapses.

Future work would be to migrate everything over (first split keyboards, then all the other keyboards) to shared debounce API, and add a few more algorithms. I've added a few ideas for algorithms, some which are already implemented in other keyboards. Look at readme.md inside tmk_core/common/debounce folder.

Partial solution to #3646

Since this is a potentially keyboard-breaking change, i recommend letting this get pushed into qmk/debounce_refactor rather than qmk/master (I couldn't figure out how to make a new branch)

[drashna]

What about the split common stuff, which has a matrix used? (and sets the custom matrix, as well)

https://github.com/qmk/qmk_firmware/blob/master/common_features.mk#L212-L226
https://github.com/qmk/qmk_firmware/blob/master/quantum/split_common/matrix.c

[alex-ong]

@drashna
For split all the split keyboards, i've manually added DEBOUNCE_ALGO = manual.
Since all the debouncing occurs inside split_common/matrix.c, everything should still work.

Making debouncing working with the debounce API would be a future work, but would be as simple as:

1. remove all debounce code from split_common/matrix.c
2. import debounce.h in split_common/matrix.c
3. call debounce_matrix() right after _scan_matrix

I havent included it yet since i'm guessing we should check this huge change for stability first.

[alex-ong]

I've now changed it so (SPLIT_KEYBOARD = yes) results in manual debouncing. In the future we can remove that and apply the debounce API to split_common stuff.

[yiancar]

is there a way to automate this when custom_matrix=yes as eg i have a few keyboards that are not split, but have a custom matrix

[alex-ong]

@yiancar not sure what you are asking:
a) Do you want automatic debouncing
b) Does your matrix.c currently do debouncing?

yes, yes -> Remove all debouncing code, scan straight to matrix, call import debounce.h, call debounce
no, yes -> No change, since you have CUSTOM_MATRIX=yes, debounce.h doesn't do anything

If this change gets merged, all your keyboards with custom_matrix=yes will be completely unaffected by this pull request.

# Debounce Modules. If implemented in matrix.c, don't use these.
ifeq ($(strip $(CUSTOM_MATRIX)), yes)
    # Do nothing. Custom matrix code.
else ifeq ($(strip $(SPLIT_KEYBOARD)), yes)
    # Do nothing, debouncing is inside matrix.c inside split_common
else ifeq ($(strip $(DEBOUNCE_ALGO)), manual)
    # Do nothing. do your debouncing in matrix.c
else ifeq ($(strip $(DEBOUNCE_ALGO)), sym_g)
    TMK_COMMON_SRC += $(DEBOUNCE)/debounce_sym_g.c
else ifeq ($(strip $(DEBOUNCE_ALGO)), eager_pk)
    TMK_COMMON_SRC += $(DEBOUNCE)/debounce_eager_pk.c
else # default algorithm
    TMK_COMMON_SRC += $(DEBOUNCE)/debounce_sym_g.c
endif

[yiancar]

Perfect! can you add this information in docs as well so people know what to do?

[alex-ong]

Due to the fact that CUSTOM_MATRIX=YES will result in no debouncing, DEBOUNCE_ALGO = manual could be safely removed from the 100~ keyboards i added it to.

I've added documentation so that it is more clear.
I've included a table to explain which algorithm is used.

https://github.com/qmk/qmk_firmware/blob/a55c838961c89097ab849ed6cb1f261791e6b9b4/docs/feature_debounce_algo.md

[alex-ong]

Is this api going to ever be merged/rejected? @drashna

[drashna]

@alex-ong "eventually".

It would be @jackhumbert or @skullydazed that would make that decision, ultimately though. And they've been quite busy "IRL". Tagged them, just to "bump" this.

[TerryMathews]

I know no one is asking about feedback on this, but I grabbed a copy of OP's fork and updated TKC1800 to use eager_pk on my test bed/work 1800.

It works perfectly, and it fixed an issue I was having with one of the passwords I use - I type it faster than the keyboard can keep up with

[alex-ong]

I know no one is asking about feedback on this, but I grabbed a copy of OP's fork and updated TKC1800 to use eager_pk on my test bed/work 1800.

It works perfectly, and it fixed an issue I was having with one of the passwords I use - I type it faster than the keyboard can keep up with

Thanks for the feedback. Yeah i'm a bit of a performance junkie - eager_pk_row also should help the dreaded latency from the ergodox. It is a memory/speed tradeoff. Also if scanning isn't fast enough, eager-pk can also be slower. In my case, i got the following vague results:

• SYM_G (original) - scan rate = 3k. Latency for keydown = 5ms
• SYM_G, changed scan for no sleeps - scan rate 6k. Latency for keydown = 5ms.
• SYM_G with custom scan code (compiler optimized, no array lookup) - scan rate 10k. Latency for keydown = 5ms
• Eager-pk with custom scan code (compiler optimized, no array lookup) - scan rate 3.5k. Latency for keydown = 0ms
• Eager-pk, scan rate no sleeps = scan rate 2.5k. Latency for keydown = 0ms
• Eager-pk, scan rate with sleeps = scan rate 1.5k. Latency for keydown = 0ms. Spamming keys took scan rate down to 800ms, meaning signals were sent 1-2ms late.

I tested scan rate consistency by printing out whenever the timer had shifted by more than 1ms every scan. This would show when processing took too long.

SYM_G, although fast and memory efficient had the problem of resetting the timer on consective presses, and then only sending one key per millisecond by default. Imagine pressing "fdsa" 4 ms apart - it would wait 4+4+4+5ms, then send (a, s, d, f) 1ms apart (default behavior, QMK_KEYS_PER_SCAN == 1). Eager-pk with QMK_KEYS_PER_SCAN ==1 would at least get the order right.

[alex-ong]

Fingers crossed that it passes TravisCI! After a bajillion commits/reverts, it was actually only a very small change in the end to get everything working. It now supports split_keyboards too, thanks to #4772

[alex-ong]

Quick update on usage:

ifeq ($(strip $(DEBOUNCE_ALGO)), manual)
    # Do nothing. do your debouncing in matrix.c
else ifeq ($(strip $(DEBOUNCE_ALGO)), sym_g)
    QUANTUM_SRC += $(DEBOUNCE_DIR)/debounce_sym_g.c
else ifeq ($(strip $(DEBOUNCE_ALGO)), eager_pk)
    QUANTUM_SRC += $(DEBOUNCE_DIR)/debounce_eager_pk.c
else # default algorithm. Won't be used if we have a custom_matrix that doesn't utilize it
    QUANTUM_SRC += $(DEBOUNCE_DIR)/debounce_sym_g.c
endif

• You can use your own algo
• Specifying nothing uses existing 5ms global algo
• I've included a per-key debounce (more expensive but faster response) algo.
• It doesn't care about CUSTOM_MATRIX any more. If you have a CUSTOM_MATRIX, it will include debounce_sym_g.c, which won't end up in the final hex/elf, since most custom_matrix currently include their own debouncing within their matrix.c file, and no references to the debounce, debounce_init() etc functions.

Switching algos (once people write more algos) should be as simple as a one line change in rules.mk.

[pelrun]

Just a couple of minor nitpicks from me:

Don't use MATRIX_ROWS, you'll cause a buffer overflow on a split kb. Always use the num_rows passed in to the functions. This does mean if you need extra state you have to malloc it in debounce_init(), but it's a restricted case (allocated at startup, never freed) so I feel it's justifiable. The debounce.c I had did it properly, but the almost identical debounce_sym_g.c it was replaced with doesn't. (I don't understand the name there, btw)

DEBOUNCE_ALGO = manual seems icky to me; I'd prefer DEBOUNCE_TYPE = custom or just DEBOUNCE = custom (which continues the meaning of "custom" used for other settings)

#define "matrix.h" shouldn't be in debounce.h (I'm not a fan of nested includes.) The debounce*.c files already include it manually anyway.

[alex-ong]

Just a couple of minor nitpicks from me:

Don't use MATRIX_ROWS, you'll cause a buffer overflow on a split kb. Always use the num_rows passed in to the functions. The debounce.c I had did it properly, but the almost identical debounce_sym_g.c it was replaced with doesn't.

Thanks i'll refer to that code. EDIT: oh i see, yeah it was a typo when i copy pasted my old code and then ended up modifying it to be the "same" as yours.

This does mean if you need extra state you have to malloc it in debounce_init(), but it's a restricted case (allocated at startup, never freed) so I feel it's justifiable.
Right, i should modify eager_pk to do that.

(I don't understand the name there, btw)
sym = symmetric, g = global, i.e.. symmetric debounce using a global timer.

• symmetric, (5ms delay on keydown/keyup)
• assymetric, (e.g. 2ms on keydown, 5ms on keyup)
• eager (0ms delay on event change, then 5ms before next event

for pk/g/pr/pc

• pk = per key
• pr/pc could be per row/column. uses less memory than per key
• global = a single timer for the whole keyboard

DEBOUNCE_ALGO = manual seems icky to me; I'd prefer DEBOUNCE_TYPE = custom or just DEBOUNCE = custom (which continues the meaning of "custom" used for other settings)

Sure, DEBOUNCE_TYPE = custom sounds the best to me. I'm hesitant to use DEBOUNCE = custom since DEBOUNCE is used in the code a whole bunch already.

#define "matrix.h" shouldn't be in debounce.h (I'm not a fan of nested includes.) The debounce*.c files already include it manually anyway.

Right - i had a commit somewhere where i #defined a new constant/type for row/column count, to remove all the scan_row/scan_col and replace with scan_direction, but scrapped it. removing it now...

[alex-ong]

update: working on a non-split. Just need to test on a split keyboard when i get into work....

[drashna]

Some of the docs are inconsistent with the actual code

[drashna]

Also, testing on my Iris (split, via split common), and this appears to work just fine.

[alex-ong]

Some of the docs are inconsistent with the actual code

FIxed it up, should be good to go.

[drashna]

@alex-ong or @pelrun could this change be applied to boards like the Ergodox EZ?
(as in, could one you see about adding it?)

I'm not exactly sure how.

[alex-ong]

I've added an eager_pr algorithm (same as eager_pk, but 10~ counters instead of 60+). Next step is to integrate with ergodox. I'm gonna do it as a new request...

See #5496. After thats done i can patch ErgoDox_EZ but i don't have one so someone will have to be my guinea pig :)

Also see #5498 - this enables someone with ergodox_ez to test the new debounce algorithm / api. I think that it should increase scanrate quite a bit, since it debounces using 14 counters instead of 80+, and updating those counters is can actually be quite time consuming.

For the EZ, It might also be worth trying the "old" algorithm, which just has as global counter (1 counter for entire keyboard), since that would increase scanrate at the expense of keys being out of order.

[drashna]

Awesome, thanks!

And as for guinea pig, that's what Drashna CI is for, :D