[New Feature] Custom Modified Values #2900

AdyaAdya · 2018-05-05T22:12:46Z

CMV is a feature that allows the user to customize the keycodes processed by the firmware and sent to the host depending on if Left Shift and/or Right Shift and/or Right Alt are being pressed or not.

For example, the user could assign the following to a key:

MODIFIERS PRESSED ALONG WITH THE KEY	KEYCODE
None	KC_DOT
Left and/or Right shift	KC_COMMA
Right alt	KC_SCOLON
Left and/or Right shift AND Right alt	KC_COLON

The feature is documented in more details in docs/feature_custom_modified_values.md and the CMV-1.0-Lite version weights about 1350 bytes in the hex file. CMV-2.0-Lite weights about 400 bytes

This the feature that would solve the Issue #1495.

This was resulting in build errors when travis CI builds all keyboards.

drashna · 2018-05-08T23:12:53Z

Looks like there is a conflict that needs manual resolution.

- Completely reviewed the architecture of the feature. - Oneshot modifiers are supported in CMV-v2. - Updated CMV feature documentation. - Deleted the atom47.h file which was causing conflicts

AdyaAdya · 2018-05-10T18:25:02Z

Designed a much cleaner and lighter (~400 bytes) version (CMV-2.0-Lite) of the feature which supports Oneshot modifiers as well. The usage of the feature is still the same.
See updated documentation under docs/feature_custom_modified_values.md for more infos.
(And I also resolved the conflict)

- Removed CMV_NUMBER_OF_HELD_KEYS from the CMV-2.0 version - Updated documentation

AdyaAdya · 2018-05-11T18:14:08Z

To make the reviewers' job easier and for those of you interested in understanding how the feature works, I'm going to explain the CMV-2.0-Lite code: (It is recommended to read the general specifications of the feature in the docs/custom_modified_values.md file first)

The magic happens mainly in quantum/keymap_common.c where we modify the keymap_key_to_keycode function to return the keycode returned by our function keymap_key_to_keycode_cmv:

// in quantum/keymap_common.c near line 232
__attribute__ ((weak))
uint16_t keymap_key_to_keycode(uint8_t layer, keypos_t key)
{
#if defined(CUSTOM_MODIFIED_VALUES_ENABLE)
    return keymap_key_to_keycode_cmv(layer, key);
#else
    // Read entire word (16bits)
    return pgm_read_word(&keymaps[(layer)][(key.row)][(key.col)]);
#endif
}

Fisrt thing we do in keymap_key_to_keycode_cmv is calling the functions get_kcid and get_custom_modified_values_for_key to determine if the user assigned custom modified values to the currently processed key:

// in quantum/keymap_common.c near line 220
static uint16_t keymap_key_to_keycode_cmv(uint8_t layer, keypos_t key) {
  uint8_t kcid = get_kcid(get_custom_modified_values_for_key(pgm_read_word(&keymaps[layer][key.row][key.col]), layer, key));
  if (kcid) {
    set_mods_blocker(true);
  } else {
    set_mods_blocker(false);
  }
  return cmv_buffer[kcid];
}

The function get_custom_modified_values_for_key calls the user-overridable function keycodes_for_key which returns false if custom modified values are assigned to the currently processed key or true otherwise.

// in quantum/keymap_common.c near line 205
static uint16_t* get_custom_modified_values_for_key(uint16_t default_kc, uint8_t layer, keypos_t key) {
  if(keycodes_for_key(default_kc, layer, key)) {
    set_cmv_buffer(default_kc, 0, 0, 0);
  }
  return cmv_buffer;
}

Inside keycodes_for_key, the user must use the CMV(...) macro defined as following and passing it the wanted custom modified values which can be any 16-bit keycodes:

// in quantum/custom_modified_values.h near line 13
#define CMV(kc_default, kc_shifted, kc_altgred, kc_sftralt) set_cmv_buffer(kc_default, kc_shifted, kc_altgred, kc_sftralt)

