Command feature not working since single HID endpoint was introduced #4838

vomindoraan · 2019-01-13T04:07:06Z

Describe the bug

The Command feature is not working. It appears that the single endpoint PR (#3951) broke it. I was able to reproduce the bug on AVR and ChibiOS (Kiibohd) boards.

When holding down the Command combination (default: LShift+RShift) and pressing one of the magic keys (e.g. D for debug mode), the key gets passed straight to the host without triggering the command. On versions prior to the aforementioned HID endpoint change, the keyboard would activate debug mode correctly and would never send the D key to the host machine.

Update: It works only with KEYBOARD_SHARED_EP = yes. See here for how it behaves.

System Information

Keyboard: KBD6X, Melody96, Whitefox
Operating System: Windows (MSYS2), Docker
avr-gcc version: 5.4.0 and whatever qmkfm/qmk_firmware uses (presumably latest)
arm gcc version: 6.3.1 and whatever qmkfm/qmk_firmware uses (presumably latest)
QMK Firmware version: 0.6.231
Any keyboard related software installed? WinCompose, KbdEdit

The text was updated successfully, but these errors were encountered:

vomindoraan · 2019-01-14T13:02:35Z

@abrasive Do you have any idea as to which part of your PR might have caused this?

fauxpark · 2019-01-14T13:50:01Z

Weird... I have no issues with it here. I've got a Satan with basically default rules.mk except for:

#CONSOLE_ENABLE = no
#COMMAND_ENABLE = no
RGBLIGHT_ENABLE = no
KEYBOARD_SHARED_EP = yes

I get this in the toolbox:

*** SATAN: GH60 Akuma connected  -- F055:60AA:0003 (8&314c9ff2&0&0000)
  > 
    	- Version -
    DESC: QMK keyboard firmware for Satan GH60 with WS2812 support
    VID: 0xF055(SATAN) PID: 0x60AA(GH60 Akuma) VER: 0x0003
    BUILD: "929065" (00:34:18 Jan 15 2019)
    OPTIONS: LUFA EXTRAKEY CONSOLE COMMAND NKRO 4096
    GCC: 4.9.2 AVR-LIBC: 2.0.0 AVR_ARCH: avr5

Might be at the keyboard level?

vomindoraan · 2019-01-14T14:06:07Z

Here's how it behaves for me on Whitefox: https://i.imgur.com/VYkbX8e.gif
Same for the other two boards I tried it with (KBD6X and Melody96).

vomindoraan · 2019-01-14T14:12:56Z

I just tried it with KEYBOARD_SHARED_EP = yes, and it does work. Now to get to the bottom of why it doesn't work without that.

fauxpark · 2019-01-14T14:12:59Z

What's with the tab? Also, what happens when you hit C? (I believe that actually enables the console printing)

vomindoraan · 2019-01-14T14:20:15Z

It prints fine without C when it's working (see here).

fauxpark · 2019-01-15T09:22:41Z

Oh dear. I have returned from my research, and it looks like there is another report ID-related misalignment, this time in report_keyboard_t. When KEYBOARD_SHARED_EP is not defined, the first byte of the 6KRO report is the modifiers, and the report ID for the NKRO report.

Before the shared endpoint stuff neither of them had report IDs, so using keyboard_report->mods in IS_COMMAND() would work because the byte offset within the struct just happened to be the same.
Now, though, we can't rely on that.

Introducing the new and improved default IS_COMMAND():

#define IS_COMMAND() ( \
	(!keymap_config.nkro && (keyboard_report->mods      == (MOD_BIT(KC_LSFT) | MOD_BIT(KC_RSFT)))) || \
	( keymap_config.nkro && (keyboard_report->nkro.mods == (MOD_BIT(KC_LSFT) | MOD_BIT(KC_RSFT)))) \
)

I feel like at this point it might as well be turned into a weak function.

vomindoraan · 2019-01-15T09:32:27Z

Can't we just use

#define IS_COMMAND() (get_mods() == (MOD_BIT(KC_LSFT) | MOD_BIT(KC_RSFT)))

instead, so we don't have to depend on the report formats? I'm not sure why keyboard_report->mods was chosen in the first place. The only difference would be that weak mods and one shot mods aren't accounted for in get_mods(). Weak mods shouldn't be a problem, and I'm sure it can be worked around in the case of one shot mods.

By the way, props for getting down and dirty with this and figuring out what was causing it. I appreciate it.

Edit: Tested the above and it works fine in all cases (shared EP on/off, NKRO on/off), as expected. However, it likely wouldn't work if I tried to use one shot mods to trigger commands.

fauxpark · 2019-01-15T09:45:28Z

Oh, that's much nicer, I didn't even realise that was a thing.

