qmk / qmk_firmware

Open-source keyboard firmware for Atmel AVR and Arm USB families
https://qmk.fm
GNU General Public License v2.0
18.24k stars 39.32k forks source link

[Bug] Mod-tap doesn't work with KC_TRANSPARENT #23365

Open VitalyArtemiev opened 7 months ago

VitalyArtemiev commented 7 months ago

Describe the Bug

According to docs, MT(mod, kc) should work with KC_TRANSPARENT.

Currently, the kc argument of MT() is limited to the Basic Keycode set

KC_TRANSPARENT is present in Basic Keycode set: https://docs.qmk.fm/#/keycodes_basic?id=special-keys

Yet, when I setup a layer with keycodes such as LCTL_T(KC_TRANSPARENT), tapping the key does not yield the key from the preceding layer.

Here is what I was trying to achieve:

#define _ KC_TRANSPARENT

const uint16_t PROGMEM keymaps[][MATRIX_ROWS][MATRIX_COLS] = {
    [CMK_BASE] = LAYOUT_91_ansi(
        KC_SLEP,KC_ESC, KC_F1,       KC_F2,       KC_F3,       KC_F4,       KC_F5, KC_F6, KC_F7,       KC_F8,       KC_F9,       KC_F10,        KC_F11,  KC_F12,   KC_INS,  KC_DEL, KC_MUTE,
        MC_1,   KC_GRV, KC_1,        KC_2,        KC_3,        KC_4,        KC_5,  KC_6,  KC_7,        KC_8,        KC_9,        KC_0,          KC_MINS, KC_EQL,   KC_BSPC,         KC_PGUP,
        MC_2,   KC_TAB, KC_Q,        KC_W,        KC_F,        KC_P,        KC_B,  KC_J,  KC_L,        KC_U,        KC_Y,        KC_SCLN,       KC_LBRC, KC_RBRC,  KC_BSLS,         KC_PGDN,
        MC_3,   KC_CAPS,KC_A,        KC_R,        KC_S,        KC_T,        KC_G,  KC_M,  KC_N,        KC_E,        KC_I,        KC_O,          KC_QUOT,           KC_ENT,          KC_HOME,
        MC_4,   KC_LSFT,             KC_X,        KC_C,        KC_D,        KC_V,  KC_Z,  KC_K,        KC_H,        KC_COMM,     KC_DOT,        KC_SLSH,           KC_RSFT, KC_UP,
        MC_5,   KC_LCTL,KC_LWIN,     KC_LALT,     MO(FN1),                  KC_SPC,                    LT(FN1,KC_SPC),           KC_RALT,       MO(FN1), KC_RCTL,  KC_LEFT, KC_DOWN,KC_RGHT),

    [QWR_BASE] = LAYOUT_91_ansi(
        KC_SLEP,KC_ESC, KC_F1,       KC_F2,       KC_F3,       KC_F4,       KC_F5, KC_F6, KC_F7,       KC_F8,       KC_F9,       KC_F10,         KC_F11,  KC_F12,  KC_INS,  KC_DEL, KC_MUTE,
        MC_1,   KC_GRV, KC_1,        KC_2,        KC_3,        KC_4,        KC_5,  KC_6,  KC_7,        KC_8,        KC_9,        KC_0,           KC_MINS, KC_EQL,  KC_BSPC,         KC_PGUP,
        MC_2,   KC_TAB, KC_Q,        KC_W,        KC_E,        KC_R,        KC_T,  KC_Y,  KC_U,        KC_I,        KC_O,        KC_P,           KC_LBRC, KC_RBRC, KC_BSLS,         KC_PGDN,
        MC_3,   KC_CAPS,KC_A,        KC_S,        KC_D,        KC_F,        KC_G,  KC_H,  KC_J,        KC_K,        KC_L,        KC_SCLN,        KC_QUOT,          KC_ENT,          KC_HOME,
        MC_4,   KC_LSFT,             KC_Z,        KC_X,        KC_C,        KC_V,  KC_B,  KC_N,        KC_M,        KC_COMM,     KC_DOT,         KC_SLSH,          KC_RSFT, KC_UP,
        MC_5,   KC_LCTL,KC_LWIN,     KC_LALT,     MO(FN1),                  KC_SPC,                    LT(FN1,KC_SPC),           KC_RALT,        MO(FN1), KC_RCTL, KC_LEFT, KC_DOWN,KC_RGHT),

    [MOD_TAP] = LAYOUT_91_ansi(
        _______,  _______,  _______,  _______,  _______,   _______,  _______,  _______,  _______,  _______,  _______,  _______,  _______,    _______,  _______,  _______,  _______,
        _______,  _______,  _______,  _______,  _______,   _______,  _______,  _______,  _______,  _______,  _______,  _______,  _______,    _______,  _______,            _______,
        _______,  _______,  _______,  _______,  _______,   _______,  _______,  _______,  _______,  _______,  _______,  _______,  _______,    _______,  _______,            _______,
        _______,  _______, LCTL_T(_),LSFT_T(_), LALT_T(_), LGUI_T(_),_______,  _______,RGUI_T(_),  RALT_T(_),RSFT_T(_),RCTL_T(_),_______,    _______,  _______,
        _______,  _______,            _______,  _______,   _______,  _______,  _______,  _______,  _______,  _______,  _______,  _______,              _______,  _______,
        _______,  _______,  _______,  _______,  _______,             _______,                      _______,            _______,  _______,    _______,  _______,  _______,  _______),

    [FN1] = LAYOUT_91_ansi(
        RGB_TOG,  _______,  KC_BRID,  KC_BRIU,  KC_TASK,  KC_FLXP,  RGB_VAD,   RGB_VAI,  KC_MPRV,  KC_MPLY,  KC_MNXT,  KC_MUTE,  KC_VOLD,    KC_VOLU,  RGB_RMOD, RGB_MOD,  NK_TOGG,
        TG(1),    _______,  _______,  _______,  _______,  _______,  _______,   _______,  _______,  _______,  _______,  _______,  _______,    _______,  _______,            _______,
        TG(2),    KC_SCRL,  _______,  _______,  _______,  _______,  _______,   _______,  _______,  KC_UP,    _______,  _______,  _______,    _______,  _______,            _______,
        TG(3),    _______,  KC_ESCAPE,_______,  _______,  _______,  _______,   _______,  KC_LEFT,  KC_DOWN,  KC_RIGHT, KC_ENTER, _______,              _______,            _______,
        TG(4),    _______,            _______,  _______,  _______,  _______,   _______,  _______,  _______,  _______,  _______,  _______,              _______,  _______,
        TG(5),    _______,  _______,  _______,  _______,            _______,                       KC_BSPC,            KC_DEL,   _______,    _______,  _______,  _______,  _______),

},

