Combos interfer with hold-tap (but only if Combos module is loaded after Holdtap module!)

[ald0o]

The issue

• load modules HoldTap and Combos in this order
• declare a hold-tap key with hold_preferred=True; tap_interrupted can be True or False (the bug will be triggered in both cases through different mechanisms). For instance:

HT = KC.MT(KC.ENT, KC.LSFT, prefer_hold=True)

• declare a chord using HT

Within both holdtap timeout and combo timeout, do: press HT, press A, release A, release HT:

Expected: the keyboard should send "A" (shifted A). Happening instead: a "a" (unshifted) followed by a new line (or a new line followed by a "a").

Proposed explanation

process_key method from the Combos module returns None for a key that belongs to a combo. Consequence: in class KMKKeyboard, method pre_process_key, the on_press handler is not called for HT. But this handler is supposed to register the fact that this holdtap key is being held (in array key_states in class HoldTap), so it does not happen (at least not until the combo is cancelled... which means too late!).

So, when we press A, because key_states is empty (or at least does not contain HT), nothing particular happens in the process_key method of class HoldTap. More precisely:

• if tap_interrupted is False for HT (i.e. the interruption should happen on press, that is now), there is no interruption and the hold behavior is not triggered (at least not before timeout)
• if tap_interrupted is True (i.e. the interruption should happen on release), the press of A is not saved in send_buffer, preventing the release of A to trigger the interruption (so, here too, HT remains a tap).

What about changing the module order?

Well, if Combos is loaded first, when Combos' process_key runs for A, the hypothetical current combo will be cancelled and the module will replay all the keys it had stored in this prospect, before returning. Hence, in particular, the on_press handler of HT will be executed before HoldTap's process_key, which will now do what it is supposed to do, since HoldTap module knows a hold-tap key is currently pressed.

Proposed fix

Easy one: write in the doc that Combos should be loaded first.

More failsafe one: add a priority attribute in every module and have the modules list sorted according to priority (Combos would have higher priority than HoldTap).

[xs5871]

Yes, this is known behavior and "technically" mentioned in the Combos docs. The order of modules is intentionally not enforced and it's up to the user to decide which module they want to give precedence. However: better documentation is always welcome, if that's something you'd like to contribute.

[adegorre]

Oops, I did not see the small remark in the middle of the Combos doc! Sorry for the ruckus.

In my defense, I realized my issue was related to Combos only at the very end of this lengthy debugging session, so I read and re-read ModTap documentation many timed but hardly skimmed that of Combos.

So probably rather than blinking lights in the doc of Combos module, the "fix" would be to replicate the warning in that of ModTap and Layers.

Since we are talking about improving the doc, may I suggest to make it more specific ?

In combos.md, after

You can define combos to listen to any valid KMK key, even internal or functional keys, like HoldTap. When using internal KMK keys, be aware that the order of modules matters.

insert:

In particular, some HoldTap keys (e.g. KC.MT and KC.LT keys with prefer_hold=True) are known not to work when Combos module is loaded after ModTap or Layers.

(Not that it would have changed anything in my case if this warning is only in combos.md... )

And in modtap.md, insert:

Remark that option prefer_hold=True (which is enabled by default) can be prevented from working correctly if the KC.MT key is part of a combo and the Combos module is loaded after ModTap. For typically, you want them loaded in the reverse order.

In layers.md (before "Checkout the ModTap doc"), insert:

In particular, the same caveat applies with respect to interaction with combos.

[xs5871]

No worries, your point still stands. The documentation should state that more clearly. Would you like to submit your suggestions as a pull request?

[glebsexy]

I have the same issue and changing the order doesn't help.

1. If combos are added to the list first, the combos would work as expected but the hold tap will ignore prefer_hold=True option.

2. If combos are added after layers and modtap, they will not work if they contain modtap keys, but the hold tap behavior would work correctly.

Is there currently a way to make combos work with holdtap that prefers hold? If not, any pointers to how I can fix that in code?

Edit: I don't know how, but it works now? I tried before and it didn't. Not sure what changed.

Here is my code:

keyboard = KMKKeyboard()
layers = Layers()
encoder_handler = EncoderHandler()
modtap = ModTap()
combos = Combos()
# put combos before layers & modtap to enable modtap in combos
keyboard.modules = [layers, modtap, combos, encoder_handler, MouseKeys()]
keyboard.extensions = [MediaKeys()]

# holdtap
HT_A_LCMD = KC.MT(KC.A, KC.LCMD)
HT_SP_L1 = KC.LT(L_SYM_NUM, KC.SPACE, prefer_hold=True, tap_time=20)

This way I can use the first holdtap key in a combo and the other key still respects the prefer hold setting.

[ald0o]

I ran into similar issues as you did (it's one of the reasons I did not submit the PR we talked about... this and lack of time). The amount of option combinations makes it really hard to be sure you have the proper fix!

Unitary tests are great, but there is currently no such test for hold-tap in combination with combos.

So what tests should be added? At least every combination of the following:

• combos loaded before hold-tap versus the opposite
• hold_preferred either True or False
• tap_interrupted either True or False
• all valid and relevant orderings of the 4 events: release of hold-tap key, press of another key, release of this key, timeout expiration
• the hold-tap or the other key or both belong to a macro

(that's already 144 tests, removing absurd or already covered cases!)

Other criteria ?

[xs5871]

I'm working on a possible solution that may not fully rely on module order. The problem right now is, that modules that buffer keys for later evaluation (i.e. holdtap and combos), don't have a proper mechanism to drop those buffered keys back into the key processing pipeline. We can either run the entire pipeline again (which in some instances would result in a quasi infinite loop), or not at all. Those two scenarios cover 90% of cases, but obviously not all.

[ald0o]

So you are saying that these ad hoc mechanism for replaying retained keys competes with the primary feed of events and creates bugs because their relative order is messed up?

Why not condensate everything to a single mechanism?

I am thinking about something like this: instead of having the main loop calling each module (in order) for each event, have it call only the first one and delegate to each module the responsibility to call the next one (so the module order still matters, sorry!). The benefit is to control when a module transmits an event (for instance it could be triggered only by a future input), without having to feed buffered events back to the main loop (for instance, for unused events from a cancelled combo). Doing so, you can make sure you preserve the initial order of events when you forward them to the next module.

One drawback, easily overcome: the modules don't handle events in real time anymore. The fix is to add a timestamp when events are created and read the timestamp when time needs to be measured (for checking hold-tap or macro timeouts). This would be similar to the handling of time in project KMonad (it was the natural thing to do in Haskell, but that should also work with Python!). The timestamps can also be used to ensure that the order of events transmitted to the next module is preserved.

[xs5871]

So you are saying that these ad hoc mechanism for replaying retained keys competes with the primary feed of events and creates bugs because their relative order is messed up?

No... that's.. actually not at all what's happening. Order is preserved, it's just that key modifications, through side-effects like layer changes, aren't always evaluted when they should.

Why not condensate everything to a single mechanism? [...]

While I admire the enthusiasm and lateral thinking, that sounds like a really cool and challenging idea algorithmically, it might be just a tiny bit absolute overkill :). We'd basically have to implement cooperative multitasking on top of CP, not considering that would probably blow the roof of the stack.

[ald0o]

With naive implementation, this would only require linear stack space with respect to the number of active modules (which is already the current worst case complexity) and hopefully really very few code changes.

But all this discussion is probably off-topic and I am of course looking forward to your fix!

[xs5871]

Should be (by now) resolved by the new processing pipeline. If you're still experiencing problems, please submit a new issue.