The set_cmv_buffer function fills the array used as a buffer by the feature which is initialized like this:

// in quantum/keymap_common.c near line 190
static uint16_t cmv_buffer[4] = {0,0,0,0};

So back in keymap_key_to_keycode_cmv, get_custom_modified_values_for_key returns a pointer to cmv_buffer which is being passed as a parameter to the get_kcid function. This latter returns the KCID depending on the current state of the modifiers (we are going to use it as an index in cmv_buffer). The KCID is an integer from 0 to 4. For example, if KC_LSHIFT is currently being held down, get_kcid will return 1 IF AND ONLY IF there is a custom shifted keycode assigned to the key and this keycode is different than 0.

// in quantum/keymap_common.c near line 212
static uint8_t get_kcid(uint16_t* kcs) {
  if(are_there_non_charmods()) return 0;
  else if(are_there_shifts() && are_there_ralts() && kcs[3]) return 3;
  else if(are_there_shifts() && kcs[1]) return 1;
  else if(are_there_ralts() && kcs[2]) return 2;
  return 0;
}

(Note: I think it is a good idea to return the default keycode (i.e. KCID=0) if there are any non-charmods modifiers active, so the user can still use shortcuts such as Ctrl+Shift+; (for example, in Visual Studio Code) even if they assigned a custom shifted keycode to the ; key)

The are_there_* functions called in get_kcid are defined in tmk_core/common/action_util.c. They use the appropriate modifiers mask to see if the wanted modifiers are set in any of the four uint8_t variables holding the modifiers' state: real_mods, macro_mods, oneshot_mods and oneshot_locked_mods:

// in tmk_core/common/action_util.c near line 69
// #define CMV_NON_CHARMODS_MASK (0b00000000 | MOD_BIT(KC_RGUI) | MOD_BIT(KC_RCTRL) | MOD_BIT(KC_LGUI) | MOD_BIT(KC_LALT) | MOD_BIT(KC_LCTRL))
#define CMV_NON_CHARMODS_MASK 0b10011101
// #define CMV_SHIFTS_MASK (0b00000000 | MOD_BIT(KC_RSHIFT) | MOD_BIT(KC_LSHIFT))
#define CMV_SHIFTS_MASK 0b00100010
// #define CMV_RALT_MASK (0b00000000 | MOD_BIT(KC_RALT))
#define CMV_RALT_MASK 0b01000000

#ifndef NO_ACTION_ONESHOT
# define ARE_THERE_THESE_MODS(mods_mask) (  \
    (mods_mask & real_mods)     ||          \
    (mods_mask & macro_mods)    ||          \
    (mods_mask & oneshot_mods)  ||          \
    (mods_mask & oneshot_locked_mods))
#else
# define ARE_THERE_THESE_MODS(mods_mask) ((mods_mask & real_mods) || (mods_mask & macro_mods))
#endif

bool are_there_non_charmods(void) { return ARE_THERE_THESE_MODS(CMV_NON_CHARMODS_MASK); }
bool are_there_shifts(void) { return ARE_THERE_THESE_MODS(CMV_SHIFTS_MASK); }
bool are_there_ralts(void) { return ARE_THERE_THESE_MODS(CMV_RALT_MASK); }

Back in keymap_key_to_keycode_cmv now. If KCID value returned by get_kcid is 0 this means that we want to return the default keycode to the firmware. If the KCID is any other value (i.e. 1, 2 or 3) this means there is a custom modified value and we have to return it to the firmware. However, since we want the user to be able to assign any 16-bit keycode as a custom modified value, we have to NOT send the charmods currently pressed so for example, KC_X assigned as a custom shifted value gives x (it would give X if we sent the currently pressed Shift). To block (or to unblock) the modifiers, we use the set_mods_blocker function which set the value of the variable dont_send_mods defined as:

// in tmk_core/common/action_util.c near line 86
static bool dont_send_mods = false;

