Redefining LSPO_KEY with shifted key

[anthonyrichir]

I'm trying the add the space cadet shift to my keyboard layout, but as my computer is set to use a belgian layout, the default keycode of LSPO_KEY gives a '9'.
To make it work on a belgian layout, I'm using the keymap-belgian.h, and in my layout, I'm using this key:

#define BE_LPRN KC_5

When I use it in my layout, it prints the left parenthesis, as I expect.

If I do the following definition in config.h, KC_LSPO prints 5.

#define LSPO_KEY KC_5

So I tried this one:

#define LSPO_KEY LSFT(KC_5)

But it gives me the following compilation error.

QMK Firmware 0.6.96
WARNING:
 Some git sub-modules are out of date or modified, please consider running:
 make git-submodule
 You can ignore this warning if you are not compiling any ChibiOS keyboards,
 or if you have modified the ChibiOS libraries yourself.

Making planck/rev4 with keymap anthony and target dfu

avr-gcc (GCC) 5.4.0
Copyright (C) 2015 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

Size before:
   text    data     bss     dec     hex filename
      0   27484       0   27484    6b5c .build/planck_rev4_anthony.hex

Compiling: quantum/audio/muse.c                                                                     [OK]
Compiling: keyboards/planck/planck.c                                                                [OK]
Compiling: keyboards/planck/keymaps/anthony/keymap.c                                                [OK]
Compiling: quantum/quantum.c                                                                       In file included from quantum/keymap.h:44:0,
                 from quantum/quantum.h:26,
                 from quantum/quantum.c:17:
quantum/quantum.c: In function ‘process_record_quantum’:
quantum/quantum_keycodes.h:463:18: error: large integer implicitly truncated to unsigned type [-Werror=overflow]
 #define LSFT(kc) (kc | QK_LSFT)
                  ^
./keyboards/planck/keymaps/anthony/config.h:45:18: note: in expansion of macro ‘LSFT’
 #define LSPO_KEY LSFT(KC_5)
                  ^
quantum/quantum.c:636:25: note: in expansion of macro ‘LSPO_KEY’
           register_code(LSPO_KEY);
                         ^
quantum/quantum_keycodes.h:463:18: error: large integer implicitly truncated to unsigned type [-Werror=overflow]
 #define LSFT(kc) (kc | QK_LSFT)
                  ^
./keyboards/planck/keymaps/anthony/config.h:45:18: note: in expansion of macro ‘LSFT’
 #define LSPO_KEY LSFT(KC_5)
                  ^
quantum/quantum.c:637:27: note: in expansion of macro ‘LSPO_KEY’
           unregister_code(LSPO_KEY);
                           ^
cc1: all warnings being treated as errors
 [ERRORS]
 |
 |
 |
tmk_core/rules.mk:357: recipe for target '.build/obj_planck_rev4_anthony/quantum/quantum.o' failed
make[1]: *** [.build/obj_planck_rev4_anthony/quantum/quantum.o] Error 1
Make finished with errors
Makefile:529: recipe for target 'planck/rev4:anthony:dfu' failed
make: *** [planck/rev4:anthony:dfu] Error 1

Why do I have this strange behavior and how can I fix it ?

[frederik-h]

The trouble seems to be that you have to hold shift to get a 5 on the belgian keyboard, is this right?
The implementation of KC_LSPO sets the shift modifier for you. You would have to unset it. I don't know if this is supported. You could have a look at the implementation in quantum.c

One solution would be to define your own tap dance.

[anthonyrichir]

You're probably right.

I assume that this is the piece of code which is handling it:

    case KC_LSPO: {
      if (record->event.pressed) {
        shift_interrupted[0] = false;
        scs_timer[0] = timer_read ();
        register_mods(MOD_BIT(KC_LSFT));
      }
      else {
        #ifdef DISABLE_SPACE_CADET_ROLLOVER
          if (get_mods() & MOD_BIT(KC_RSFT)) {
            shift_interrupted[0] = true;
            shift_interrupted[1] = true;
          }
        #endif
        if (!shift_interrupted[0] && timer_elapsed(scs_timer[0]) < TAPPING_TERM) {
          register_code(LSPO_KEY);
          unregister_code(LSPO_KEY);
        }
        unregister_mods(MOD_BIT(KC_LSFT));
      }
      return false;
    }

