Bug in USBHIDKeyboard.cpp (libraries/USB/src/USBHIDKeyboard.cpp)

[tommybee456]

Board

esp32s2

Device Description

N/A

Hardware Configuration

N/A

Version

v3.0.4

IDE Name

Arduino IDE

Operating System

Windows 11

Flash frequency

40

PSRAM enabled

yes

Upload speed

921600

Description

There is a bug in the USBHIDKeyboard.cpp source file that will not allow just modifier keys to be sent in an HID report. This means that, for example, if shift is being held, the computer will only know that when another key is pushed. This also means the only way shift can be released on the computer side is if shift is released on the keyboard while another key is being pressed. I hope that makes sense.

This also causes issues in games like CS2 where crouch is set to control but a crouch command is not send until another key is pushed and this also causes some very strange behavior.

In the source code you can see that if a modifier is pressed with USBHIDKeyboard::press it is stored in the keyreport and then it calls pressRaw(). The issue is in pressedRaw() is looking for a modifier in the range of 0xE0-0xE8(these are keypad numbers?) but shift(0x80) is not in that range nor are any other modifiers so pressRaw() will just return 0 without sending the keyreport. Additionally pressRaw() should not be trying to store modifiers in the keyreport since press() already did. I'm not sure what the right fix should be but for my use case if I comment out return 0 in pressRaw and releaseRaw this solves the problem(but also makes it possible to send invalid keys commands-ill just be careful lol).

Sketch

size_t USBHIDKeyboard::pressRaw(uint8_t k) {
  uint8_t i;
  if (k >= 0xE0 && k < 0xE8) {
    // it's a modifier key
    _keyReport.modifiers |= (1 << (k - 0xE0));
  } else if (k && k < 0xA5) {
    // Add k to the key report only if it's not already present
    // and if there is an empty slot.
    if (_keyReport.keys[0] != k && _keyReport.keys[1] != k && _keyReport.keys[2] != k && _keyReport.keys[3] != k && _keyReport.keys[4] != k
        && _keyReport.keys[5] != k) {

      for (i = 0; i < 6; i++) {
        if (_keyReport.keys[i] == 0x00) {
          _keyReport.keys[i] = k;
          break;
        }
      }
      if (i == 6) {
        return 0;
      }
    }
  } else {
    //not a modifier and not a key
    return 0;
  }
  sendReport(&_keyReport);
  return 1;
}

size_t USBHIDKeyboard::releaseRaw(uint8_t k) {
  uint8_t i;
  if (k >= 0xE0 && k < 0xE8) {
    // it's a modifier key
    _keyReport.modifiers &= ~(1 << (k - 0xE0));
  } else if (k && k < 0xA5) {
    // Test the key report to see if k is present.  Clear it if it exists.
    // Check all positions in case the key is present more than once (which it shouldn't be)
    for (i = 0; i < 6; i++) {
      if (0 != k && _keyReport.keys[i] == k) {
        _keyReport.keys[i] = 0x00;
      }
    }
  } else {
    //not a modifier and not a key
    return 0;
  }

  sendReport(&_keyReport);
  return 1;
}

// press() adds the specified key (printing, non-printing, or modifier)
// to the persistent key report and sends the report.  Because of the way
// USB HID works, the host acts like the key remains pressed until we
// call release(), releaseAll(), or otherwise clear the report and resend.
size_t USBHIDKeyboard::press(uint8_t k) {
  if (k >= 0x88) {  // it's a non-printing key (not a modifier)
    k = k - 0x88;
  } else if (k >= 0x80) {  // it's a modifier key
    _keyReport.modifiers |= (1 << (k - 0x80));
    k = 0;
  } else {  // it's a printing key
    k = _asciimap[k];
    if (!k) {
      return 0;
    }
    if (k & 0x80) {  // it's a capital letter or other character reached with shift
      // At boot, some PCs need a separate report with the shift key down like a real keyboard.
      if (shiftKeyReports) {
        pressRaw(HID_KEY_SHIFT_LEFT);
      } else {
        _keyReport.modifiers |= 0x02;  // the left shift modifier
      }
      k &= 0x7F;
    }
  }
  return pressRaw(k);
}

// release() takes the specified key out of the persistent key report and
// sends the report.  This tells the OS the key is no longer pressed and that
// it shouldn't be repeated any more.
size_t USBHIDKeyboard::release(uint8_t k) {
  if (k >= 0x88) {  // it's a non-printing key (not a modifier)
    k = k - 0x88;
  } else if (k >= 0x80) {  // it's a modifier key
    _keyReport.modifiers &= ~(1 << (k - 0x80));
    k = 0;
  } else {  // it's a printing key
    k = _asciimap[k];
    if (!k) {
      return 0;
    }
    if (k & 0x80) {  // it's a capital letter or other character reached with shift
      if (shiftKeyReports) {
        releaseRaw(k & 0x7F);    // Release key without shift modifier
        k = HID_KEY_SHIFT_LEFT;  // Below, release shift modifier
      } else {
        _keyReport.modifiers &= ~(0x02);  // the left shift modifier
        k &= 0x7F;
      }
    }
  }
  return releaseRaw(k);
}

