Update USB HID Usage Tables with new System & Consumer keys #901
Conversation
| #define HID_TYPE_LC B00000100 | ||
| #define HID_TYPE_NARY B00001000 | ||
| #define HID_TYPE_OOC B00001100 | ||
| #define HID_TYPE_OSC B00010000 | ||
| #define HID_TYPE_RTC B00010100 | ||
| #define HID_TYPE_SEL B00011000 |
There was a problem hiding this comment.
These HID_TYPE_* constants were being combined with others in the flags byte of both Consumer and System Control Key definitions, but this was incorrect. These bits overlap with the IS_CONSUMER and SWITCH_TO_KEYMAP bits, which are used to detect ConsumerControl and Layer keys, respectively.
For example HID_TYPE_RTC would be detected in Layer.eventHandler() as SWITCH_TO_KEYMAP, and since all of these Key types have the SYNTHETIC bit set, this would result in any System or Consumer keys with usage type RTC triggering a call to Layer.handleKeymapKeyswitchEvent() (which probably wouldn't do anything, because the keycode byte would be out of range, but it's still incorrect). Likewise, any System key with HID_TYPE_OOC would be misidentified as a Consumer key in handleSyntheticKeyswitchEvent(), and the System key's HID report would never get sent.
There was a problem hiding this comment.
That indeed sounds like a bug, but one to be fixed, rather than just throwing away the key type data.
There was a problem hiding this comment.
I don't think there are enough bits to spare.
src/kaleidoscope/key_defs.h
Outdated
|
|
||
| /* Most Consumer keys are more then 8bit, the highest Consumer hid code | ||
| uses 10bit. By using the 11bit as flag to indicate a consumer keys was activate we can | ||
| use the 10 lsb as the HID Consumer code. If you need to get the keycode of a Consumer key | ||
| use the CONSUMER(key) macro this will return the 10bit keycode. | ||
| */ | ||
| #define CONSUMER(key) (key.getRaw() & 0x03FF) | ||
| #define CONSUMER_KEY(code, flags) Key((code) | ((flags | SYNTHETIC|IS_CONSUMER) << 8)) | ||
| #define CONSUMER_KEY(code) Key((code & 0x03FF) | ((SYNTHETIC | IS_CONSUMER) << 8)) |
There was a problem hiding this comment.
This is a simpler and more correct version of CONSUMER_KEY(). The high six bits should always be the same on any Consumer key, regardless of HID usage type.
Really, I think this should be a static constexpr function inside the Key class: Key::consumer(uint16_t code), rather than a macro, but that requires more extensive code reorganization.
There was a problem hiding this comment.
Now that we're using that magic 0x03FF constant more than once, It should get defined as a named constant. Really it should have been one from the beginning.
There was a problem hiding this comment.
Excellent point. I'll get on that.
src/kaleidoscope/key_defs.h
Outdated
| @@ -255,14 +241,15 @@ typedef kaleidoscope::Key Key_; | |||
| #define KEY_LEFT_FN2 0xff | |||
| #define Key_LFN2 Key(KEY_LEFT_FN2, KEY_FLAGS) | |||
|
|
|||
| #define SYSTEM_KEY(code) Key(code, SYNTHETIC | IS_SYSCTL) | |||
There was a problem hiding this comment.
To simplify the definition of System Control keys, I added this macro as the counterpart to the existing CONSUMER_KEY() macro below.
|
I'm going to start to break this apart. The data update to the HID table is a no-brainer, but mixing in functionality changes into the same PR makes me a little uneasy. (I'd probably have done the changes to the Consumer and System key macros each as its own PR) |
|
All the changes to HIDTables.h are now on master as 9404dcc. |
I did try to keep the commits separate to make it easy to reconfigure things, since I figured you might want adjustments made. |
| #define System_MenuHelp Key(HID_SYSTEM_MENU_HELP, KEY_FLAGS | SYNTHETIC|IS_SYSCTL | HID_TYPE_OSC) | ||
| #define System_MenuExit Key(HID_SYSTEM_MENU_EXIT, KEY_FLAGS | SYNTHETIC|IS_SYSCTL | HID_TYPE_OSC) | ||
| #define System_MenuSelect Key(HID_SYSTEM_MENU_SELECT, KEY_FLAGS | SYNTHETIC|IS_SYSCTL | HID_TYPE_OSC) | ||
| #define System_MenuRight Key(HID_SYSTEM_MENU_RIGHT, KEY_FLAGS | SYNTHETIC|IS_SYSCTL | HID_TYPE_RTC) |
There was a problem hiding this comment.
Not all of the System control keys -are- OSC keys. Even if we're not yet using this data, I really don't want to just discard it from the tables.
There was a problem hiding this comment.
The problem is that the HID_TYPE_* bits overlap with other bits that are in use in the flags byte of a Key object. If you include HID_TYPE_OOC, you're also including IS_CONSUMER along with it. handleSyntheticKeyswitchEvent() could be changed to check for System keys before Consumer keys to solve that problem, but Consumer keys can't use those HID_TYPE_* constants.
There was a problem hiding this comment.
I want @algernon's input before I sign off on it, but I think I'd be ok with a solution where, for now, we define all the HID_TYPE_XX constants to 0, rather than just removing them from the key definitions.
There was a problem hiding this comment.
Correction: we can't solve it by changing handleSyntheticKeyswitchEvent(), because Consumer keys might have the IS_SYSCTL bit set, depending on the value.
There was a problem hiding this comment.
I think we could call out four HID types (using the two bits above IS_CONSUMER and below SYNTHETIC). Or a default, and three that get special handling.
There was a problem hiding this comment.
I'd rather defer the exact bit math until we understand our needs.
|
@obra – At this point, would you like me to rebase this PR onto the master branch? |
|
Sure. Or break it into a pair of PRs to handle consumer and system keys
…On Mon, Sep 14, 2020 at 2:10 PM Michael Richters ***@***.***> wrote:
@obra <https://github.com/obra> – At this point, would you like me to
rebase this PR onto the master branch?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#901 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAALC2FII6BCJPHHVN7MLX3SF2BFHANCNFSM4RI7JKIQ>
.
|
Signed-off-by: Michael Richters <gedankenexperimenter@gmail.com>
These shouldn't have been added in the first place. Signed-off-by: Michael Richters <gedankenexperimenter@gmail.com>
Signed-off-by: Michael Richters <gedankenexperimenter@gmail.com>
Signed-off-by: Michael Richters <gedankenexperimenter@gmail.com>
`KEY_FLAGS` is just 0, so was just extra clutter, and `HID_TYPE_*` shouldn't be there in the first place, as it can cause a System Control key to be marked as a Consumer Control key. Signed-off-by: Michael Richters <gedankenexperimenter@gmail.com>
Signed-off-by: Michael Richters <gedankenexperimenter@gmail.com>
Signed-off-by: Michael Richters <gedankenexperimenter@gmail.com>
Signed-off-by: Michael Richters <gedankenexperimenter@gmail.com>
Signed-off-by: Michael Richters <gedankenexperimenter@gmail.com>
These constants may have served a purpose at some point, but now all they can accomplish is causing a System key of usage type OOC to get misidentified as a Consumer key (since `HID_TYPE_OOC` has the `IS_CONSUMER` bit set). Signed-off-by: Michael Richters <gedankenexperimenter@gmail.com>
There were a few minor problems in the way Consumer `Key` objects were constructed (using `CONSUMER_KEY()`). First, no masking of the high bits was being done, so it was possible to create a "Consumer" key with the `RESERVED` bit set (e.g. `Key_Transparent`), or the `SWITCH_TO_KEYMAP` bit (in fact, any `Key` value with both the `SYNTHETIC` and `IS_CONSUMER` bits set was possible). Second, the high six bits of the raw `Key` value should always be `010010` (`SYNTHETIC | IS_CONSUMER`), as Consumer keys don't have any flags. The macro should really only take one argument: a 16-bit integer keycode value. The `HID_TYPE_*` constants really shouldn't be used at all in defining the keys in key_defs_consumer.h, because setting those bits could potentially cause a key to be misidentified by some plugin. This change fixes these potential problems by ignoring the `flags` parameter, masking the high six bits of the `code` supplied, and setting those high six bits to the correct value. Signed-off-by: Michael Richters <gedankenexperimenter@gmail.com>
Signed-off-by: Michael Richters <gedankenexperimenter@gmail.com>
They are now all set to 0 to avoid conflicts with other flags bits, and I've added a `HID_TYPE_MASK` constant to use when constructing System and Consumer keys. Signed-off-by: Michael Richters <gedankenexperimenter@gmail.com>
Signed-off-by: Michael Richters <gedankenexperimenter@gmail.com>
Signed-off-by: Michael Richters <gedankenexperimenter@gmail.com>
Signed-off-by: Michael Richters <gedankenexperimenter@gmail.com>
Signed-off-by: Michael Richters <gedankenexperimenter@gmail.com>
Signed-off-by: Michael Richters <gedankenexperimenter@gmail.com>
|
I've done a rebase in my working repository. I'm checking it now to make sure I didn't screw anything up in the process, then I'll force-push the new version. |
gedankenexperimenter
force-pushed
the
usb-hid-tables-update
branch
from
07d7416
to
4b590e0
Compare
|
|
||
| /* Most Consumer keys are more then 8bit, the highest Consumer hid code | ||
| uses 10bit. By using the 11bit as flag to indicate a consumer keys was activate we can | ||
| use the 10 lsb as the HID Consumer code. If you need to get the keycode of a Consumer key | ||
| use the CONSUMER(key) macro this will return the 10bit keycode. | ||
| */ | ||
| #define CONSUMER(key) (key.getRaw() & 0x03FF) | ||
| #define CONSUMER_KEY(code, flags) Key((code) | ((flags | SYNTHETIC|IS_CONSUMER) << 8)) | ||
| constexpr uint16_t CONSUMER_KEYCODE_MASK = 0x03FF; |
There was a problem hiding this comment.
Magic number replaced with defined constant. The official code style guide says it should be named kConsumerKeycodeMask, but that style isn't used anyplace else in Kaleidoscope that I can find, so I'm leaving it in all caps for the moment.
| #define HID_TYPE_SEL B00000000 | ||
| #define HID_TYPE_SV B00000000 | ||
| // Mask defining the allowed usage type flag bits: | ||
| #define HID_TYPE_MASK B00110000 |
There was a problem hiding this comment.
These are the only two non-conflicting bits available. Enough for a base 00 default, and three "special" usage types (01, 10, 11), to be defined later.
| #define SYSTEM_KEY(code, hid_type) \ | ||
| Key(code, SYNTHETIC | IS_SYSCTL | (hid_type & HID_TYPE_MASK)) |
There was a problem hiding this comment.
I've added back in the HID Usage Type parameter. I'm using the HID_TYPE_MASK to ensure that only the allowed bits can be set in the flags byte.
| #define CONSUMER_KEY(code, hid_type) \ | ||
| Key((code & CONSUMER_KEYCODE_MASK) | \ | ||
| ((SYNTHETIC | IS_CONSUMER | (hid_type & HID_TYPE_MASK)) << 8)) |
There was a problem hiding this comment.
HID Usage Type parameter also added here.
|
I've just spent a couple hours working through this PR, rewriting the history to contain separable logical changes, since the flow got a little hard to follow with the rework. Closing in favor of #915 |
There has been an update to the USB HID Usage Tables (v1.2), and it includes several new System Control and Consumer Control keys, some of which are being used already by major operating systems. This PR updates Kaleidoscope's copy of the keycodes table, and adds the new
Keydefinitions. I also did some reformatting and added comments to hopefully make the table more readable and make it easier to spot errors.In addition, I have included changes to make the definition of System Control and Consumer Control keys simpler, and more robust.
This PR includes (and replaces) #899 & #897.