This variable is then used inside the send_keyboard_report function to prevent the mods part of the keyboard_report to be filled by real_mods, macro_mods, oneshot_mods or oneshot_locked_mods (But not weak_mods since it is used for keycodes such as LFST(KC_DOT)):

// in tmk_core/common/action_util.c near line 187
void send_keyboard_report(void) {
#ifdef CUSTOM_MODIFIED_VALUES_ENABLE
    if (dont_send_mods) {
      keyboard_report->mods = weak_mods;
# ifndef NO_ACTION_ONESHOT
          if (oneshot_mods) {
#   if (defined(ONESHOT_TIMEOUT) && (ONESHOT_TIMEOUT > 0))
              if (has_oneshot_mods_timed_out()) {
                  dprintf("Oneshot: timeout\n");
                  clear_oneshot_mods();
              }
#   endif
              if (has_anykey(keyboard_report)) {
                  clear_oneshot_mods();
                  host_keyboard_send(keyboard_report); // These two lines are here to  
                  clear_keys();                        // prevent the keycode to get stuck
              }
          }
# endif
    } else {
#endif
    keyboard_report->mods  = real_mods;
    keyboard_report->mods |= weak_mods;
    keyboard_report->mods |= macro_mods;
#ifndef NO_ACTION_ONESHOT
    if (oneshot_mods) {
#if (defined(ONESHOT_TIMEOUT) && (ONESHOT_TIMEOUT > 0))
        if (has_oneshot_mods_timed_out()) {
            dprintf("Oneshot: timeout\n");
            clear_oneshot_mods();
        }
#endif
        keyboard_report->mods |= oneshot_mods;
        if (has_anykey(keyboard_report)) {
            clear_oneshot_mods();
        }
    }
#endif
#ifdef CUSTOM_MODIFIED_VALUES_ENABLE
    }
#endif

    host_keyboard_send(keyboard_report);
}

Finally, a last problem needed to be solved: if the user release a key at a moment where the modifiers' state is different than it was when the key was pressed, it is most likely that the returned keycode will not be the same as the one returned when the key was pressed in the first place which will result in a stuck key. To solve this issue without having to store the keycodes processed when the key was pressed (that would make the feature notably heavier (this is done in the CMV-2.0-Normal version)), we have to clear keys when the modifiers' state changes (for oneshots modifiers it is done by the two lines in the code right above this):

// in tmk_core/common/action_util.c near line 221
void add_mods(uint8_t mods) {
#ifdef CUSTOM_MODIFIED_VALUES_ENABLE
  clear_keys();
#endif
  real_mods |= mods;
}

void del_mods(uint8_t mods) {
#ifdef CUSTOM_MODIFIED_VALUES_ENABLE
  clear_keys();
#endif
  real_mods &= ~mods;
}

