Custom Tapping Term per key #5009

drashna · 2019-01-30T21:33:34Z

Description

Allow a user customizable tapping term, that is configurable, per key. In theory.

However, it doesn't work correctly. The default value works, but I can't seem to get it to properly work using the keycode, but does work if you specify the Row and Column of the key.

This is not ideal, at all. But I'm not sure sure how to fix this issue. So, I'm opening the pull request to get some input on this, and see if somebody can help me get this working "properly".

This isn't originally my code, but is fredizzimo and @danielo515

Types of changes

Issues Fixed or Closed by this PR

Increase the time required for a hold without affecting TAP_DANCE #2477

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document. (https://docs.qmk.fm/#/contributing)
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).

fauxpark · 2019-01-30T21:51:36Z

keyevent_t only provides row, column and timing information:

// tmk_core/common/keyboard.h
typedef struct {
    keypos_t key;
    bool     pressed;
    uint16_t time;
} keyevent_t;

I see someone else has gone down this path before, using keymap_key_to_keycode() to conjure up the keycode from the position:

https://github.com/qmk/qmk_firmware/blob/master/keyboards/ergodox_infinity/keymaps/gordon/keymap.c#L21

EDIT: Whoops, this is pretty much the same code from the linked issue...

drashna · 2019-01-30T22:18:42Z

keyevent_t only provides row, column and timing information:

In theory, that's all we need.

qmk_firmware/quantum/keymap_common.c

Lines 209 to 213 in 3c0c432

    
           uint16_t keymap_key_to_keycode(uint8_t layer, keypos_t key) 
        
           { 
        
               // Read entire word (16bits) 
        
               return pgm_read_word(&keymaps[(layer)][(key.row)][(key.col)]); 
        
           }

In, fact, all we really need is the keypos_t structure. We could skip everything else.

But it doesn't seem to work properly.

fauxpark · 2019-01-30T22:24:22Z

This probably isn't related, but I noticed layer_switch_get_layer returns an int8_t instead of uint8_t:

qmk_firmware/tmk_core/common/action_layer.h

Line 100 in 1d49f76

int8_t layer_switch_get_layer(keypos_t key);

Presumably everywhere else is uint8_t, so I doubt this is the root cause.

drashna · 2019-01-30T22:39:09Z

Yeah, there are a number of cases where this is the case. But yeah, is shouldn't matter. Feel free to correct it though. :)

fauxpark · 2019-01-30T23:10:08Z

Hm. Another thought: should event->key not be event.key? Neither event nor key are declared as pointers, so (as far as I understand the arrow operator) it does not need to be dereferenced.

drashna · 2019-01-30T23:44:07Z

I've tried:

