Pressing dual-function keys disables NKRO

[florentc]

keyboard: Ergodox Infinity OS: ArchLinux (linux 4.12.4) layout: custom layout based on default compilation: FORCE_NKRO and NKRO_ENABLE=yes

I encounter a problem regarding n-key rollover and dual-function keys. Whenever I tap a dual-function key or hold it down, n-key rollover is then disabled and I have to turn it back on with lshift+rshift+n.

1. Plug-in the keyboard
2. Press more than 6 keys (regular letters, no dual-function) at the same time: all the presses register
3. Pick a dual-function key (e.g. Shift/Backspace): press then release
4. Press the same keys as step 2: only 6 register
5. Enable NKRO back with lshift+rshift+n
6. Back to step 2

Note: Provided the dual-function key corresponds to modifier mod (holding) and command (pressing), pressing regular keys that send the keycodes for mod or command do not trigger the issue. This is a matter of dual-function keys and NKRO.

I may join the custom layout if required.

[pete-debiase]

Experiencing the same problem on an ErgoDox EZ on Windows 10, also with #define FORCE_NKRO and NKRO_ENABLE = yes in config file and makefile, respectively.

I have a copy of the QMK firmware from approximately three weeks ago that does not exhibit this problem. I have been running diffs between this older version and the latest version and have so far been unsuccessful in identifying what may be causing this.

One of the biggest changes in the newer version is the addition of the qmk_firmware/drivers directory and associated changes to the codebase. Could this be possibly be a driver-related issue?

There do not appear to be any changes of concern in the Ergodox-specific files in qmk_firmware\keyboards\ergodox.

[florentc]

Do you know which commit does not exhibit the problem? I plan on trying them one by one to narrow it down to a specific "guilty" commit.

[pete-debiase]

That sounds like a good approach. It looks like the last commit on master in my working copy is 8edb67b08242f2ab641d7e658a0a7adb579bbae2 ("fix line endings" from Jack Humbert on 2017-07-21 16:05:13), so presumably it's something between then and now.

[florentc]

More info:

• In my case, the problem also occurs with commit 8edb67b08242f2ab641d7e658a0a7adb579bbae2
• It occurs with the default layout
• FORCE_NKRO does not matter

[jackhumbert]

This diff may make things a bit easier to look into - I haven't been able to find anything yet though.

[pete-debiase]

Dang, it sounds like it may be older than 8edb67b then. I will take another look at it this weekend and see if I can find anything. Let me know if you have any luck narrowing it down further.

[florentc]

Picked a commit at random: 74d752b531f4c5e90ceb3b8e55fdc68da35aa2ff This one is ok.

[pete-debiase]

Howdy @jackhumbert and @florentc,

I finally found a bit of time to work on this issue. This bug seems to have been introduced in a25dbaa in the qmk_firmware/quantum/keymap_common.c file. Or, at least that's one place in the history at which it can be isolated. Here's a diff.

As a temporary workaround, you can replace the keymap_common.c file with the version of the keymap_common.c file from the commit prior to a25dbaa (which is b82604dadad81872ab). Upon doing this, the firmware can still be built successfully, and NKRO will no longer be disabled after a dual-function key is pressed. I have confirmed that this works with the very latest version of the QMK firmware and have not noticed any side effects from reverting keymap_common.c to this earlier state.

Note that #define FORCE_NKRO must still be present in the appropriate config.h file and NKRO_ENABLE = yes must still be present in your rules.mk file.

I hesitate to submit a patch because I am unsure how this workaround will affect pull request #1202, for which these changes were originally made, and will therefore defer to Jack's judgment.

Cheers,

Pete

[fredizzimo]

To me it's pretty clear what's happening. If you look here https://github.com/qmk/qmk_firmware/commit/61cdc9aaa462afbcbaf57f2c5991e06924caed0e#diff-f451c4a749badc1f3d3f2e5f5cbc1c6aR93

It call's keymap_config.raw = eeconfig_read_keymap();, which forces a read of the configuration from the EEPROM. The NKRO setting is stored in the same keymap_config, so it gets overridden, by doing this. When you are using FORCE_NKRO, it normally ignores the value stored in EEPROM, by setting keymap_config.nkro=true; after it has been read. But now the extra eeconfig_read_keymap(); call overrides that value.

The fix should be as easy as just deleting the line that reads the configuration. It should always be read at the keyboard boot anyway, and the struct should already contain the right values.