getreuer / qmk-keymap

My keymap & reusable QMK gems
Apache License 2.0
301 stars 45 forks source link

[Bug] Repeat keys sends 1 when used with LT() #30

Closed adaviloper closed 1 year ago

adaviloper commented 1 year ago

Describe the bug

When trying to add the repeat_keys functionality to my keyboard, I noticed that it seems to send KC_1 when used with a LT() function. I've tried it as a standard key on its own and that works fine.

  1. Add the REPEAT key as the tap argument for a LT function (LT(RAISE, REPEAT)
  2. Type
  3. Tap the LT key

Information

Do the keys involved use any of the following features?

getreuer commented 1 year ago

Thanks for trying out Repeat Key! Right, this is a limitation with QMK's compound keycodes. The "kc" in LT(layer, kc) must be a "basic keycode" in the 0–255 range [similarly with MT(mod, kc) and modifier keycodes like LSFT(kc)]. Custom keycodes like REPEAT are not basic. The definition of LT(layer, kc) does a bit mask (kc & 255) to force the tapping keycode to 8 bits. It sounds like your REPEAT is 0x7e1e, then LT masked this to 0x1e, which is KC_1.

The crux of the problem is that for space efficiency, keycodes in QMK are stored as 16-bit values, and there is only so much that can be crammed into 16 bits.

To work around this, you can pick a placeholder tapping keycode, then intercept the tap function to have it perform Repeat Key. To avoid a pathology of the Repeat Key trying to repeat itself, define the get_repeat_key_eligible() callback to ignore the LT-Repeat key (described in Ignoring certain keys). Something like:

// Copyright 2023 Google LLC.
// SPDX-License-Identifier: Apache-2.0

// The KC_F1 is an arbitrary placeholder and won't be sent to the host.
#define LT_REP LT(RAISE, KC_F1)

// Use LT_REP in your layout...

bool get_repeat_key_eligible(uint16_t keycode, keyrecord_t* record) {
  switch (keycode) {
    // Ignore LT_REP:
    case LT_REP:
      return false;

    // Ignore MO, TO, TG, and TT layer switch keys.
    case QK_MOMENTARY ... QK_MOMENTARY_MAX:
    case QK_TO ... QK_TO_MAX:
    case QK_TOGGLE_LAYER ... QK_TOGGLE_LAYER_MAX:
    case QK_LAYER_TAP_TOGGLE ... QK_LAYER_TAP_TOGGLE_MAX:
    // Ignore mod keys.
    case KC_LCTL ... KC_RGUI:
    case KC_HYPR:
    case KC_MEH:
    // Ignore one-shot keys.
#ifndef NO_ACTION_ONESHOT
    case QK_ONE_SHOT_LAYER ... QK_ONE_SHOT_LAYER_MAX:
    case QK_ONE_SHOT_MOD ... QK_ONE_SHOT_MOD_MAX:
#endif  // NO_ACTION_ONESHOT
      return false;

    // Ignore hold events on tap-hold keys.
#ifndef NO_ACTION_TAPPING
    case QK_MOD_TAP ... QK_MOD_TAP_MAX:
#ifndef NO_ACTION_LAYER
    case QK_LAYER_TAP ... QK_LAYER_TAP_MAX:
#endif  // NO_ACTION_LAYER
      if (record->tap.count == 0) {
        return false;
      }
      break;
#endif  // NO_ACTION_TAPPING

#ifdef SWAP_HANDS_ENABLE
    case QK_SWAP_HANDS ... QK_SWAP_HANDS_MAX:
      if (IS_SWAP_HANDS_KEYCODE(keycode) || record->tap.count == 0) {
        return false;
      }
      break;
#endif  // SWAP_HANDS_ENABLE
  }

  return true;
}

bool process_record_user(uint16_t keycode, keyrecord_t* record) {
  switch (keycode) {
    case LT_REP: 
      if (record->tap.count > 0) {  // Key is being tapped.
        if (record->event.pressed) {
          repeat_key_register();
        } else {
          repeat_key_unregister();
        }
        return false;  // Skip default handling.
      }
      return true;  // Continue default handling.

    // Other macros...
  }

  return true;
}
adaviloper commented 1 year ago

Thanks for developing this! Your posts on QMK have been super helpful! I've, so far, implemented a version of your sentence macros, as well as a variant of your symbol layer. These have all been wildly useful, though I'm still trying to get used to the sentence case macros.

Incredible! It seems like we don't need to add a case for custom this keycodes since LT is already covered by case QK_LAYER_TAP ... QK_LAYER_TAP_MAX:. It also took me a while to realize that since we're short circuiting the LT_REP, we need to also move the process_repeat_keys function call to after the process_record_user's switch case.

bool process_record_user_adaviloper(uint16_t keycode, keyrecord_t *record) {
    mod_state = get_mods();
    switch (keycode) {
        case LT_REP:
            if (record->tap.count > 0) {  // Key is being tapped.
                if (record->event.pressed) {
                    repeat_key_register();
                } else {
                    repeat_key_unregister();
                }
                return false;  // Skip default handling.
            }
            return true;  // Continue default handling.
    }
    if (!process_repeat_key(keycode, record, RE_PEAT)) { return false; }
    return true;
}