I'm quite new to QMK and I'm a little bit rusty in C but I assume that I could add a flag like DISABLE_SPACE_CADET_AUTOSHIFT and modify a little the code like this:

    case KC_LSPO: {
      if (record->event.pressed) {
        shift_interrupted[0] = false;
        scs_timer[0] = timer_read ();
        register_mods(MOD_BIT(KC_LSFT));
      }
      else {
        #ifdef DISABLE_SPACE_CADET_ROLLOVER
          if (get_mods() & MOD_BIT(KC_RSFT)) {
            shift_interrupted[0] = true;
            shift_interrupted[1] = true;
          }
        #endif
        if (!shift_interrupted[0] && timer_elapsed(scs_timer[0]) < TAPPING_TERM) {
          #ifdef DISABLE_SPACE_CADET_AUTOSHIFT
            unregister_mods(MOD_BIT(KC_LSFT));
          #endif
          register_code(LSPO_KEY);
          unregister_code(LSPO_KEY);
        }
        unregister_mods(MOD_BIT(KC_LSFT));
      }
      return false;
    }

So we would have something like this ? It's working fine for me. I will submit a PR if it's ok for you.

[frederik-h]

Sounds reasonable, but my experience with QMK is probably as limited at yours.
One could also strive for a greater generality and, instead of allowing only to disable the modifier, allow for setting an arbitrary modifier (say, #define LSPO_MOD KC_RALT or #define LSPO_MOD 0 to have no modifier) for the tapped key. This could look more or less like so:

      if (record->event.pressed) {
        shift_interrupted[0] = false;
        scs_timer[0] = timer_read ();
        register_mods(MOD_BIT(KC_LSFT));
      }
      else {
        if (!shift_interrupted[0] && timer_elapsed(scs_timer[0]) < TAPPING_TERM) {
          if(LSPO_MOD != KC_LSFT) {
            unregister_mods(MOD_BIT(KC_LSFT));
            if(IS_MOD(LSPO_MOD))
              register_mods(MOD_BIT(LSPO_MOD));
          }
          register_code(LSPO_KEY);
          unregister_code(LSPO_KEY);
        }
        if(IS_MOD(LSPO_MOD))
          unregister_mods(MOD_BIT(LSPO_MOD));
      }
      return false;
    }

[anthonyrichir]

You're right, it would be nice to be a bit more flexible and allow users to choose a different modifier, but I'm not a fan of setting the LSPO_MOD to 0 in order to disable the modifier.

My suggestion is to create both LSPO_MOD and RSPC_MOD, having both a default value of KC_LSFT, which could be overriden to whatever the user likes.

And then, I guess we could undefine LSPO_MOD/RSPC_MOD in the config.h of the keyboard to remove that default modifier, and check in quantum.c if they are defined in order to register them or not.

But what is the right place for defining the default LSPO_MOD and RSPC_MOD ?

[anthonyrichir]

I assume they could be defined next to LSPO_KEY and RSPC_KEY like that.

#ifndef LSPO_KEY
  #define LSPO_KEY KC_9
#endif
#ifndef RSPC_KEY
  #define RSPC_KEY KC_0
#endif

#define LSPO_MOD KC_LSFT
#define RSPC_MOD KC_LSFT

And then, it could be handled like this ? What do you think ?

#ifdef DISABLE_SPACE_CADET_ROLLOVER
  if (get_mods() & MOD_BIT(RSPC_MOD)) {
    shift_interrupted[0] = true;
    shift_interrupted[1] = true;
  }
#endif
if (!shift_interrupted[0] && timer_elapsed(scs_timer[0]) < TAPPING_TERM) {
  unregister_mods(MOD_BIT(KC_LSFT));
  #ifdef LSPO_MOD
    register_mods(MOD_BIT(LSPO_MOD));
  #endif
  register_code(LSPO_KEY);
  unregister_code(LSPO_KEY);
  #ifdef LSPO_MOD
    unregister_mods(MOD_BIT(LSPO_MOD));
  #endif
}
unregister_mods(MOD_BIT(KC_LSFT));

But undefining LSPO_MOD in my config.h seems to have no effect.

[frederik-h]

I agree with you regarding setting LSPO_MOD to 0, but I prefer to limit the use of preprocessor directives to a few controlled places. But, judging from the QMK code that I know, I don't think that this is a design goal of QMK ;-).

[anthonyrichir]

Yes, there is an extensive usage of preprocessor directives, I have the feeling that it's for reducing the compiled code to the minimum required, as the firmware is installed on keyboards with limited space. But my usual language is Java so ... not sure 😄