[Feature Request] Per-key tapping term API simplification

[joukewitteveen]

Working on #16394, I noted two things about per-key tapping terms:

1. get_tapping_term does not see much use yet and none of the usage depends on the second argument (of keyrecord_t type).
2. TAPPING_TERM_PER_KEY has little effect on firmware size/speed and makes qmk code more convoluted.

It is possible that these observations generalize to more than just per-key tapping terms. Likely, they apply also to per key

• retro-tapping
• mod-tap interrupt
• tapping force-hold
• permissive-hold

Feature Request Type

• Core functionality
• Add-on hardware support (eg. audio, RGB, OLED screen, etc.)
• Alteration (enhancement/optimization) of existing feature(s)
• New behavior

Description

I suggest removing the second argument of the get_… functions, leading to an API with only one argument: the keycode. Additionally, while the documentation states that there is no need for separate functions at quantum/keyboard/user level, having a quantum level wrapper would make it easier to ignore any user implementations of these functions. Currently, we cannot simply put #define get_tapping_term(keycode) (TAPPING_TERM) in any of our code paths without forcing the user to place guards around their get_tapping_term implementations.

I also suggest to reconsider whether we need the TAPPING_TERM_PER_KEY guard at all. Disabling the aforementioned per-key features yield little to no benefit to the built firmware. If there are conceptual reasons to have these flags, it would be nice if they could be dealt with at a lower level, so that the call sites can just use, say, get_tapping_term_quantum(…) without having to resort to #ifdef blocks. Essentially, this is the same point as mentioned in the previous paragraph.

Outlook

If there is a desire to improve on either or both these points, I can prepare a merge request. Rather than doing so right away, I wanted to hear what more experienced QMK developers think of this.

[drashna]

i've actually seen a number of people implement the record part, mostly for the row/column information. Which is actually why I added it in the first place. However, i've mostly see it outside of the main repo.

[joukewitteveen]

Ah, in that case it should not be dropped. Do note that in some places (at least tap-dance), an empty record is provided since no real record is (easily) available. Are there other ways to provide row/column information that may be better suited than a record?

This does leave the second point: would it make sense to drop TAPPING_TERM_PER_KEY and always use get_tapping_term, or to add a wrapper that either expands to get_tapping_term or TAPPING_TERM, depending on TAPPING_TERM_PER_KEY? (and likewise for similar constructs)

[drashna]

yeah, I would rather it's not dropped.

That said, on develop, tzarc has made it much easier to create empty records, so there is that. But it may be possible to record the keyrecord to be passed on, but my concern is bloat.

As for dropping the "per key" option and make it default, my concern there is also bloat, as a majority of the boards are AVR, and a lot of them are already close to the limit for firmware size.

[joukewitteveen]

Wouldn't the last concern also be solved with a single access point (called get_tapping_term_quantum in the issue description), to prevent repeating the following pattern?

#ifdef TAPPING_TERM_PER_KEY
get_tapping_term(…)
#else
TAPPING_TERM
#endif

[sigprof]

Actually get_tapping_term() always exists at least as a weak function even when TAPPING_TERM_PER_KEY is not defined (the only thing which removes it completely is NO_ACTION_TAPPING). The downside of using get_tapping_term() directly is that even in non-TAPPING_TERM_PER_KEY configs it won't be optimized to a single variable access (g_tapping_term) without also enabling LTO; and the calculation of the keyrecord_t *record argument also cannot be optimized out in many cases, because it involves function calls for keymap lookup.

[joukewitteveen]

Actually get_tapping_term() always exists at least as a weak function even when TAPPING_TERM_PER_KEY is not defined (the only thing which removes it completely is NO_ACTION_TAPPING). The downside of using get_tapping_term() directly is that even in non-TAPPING_TERM_PER_KEY configs it won't be optimized to a single variable access (g_tapping_term) without also enabling LTO; and the calculation of the keyrecord_t *record argument also cannot be optimized out in many cases, because it involves function calls for keymap lookup.

The linked MR realizes a best of both worlds scenario through a new GET_TAPPING_TERM macro. Using this instead of get_tapping_term always gives you the correct and fully optimized result. It is safe to use everywhere in place of TAPPING_TERM and ensures correct behavior in case either a dynamic tapping term or per-key tapping term is enabled.

[joukewitteveen]

Closed via #16681.