ARMmbed / mbed-os

Arm Mbed OS is a platform operating system designed for the internet of things
https://mbed.com
Other
4.66k stars 2.97k forks source link

USBKeyboard limited to 6 keys (Missing NKRO) #15332

Open LoaderB0T opened 2 years ago

LoaderB0T commented 2 years ago

Description of defect

The Implementation of USBKeyboard and USBMouseKeyboard are limiting the transferrable keys to 6. (6-key-rollover aka 6-KRO).

This is a mostly unnecessary limitation as most OSs correctly implement the HID standard for keyboards and can accept more than 6 bytes of key data. However, older BIOSs might actually still have this limitation, hence the widespread belief that more than 6 is impossible. (SOURCE)

It would be really cool to be able to use mbed os to write keyboard firmware with NKRO (N-key-rollover).

The send method defined in the USBHID class, accepts any report with any length. Setting the length of the report to above 9 will fail, because the host (In my case windows 11) reports a IRP USBD_STATUS of 0xc0000012 which translates to USBD_STATUS_BABBLE_DETECTED which basically means I got more data than expected and will therefore ignore this event and reset the device. https://github.com/ARMmbed/mbed-os/blob/807fd7965164737a1c08e1b50e710b2f854ef193/drivers/usb/source/USBHID.cpp#L172

Windows expects only 6 keys to be sent because of the HID descriptor sent to Windows at the beginning. This descriptor is defined here: https://github.com/ARMmbed/mbed-os/blob/807fd7965164737a1c08e1b50e710b2f854ef193/drivers/usb/source/USBKeyboard.cpp#L381

The count of keys is defined here https://github.com/ARMmbed/mbed-os/blob/807fd7965164737a1c08e1b50e710b2f854ef193/drivers/usb/source/USBKeyboard.cpp#L413

I am by no means an expert when it comes to HID, c++, USB, or anything involved here, but based on a couple of hours of research and lots of trial and error, I figured out that by increasing this 0x06 to something larger allows for more keys to be sent. I changed it to 0x3D which is the maximum possible without changing the rest of the descriptor (Because somewhere the maximum report size is defined to be 64 (0x3D = 61 and 3 additional bytes needed by the protocol)

My suggestion: Without introducing a breaking change, it would be possible to switch this 0x06 to 0x3D (or something else) with a compiler flag (eg. USB_KEYBOARD_USE_NKRO) or something.

Also, I know that 61 is not technically "N" so it would actually be a 61-KRO but seriously how many fingers do you have? 😆

For now, I worked around this by extending the USBKeyboard class and overriding the descriptor method with the adjusted data.

I will gladly create a PR to introduce the flag if the community finds this useful.

Target(s) affected by this defect ?

Discovered with NUCLEO_F446ZE Affected are all targets

Toolchain(s) (name and version) displaying this defect ?

arm-none-eabi-gcc (GNU Arm Embedded Toolchain 10.3-2021.07) 10.3.1 20210621 (release)

What version of Mbed-os are you using (tag or sha) ?

mbed-os-6.16.0 54e8693ef4ff7e025018094f290a1d5cf380941f

What version(s) of tools are you using. List all that apply (E.g. mbed-cli)

mbed-cli version 1.10.5

How is this defect reproduced ?

#include "mbed.h"
#include "USBKeyboard.h"

USBKeyboard keyboard;

void main()
{
  HID_REPORT report;

  report.data[0] = 0x01;
  report.data[1] = 0;
  report.data[2] = 0;
  report.data[3] = 0;
  report.data[4] = 0;
  report.data[5] = 0;
  report.data[6] = 0;
  report.data[7] = 0;
  report.data[8] = 0;
  report.data[9] = 0;

  report.length = 10;

  keyboard.send(&report);
}
mbedmain commented 2 years ago

@LoaderB0T thank you for raising this issue.Please take a look at the following comments:

What toolchain(s) are you using?

NOTE: If there are fields which are not applicable then please just add 'n/a' or 'None'. This indicates to us that at least all the fields have been considered. Please update the issue header with the missing information.

Epipeppy commented 1 year ago

@LoaderB0T I'm working with an arduino I got earlier this year that is using the RP2040 chip (Arduino RP2040 Connect). The core for this board uses the mbed library, and I was hoping to make a game controller using the device. One of the games I planned on playing (Guitar Hero) could easily hit the 6-key limit. Is fixing this as simple as changing the count, or is there more to it?

I found that the MAX_HID size is set here (in case that's of any interest): https://github.com/ARMmbed/mbed-os/blob/c299c8bc13903210050489d5064f4f65ecba1dd4/drivers/usb/include/usb/internal/USBHID_Types.h#L86

LoaderB0T commented 1 year ago

Hi @Epipeppy yes for me increasing this one number is the only required change for changing the number of keys. Beware: You always have to send the full report length. So if you increase 0x06 to 0x08 for example, you HAVE to send 8 keys (or zeros).

Thanks for the info about the max report size, would be interesting to see if increasing it would also adjust the limit to something larger than 61 (although probably not necessary).

Epipeppy commented 1 year ago

Appreciate it!