qmk / qmk_firmware

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

Can't get GUI_T(LGUI(KC_SPC)) to work #511

Closed edasque closed 8 years ago

edasque commented 8 years ago

If I understand correctly that should give me:

But it gives me a space on tap instead.

I validated that LGUI(KC_SPC) works for COMMAND+SPACE.

Any idea?

(Ergodox Ez)

jackhumbert commented 8 years ago

Sorry, those functionalities aren't chainable like that. GUI_T() only accepts 8bit keycodes, and LGUI() returns a 16bit keycode!

We're hoping to be able to support things like this in the future, but for now you may want to try to implement this as a custom function :)

edasque commented 8 years ago

@jackhumbert than you for your response. Can you give me an example of what that would look like?

jackhumbert commented 8 years ago

The default planck keymap uses the process_record_user in this one - you'd just define your custom keycode and put the code/logic in the switch statement.

ezuk commented 8 years ago

@edasque were you able to get what you wanted as a custom function?

edasque commented 8 years ago

I haven't had a chance to dive into custom functions yet. I get the gist of it but not really how to declare them or link them to a key

edasque commented 8 years ago

@ezuk , @jackhumbert - why use those custom functions instead of macros?

If I do use a process_record_user, how do I output a key, for example T or Command-T ? register_code() ?

algernon commented 8 years ago

You can do this on top of tap-dance, with something like this. Just replace the register_code calls: whenever it (un)registers KC_LALT, replace that with whatever the Command keycode is, and replace the other one with KC_LGUI and KC_SPC.

Once I'm done with ironing out some tap-dance issues, I will make this kind of thing a lot easier to do with tap-dance.

edasque commented 8 years ago

Seems there are a few ways to implement this. This is what I have thus far. Can you please comment (if if you're so inclined, tell me why it doesn't work? :) )