This would allow toggling mod-tap behaviour by toggling the MOD_TAP layer. I am using my dip switch to set layer 0 or layer 1 as default, which are COLEMAK and QWERTY layouts respectively.

You can find full example here: https://github.com/VitalyArtemiev/qmk_firmware/blob/mt_trsp_bug/keyboards/keychron/q11/ansi_encoder/keymaps/vital/keymap.c

Expected behaviour: When tapping the A key, I expect the letter A. When holding the A key, I expect the Ctrl modifier. Actual behaviour: When tapping the A key, nothing. Holding the A key activates Ctrl as expected. This is regardless of the selected default layer (0 or 1).

Keyboard Used

keychron/q11/ansi_encoder

Link to product page (if applicable)

https://www.keychron.com/products/keychron-q11-qmk-custom-mechanical-keyboard

Operating System

Win10

qmk doctor Output

$ qmk doctor
Ψ QMK Doctor is checking your environment.
Ψ CLI version: 1.1.5
Ψ QMK home: C:/Users/vital/qmk_firmware
Ψ Detected Windows 10 (10.0.19045).
Ψ QMK MSYS version: 1.9.0
Ψ Userspace enabled: False
Ψ Git branch: master
Ψ Repo version: 0.24.6
⚠ Git has unstashed/uncommitted changes.
Ψ - Latest master: 2024-03-27 14:43:37 +0300 (4434af48b5) -- Add game layer (without mod-tap)
Ψ - Latest upstream/master: 2024-03-27 07:30:58 +0800 (1d58530e79) -- [Keyboard] Add T75 (#23344)
Ψ - Latest upstream/develop: None
Ψ - Common ancestor with upstream/master: 2024-03-22 18:30:30 -0500 (4afbade6d1) -- Add ES_GRV to latam language-specific keycodes (#23333)
Ψ - Common ancestor with upstream/develop: None
Ψ All dependencies are installed.
Ψ Found arm-none-eabi-gcc version 12.2.0
Ψ Found avr-gcc version 12.2.0
⚠ We do not recommend avr-gcc newer than 8. Downgrading to 8.x is recommended.
Ψ Found avrdude version 7.0
Ψ Found dfu-programmer version 1.1.0
Ψ Found dfu-util version 0.11
Ψ Submodules are up to date.
Ψ Submodule status:
Ψ - lib/chibios: 2023-04-15 13:48:04 +0000 --  (11edb1610)
Ψ - lib/chibios-contrib: 2023-11-27 18:15:44 +0100 --  (9d7a7f90)
Ψ - lib/googletest: 2021-06-11 06:37:43 -0700 --  (e2239ee6)
Ψ - lib/lufa: 2022-08-26 12:09:55 +1000 --  (549b97320)
Ψ - lib/vusb: 2022-06-13 09:18:17 +1000 --  (819dbc1)
Ψ - lib/printf: 2022-06-29 23:59:58 +0300 --  (c2e3b4e)
Ψ - lib/pico-sdk: 2023-02-12 20:19:37 +0100 --  (a3398d8)
Ψ - lib/lvgl: 2022-04-11 04:44:53 -0600 --  (e19410f8)
Ψ QMK is ready to go, but minor problems were found

Is AutoHotKey / Karabiner installed

Other keyboard-related software installed

No response

Additional Context

No response

sigprof commented 7 months ago

Unfortunately, the behavior that you expected here is not actually implemented — KC_TRANSPARENT is handled only by layer_switch_get_layer() during the keymap lookup, and does not work when embedded inside MT() or LT(). So KC_TRANSPARENT is not really a basic keycode for this kind of usage (i.e., something that could be sent directly to the host by register_code()), even though it fits into a single byte.

And internally QMK has some other definitions of what a “basic” keycode is — e.g., IS_BASIC_KEYCODE() and BASIC_KEYCODE_RANGE accept only the KC_AKC_EXSEL range, which does not even include the modifiers, even though they are also a part of the Keyboard usage page; but IS_QK_BASIC() accepts the whole 0x000xFF range — i.e., anything which fits into a byte. So the issue of what is actually a basic keycode is somewhat complicated.

You may try to implement the behavior that you want in custom code — see the Intercepting Mod-Taps section for a way to change the tap function. Although you may need to be careful here — if the layer state changes between the press and release (which may happen for a tap if quick tap is enabled, which is the default), just using the current layer state to determine which keycode to register or unregister for a tap may result in stuck keys (although if you don't care about reporting holds with the quick tap feature properly, you may just call tap_code() on press and do nothing on release, which would avoid the problem).

Implementing that behavior in core (where it would need to work in the most generic way possible) may be difficult though — the problem is that it would require two instances of the source layers cache (one which would remember the layer where the MT() or LT() keycode came from, and another which would remember the layer where the underlying tap keycode had been found). And there is an even worse problem — what to do if the keycode under MT(mods, KC_TRANSPARENT) is not a basic keycode? (The existing code for mod-taps just does register_code(action.key.code) for a tap; this won't work if the second keymap lookup replaces KC_TRANSPARENT with a non-basic keycode.)

VitalyArtemiev commented 7 months ago

That's what I suspected. Can I submit a pull request for docs to communicate this caveat more clearly? Thank you for your time.

drashna commented 5 months ago

Can I submit a pull request for docs to communicate this caveat more clearly?

Please do!