Process combos earlier & overlapping combos

[sevanteri]

General Combo improvements.

Description

These changes move Combo processing before TMK's tapping. Before Combos were not really usable with ModTaps (#8003) because the tapping code was interfering and made both the Combo and ModTap unusable.

Combos now don't fire immediately when a complete combo is detected. Instead the whole COMBO_TERM (now defaults to 50) is now waited. This allows overlapping chords to be defined. So it is now possible to define combos that have keys that overlap with other combos and have only one of the combos trigger. For example a combo with keys A and B, and a combo with keys A, B and C. Before this PR both combos would trigger when all three keys are pressed, now only the latter combo triggers.

This PR also enables advanced keycodes as the result of a combo. Before only basic keycodes could be used. Now you can have Mod-Taps, Layer-Taps, One Shot Modifiers, RESET and many others.

There are also new options for setting COMBO_TERM per combo, and combos resulting to modifiers needing to be held for the whole COMBO_TERM (little reasons to just tap modifiers), and some other, more advanced options too. Check the docs if your interested.

This PR possibly would help with #8212 and definitely would solve #8003.

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

• [Feature Request] allow combos with dual function keys #8003

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.
• 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).

[sevanteri]

Just realized I didn't test this with the default COMBO_TERM (TAPPING_TERM). Most likely something is not going to work as supposed to.

[sevanteri]

Huh, nothing broke with default COMBO_TERM.

[sevanteri]

Moving the processing to keyboard.c wasn't actually useful at all.

But I got Action Combos working!! Not sure if it's actually a good implementation but seems to work fine. Also it might be completely useless because of process_combo_event. :P

[sevanteri]

There might be a bug somewhere. My combos get disabled completely sometimes and I have to re-plug the keyboard to get them back working.

Edit: Just I posted this something new happens... They do sometimes come back without resetting.

Edit 2: Okay. Pressing a LayerTap key and a combo key fast is the problem. While the LT is still waiting for the end of TAPPING_TERM or an interrupting key, the combo key from layer 1 is registered as a combo key down. Then the LT kicks in and the combo key from layer 1 is never unregistered as the key released is now a key from layer 2.

[drashna]

@germ since you use the combo feature extensively, would you mind taking a look and maybe testing this, if you have the time?

[germ]

I don't have time to fix the bug, but I have verified that it exists. @sevanteri My combos get disabled due to that bug.

When you have a fix give me a tag and I'll give it a test on my GergoPlex/Ginny builds! 😄

[sevanteri]

@germ I thought the latest commit should have fixed it. But if not, I'll get back to you. :P

[germ]

Solid. I just pulled down the most recent a few minutes before that comment. #8262 has the gergoplex keymaps in it (which use combos heavily) if that can help you track down what's messing it up :)

[sevanteri]

I honestly can't reproduce the bug with the latest commit.

I took a peek at your Gergoplex config, @germ, and all I got from it that you don't use permissive hold or ignore mod tap interrupts. I tried playing around with those options but neither really affect the bug. I also tried changing combo term, yet again without effect.

[sevanteri]

Hmmm... got some completely different bug now. Again with a LayerTap key and a combo key. Now the key behind the layer got stuck and started repeating and after that the whole keyboard was stuck until I pressed the LayerTap+combokey again. Some sort of timing thing again.

[sevanteri]

Trying to fix the latest bug, I just found that it only happens on my Ergodox Infinity but not on my Gergo...

AND the only keys I got the bug to appear was Play, VolUp and VolDown, all in the same column, behind one layer. None of the other keys behind the layer were bugged.

[sevanteri]

Okay the Ergodox bug is not related to combos in any way. It happens with combos disabled.
Yippee...

[sevanteri]

@germ could you test with the latest commit. I haven't had stuck combos for a while but I did encounter stuck mods from ModTaps that were also part of chords.

I also played around with mods from combos and pressing combos when the mod combo is pressed. Currently pressing other combos don't work correctly. The timer is not reset accordingly because there are combo keys still being held.

[germ]

Sure, I'llgive it a shot later tonight

[sevanteri]

I'll clarify my earlier comment about stuck modifiers: I did also fix it... I did mention it in the commit message but I should have included that in the previous comment too.

[cari66ean]

Still found some weird behaviors here. Usually it does not matter which key you hit first in the combo. When I'm using the LT()s within the combo in combination with basic KC_ keys however, I need to hit the LT() slightly before the KC_ keys. Every time I hit the KC_ key before the LT() I simply get the output of the KC_ key and the combo is ignored.

[sevanteri]

@cari66ean Did you remember to define COMBO_ALLOW_ACTION_KEYS? I can only reproduce that behavior if it is undefined.

But... maybe that is a bug too. Even though COMBO_ALLOW_ACTION_KEYS is not defined the combos shouldn't interfere the normal behavior.

I've also been thinking about if this would need its own name instead of the old COMBO_ALLOW_ACTION_KEYS as it isn't accurate for this. Maybe COMBO_ALLOW_TAPPING_KEYS? Any thoughts on this @drashna?

[cari66ean]

@sevanteri weird, I had COMBO_ALLOW_ACTION_KEYS defined. I thought I had to have it defined for keys like LT() to even work in combos?!

[sevanteri]