Debug Message

No Debug messaged

Other Steps to Reproduce

Try using USBHIDKeyboard.h to send just a shift press and release and use a website like keyboardtester to see what happens, compare that with your normal keyboard and see what happens.

I have checked existing issues, online documentation and the Troubleshooting Guide

• I confirm I have checked existing issues, online documentation and Troubleshooting guide.

[KezzaWezza]

Hi,
This bug is effecting me and my project at the moment, I am building a controller (for gaming). Sorry I don't have the knowledge to help with a solution, for now I am using regular keys and rebinding them in-game ('L' for crouch, 'k' for sprint). I hope someone can fix this issue.

[SuGlider]

@tommybee456 @KezzaWezza

There are 2 possible ways to send a report to the Host.
You shall use USBHIDKeyboard::pressRaw(uint8_t HID_keycode)/ USBHIDKeyboard::releaseRaw(uint8_t HID_keycode) in order to see just SHIFT/CTRL/ALT key press/release effect in keyboardtester

1- Sending ASCII code characters:

USBHIDKeyboard::press(uint8_t ascii_key) key is an ASCII code.
For instance, USBHIDKeyboard::press('A') will send a report with a (0x04 => HID KeyCode for a) + SHIFT Modifier.
USBHIDKeyboard::press(KEY_LEFT_CTRL) will just set the SHIFT Modifier and send nothing to the USB Host

2- Sending HID KeyCodes directly

USBHIDKeyboard::pressRaw(uint8_t hid_keycode) and USBHIDKeyboard::releaseRaw(uint8_t HID_keycode) work with HID KeyCodes.
HID Code Table at https://github.com/hathach/tinyusb/blob/master/src/class/hid/hid.h#L366-L586
In particular this is the HID KeyCodes table for Modifier Keys:

#define HID_KEY_CONTROL_LEFT                0xE0
#define HID_KEY_SHIFT_LEFT                  0xE1
#define HID_KEY_ALT_LEFT                    0xE2
#define HID_KEY_GUI_LEFT                    0xE3
#define HID_KEY_CONTROL_RIGHT               0xE4
#define HID_KEY_SHIFT_RIGHT                 0xE5
#define HID_KEY_ALT_RIGHT                   0xE6
#define HID_KEY_GUI_RIGHT                   0xE7

[tommybee456]

@SuGlider

Hmm I still feel like that is a workaround rather than a fix. Why would anyone want to set the modifiers in the keyreport but not send the report? If you look at the regular press() and release() functions it already deals with capital letters correctly so I still think this is a bug that should be fixed. Could just do something like this:

size_t USBHIDKeyboard::pressRaw(uint8_t k) {
  uint8_t i;
  if (k >= 0xE0 && k < 0xE8) {
    // it's a modifier key
    _keyReport.modifiers |= (1 << (k - 0xE0));
  } else if (k && k < 0xA5) {
    // Add k to the key report only if it's not already present
    // and if there is an empty slot.
    if (_keyReport.keys[0] != k && _keyReport.keys[1] != k && _keyReport.keys[2] != k && _keyReport.keys[3] != k && _keyReport.keys[4] != k
        && _keyReport.keys[5] != k) {

      for (i = 0; i < 6; i++) {
        if (_keyReport.keys[i] == 0x00) {
          _keyReport.keys[i] = k;
          break;
        }
      }
      if (i == 6) {
        return 0;
      }
    }
  } else if(k >= 0x80 && k <= 0x87){
    //is modifier and already in keyreport
    sendReport(&_keyReport);
    return 1;
  } else {
    //not a modifier and not a key
    return 0;
  }
  sendReport(&_keyReport);
  return 1;
}

[SuGlider]

Why would anyone want to set the modifiers in the keyreport but not send the report?

Looking at the example in https://github.com/espressif/arduino-esp32/blob/master/libraries/USB/examples/Keyboard/KeyboardLogout/KeyboardLogout.ino

There is this code:

    case WINDOWS:
      // CTRL-ALT-DEL:
      Keyboard.press(KEY_LEFT_CTRL);
      Keyboard.press(KEY_LEFT_ALT);
      Keyboard.press(KEY_DELETE);
      delay(100);
      Keyboard.releaseAll();

This is how that was intended to be used.

[SuGlider]

This other example uses CTRL-keys to control Arduino IDE new file, code insert, formating, uploading...
https://github.com/espressif/arduino-esp32/blob/master/libraries/USB/examples/Keyboard/KeyboardReprogram/KeyboardReprogram.ino