(Please take note that this solves the problem for MOST of the keycodes but the problem can still potentially occur for some advanced keycodes. If you're absolutely willing to use these latter keycodes, please consider allocating a little more space to the feature and use the CMV-2.0-Normal version (currently in development) instead which will solve this problem.)

drashna · 2018-10-21T17:55:49Z

Re-reviewing this..... this shouldn't be handled as a define, but as a full on feature .

Eg, you should add this to the common_features.mk file:

  ifeq ($(strip $(CUSTOM_MODIFIED_VALUES_ENABLE)), yes)
    OPT_DEFS += -DCUSTOM_MODIFIED_VALUES_ENABLE
  endif

Additionally, it may be a good idea to rebase, as it's been a while.

This was resulting in build errors when travis CI builds all keyboards.

- Completely reviewed the architecture of the feature. - Oneshot modifiers are supported in CMV-v2. - Updated CMV feature documentation. - Deleted the atom47.h file which was causing conflicts

- Removed CMV_NUMBER_OF_HELD_KEYS from the CMV-2.0 version - Updated documentation

As drashna suggested here qmk#2900 (comment)

…into temp

AdyaAdya · 2018-10-29T15:10:32Z

I think I correctly added CMV as a full on feature, however I can't be 100% sure.
I had also quite a lot of trouble for rebasing (never done anything like that before) so it is possible that there are mistakes.
The automatic check is successful though so that's good I suppose ^^

canatella · 2018-11-22T00:11:43Z

I found a simpler way to do that:

#define maybe_add_weak_mods(keycode, mod)                               \
  if (keycode < QK_MODS_MAX &&                                          \
      (keycode & 0xff00) == QK_ ## mod)                                 \
    add_weak_mods(MOD_BIT(KC_ ## mod))

static bool alternate_modifier(uint16_t modifier, uint16_t keycode, keyrecord_t *record) {
  static bool in_alternate_modifier;

  /* when it's a key press and modifier state is pressed */
  if (record->event.pressed && (get_mods() & MOD_BIT(modifier))) {
    in_alternate_modifier = true;

    /* will send modifier up so that the os won't shift the keycode we will send */
    del_mods(MOD_BIT(modifier));

    /* send mods if keycode needs it */
    maybe_add_weak_mods(keycode, LCTL);
    maybe_add_weak_mods(keycode, LSFT);
    maybe_add_weak_mods(keycode, LALT);
    maybe_add_weak_mods(keycode, LGUI);
    maybe_add_weak_mods(keycode, RCTL);
    maybe_add_weak_mods(keycode, RSFT);
    maybe_add_weak_mods(keycode, RALT);
    maybe_add_weak_mods(keycode, RGUI);

    /* send mods modifications */
    send_keyboard_report();

    /* send alternate key code */
    register_code(keycode);

    /* we changed the internal state by releasing the modifier key, marked against
       as pressed so that we are back in the real state */
    add_mods(MOD_BIT(modifier));

    return false;
  }

  /* when releasing the key and we activated alternate modifier */
  if (!record->event.pressed && in_alternate_modifier) {
    in_alternate_modifier = false;

    /* release the alternate key */
    unregister_code(keycode);

    /* make sure all mods we sat up earlier are released */
    clear_weak_mods();

    /* send mods modification */
    send_keyboard_report();

    return false;
  }


  return true;
}

bool process_record_user(uint16_t keycode, keyrecord_t *record) {
  switch(keycode) {
    case BP_UNDS:
      return alternate_modifier(KC_LSFT, BP_EXCLAIM, record);
    case SHF_LPRN:
      return alternate_modifier(KC_LSFT, BP_RPRN, record);
    case BP_LCBR:
      return alternate_modifier(KC_LSFT, BP_RCBR, record);
    case BP_LBRC:
      return alternate_modifier(KC_LSFT, BP_RBRC, record);
    case BP_LESS:
      return alternate_modifier(KC_LSFT, BP_GRTR, record);
  }

  return true;
}

ghost · 2018-11-30T20:34:25Z

I took the code for my personal keymap. Hope to see it as an official feature soon.

drashna · 2018-12-13T16:06:29Z

quantum/quantum.c

@@ -201,7 +201,7 @@ bool process_record_quantum(keyrecord_t *record) {
  keypos_t key = record->event.key;
  uint16_t keycode;

-  #if !defined(NO_ACTION_LAYER) && !defined(STRICT_LAYER_RELEASE)
+  #if !defined(NO_ACTION_LAYER) && defined(PREVENT_STUCK_MODIFIERS)


This needs to be reverted. There is no more PSM, as it is now the default behavior.

Suggested change

#if !defined(NO_ACTION_LAYER) && defined(PREVENT_STUCK_MODIFIERS)

#if !defined(NO_ACTION_LAYER) && !defined(STRICT_LAYER_RELEASE)

drashna · 2018-12-13T16:06:52Z

tmk_core/common/action.c

@@ -120,7 +120,7 @@ void process_hand_swap(keyevent_t *event) {
 }
 #endif

-#if !defined(NO_ACTION_LAYER) && !defined(STRICT_LAYER_RELEASE)
+#if !defined(NO_ACTION_LAYER) && defined(PREVENT_STUCK_MODIFIERS)