@cari66ean Ok, if it was defined then there really is a bug somewhere... Maybe I'll just start looking how to make tests for this next.

[sevanteri]

I've been using my Ergodox Infinity and my Gergo with this branch for a while now and I have not encountered any bugs. Could someone else come and test this too?

@cari66ean The bug you described is exactly how QMK behaves on the master branch and I can't reproduce it with this branch. Are you sure you merged this branch? (Sorry, not trying to be rude, just genuinely wondering...)

@germ Have you had time testing this? If yes, have you encountered any issues?

[germ]

Sorry, I've been busy attempting to get overlapping combos working. It's a mess and I'm on my third rewrite at the moment. It's over in germ/qmk_firmware/overlapping-combos (go one commit back, I reverted).

If you're looking for some combo shenanigans to hack on, could you try hacking on that? Once that's clear having a few hundred combos on a board is possible. Right now given combos ASD and AS, if ASD is stroked both fire. :(

[germ]

Glad to see this on track to be merged in! Maybe I can get my boards moved over to master now 😅

[leon-anavi]

@tzarc thank you for the heads-up. Although I'm sorry to hear the news, I understand the technical limitation of Microchip ATtiny85 and the benefits this PR creates for QMK in long term. In the coming days I will update the user's guide of ANAVI Macro Pad 2 to point out particular QMK versions (commits) where it is still supported.

Thanks, Leon

[sevanteri]

@tzarc @leon-anavi I wonder if defining EXTRA_SHORT_COMBOS would help, as it can save a little bit of space.

https://github.com/sevanteri/qmk_firmware/blob/master/docs/feature_combo.md#buffer-and-state-sizes

EDIT: Tried it out, no dice :(
Only removes 10 bytes which is nowhere near enough.

[tzarc]

@tzarc thank you for the heads-up. Although I'm sorry to hear the news, I understand the technical limitation of Microchip ATtiny85 and the benefits this PR creates for QMK in long term. In the coming days I will update the user's guide of ANAVI Macro Pad 2 to point out particular QMK versions (commits) where it is still supported.

Thanks, Leon

We're more than happy to tag a known-good revision on your behalf, FWIW. If there's the possibillty of reintegrating the board, great! However when I looked at what was compiled in, there wasn't a lot that looked promising:

00001048 T action_exec
00000922 t process_record
00000724 T main
00000632 T usbPoll
00000346 t action_for_keycode
00000336 t apply_combos
00000186 T matrix_scan
00000172 t usbFunctionDescriptor
00000168 T eeconfig_init_quantum
00000158 t dump_key_buffer
00000148 T vusb_transfer_keyboard
00000090 t get_record_keycode
00000084 T send_keyboard.lto_priv.17
00000084 t drop_combo_from_buffer
00000082 T __vector_12
00000078 T bootloader_jump_after_watchdog_reset
00000078 t backlight_step
00000076 T __vector_10
00000064 t keyboard_hid_report
00000046 T send_keyboard_report
00000040 t biton
00000036 T eeprom_update_byte
00000036 T backlight_set
00000034 t usbConfigurationDescriptor
00000034 T __mulhi3
00000032 T eeprom_read_block
00000030 T timer_elapsed
00000026 t usbStringDescriptorProduct
00000026 t clear_combos
00000024 T timer_read
00000024 T host_keyboard_leds
00000022 T __do_copy_data
00000018 t usbDeviceDescriptor
00000016 T eeprom_update_dword
00000016 T eeprom_read_byte
00000016 T __do_clear_bss
00000014 t usbStringDescriptorManufacturer
00000010 T eeprom_read_word
00000008 T eeprom_update_word
00000008 T eeconfig_update_backlight
00000006 t test_combo
00000006 T keyboard_leds.lto_priv.16
00000004 t usbStringDescriptorZero
00000004 T sendchar
00000004 t keymaps
00000002 T send_system.lto_priv.19
00000002 T send_mouse.lto_priv.18
00000002 T send_consumer.lto_priv.20

When I checked, we'd need to find 600-odd bytes of stuff to remove... perhaps some aggressive tuning could be done to get it to fit but I'm not sure that'd be much use once the next PR from someone gets made. :(

[sevanteri]

Fixed some issues with combo_disable in PR #13988.
No more hanging keys.

[janniks]

Does this currently work (home-row mods+combos)? I am trying here, but can't seem to get it to work (combos are ignored if overlapped with ModTap). Any help would be greatly appreciated.

[roubaobaozi]

Does this currently work (home-row mods+combos)? I am trying here, but can't seem to get it to work (combos are ignored if overlapped with ModTap). Any help would be greatly appreciated.

Your combos aren't written correctly :)

Your keymap says:
MT(MOD_LSFT, KC_A), MT(MOD_LCTL, KC_S)

Your combo says:
const uint16_t PROGMEM combo_as[] = {KC_A, KC_S, COMBO_END};

It needs to say:
const uint16_t PROGMEM combo_as[] = {MT(MOD_LSFT, KC_A), MT(MOD_LCTL, KC_S), COMBO_END};

[janniks]

Thank you soooo much! I thought about this a couple of times, but couldn't figure out how to correctly write it from the docs.
Will try out now! 🚀 EDIT: SOLVED