fix(behaviors): Fixing erroneous combo triggering

[andrewjrae]

This is a very simple fix to a rather complicated issue. Essentially,
hold-taps will "release" (raise) their captured keys before actually
telling the event manager they have captured a key. This means the event manager ends up assigning the last_listener_index to the hold-tap subscription rather than the combo. So when the combo calls ZMK_EVENT_RELEASE it raises after the hold-tap instead of after the
combo as the combo code expects.

The corresponding test (which fails without this change) has also been added.

[jmding8]

I merged in this fix into the keymap where I originally encountered the problem. This seems to be the fix. I have not been able to reproduce the original problem since, and things seem to be working as expected otherwise as well. I'll continue to test tomorrow.

[petejohanson]

This workaround may fix combos, but I suspect you might have caught the core issue also causing #986

In particular, this happens:

static int position_state_changed_listener(const zmk_event_t *eh) {
    struct zmk_position_state_changed *ev = as_zmk_position_state_changed(eh);

    // blah blah blah, skipped for clarity.

    LOG_DBG("%d capturing %d %s event", undecided_hold_tap->position, ev->position,
            ev->state ? "down" : "up");
    capture_event(eh);
    decide_hold_tap(undecided_hold_tap, ev->state ? HT_OTHER_KEY_DOWN : HT_OTHER_KEY_UP);
    return ZMK_EV_EVENT_CAPTURED;
}

So capturing the current event, then possibly rasing it, before it's actually been noted as captured by the return. Perhaps this needs to be fixed to not capture at all based on the decision.

@okke-formsma any thoughts?

[okke-formsma]

Let's have a look at what's going on. The position state changed event is potentially handled multiple times by the hold-tap handler, because that's often needed if there are multiple hold-taps that are being decided by the same event (e.g., a key release which causes multiple hold-taps to decide).

struct zmk_position_state_changed *ev = as_zmk_position_state_changed(eh);
capture_event(eh);
... in decide_hold_tap ...
ZMK_EVENT_RAISE_AT(captured_event, behavior_hold_tap)

if we would not capture the position state changed event and raise it again at behavior_hold_tap, we might not release all hold-taps correctly.

Example, multiple tap-preferred hold-taps:

• press &ht A B (new undecided hold-tap)
• press &ht C D (position event queued)
• press &kp Z (position event queued)
• release &kp Z (position event queued)

At this point, we have three position events in the queue, and an undecided hold tap. Then, by the release &kp Z, the undecided hold-tap is decided, and the three position events are released, causing:

• press &ht C D (new undecided hold-tap)
• press &kp Z (position event queued)
• release &kp Z (position event queued)

Now the release &kp Z position event caused the &ht C D undecided hold-tap to be decided, and the remaining two position events are released, which cause a normal press and release of Z.

TL;DR: we need to queue and release the position events, otherwise we do not properly handle nested hold-taps.

[okke-formsma]

There is one more thing!

By re-raising the event, it may actually be fully handled! But the event handler code doesn't know!

event_manager.c
        case ZMK_EV_EVENT_CAPTURED:
            LOG_DBG("Listener captured the event");
            event->last_listener_index = i;
            // Listeners are expected to free events they capture
            return 0;

the event->last_listener_index is set to hold_tap, and that will cause it to be improperly re-released when the combo releases it!

Instead, the event->last_listener_index should be set before the handler is called!

[okke-formsma]

would this be the right fix instead?

diff --git a/app/src/event_manager.c b/app/src/event_manager.c
index eef5d839..471432a8 100644
--- a/app/src/event_manager.c
+++ b/app/src/event_manager.c
@@ -25,6 +25,7 @@ int zmk_event_manager_handle_from(zmk_event_t *event, uint8_t start_index) {
         if (ev_sub->event_type != event->event) {
             continue;
         }
+        event->last_listener_index = i;
         ret = ev_sub->listener->callback(event);
         switch (ret) {
         case ZMK_EV_EVENT_BUBBLE:
@@ -35,7 +36,6 @@ int zmk_event_manager_handle_from(zmk_event_t *event, uint8_t start_index) {
             goto release;
         case ZMK_EV_EVENT_CAPTURED:
             LOG_DBG("Listener captured the event");
-            event->last_listener_index = i;
             // Listeners are expected to free events they capture
             return 0;
         default:

The flow is as follows:

• key is queued and re-raised by hold-taps. The last_listener_index is not updated yet, so it's still 0. (hold_tap.c position_state_changed_listener is still on the stack!)
• key is queued and released by combos. The last_listener_index is still 0.
• when the combo releases the event, it starts playing at index 0 instead of after the combos.
• the event is now handled multiple times by the hold-tap handler, causing duplicate behavior!
• finally the hold-tap position_state_changed_listener returns ZMK_EV_EVENT_CAPTURED and the events last_listener_index is set to the hold-tap index, but it's too late.

[andrewjrae]

Making the change in the event handler seems like a sensible solution in general to me, it certainly provides a more consistent value of last_listener_index.

[andrewjrae]

Closing because #1411 encapsulates this fix