Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug] NKRO distorts keymap on Drop CTRL board #23939

Open
2 tasks
Izumemori opened this issue Jun 16, 2024 · 2 comments · May be fixed by #23945
Open
2 tasks

[Bug] NKRO distorts keymap on Drop CTRL board #23939

Izumemori opened this issue Jun 16, 2024 · 2 comments · May be fixed by #23945

Comments

@Izumemori
Copy link

Izumemori commented Jun 16, 2024

Describe the Bug

When enabling NKRO (via the QK_MAGIC_TOGGLE_NKRO or QK_MAGIC_NKRO_ON Magic Keycode) the keyboard keymap gets distorted.
For instance W types 5, X types K, \ toggles Caps lock (even though I have no Caps lock in my actual keymap!).
Disabling NKRO brings the keymap back to normal but C, I, and Backspace stop functioning and everything else is typed as if caps lock is enabled while not actually enabled (Pressing shift doesn't do anything).
Only a power cycle of the keyboard fixes this, as long as NKRO is off or not written to EEPROM before the power cycle.

Keyboard Used

massdrop/ctrl

Link to product page (if applicable)

https://drop.com/buy/drop-ctrl-mechanical-keyboard

Operating System

Arch Linux 64bit & NixOS 24.11 unstable

qmk doctor Output

Ψ QMK Doctor is checking your environment.
Ψ CLI version: 1.1.5
Ψ QMK home: /workspaces/qmk_firmware
Ψ Detected Linux (Debian GNU/Linux 11 (bullseye)).
⚠ Missing or outdated udev rules for 'atmel-dfu' boards. Run 'sudo cp /workspaces/qmk_firmware/util/udev/50-qmk.rules /etc/udev/rules.d/'.
⚠ Missing or outdated udev rules for 'kiibohd' boards. Run 'sudo cp /workspaces/qmk_firmware/util/udev/50-qmk.rules /etc/udev/rules.d/'.
⚠ Missing or outdated udev rules for 'stm32-dfu' boards. Run 'sudo cp /workspaces/qmk_firmware/util/udev/50-qmk.rules /etc/udev/rules.d/'.
⚠ Missing or outdated udev rules for 'apm32-dfu' boards. Run 'sudo cp /workspaces/qmk_firmware/util/udev/50-qmk.rules /etc/udev/rules.d/'.
⚠ Missing or outdated udev rules for 'gd32v-dfu' boards. Run 'sudo cp /workspaces/qmk_firmware/util/udev/50-qmk.rules /etc/udev/rules.d/'.
⚠ Missing or outdated udev rules for 'wb32-dfu' boards. Run 'sudo cp /workspaces/qmk_firmware/util/udev/50-qmk.rules /etc/udev/rules.d/'.
⚠ Missing or outdated udev rules for 'bootloadhid' boards. Run 'sudo cp /workspaces/qmk_firmware/util/udev/50-qmk.rules /etc/udev/rules.d/'.
⚠ Missing or outdated udev rules for 'usbasploader' boards. Run 'sudo cp /workspaces/qmk_firmware/util/udev/50-qmk.rules /etc/udev/rules.d/'.
⚠ Missing or outdated udev rules for 'usbtinyisp' boards. Run 'sudo cp /workspaces/qmk_firmware/util/udev/50-qmk.rules /etc/udev/rules.d/'.
⚠ Missing or outdated udev rules for 'md-boot' boards. Run 'sudo cp /workspaces/qmk_firmware/util/udev/50-qmk.rules /etc/udev/rules.d/'.
⚠ Missing or outdated udev rules for 'caterina' boards. Run 'sudo cp /workspaces/qmk_firmware/util/udev/50-qmk.rules /etc/udev/rules.d/'.
⚠ Missing or outdated udev rules for 'hid-bootloader' boards. Run 'sudo cp /workspaces/qmk_firmware/util/udev/50-qmk.rules /etc/udev/rules.d/'.
Ψ Testing userspace candidate: /workspaces/qmk_userspace -- Valid `qmk.json`
Ψ QMK userspace: /workspaces/qmk_userspace
Ψ Userspace enabled: True
Ψ Git branch: master
Ψ Repo version: 0.25.8
⚠ The official repository does not seem to be configured as git remote "upstream".
Ψ All dependencies are installed.
Ψ Found arm-none-eabi-gcc version 8.3.1
Ψ Found avr-gcc version 8.3.0
Ψ Found avrdude version 6.3-20171130
Ψ Found dfu-programmer version 0.6.1
Ψ Found dfu-util version 0.9
Ψ Submodules are up to date.
Ψ Submodule status:
Ψ - lib/chibios: 2024-02-17 19:20:06 +0000 --  (be44b3305)
Ψ - lib/chibios-contrib: 2024-04-03 20:39:24 +0800 --  (77cb0a4f)
Ψ - lib/googletest: 2021-06-11 06:37:43 -0700 --  (e2239ee6)
Ψ - lib/lufa: 2022-08-26 12:09:55 +1000 --  (549b97320)
Ψ - lib/vusb: 2022-06-13 09:18:17 +1000 --  (819dbc1)
Ψ - lib/printf: 2022-06-29 23:59:58 +0300 --  (c2e3b4e)
Ψ - lib/pico-sdk: 2023-02-12 20:19:37 +0100 --  (a3398d8)
Ψ - lib/lvgl: 2022-04-11 04:44:53 -0600 --  (e19410f8)
Ψ QMK is ready to go, but minor problems were found

Is AutoHotKey / Karabiner installed

  • AutoHotKey (Windows)
  • Karabiner (macOS)

Other keyboard-related software installed

None

Additional Context

I have recently upgraded my keyboards firmware from a 3 year old version (fd54992) to the latest commit. Everything worked fine with the old firmware.

My config is at https://github.com/Izumemori/qmk_userspace, but it also happens with the default keymap at commit 751fbd7.

The udev rules from the qmk doctor output shouldn't matter since I'm developing in a dev container.

@sigprof
Copy link
Contributor

sigprof commented Jun 16, 2024

This was probably broken by #22267 — apparently the NKRO implementation for arm_atsam uses a completely separate HID interface and therefore does not use a report ID byte for NKRO, but the new code always allocates uint8_t report_id; in report_nkro_t.

The ATSAM case was apparently the only one where NKRO_SHARED_EP was not defined in the old code (and apparently the same thing had been done with MOUSE_SHARED_EP, so the mouse emulation might also be broken on that platform).

#define NKRO_SHARED_EP
/* key report size(NKRO or boot mode) */
#if defined(NKRO_ENABLE)
#    if defined(PROTOCOL_LUFA) || defined(PROTOCOL_CHIBIOS)
#        include "protocol/usb_descriptor.h"
#        define NKRO_REPORT_BITS (SHARED_EPSIZE - 2)
#    elif defined(PROTOCOL_ARM_ATSAM)
#        include "protocol/arm_atsam/usb/udi_device_epsize.h"
#        define NKRO_REPORT_BITS (NKRO_EPSIZE - 1)
#        undef NKRO_SHARED_EP
#        undef MOUSE_SHARED_EP
#    else
#        error "NKRO not supported with this protocol"
#    endif
#endif

@Izumemori
Copy link
Author

Izumemori commented Jun 17, 2024

This seems to be what caused it.
My local changes are:

diff --git a/tmk_core/protocol/host.c b/tmk_core/protocol/host.c
index 732fbdc37d..d5d56fcec7 100644
--- a/tmk_core/protocol/host.c
+++ b/tmk_core/protocol/host.c
@@ -97,7 +97,9 @@ void host_keyboard_send(report_keyboard_t *report) {
 
 void host_nkro_send(report_nkro_t *report) {
     if (!driver) return;
+#ifdef NKRO_SHARED_EP
     report->report_id = REPORT_ID_NKRO;
+#endif
     (*driver->send_nkro)(report);
 
     if (debug_keyboard) {
diff --git a/tmk_core/protocol/report.h b/tmk_core/protocol/report.h
index 0e4f6e9def..03acb3799e 100644
--- a/tmk_core/protocol/report.h
+++ b/tmk_core/protocol/report.h
@@ -133,8 +133,21 @@ enum desktop_usages {
 };
 
 // clang-format on
-
-#define NKRO_REPORT_BITS 30
+#if defined(PROTOCOL_ARM_ATSAM)
+#    include "protocol/arm_atsam/usb/udi_device_epsize.h"
+#    define NKRO_REPORT_BITS (NKRO_EPSIZE - 1)
+#    undef NKRO_SHARED_EP
+#    undef MOUSE_SHARED_EP
+#else
+#    define NKRO_REPORT_BITS 30
+#endif
 
 #ifdef KEYBOARD_SHARED_EP
 #    define KEYBOARD_REPORT_SIZE 9
@@ -178,7 +191,9 @@ typedef struct {
 } PACKED report_keyboard_t;
 
 typedef struct {
+#ifdef NKRO_SHARED_EP
     uint8_t report_id;
+#endif
     uint8_t mods;
     uint8_t bits[NKRO_REPORT_BITS];
 } PACKED report_nkro_t;

With that at least NKRO works again, have not tried mouse emulation yet.

But since host_mouse_send already respects MOUSE_SHARED_EP, I don't see why it shouldn't work

void host_mouse_send(report_mouse_t *report) {
#ifdef BLUETOOTH_ENABLE
if (where_to_send() == OUTPUT_BLUETOOTH) {
bluetooth_send_mouse(report);
return;
}
#endif
if (!driver) return;
#ifdef MOUSE_SHARED_EP
report->report_id = REPORT_ID_MOUSE;
#endif
#ifdef MOUSE_EXTENDED_REPORT
// clip and copy to Boot protocol XY
report->boot_x = (report->x > 127) ? 127 : ((report->x < -127) ? -127 : report->x);
report->boot_y = (report->y > 127) ? 127 : ((report->y < -127) ? -127 : report->y);
#endif
(*driver->send_mouse)(report);
}

@Izumemori Izumemori linked a pull request Jun 17, 2024 that will close this issue
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants