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

Improvements to extrakey HID descriptors #8156

Merged
merged 1 commit into from
Feb 25, 2020

Conversation

fauxpark
Copy link
Member

Description

As mentioned in #963, these descriptors are declared as Array, so the logical value sent over the wire is the 1-based index of the usage between Usage Minimum and Usage Maximum. Only one value can be asserted at a time, and a value of 0 means nothing pressed.

I've reverted the range of the System usages from just the three power keycodes to the entire thing so that one can send arbitrary usages with host_system_send(). Now we don't need to do the whole data - SYSTEM_POWER_DOWN + 1 first, as usage 1 == index 1.

Similarly I increased the range of the Consumer usages slightly, to allow for some of the newer additions to the page such as the macOS Launchpad and Mission Control usages (#4762). They still don't have their own keycodes yet, but you can now at least DIY.

I couldn't reproduce the issue with macOS seeing QMK as a mouse/tablet (#510) on Catalina... except for with V-USB, until I realised that the extrakey descriptor is part of the mousekey descriptor's array, and the latter is always present even if MOUSE_ENABLE is not defined. So I chucked a bunch of ifdefs in vusb.c to handle the cases of either extrakeys or mouse being disabled, and both at the same time. Should save some bytes!

Tested and working on LUFA, ChibiOS and V-USB. Again, I can't test ATSAM, but the changes to the descriptors are much the same as the other stacks, and the CTRL and ALT both compile 😄

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Keyboard (addition or update)
  • Keymap/layout/userspace (addition or update)
  • Documentation

Issues Fixed or Closed by This PR

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

@tzarc
Copy link
Member

tzarc commented Feb 12, 2020

Confirming that extrakey functionality still working on a CTRL with this patch.

@tzarc
Copy link
Member

tzarc commented Feb 12, 2020

I've reverted the range of the System usages from just the three power keycodes to the entire thing so that one can send arbitrary usages with host_system_send(). Now we don't need to do the whole data - SYSTEM_POWER_DOWN + 1 first, as usage 1 == index 1.

I'm not massively familiar w ith the HID usage tables, so I'm not sure if the following is an issue (or if I'm reading it correctly).

The power buttons (0x81-0x83) are all "on/off control" usage type, which I'd imagine matches a normal keypress in the eyes of the system... and can be represented by on/off bitmask.
Other parts of the desktop page have different usage types, including dynamic values (0x30-0x39, etc) -- is it appropriate that they're on/off bits in the array?

Should we really be extending the range outside the "button press" section?

EDIT: I'm now not even sure my interpretation is correct, anyway.
If it's a bitmask, and if we're now allowing for 0x01->0xB7, then16 bytes won't be enough for 0/1 for each of the IDs.
If it's "usage ID + payload of 16 bytes" then sure, but I don't see it.

@fauxpark
Copy link
Member Author

It's not a bitmask, it's an array index. From the HID Device Class Definition, section 6.2.2.5:

An array provides an alternate means for describing the data returned from a group of buttons. [...] Rather than returning a single bit for each button in the group, an array returns an index in each field that corresponds to the pressed button (like keyboard scan codes). An out-of range value in and array field is considered no controls asserted.

For example, currently the usage min/max of the System descriptor is 0x81-0x83, and the logical min/max is 0x01-0x03. So to assert the 0x81 (System Power Down) usage, you send 0x01, and so on. This is why SYSTEM_POWER_DOWN + 1 had to be subtracted from data before actually sending it. Now that the usage min/max is the same as logical, we don't need to do that anymore.

As for extending the range outside of the button press "section" - there is no such thing really, the usage types are all over the place. The Consumer descriptor does the same thing. Sure, some of the declared usages don't make total sense wrt their types but it's whatever. I don't think you can declare multiple non-contiguous ranges for the same field anyway.

@drashna drashna requested a review from a team February 23, 2020 22:43
@tzarc
Copy link
Member

tzarc commented Feb 25, 2020

Confirmed that this fixes #6203 by users "Daniel Goodman" and "colon-d" on discord with their dz65's.

@tzarc tzarc merged commit 088b64a into qmk:master Feb 25, 2020
@fauxpark fauxpark deleted the extrakey-descriptor-improvements branch February 25, 2020 02:46
Shinichi-Ohki added a commit to Shinichi-Ohki/qmk_firmware that referenced this pull request Feb 25, 2020
* 'master' of https://github.com/qmk/qmk_firmware: (108 commits)
  `send_unicode_string()`: Add support for code points > 0xFFFF (qmk#8236)
  [Keyboard] Add Wete (qmk#8229)
  Improvements to extrakey HID descriptors (qmk#8156)
  Hineybush h87a lock indicators (qmk#8237)
  Add VIA support for Prime_L (qmk#8233)
  Hub16 - Bug removal + clean up code (qmk#8227)
  [Keyboard] ai03 Equinox (qmk#8224)
  [Keyboard] Add zfrontier/big_switch (qmk#8205)
  Gingham Update (qmk#8225)
  A proper `send_string()` for the Unicode feature (qmk#8155)
  Rollback PR qmk#7967 in preference of fixing I2C start/stop properly, in a followup PR. (qmk#8173)
  Add mouse support to SEND_STRING (qmk#8223)
  Add link to "Useful functions" in macro docs (qmk#7446)
  New functionality for cformat (qmk#7893)
  Update main.c (qmk#8198)
  format code according to conventions [skip ci]
  Fix QWIIC OLED for AVR (qmk#7769)
  VIA Support: KBD75 rev1/rev2 (qmk#8214)
  Update TMOv2 for new key (qmk#7759)
  Added custom keymap for preonic (qmk#7548)
  ...
nesth pushed a commit to nesth/qmk_firmware that referenced this pull request Feb 27, 2020
* upstream/master: (26 commits)
  Fixed OS detection such that OSX doesn't take over the world (qmk#8248)
  [Keyboard] Add Prime_EXL Plus to handwired (qmk#8238)
  format code according to conventions [skip ci]
  New feature: PERMISSIVE_HOLD_PER_KEY (qmk#7994)
  Split - Avoid race condition during matrix_init_quantum (qmk#8235)
  Acheron VIA support (qmk#8204)
  `send_unicode_string()`: Add support for code points > 0xFFFF (qmk#8236)
  [Keyboard] Add Wete (qmk#8229)
  Improvements to extrakey HID descriptors (qmk#8156)
  Hineybush h87a lock indicators (qmk#8237)
  Add VIA support for Prime_L (qmk#8233)
  Hub16 - Bug removal + clean up code (qmk#8227)
  [Keyboard] ai03 Equinox (qmk#8224)
  [Keyboard] Add zfrontier/big_switch (qmk#8205)
  Gingham Update (qmk#8225)
  A proper `send_string()` for the Unicode feature (qmk#8155)
  Rollback PR qmk#7967 in preference of fixing I2C start/stop properly, in a followup PR. (qmk#8173)
  Add mouse support to SEND_STRING (qmk#8223)
  Add link to "Useful functions" in macro docs (qmk#7446)
  New functionality for cformat (qmk#7893)
  ...
HokieGeek pushed a commit to HokieGeek/qmk_firmware that referenced this pull request Mar 5, 2020
c0psrul3 pushed a commit to c0psrul3/qmk_firmware that referenced this pull request Mar 23, 2020
drashna pushed a commit to zsa/qmk_firmware that referenced this pull request Mar 26, 2020
sowbug pushed a commit to sowbug/qmk_firmware that referenced this pull request Apr 2, 2020
kylekuj pushed a commit to kylekuj/qmk_firmware that referenced this pull request Apr 21, 2020
jakeisnt pushed a commit to jakeisnt/qmk_firmware that referenced this pull request Aug 20, 2020
xwu added a commit to xwu/qmk_firmware that referenced this pull request Jul 27, 2021
- Gentoli:drop-ok@a0c746e USBCV HID tests (partially applied, upstream diverged in qmk#8156)
- Gentoli:drop-ok@1a106ef USBCV Chapter 9 disabled remote wakeup failure
- Gentoli:drop-ok@b5a9a23 USB30CV Chapter 9 failure
BorisTestov pushed a commit to BorisTestov/qmk_firmware that referenced this pull request May 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants