Refactor combo code for consistent triggering #16006
Conversation
- Resolves issue qmk#15911. Combos now trigger if and only if 1. all combo keys pressed within combo term, and 2. no ennumerated exceptions apply (interruption of combo sequence, tap/hold requirements, higher priority combo triggers, buffer overflow) - Adds new combo features: detailed combo events, and contiguous/noncontiguous combos - Breaking changes: - EXTRA_SMALL_COMBOS now limited to length 4 rather than 6 - If COMBO_COUNT not defined but user has many combos, they must define MANY_COMBOS - no COMBO_NO_TIMER or COMBO_STRICT_TIMER options - For overlapping combos, priority is determined by combo index: lower index is higher priority. Ripe combos only wait for high priority combos that involve a superset of trigger keys - Combos now trigger in sequence with their first key press rather than last
|
Are there instructions on how to adapt pre-existing combos keymap code for this new PR? Compiling my polilla keymap with this PR produces the following errors: Although my Dactyl Manuform keymap compiles fine so this might just be an isolated problem with my specific polilla keymap. EDIT: I also tried to run |
Sometimes the compiler can't figure out that a pointer is never used unitialized, so we help it out.
|
Thanks for checking it out. I have no idea about the polilla error, but if you point me to the keymap I can try to check it out. I'm also confused about why the compiler is only sporadically able to figure out that this combo pointer is never used uninitialized, but I added a redundant initialization to calm it down. Edit: Which is to say, there shouldn't be any changes needed to existing configurations, beyond those mentioned in "Breaking changes" at the top. |
|
I can confirm that 018db44 solves the compiler errors related to PS: My polilla keymap can be found here: https://github.com/precondition/polilla-keymap |
|
I'm testing this PR and I find that I struggle a lot with any combos involving mostly mod-taps. I get a ton of modifier misfires, it's not very usable. MWE of combos that pose many problems: enum combo_events {
BSPCT_THE,
BSPCTS_THIS,
BSPCTA_THAT,
// This must be the last item in the enum.
// This is used to automatically update the combo count.
COMBO_LENGTH
};
uint16_t COMBO_LEN = COMBO_LENGTH;
const uint16_t PROGMEM BSPC_T_COMBO[] = {KC_BSPC, LCTL_T(KC_T), COMBO_END};
const uint16_t PROGMEM BSPC_T_S_COMBO[] = {KC_BSPC, LCTL_T(KC_T), LSFT_T(KC_S), COMBO_END};
const uint16_t PROGMEM BSPC_T_A_COMBO[] = {KC_BSPC, LCTL_T(KC_T), LGUI_T(KC_A), COMBO_END};
combo_t key_combos[] = {
[BSPCT_THE] = COMBO_ACTION(BSPC_T_COMBO),
[BSPCTS_THIS] = COMBO_ACTION(BSPC_T_S_COMBO),
[BSPCTA_THAT] = COMBO_ACTION(BSPC_T_A_COMBO),
}
void process_combo_event(uint16_t combo_index, bool pressed) {
switch(combo_index) {
case BSPCT_THE:
if (pressed) {
SEND_STRING("the");
}
break;
case BSPCTS_THIS:
if (pressed) {
SEND_STRING("this");
}
break;
case BSPCTA_THAT:
if (pressed) {
SEND_STRING("that");
}
break;
}
}I find it nearly impossible to trigger Backspace+T+A without accidentally opening the Tor browser (mapped to GUI+T on my computer) and trying to write "this" requires very, very fast chording of Backspace, T and S in order to actually produce "this" instead of "THE".
|
|
Thanks for the feedback. Can you try reordering the combos, to put BSPCT_THE below the others (in the enum, i.e., with a higher index)? In this rewrite, we only wait for longer nesting combos if they are higher priority (lower index). |
- now higher-index combos are preferred over lower-index combos, rather than vice versa - conflicts are resolved on each key expiration so combos that are begun earlier can be preferred - get_combo_is_contiguous replaced with get_combo_interrupted, that additionally provides the interrupting keyrecord so that finer-grained contiguity decisions can be made
|
I've edited so that higher index combos have higher priority, since on reflection this seemed more natural. I expect the example you were referring to should now work as expected without reordering it. It's possible that you'll want to bump up the COMBO_TERM a bit, particularly for longer combos, because the timing is now more similar to the former COMBO_STRICT_TIMER option |
|
Thank you for your contribution! |
|
It would be incredibly useful to add tests for combos. That way, edge cases and the like can be better found and identified. You can see some in |
|
Sure, thanks for the pointers. I can take a stab at adding some tests when I find time |
|
Thank you for your contribution! |
github-actions
bot
added
the
stale
|
Tested this branch bit and most of my combos don't fire at all. And I think it's because of the longer chords only working if the index number is higher. I have my combos ordered by their "categories" like "left top row", "left home row", "vertical combos" , "random", and so on. I'd rather not start ordering them for some implementation detail. And the current overlap check just drops combos that have overlapping keys but less keys in the chord. Or if there's the same amount of keys, it gives "a higher priority" for the combo with the higher index. Not for any specific reason, I just happened to code it like that. There shouldn't exists combos with exactly the same keys. I like the idea of the (non-)contiguous chords. The current implementation works as it does because someone requested that multiple (non-overlapping) combos should be possible within the same combo term. But it indeed does feel inconsistent with keys that are not part of any combo, which I had missed completely because I have combos on pretty much every key. 😅 The linked list stuff is also intriguing. |
|
Thanks for testing! The higher/lower index is only relevant in two situations - you have actually triggered two combos, and it needs to decide between them, or you have an actually nesting combo where one combo's keys are a proper subset of the other combo. Glancing at your combos.def, I wouldn't think this would be the issue for most of them. If it is really an issue, I'm happy for suggestions about a better way of doing this. In principle, we could have a separate priority parameter for each combo, but this adds a lot of overhead for what I expect is probably not much benefit in practice. Is no combo firing at all, or do you get something unintended firing? If nothing is firing, maybe try bumping up the combo term? There should be fewer misfires with a higher combo term due to the default contiguity constraint. |
I think this is the case that QMK does handle already by not firing the combo that came first (lower index). And I think it is a configuration error by the user if there's two combos with the same keys (not counting the "must press in order" combos).
In this case I don't think the index should matter. It's the longer chord that is being pressed and should win against the subset chord. Currently the subset is dropped immediately which results in the issues with tap and hold combos you described. I think that just holding onto the complete buffer of "ripe" combos until the end of
Some combos fired just as expected, but can't exactly say the reason why most of them didn't. I didn't try with a small amount of combos. |
|
Aaand I just figured out what you actually meant with the first case... Let there be combos AB and BC. We press A, B, and C. Which combo indeed should fire...? Currently QMK does indeed drop the first combo in the array and fires the second one, giving the second one a higher priority. |
|
If I recall correctly, in this PR, given combos AB and BC, which are in the default "contiguous only" setting, you have the following possibilities:
Personally I think this is good behavior that produces reliable results even with fast typing. Without AB/BC set to allow noncontiguous presses, then 3+4 above will resolve to whichever has the higher index. Noncontiguous combos are somewhat more advanced/specialized combos and I think users who enable them can be trusted to think through the consequences, but resolving based on index allows for reasonably customizable behavior. |
github-actions
bot
removed
the
stale
|
Okay, I remembered why I didn't do "prefer superset combos even if they have lower priority as measured by index." It's because if I have combos ABC, CD, and BC, listed in that order, it's unclear what I'm supposed to do when ABCD is pressed together. ABC is better than BC (because superset), but BC is better than CD is better than ABC by the ordering. I just used the index because it gives a simple total ordering and I didn't think it was a big deal to list combos in order of priority, but it sounds like that was a faulty assumption. By the way, here's an example of why "just don't activate any combos" is probably not the right solution. Suppose I have combo AB producing shift, and combo BC producing some other character. If I press AB, followed by C, I want to get shift C, and it would be annoying if I get a very different result as a result of pressing C too soon after pressing AB. This example suggests preferring combos that are initiated first, but a tiebreaker would still be needed (e.g. what if the second combo was AC instead of BC). |
|
Thank you for your contribution! |
github-actions
bot
added
the
stale
|
Thank you for your contribution! |
This comment was marked as outdated.
This comment was marked as outdated.
tap/hold requirements, higher priority combo triggers, buffer
overflow)
contiguous/noncontiguous combos
define MANY_COMBOS
lower index is higher priorityhigher index is higher priority. Ripe combos only wait for highpriority combos that involve a superset of trigger keys
Although the code should work as-is without issues, it is not intended to be merged directly. Instead I'd like this to be a starting point for a discussion about a potential broader refactoring (see below). As such, some smaller features of the existing combo code have not been re-implemented in this PR, and I have not yet bothered with updating the documentation. However, if the QMK Collaborators prefer to not move forward with a larger refactoring, I'm happy to make the small changes needed so that this edit can preserve as much existing functionality as possible while also fixing #15911. But in any case, there's a lot happening in this PR and it presumably needs some discussion.
Description
This is a complete rewrite of the combo code. Previously, combos had inconsistent triggering behavior, as described in #15911
With this PR, we now have the following combo guarantees:
A combo will trigger if and only if
Combos and key events are released from the buffer in the appropriate order, without exception.
In particular, we get rid of the COMBO_STRICT_TIMER option - instead, everything is "strict", but it actually works as expected.
As described in #15911, one of the inconsistencies in triggering was how to handle an key press interrupting a combo. If a+b is a combo and the sequence a,x,b is pressed, existing code triggers the combo if x belongs to some other combo, and doesn't trigger if x belongs to no combo. With this PR, combos are "contiguous" by default, so the sequence a,x,b would not trigger the combo a+b, regardless of whether or not x belongs to an unrelated combo. But there is a user-definable override to allow per-combo contiguous or non-contiguous behavior (define COMBO_CONTIGUOUS_PER_COMBO and implement the function get_combo_interrupted). Non-contiguous triggering behavior is useful for things like mods where you might want to press multiple combos simultaneously, but is probably not otherwise the intended behavior.
Combo overlap handling has also been changed. The changes and their motivation are mostly described in #15911. In order to give users a way to express preferences for how conflicting combos should be handled, we always prefer lower indexed combos over higher indexed combos - users can order their combos to achieve the desired results. But for nested combos, it is sometimes still necessary to wait for a larger combo to ripen - this is handled appropriately.
Implementation details
Instead of keeping a bit array of pressed keys for each combo, we use such a bit array only for the currently active combos. For the rest, combo state consists of an index to the next combo in a linked list, with a few bits left over to count the number of keys currently pressed. Combos are organized into several disjoint linked lists: one with all the inactive combos, one with combos about to fire, one with active combos, etc. In addition, there is a linked list for every queued key press in the buffer, containing all the combos for which that key press is the first to occur. This makes it straightforward to keep track of different expiration times for different combos, without having to use much memory or processing time.
Broader refactor
If broader breaking changes can be considered, I think there is still more room for improvement in how combos work. In particular, it would be nice if instead of re-implementing tap/hold logic for combos, we could just re-use the existing tap-hold code. In addition to simplifying (and speeding up) the processing of combos, this would allow using options like PERMISSIVE_HOLD for combos. Of course this wouldn't work for combo actions (with keycode=0), but there is a sense in which those are also redundant - we can always define custom keycodes and handle in process_record_user, albeit later in the pipeline.
The recent PR #15083 added the COMBO_MUST_PRESS_IN_ORDER_PER_COMBO option. Instead of implementing that, I propose the more general COMBO_DETAILED_EVENTS_PER_COMBO option implemented here. Users can implement the function get_combo_needs_details to specify which combos should receive detailed event implementation. Those combos will receive all the triggering keyrecords in process_combo_detailed_press (instead of process_combo_event), including the event times and matrix positions. This is a much more general interface - e.g., the user could decide to trigger based on a particular key coming last and the other keys in arbitrary order, etc.
I also did not implement the per-combo disabling interface from #15083. With the implementation proposed in this PR, it would be possible to instead have users call a function (like disable_single_combo(combo_index)) when they want to switch off a particular combo - it can then be moved to a separate "disabled" linked list so that we don't have to touch it at all for ongoing combo processing. I'd be grateful for guidance about the preferred way forward for this feature.
Types of Changes
Issues Fixed or Closed by This PR
Checklist