uint16_t get_tapping_term(keyevent_t event) {
  switch (keymap_key_to_keycode(layer_switch_get_layer(event.key), event.key)) {

and

uint16_t get_tapping_term(keyevent_t* event) {
  switch (keymap_key_to_keycode(layer_switch_get_layer(event->key), event->key)) {

And the correct versions of the define. Neither way works.

I know the code change itself works, since, if you change the default, it absolutely has an affect. But .... it's some issue with detecting the keycodes.

fauxpark · 2019-01-30T23:59:44Z

process_tapping() seems to be able to grab it seemingly perfectly well:

qmk_firmware/tmk_core/common/action_tapping.c

Lines 89 to 91 in c2080d3

    
           if (WITHIN_TAPPING_TERM(event)) { 
        
               if (tapping_key.tap.count == 0) { 
        
                   if (IS_TAPPING_KEY(event.key) && !event.pressed) {

I notice that IS_TAPPING_KEY(k) surrounds k with parentheses (I'm not well versed enough in C to understand the significance of this, if any):

qmk_firmware/tmk_core/common/action_tapping.c

Line 20 in c2080d3

    
           #define IS_TAPPING_KEY(k)       (IS_TAPPING() && KEYEQ(tapping_key.event.key, (k)))

drashna · 2019-01-31T00:43:42Z

Tried that, doesn't make a difference. z

#define WITHIN_TAPPING_TERM(e)  (TIMER_DIFF_16(e.time, tapping_key.event.time) < get_tapping_term((e)))

drashna · 2019-01-31T01:23:20Z

Okay, I added some debugging to it, and it looks like the issue is that the column and row are not being outputted correctly.

  xprintf("get_tapping_term (main): ");
  xprintf("col: %i, row: %\n", event.key.col, event.key.row);

is outputting

    get_tapping_term (main) col: 255, row: 255
    get_tapping_term (main) col: 255, row: 255
    get_tapping_term (main) col: 255, row: 255

Which means that it's not getting a valid structure at the time that this is occurring. :(

fauxpark · 2019-01-31T01:27:31Z

Is that xprintf truncated for brevity? Cause that does not look right...

drashna · 2019-01-31T01:39:53Z

It was bad editing :D

But yeah, looks like if I want to fix this, I'll have to convert WITHIN_TAPPING_TERM to an actual function. And hope that works.

fauxpark · 2019-01-31T01:45:49Z

KEYEQ is also doing some funky parenthesis stuff:

qmk_firmware/tmk_core/common/keyboard.h

Line 43 in c2080d3

    
           #define KEYEQ(keya, keyb)       ((keya).row == (keyb).row && (keya).col == (keyb).col)

Sorry I can't be of more help right now, I don't have a QMK board in front of me...

drashna · 2019-01-31T02:01:25Z

Even changing the funky parenthesis stuff didn't affect anything. Using a pointer or not didn't make a difference.

Basically, it's not passing on the information correctly, from what I can tell

fauxpark · 2019-01-31T02:05:39Z

~~What if you pass WITHIN_TAPPING_TERM event.key instead? As you said, that's all that's really needed here.~~

Actually nope, it requires event.time.

drashna · 2019-01-31T02:31:47Z

Got it! e is wrong. Need to pass tapping_key.event

drashna · 2019-01-31T02:49:03Z

@fauxpark I tested it, and then to confirm, I dug and dug.

The issue is that "e" is actually "TICK". Which is this:

qmk_firmware/tmk_core/common/keyboard.h

Lines 54 to 58 in 9f63cd0

    
           #define TICK                    (keyevent_t){           \ 
        
               .key = (keypos_t){ .row = 255, .col = 255 },           \ 
        
               .pressed = false,                                   \ 
        
               .time = (timer_read() | 1)                          \ 
        
           }

This is why I was seeing 255 for both the row and column, above.

Thank you for trying to help. At the very lest, it kept me digging into this, for a proper solution. And it was found, so yay!

drashna · 2019-01-31T02:57:24Z

Now, I have to figure out this:
https://github.com/qmk/qmk_firmware/blob/30bf35b57a0c829ca2385680d9530c829baac2c1/tmk_core/common/action_tapping.c#L108-L121

Specifically, I need to replace #if TAPPING_TERM >= 500 || defined PERMISSIVE_HOLD with something that checks get_tapping_term and see if that's higher than 500.

But I'm .... done for today. Tomorrow's drashna gets to worry about that

fauxpark · 2019-01-31T02:57:38Z

Argh, I saw that and didn't even register it! 😆

Looks like the chain goes from all the way up in keyboard_task() -> action_exec() -> action_tapping_process() -> process_tapping(). I got confused by the second call to process_tapping() with all the buffer stuff.

drashna · 2019-01-31T02:59:23Z

LOL, yeah. But hey, i learned some more QMK, so it's all worth it!

drashna · 2019-01-31T03:12:52Z

I lied, tomorrow's drashna is off the hook.

Changed one of the functions to check if the tapping term is 500ms or greater (based on the keycode, not the define)

Co-Authored-By: drashna <drashna@live.com>

Since we can't be sure that tapping term is actually 500

Clean up function so that users don't need to call the event function, and instead only check the keycode

ezuk · 2019-04-05T19:42:24Z

👍 from me on the docs!

jackhumbert · 2019-04-05T19:47:28Z

Thanks :)

* Add customizable tapping terms * Add Documentation * Fix function * Fixes * It's not a pointer * Add debugging output * Update documentation to be at least vaguely accurate * Use `get_tapping_term(tapping_key.event)` instead `e` doesn't include column and row information, properly. It registers as 255, regardless of the actual keypress. However `tapping_key.event` actually gives the correct column and row information. It appears be the correct structure to use. In fact, it looks like the issue is that `e` is actually the "TICK" structure, as defined in keyboard.h * Use variable tapping term value rather than define * Silly drashna - tapping_key.event, not event * add get_event_keycode() function * Fix typo Co-Authored-By: drashna <drashna@live.com> * Remove post_process_record_quantum since it's the wrong PR * Update quantum/quantum.c Co-Authored-By: drashna <drashna@live.com> * Better handle ifdef statement for permissive hold Since we can't be sure that tapping term is actually 500 * Update quantum.c comments based on feedback * Clean up get_tapping_term function Clean up function so that users don't need to call the event function, and instead only check the keycode * Add ability to run functionality on and off * Make ifdef's more compact

caused by #5009 (aka, me)

* Fix bug in PERMISSIVE_HOLD check caused by #5009 (aka, me) * Remove check for per key

* Fix bug in PERMISSIVE_HOLD check caused by qmk#5009 (aka, me) * Remove check for per key

drashna added enhancement in progress on hold core documentation labels Jan 30, 2019

drashna removed in progress on hold labels Jan 31, 2019

drashna mentioned this pull request Jan 31, 2019

Increase the time required for a hold without affecting TAP_DANCE #2477

Closed

drashna changed the title ~~[WIP] Custom Tapping Term per key~~ Custom Tapping Term per key Jan 31, 2019

drashna requested a review from mechmerlin January 31, 2019 03:54

drashna and others added 7 commits March 22, 2019 19:54

Remove post_process_record_quantum since it's the wrong PR

5c6544f

Update quantum/quantum.c

b477bc7

Co-Authored-By: drashna <drashna@live.com>

Better handle ifdef statement for permissive hold

bdaada6

Since we can't be sure that tapping term is actually 500

Update quantum.c comments based on feedback

487fb95

Clean up get_tapping_term function

af603ea

Clean up function so that users don't need to call the event function, and instead only check the keycode

Add ability to run functionality on and off

af8dc92

Make ifdef's more compact

45d6d25

drashna force-pushed the custom_tapping_term branch from f932771 to 45d6d25 Compare March 23, 2019 02:54

ezuk approved these changes Apr 5, 2019

View reviewed changes

jackhumbert approved these changes Apr 5, 2019

View reviewed changes

jackhumbert merged commit 5701b75 into qmk:master Apr 5, 2019

drashna deleted the custom_tapping_term branch April 5, 2019 20:03

drashna added a commit that referenced this pull request Jan 10, 2020

Fix bug in PERMISSIVE_HOLD check

5ba47a9

caused by #5009 (aka, me)

ridingqwerty pushed a commit that referenced this pull request Jan 17, 2020

Cleanup check for PERMISSIVE_HOLD (#7861)

1b0854f

* Fix bug in PERMISSIVE_HOLD check caused by #5009 (aka, me) * Remove check for per key

drashna added a commit to zsa/qmk_firmware that referenced this pull request Feb 12, 2020

Cleanup check for PERMISSIVE_HOLD (qmk#7861)

bacf512

* Fix bug in PERMISSIVE_HOLD check caused by qmk#5009 (aka, me) * Remove check for per key

fdidron pushed a commit to zsa/qmk_firmware that referenced this pull request Feb 14, 2020

Cleanup check for PERMISSIVE_HOLD (qmk#7861)

1245492

* Fix bug in PERMISSIVE_HOLD check caused by qmk#5009 (aka, me) * Remove check for per key

HokieGeek pushed a commit to HokieGeek/qmk_firmware that referenced this pull request Feb 21, 2020

Cleanup check for PERMISSIVE_HOLD (qmk#7861)

1b091a6

* Fix bug in PERMISSIVE_HOLD check caused by qmk#5009 (aka, me) * Remove check for per key

fdidron pushed a commit to zsa/qmk_firmware that referenced this pull request Feb 26, 2020

Cleanup check for PERMISSIVE_HOLD (qmk#7861)

e589084

* Fix bug in PERMISSIVE_HOLD check caused by qmk#5009 (aka, me) * Remove check for per key

kylekuj pushed a commit to kylekuj/qmk_firmware that referenced this pull request Apr 21, 2020

Cleanup check for PERMISSIVE_HOLD (qmk#7861)

c4d8bd0

* Fix bug in PERMISSIVE_HOLD check caused by qmk#5009 (aka, me) * Remove check for per key

BorisTestov pushed a commit to BorisTestov/qmk_firmware that referenced this pull request May 23, 2024

Cleanup check for PERMISSIVE_HOLD (qmk#7861)

74b045c

* Fix bug in PERMISSIVE_HOLD check caused by qmk#5009 (aka, me) * Remove check for per key

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Custom Tapping Term per key #5009

Custom Tapping Term per key #5009

drashna commented Jan 30, 2019 •

edited

Loading

fauxpark commented Jan 30, 2019 •

edited

Loading

drashna commented Jan 30, 2019

fauxpark commented Jan 30, 2019 •

edited

Loading

drashna commented Jan 30, 2019

fauxpark commented Jan 30, 2019 •

edited

Loading

drashna commented Jan 30, 2019

fauxpark commented Jan 30, 2019

drashna commented Jan 31, 2019

drashna commented Jan 31, 2019 •

edited

Loading

fauxpark commented Jan 31, 2019

drashna commented Jan 31, 2019

fauxpark commented Jan 31, 2019

drashna commented Jan 31, 2019

fauxpark commented Jan 31, 2019 •

edited

Loading

drashna commented Jan 31, 2019

drashna commented Jan 31, 2019

drashna commented Jan 31, 2019

fauxpark commented Jan 31, 2019

drashna commented Jan 31, 2019

drashna commented Jan 31, 2019

ezuk commented Apr 5, 2019

jackhumbert commented Apr 5, 2019

Custom Tapping Term per key #5009

Custom Tapping Term per key #5009

Conversation

drashna commented Jan 30, 2019 • edited Loading

Description

Types of changes

Issues Fixed or Closed by this PR

Checklist:

fauxpark commented Jan 30, 2019 • edited Loading

drashna commented Jan 30, 2019

fauxpark commented Jan 30, 2019 • edited Loading

drashna commented Jan 30, 2019

fauxpark commented Jan 30, 2019 • edited Loading

drashna commented Jan 30, 2019

fauxpark commented Jan 30, 2019

drashna commented Jan 31, 2019

drashna commented Jan 31, 2019 • edited Loading

fauxpark commented Jan 31, 2019

drashna commented Jan 31, 2019

fauxpark commented Jan 31, 2019

drashna commented Jan 31, 2019

fauxpark commented Jan 31, 2019 • edited Loading

drashna commented Jan 31, 2019

drashna commented Jan 31, 2019

drashna commented Jan 31, 2019

fauxpark commented Jan 31, 2019

drashna commented Jan 31, 2019

drashna commented Jan 31, 2019

ezuk commented Apr 5, 2019

jackhumbert commented Apr 5, 2019

drashna commented Jan 30, 2019 •

edited

Loading

fauxpark commented Jan 30, 2019 •

edited

Loading

fauxpark commented Jan 30, 2019 •

edited

Loading

fauxpark commented Jan 30, 2019 •

edited

Loading

drashna commented Jan 31, 2019 •

edited

Loading

fauxpark commented Jan 31, 2019 •

edited

Loading