[Bug] Enabling NKRO on the GMMK Pro through makes all keys but the multimedia keys unresponsive

[BB-66]

Describe the Bug

Steps to replicate the bug (this is from the develop branch to make use of the RGB implemented there):

• flash GMMK Pro with jonavin keymap - I am unsure the poor dude is at fault, I ripped only his encoder code and used a "stock" keymap that only used VIA Enable = yes and this bug still happens;
• check to see if NKRO is enabled; even though NKRO is enabled in rules.mk, keyboard only does 6KRO;
• boot VIA;
• assign NKRO to key;
• press key;
• stare sheepishly at the screen as no keys are working on the keyboard anymore;
• unplug keyboard and replug keyboard; keys are still not working;
• unplug keyboard and plugin whilst pressing Esc;
• reflash with QMK firmware I used previously (VIA not enabled, I am unsure if that matters though);
• test keys -> everything is working well;
• reflash firmware mentioned in the first row;
• rinse and repeat.

I am unsure if this is a QMK bug, a bug within the code for the GMMK Pro or a VIA bug. All I know is that keycode works all right with all my other keyboards running QMK and VIA and does what it says on the can (turns NKRO on and off).

I have not tested with forcing NKRO on from the get-go. NKRO enable = yes is present in rules.mk.

System Information

• Keyboard:
  • GMMK Pro
• Operating system: N/A
• AVR GCC version: 8.4.0

• ARM GCC version: 10.1.0

• QMK Firmware version: 0.13.21

• Any keyboard related software installed?
  • AutoHotKey
  • Karabiner
  • Other: no

Additional Context

[hmfaysal]

I changed the encoder code in the keymap.c file to this and it works with NKRO support now

    bool encoder_update_user(uint8_t index, bool clockwise) {
        if (clockwise) {
          tap_code(KC_VOLU);
        } else {
          tap_code(KC_VOLD);
        }
        return true;
    }

[drashna]

You might need #define TAP_CODE_DELAY 10 to get it working normally.

Or:

bool encoder_update_user(uint8_t index, bool clockwise) {
    if (clockwise) {
        tap_code_delay(KC_VOLU, 10);
    } else {
        tap_code_delay(KC_VOLD, 10);
    }
    return true;
}

[andrebrait]

I changed the encoder code in the keymap.c file to this and it works with NKRO support now

    bool encoder_update_user(uint8_t index, bool clockwise) {
        if (clockwise) {
          tap_code(KC_VOLU);
        } else {
          tap_code(KC_VOLD);
        }
        return true;
    }

This is already the default implementation.
I have tested all my media keys and the encoder with NKRO and with 6KRO and they both work just fine.
I don't use VIA though. You can test my keymap if you want. It doesn't have VIA, but Fn+N will toggle between 6KRO and NKRO.

It's based on the develop branch, btw.

https://github.com/andrebrait/qmk_firmware/tree/andrebrait_keymap/keyboards/gmmk/pro/ansi/keymaps/andrebrait

[andrebrait]

@drashna I think the keymap he's using has some custom logic for the encoder

[drashna]

Fir the media keys, you may need #define TAP_CODE_DELAY 10 to get them to work right with NKRO

[andrebrait]

Fir the media keys, you may need #define TAP_CODE_DELAY 10 to get them to work right with NKRO

That's already defined here

qmk_firmware/keyboards/gmmk/pro/config.h

Line 44 in dbee960

#define TAP_CODE_DELAY 10

So if he still has issues, that's probably because of something else, isn't it?

[Jonavin]

Hi. I've tested the current qmk master version of my gmmk pro keymap. It has NKRO enabled and all of the encoder function works except RSFT + encoder for PgUp/PgDn. There's a type in there. This should be fixed in the new PR.

Media function works just fine. If you use the RSFT+encoder function it could cause the other encoder function to not work properly because it did not release the Shift mod state due to the bug.

[andrebrait]

@Jonavin what part of the PR would fix this?

[Jonavin]

@andrebrait

This would be the change needed in the current published version. The current PR moves a lot of the code into user space

[Jonavin]

Not sure if you are still having issues with NKRO, but referencing this article (https://qmk.fm/changes/2019-01-27-fix-command-feature-use-get-mods-instead-of-keyboard-report-mods), I am changing the "keyboard_report->mods" to get_mods(). This seems to address the issue for GMMK Pro.

[BB-66]

Thank you everybody so much for the input! I've been away for a while, will have a look and return with feedback. Indeed I would like to retain the custom encoder functionality and at the same time enable NKRO without any issues rather than only have volume up/down mapped to it.