As for the oneshot/weak mods, you could just | them together. Along with get_mods() there is also get_weak_mods(), get_macro_mods(), and get_oneshot_mods().

vomindoraan · 2019-01-15T09:47:48Z

Indeed. I suspect that might have been the reason why keyboard_report->mods was chosen in the first place: it's exactly all of those OR'd together (see here).

vomindoraan · 2019-01-15T09:57:05Z

I think checking just the real mods with get_mods() should be fine. It will prevent people who use one shot mods from activating commands like so:

LShift↓ LShift↑ RShift↓ RShift↑ key↓ key↑

But they should still be able to do it by holding LShift+RShift and pressing the command key as you normally would. In other words,

LShift↓ RShift↓ key↓ key↑ RShift↑ LShift↑

should work even if LShift and RShift are set up as one shot mods. I'll need to test this to confirm.

fauxpark · 2019-01-15T10:01:29Z

Well I guess once #4301 is merged we can go about changing the remaining keyboard_report->mods to get_mods(). It should be a much easier job then 😁

vomindoraan · 2019-01-15T10:03:36Z

My thoughts exactly 😃
I will test the one shot mods scenario a bit later today and report back if I find any issues.

vomindoraan · 2019-01-15T19:26:16Z

Just tested it, it works fine with one shot mods. It registers the command if you hold both OSM mods (or even just one of them) down when you press the key. So it looks like get_mods() is our best bet.

vomindoraan · 2019-01-22T02:28:25Z

Just found keyboard_report->mods being used here as well:


rg -tc 'keyboard_report->mods' keyboards/massdrop/
keyboards/massdrop/alt/keymaps/abishalom/keymap.c
63:#define MODS_SHIFT  (keyboard_report->mods & MOD_BIT(KC_LSHIFT) || keyboard_report->mods & MOD_BIT(KC_RSHIFT))
64:#define MODS_CTRL  (keyboard_report->mods & MOD_BIT(KC_LCTL) || keyboard_report->mods & MOD_BIT(KC_RCTRL))
65:#define MODS_ALT  (keyboard_report->mods & MOD_BIT(KC_LALT) || keyboard_report->mods & MOD_BIT(KC_RALT))

keyboards/massdrop/alt/keymaps/mac/keymap.c
63:#define MODS_SHIFT  (keyboard_report->mods & MOD_BIT(KC_LSHIFT) || keyboard_report->mods & MOD_BIT(KC_RSHIFT))
64:#define MODS_CTRL  (keyboard_report->mods & MOD_BIT(KC_LCTL) || keyboard_report->mods & MOD_BIT(KC_RCTRL))
65:#define MODS_ALT  (keyboard_report->mods & MOD_BIT(KC_LALT) || keyboard_report->mods & MOD_BIT(KC_RALT))

keyboards/massdrop/alt/keymaps/reywood/keymap.c
63:#define MODS_SHIFT  (keyboard_report->mods & MOD_BIT(KC_LSHIFT) || keyboard_report->mods & MOD_BIT(KC_RSHIFT))
64:#define MODS_CTRL  (keyboard_report->mods & MOD_BIT(KC_LCTL) || keyboard_report->mods & MOD_BIT(KC_RCTRL))
65:#define MODS_ALT  (keyboard_report->mods & MOD_BIT(KC_LALT) || keyboard_report->mods & MOD_BIT(KC_RALT))

keyboards/massdrop/alt/keymaps/default/keymap.c
63:#define MODS_SHIFT  (keyboard_report->mods & MOD_BIT(KC_LSHIFT) || keyboard_report->mods & MOD_BIT(KC_RSHIFT))
64:#define MODS_CTRL  (keyboard_report->mods & MOD_BIT(KC_LCTL) || keyboard_report->mods & MOD_BIT(KC_RCTRL))
65:#define MODS_ALT  (keyboard_report->mods & MOD_BIT(KC_LALT) || keyboard_report->mods & MOD_BIT(KC_RALT))

keyboards/massdrop/ctrl/keymaps/default/keymap.c
66:#define MODS_SHIFT  (keyboard_report->mods & MOD_BIT(KC_LSHIFT) || keyboard_report->mods & MOD_BIT(KC_RSHIFT))
67:#define MODS_CTRL  (keyboard_report->mods & MOD_BIT(KC_LCTL) || keyboard_report->mods & MOD_BIT(KC_RCTRL))
68:#define MODS_ALT  (keyboard_report->mods & MOD_BIT(KC_LALT) || keyboard_report->mods & MOD_BIT(KC_RALT))