bool process_record_user(uint16_t keycode, keyrecord_t *record) {
  switch (keycode) {
    case KC_T:
      if (record->event.pressed) {
        if (record->event.time && record->event.time > 500)
        {
          register_code(KC_E);
          unregister_code(KC_E;
        }
        else
        {
          register_code(KC_T);
          unregister_code(KC_T);
      }
        // persistant_default_layer_set(1UL<<_QWERTY);
      }
      return false;
      break;

  }
  return true;
}

ultimately, I would replace the if result with something like this:

          register_code(LGUI(KC_T));
          unregister_code(LGUI(KC_T));
algernon commented 8 years ago

Just a hunch, but you may want timer_elapsed(record->event.time) > 500 instead.

In what way does this not work? Does it always register a KC_T?

edasque commented 8 years ago
bool process_record_user(uint16_t keycode, keyrecord_t *record) {
  switch (keycode) {
    case KC_T:
      if (record->event.pressed) {
        if (record->event.time && timer_elapsed(record->event.time)> 1000)
        {
          register_code(KC_E);
          unregister_code(KC_E);
        }
        else
        {
          register_code(KC_T);
          unregister_code(KC_T);
      }
        // persistant_default_layer_set(1UL<<_QWERTY);
      }
      return false;
      break;

  }
  return true;
}

I can't really make sense of it. I would expect a quick tap would do a T and a tape and hold would get a e but that's not the case. tettettte is the result of quick tapping the T key multiple times. tetettttttttttttttte is the result of slow and deliberate hitting T and holding a bit.

To be clear in this example I'd like quick normal taps to get a T and a long hold to get a E. Eventually I want quick taps to do T and long hold of letter T to do a Command+T.

edasque commented 8 years ago

@jackhumbert @ezuk , any idea?

ghost commented 8 years ago

Here is your solution, I basically just ripped code from keyboards/ergodox/keymaps/dvorak_spanish/keymap.c

// MACROS
#define TAndE 0

static uint16_t key_timer;
bool process_record_user(uint16_t keycode, keyrecord_t *record) {
  switch (keycode) { 
    case TAndE:
      if (record->event.pressed) {
        key_timer = timer_read();
      } else {
        if (timer_elapsed(key_timer) < 100) {
          register_code(KC_T);
          unregister_code(KC_T);
        } else {
          register_code(KC_E);
          unregister_code(KC_E);
        }
    }
    return false;
    break;
  }
  return true;
}

The 100 value can be tuned to your liking.

edasque commented 8 years ago

@kuel so I do have to put TAndE in my Keymap right? The mistake I was making regarding the timing is now obvious, thank you for providing me with a great example.

The thing I don't get is how process_record_user actually understands what TAndE actually is.

I placed #define TAndE 0 at the top of the file. Is there an interval of integers that are outside of the ones used by the normal range of keycaps? Let's say I want to define a dozen of these, is 0..11 unused?

ghost commented 8 years ago

Yeah, you need TAndE, you can refactor it to your liking. I have no idea, it just works. Look in keyboards/ergodox/keymaps/dvorak_spanish/keymap.c, they have a bunch of examples.

edasque commented 8 years ago

Thank you all for your help on this.

This is what I ended up with. The desire was for a quick tap to key in T and for a longer hold to do Command+T. This works very well.

// MACROS
#define MAGIC_T 0

...

static uint16_t key_timer;
bool process_record_user(uint16_t keycode, keyrecord_t *record) {
  switch (keycode) { 
    case MAGIC_T:
      if (record->event.pressed) {
        key_timer = timer_read();
      } else {
        if (timer_elapsed(key_timer) < 300) {
          register_code(KC_T);
          unregister_code(KC_T);
        } else {
          register_code(KC_LGUI);
          register_code(KC_T);
          unregister_code(KC_LGUI);
          unregister_code(KC_T);

        }
    }
    return false;
    break;
  }
  return true;
}

@jackhumbert I'd love to understand the #define MAGIC_T 0 part to understand how many macro I can define before I overlap with normal key definitions.

Closing this in the mean time since my main goal was achieved.

algernon commented 8 years ago

@edasque By the looks of it, you have 12 bits for macros, so that's about 4096 macros, before strange things happen.

(Based on this comment.)

edasque commented 8 years ago

Incidentally I have noticed that if I type 't' and another letter in rapid (but normal) succession, the second letter will be entered first: wt wt wt wt

I imagine that this is due to the additional processing time done on the T keypress. I don't know how to solve that. Any idea?

edasque commented 8 years ago

@jackhumbert @ezuk @ghost @algernon any idea? This latency of T makes this code unusable. Basically the handling of T doesn't finish processing by the time e is handled

algernon commented 8 years ago

You will either have to use tap-dance, or hook into process_record_user, the latter may be easier, since you already have the macro code. In process_record_user, check that the keycode pressed is anything other than t, and if so, and there is a timer for t, which is smaller than the control trigger, then register that immediately, and then continue.

That should do the trick, I believe, because this way, t will get registered as soon as any other key is pressed. Without any other code, it may get registered twice, but unregistered only once - not sure if that will cause any issues or not. If it does, then you may need a state variable to notify the macro whether it should register t, or just unregister it.

Does this make sense?

edasque commented 7 years ago

@algernon sorry for the late response but I don't understand how that's different from what I am doing, which I'll paste again here:

// MACROS
#define MAGIC_T 0

...

static uint16_t key_timer;
bool process_record_user(uint16_t keycode, keyrecord_t *record) {
  switch (keycode) { 
    case MAGIC_T:
      if (record->event.pressed) {
        key_timer = timer_read();
      } else {
        if (timer_elapsed(key_timer) < 300) {
          register_code(KC_T);
          unregister_code(KC_T);
        } else {
          register_code(KC_LGUI);
          register_code(KC_T);
          unregister_code(KC_LGUI);
          unregister_code(KC_T);

        }
    }
    return false;
    break;
  }
  return true;
}
algernon commented 7 years ago

Your code only deals with MAGIC_T. My proposed solution has logic for any other key too, and THAT is the difference. Because due to the additional logic, whenever you press te in quick succession, the e will first fire off t.

Something like this, within the switch statement:

default:
  if (record->event.pressed) {
    if (key_timer) {
      register_code(KC_T);
      unregister_code(KC_T);
      key_timer = 0;
    }
  }
  break;

This, and add a key_timer = 0; to the end of the non-pressed branch of MAGIC_T and you should be set. Except, that the non-pressed branch will also have to check that key_timer is 0, and if so, just return false.

This way, if you press t, the timer will register. Then you press e before t's depress registers, and the new logic triggers. There is a timer, so we register t, and move on, and you get te. When t unregisters, the timer is cleared, and nothing happens.

Mind you, I have not re-read the issue, so you may need to do some more in the default branch, depending on what kind of behaviour you want. But the key is, to have some action on keys other than the magic t.