The library was created for the Arduino Due and Leonard.

[SuGlider]

Sending SHIFT / CTRL / ALT reports with no keys, is an exceptional usage.

I may want to presse SHIFT + CTRL + ']' for instance. That's what the special keys in https://github.com/espressif/arduino-esp32/blob/master/libraries/USB/src/USBHIDKeyboard.h are for.

[SuGlider]

The code from USBHIDKeyboard comes from the Arduino repository.

  Copyright (c) 2015, Arduino LLC
  Original code (pre-library): Copyright (c) 2011, Peter Barrett

[tommybee456]

That would still work with the change I recommended. It does not matter if 2 key reports are sent before the Delete key press or the ']' keypress is sent.

Sorry but I still can't think of any use case where someone would want that.

Sending SHIFT / CTRL / ALT reports with no keys, is an exceptional usage.

This is how every keyboard I have ever encountered works. I have literally never seen a keyboard that doesn't. They will always send a key report with only modifier keys if those are the only keys being pressed.

The code from USBHIDKeyboard comes from the Arduino repository.

It says that but the original Arduino code is different. You can check it here:
https://github.com/arduino-libraries/Keyboard/blob/master/src/Keyboard.cpp

That library will send a key report with only modifier keys.

[SuGlider]

We shall be the most possible compatible to Arduino upstream code.
Based on the latest Keyboard Arduino Library code, it has been updated in the direction that your are pointing.

I'll look a way to update ESP32 Arduino USBHIDKeyboard into the same direction.

// press() adds the specified key (printing, non-printing, or modifier)
// to the persistent key report and sends the report.  Because of the way
// USB HID works, the host acts like the key remains pressed until we
// call release(), releaseAll(), or otherwise clear the report and resend.
size_t Keyboard_::press(uint8_t k)
{
	uint8_t i;
	if (k >= 0x88) {			// it's a non-printing key (not a modifier)
		k = k - 0x88;
	} else if (k >= 0x80) {	// it's a modifier key
		_keyReport.modifiers |= (1<<(k-0x80));
		k = 0;
	} else {				// it's a printing key
		k = pgm_read_byte(_asciimap + k);
		if (!k) {
			setWriteError();
			return 0;
		}
		if ((k & ALT_GR) == ALT_GR) {
			_keyReport.modifiers |= 0x40;   // AltGr = right Alt
			k &= 0x3F;
		} else if ((k & SHIFT) == SHIFT) {
			_keyReport.modifiers |= 0x02;	// the left shift modifier
			k &= 0x7F;
		}
		if (k == ISO_REPLACEMENT) {
			k = ISO_KEY;
		}
	}

	// Add k to the key report only if it's not already present
	// and if there is an empty slot.
	if (_keyReport.keys[0] != k && _keyReport.keys[1] != k &&
		_keyReport.keys[2] != k && _keyReport.keys[3] != k &&
		_keyReport.keys[4] != k && _keyReport.keys[5] != k) {

		for (i=0; i<6; i++) {
			if (_keyReport.keys[i] == 0x00) {
				_keyReport.keys[i] = k;
				break;
			}
		}
		if (i == 6) {
			setWriteError();
			return 0;
		}
	}
	sendReport(&_keyReport);
	return 1;
}

[SuGlider]

@tommybee456 @KezzaWezza
The PR #10591 fixes the issue and adds Keyboard Layout support to the library.

Please use this example to test Modifier Key pressing:

#include "USB.h"
#include "USBHIDKeyboard.h"
USBHIDKeyboard Keyboard;
const int buttonPin = 0;  // input pin for pushbutton

void setup() {
  // make pin 0 an input and turn on the pull-up resistor so it goes high unless
  // connected to ground:
  pinMode(buttonPin, INPUT_PULLUP);
  Serial.begin(115200);
  while (!Serial) delay(100);

  Serial.println();
  Serial.println("Starting USB Keyboard Device.");
  Serial.println();

  Keyboard.begin();
  USB.begin();
}

void pressRelease(uint8_t k) {
  Keyboard.press(k);
  delay(1000);
  Keyboard.release(k);
  delay(1000);
}

void loop() {
  while (digitalRead(buttonPin) == HIGH) {
    // do nothing until button pin goes low
    delay(50);
  }
  delay(500);

  // test all 8 modifier keys - press & release
  pressRelease(KEY_RIGHT_SHIFT);
  pressRelease(KEY_LEFT_SHIFT);
  pressRelease(KEY_RIGHT_CTRL);
  pressRelease(KEY_LEFT_CTRL);
  pressRelease(KEY_RIGHT_ALT);
  pressRelease(KEY_LEFT_ALT);
  pressRelease(KEY_RIGHT_GUI);
  pressRelease(KEY_LEFT_GUI);

  Serial.println("Done.");
}