keyboards/massdrop/ctrl/keymaps/mac/keymap.c
66:#define MODS_SHIFT  (keyboard_report->mods & MOD_BIT(KC_LSHIFT) || keyboard_report->mods & MOD_BIT(KC_RSHIFT))
67:#define MODS_CTRL  (keyboard_report->mods & MOD_BIT(KC_LCTL) || keyboard_report->mods & MOD_BIT(KC_RCTRL))
68:#define MODS_ALT  (keyboard_report->mods & MOD_BIT(KC_LALT) || keyboard_report->mods & MOD_BIT(KC_RALT))

keyboards/massdrop/ctrl/keymaps/responsive_pattern/keymap.c
698:#define MODS_SHIFT  (keyboard_report->mods & MOD_BIT(KC_LSHIFT) || keyboard_report->mods & MOD_BIT(KC_RSHIFT))
699:#define MODS_CTRL  (keyboard_report->mods & MOD_BIT(KC_LCTL) || keyboard_report->mods & MOD_BIT(KC_RCTRL))
700:#define MODS_ALT  (keyboard_report->mods & MOD_BIT(KC_LALT) || keyboard_report->mods & MOD_BIT(KC_RALT))

And here's how those macros are used:


rg -P '(?<!#define )MODS_(SHIFT|CTRL|ALT)' -C1 keyboards/massdrop/
keyboards/massdrop/alt/keymaps/abishalom/keymap.c
147-        case U_T_AUTO:
148:            if (record->event.pressed && MODS_SHIFT && MODS_CTRL) {
149-                TOGGLE_FLAG_AND_PRINT(usb_extra_manual, "USB extra port manual mode");
--
152-        case U_T_AGCR:
153:            if (record->event.pressed && MODS_SHIFT && MODS_CTRL) {
154-                TOGGLE_FLAG_AND_PRINT(usb_gcr_auto, "USB GCR auto mode");

keyboards/massdrop/alt/keymaps/default/keymap.c
147-        case U_T_AUTO:
148:            if (record->event.pressed && MODS_SHIFT && MODS_CTRL) {
149-                TOGGLE_FLAG_AND_PRINT(usb_extra_manual, "USB extra port manual mode");
--
152-        case U_T_AGCR:
153:            if (record->event.pressed && MODS_SHIFT && MODS_CTRL) {
154-                TOGGLE_FLAG_AND_PRINT(usb_gcr_auto, "USB GCR auto mode");

keyboards/massdrop/alt/keymaps/mac/keymap.c
147-        case U_T_AUTO:
148:            if (record->event.pressed && MODS_SHIFT && MODS_CTRL) {
149-                TOGGLE_FLAG_AND_PRINT(usb_extra_manual, "USB extra port manual mode");
--
152-        case U_T_AGCR:
153:            if (record->event.pressed && MODS_SHIFT && MODS_CTRL) {
154-                TOGGLE_FLAG_AND_PRINT(usb_gcr_auto, "USB GCR auto mode");

keyboards/massdrop/alt/keymaps/reywood/keymap.c
149-    case U_T_AUTO:
150:      if (record->event.pressed && MODS_SHIFT && MODS_CTRL) {
151-        TOGGLE_FLAG_AND_PRINT(usb_extra_manual, "USB extra port manual mode");
--
154-    case U_T_AGCR:
155:      if (record->event.pressed && MODS_SHIFT && MODS_CTRL) {
156-        TOGGLE_FLAG_AND_PRINT(usb_gcr_auto, "USB GCR auto mode");

keyboards/massdrop/ctrl/keymaps/default/keymap.c
150-        case U_T_AUTO:
151:            if (record->event.pressed && MODS_SHIFT && MODS_CTRL) {
152-                TOGGLE_FLAG_AND_PRINT(usb_extra_manual, "USB extra port manual mode");
--
155-        case U_T_AGCR:
156:            if (record->event.pressed && MODS_SHIFT && MODS_CTRL) {
157-                TOGGLE_FLAG_AND_PRINT(usb_gcr_auto, "USB GCR auto mode");

keyboards/massdrop/ctrl/keymaps/mac/keymap.c
150-        case U_T_AUTO:
151:            if (record->event.pressed && MODS_SHIFT && MODS_CTRL) {
152-                TOGGLE_FLAG_AND_PRINT(usb_extra_manual, "USB extra port manual mode");
--
155-        case U_T_AGCR:
156:            if (record->event.pressed && MODS_SHIFT && MODS_CTRL) {
157-                TOGGLE_FLAG_AND_PRINT(usb_gcr_auto, "USB GCR auto mode");

keyboards/massdrop/ctrl/keymaps/responsive_pattern/keymap.c
783-        case U_T_AUTO:
784:            if (record->event.pressed && MODS_SHIFT && MODS_CTRL) {
785-                TOGGLE_FLAG_AND_PRINT(usb_extra_manual, "USB extra port manual mode");
--
788-        case U_T_AGCR:
789:            if (record->event.pressed && MODS_SHIFT && MODS_CTRL) {
790-                TOGGLE_FLAG_AND_PRINT(usb_gcr_auto, "USB GCR auto mode");

I wonder if this means that these keycodes don't work properly on Massdrop boards when NKRO is enabled. @patrickmt You might want to take a look.

patrickmt · 2019-01-24T15:33:34Z

Since the arm_atsam code runs mostly its own USB implementation, I do see misalignment issues when KEYBOARD_SHARED_EP is enabled. NKRO and MOUSE sharing are already being forced undefined, so I believe the best thing to do for now is to

#if defined(PROTOCOL_ARM_ATSAM)
  #undef KEYBOARD_SHARED_EP
#endif

in tmk_core/common/report.h around line 79, before KEYBOARD_REPORT_SIZE gets modified.

I've also confirmed changing keyboard_report->mods to get_mods() works properly.

drashna · 2019-01-24T19:22:33Z

I can add the get_mods() update to my IS_COMMAND PR, right now, as that would fix it in one go. Especially, since I'm already touching so many files....

vomindoraan · 2019-01-24T21:29:21Z

It should be easy to do once your PR is merged in, so I'd keep them separate for now. For the sake of not expanding the scope of your PR further, as well as to keep the goals distinct (optimization vs bugfix).

drashna · 2019-01-25T01:05:44Z

Sounds good.

vomindoraan · 2019-01-27T02:04:48Z

PR opened here: #4955

vomindoraan mentioned this issue Jan 18, 2019

Adds a default value for IS_COMMAND for COMMAND feature #4301

Merged

vomindoraan mentioned this issue Jan 27, 2019

Fix Command feature: use get_mods() instead of keyboard_report->mods #4955

Merged

13 tasks

drashna closed this as completed in #4955 Jan 27, 2019

vomindoraan mentioned this issue Feb 20, 2019

Add get_all_mods() function + clean up and reformat action_util files #5196

Closed

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Command feature not working since single HID endpoint was introduced #4838

Command feature not working since single HID endpoint was introduced #4838

vomindoraan commented Jan 13, 2019 •

edited

Loading

vomindoraan commented Jan 14, 2019

fauxpark commented Jan 14, 2019

vomindoraan commented Jan 14, 2019

vomindoraan commented Jan 14, 2019 •

edited

Loading

fauxpark commented Jan 14, 2019

vomindoraan commented Jan 14, 2019

fauxpark commented Jan 15, 2019

vomindoraan commented Jan 15, 2019 •

edited

Loading

fauxpark commented Jan 15, 2019

vomindoraan commented Jan 15, 2019

vomindoraan commented Jan 15, 2019

fauxpark commented Jan 15, 2019

vomindoraan commented Jan 15, 2019 •

edited

Loading

vomindoraan commented Jan 15, 2019 •

edited

Loading

vomindoraan commented Jan 22, 2019

patrickmt commented Jan 24, 2019

drashna commented Jan 24, 2019

vomindoraan commented Jan 24, 2019

drashna commented Jan 25, 2019

vomindoraan commented Jan 27, 2019

Command feature not working since single HID endpoint was introduced #4838

Command feature not working since single HID endpoint was introduced #4838

Comments

vomindoraan commented Jan 13, 2019 • edited Loading

Describe the bug

System Information

vomindoraan commented Jan 14, 2019

fauxpark commented Jan 14, 2019

vomindoraan commented Jan 14, 2019

vomindoraan commented Jan 14, 2019 • edited Loading

fauxpark commented Jan 14, 2019

vomindoraan commented Jan 14, 2019

fauxpark commented Jan 15, 2019

vomindoraan commented Jan 15, 2019 • edited Loading

fauxpark commented Jan 15, 2019

vomindoraan commented Jan 15, 2019

vomindoraan commented Jan 15, 2019

fauxpark commented Jan 15, 2019

vomindoraan commented Jan 15, 2019 • edited Loading

vomindoraan commented Jan 15, 2019 • edited Loading

vomindoraan commented Jan 22, 2019

patrickmt commented Jan 24, 2019

drashna commented Jan 24, 2019

vomindoraan commented Jan 24, 2019

drashna commented Jan 25, 2019

vomindoraan commented Jan 27, 2019

vomindoraan commented Jan 13, 2019 •

edited

Loading

vomindoraan commented Jan 14, 2019 •

edited

Loading

vomindoraan commented Jan 15, 2019 •

edited

Loading

vomindoraan commented Jan 15, 2019 •

edited

Loading

vomindoraan commented Jan 15, 2019 •

edited

Loading