[Bug] Layer-lock doesn't work for MO(layer)

sporkus commented 1 year ago

Thank you for your many qmk articles and features. Achordion is fantastic! However, I'm having issues with the layer lock feature.

Describe the bug

The bitwise operation in process_layer_lock returns the wrong layer from the keycode. By having the wrong layer in is_layer_locked(), the function returns true and qmk's default handling turns off the layer instead.

I had added some debugging code, and here's what I'm seeing:

MO(3) key down -> MO(3) key up

sporkus:reviung41:1: [DEBUG layer-lock] keycode: 0x5223
sporkus:reviung41:1: [DEBUG layer-lock] keycode & 255: 0x23
sporkus:reviung41:1: [DEBUG layer-lock] QK_MOMENTARY_GET_LAYER(keycode): 0x3
sporkus:reviung41:1: [DEBUG layer-lock] is_layer_locked(keycode & 255): 0
sporkus:reviung41:1: [DEBUG layer-lock] is_layer_locked(QK_MOMENTARY_GET_LAYER(keycode)): 0
sporkus:reviung41:1: KL: kc: 0x5223, pressed: 1, time:  5411, int: 0, count: 0

The macro QK_MOMENTARY_GET_LAYER(kc) ((kc)&0x1F) from quantum_keycodes.h seem to return the correct result instead:

MO(3) key down -> LLOCK key down -> LLOCK key up ->MO(3) key up

sporkus:reviung41:1: [DEBUG layer-lock] lock_keycode pressed
sporkus:reviung41:1: [DEBUG layer_lock_set_user] locked_layers: 0x8
sporkus:reviung41:1: [DEBUG layer-lock] keycode: 0x5223
sporkus:reviung41:1: [DEBUG layer-lock] keycode & 255: 0x23
sporkus:reviung41:1: [DEBUG layer-lock] QK_MOMENTARY_GET_LAYER(keycode): 0x3
sporkus:reviung41:1: [DEBUG layer-lock] is_layer_locked(keycode & 255): 0
sporkus:reviung41:1: [DEBUG layer-lock] is_layer_locked(QK_MOMENTARY_GET_LAYER(keycode)): 1
sporkus:reviung41:1: KL: kc: 0x5223, pressed: 0, time:  9635, int: 0, count: 0

Fix

After applying the following patch, the layer lock key worked as expected.

@@ -60,7 +61,13 @@ bool process_layer_lock(uint16_t keycode, keyrecord_t* record,
   switch (keycode) {
     case QK_MOMENTARY ... QK_MOMENTARY_MAX:  // `MO(layer)` keys.
     case QK_LAYER_TAP_TOGGLE ... QK_LAYER_TAP_TOGGLE_MAX: {  // `TT(layer)`.
-      const uint8_t layer = keycode & 255;
+      /* const uint8_t layer = keycode & 255; */
+      const uint8_t layer = QK_MOMENTARY_GET_LAYER(keycode);

Comment

It looks like the correct bit mask to use is only 5 bits for the 32 maximum layers. The keycode range seems to confirm it.

keycodes.h
43:    QK_MOMENTARY                   = 0x5220,
44:    QK_MOMENTARY_MAX               = 0x523F,

The bitwise operation for layer tap matches the macro QK_LAYER_TAP_GET_LAYER(kc), and works as expected, but perhaps it would be better for readability to use the macro also.

Information

Do the keys involved use any of the following features?

[ ] Achordion (from this repo)
[ ] Auto Shift
[ ] Combos
[ ] Key Overrides
[ ] Mod-tap or Layer-tap keys
[x] Other custom userspace code: Layer-lock

getreuer commented 1 year ago

Thank you for the excellent bug report! Your analysis is correct, the keycode parsing is not getting the layer as it should. The encodings for some keycodes changed recently with QMK 0.19, and I overlooked that this code was affected.

I just pushed https://github.com/getreuer/qmk-keymap/commit/694c2ffafa369a78448f9efca5ce2c607d629821 with a fix, using the new keycode parsing helpers.

sporkus commented 1 year ago

Thank you so much for the quick fix!

getreuer / qmk-keymap