Skip to content

feat(gamepad): add a selectable standards-based HID gamepad mode - #26

Open
Fefedu973 wants to merge 4 commits into
peppapighs:devfrom
Fefedu973:codex/pr-hid-gamepad
Open

Fefedu973 wants to merge 4 commits into
peppapighs:devfrom
Fefedu973:codex/pr-hid-gamepad

Conversation

@Fefedu973

@Fefedu973 Fefedu973 commented Aug 25, 2026 •

Copy link
Copy Markdown

Context

This is part 2 of 3 extracted from #24 at the maintainer's request and addresses #11. It is based directly on dev: it contains neither the STM32F723/KBHE port nor RGB.

HID and XInput share the existing gamepad configuration and analog state rather than introducing a second subsystem.

Design

  • Adds a standards-based TinyUSB HID gamepad report and descriptor.
  • Preserves the existing XInput implementation and analog gamepad data path.
  • Selects exactly one API: disabled, XInput, or HID.
  • Reuses existing gamepad button/axis settings and commands.
  • Exposes supported APIs in compressed keyboard metadata for capability discovery.

Changes following review

One enum, unchanged options ABI

The two booleans are replaced by a two-bit gamepad_api_t gamepad_api field:

  • 0: disabled;
  • 1: XInput;
  • 2: HID;
  • 3: reserved.

eeconfig_options_t remains two bytes. The v1.5-to-v1.6 migration constructs the enum from the old XInput bit 0 and ignores old bit 1, which was previously reserved and was set by a historical migration. All other option bits, calibration, and profile bytes are preserved.

New SET_OPTIONS writes reject value 3. Malformed existing storage containing 3 retains the previous documented XInput fallback. Accessors keep descriptor selection mutually exclusive.

Why gamepadApis is retained

The additive gamepadApis: ["xinput", "hid"] metadata field distinguishes firmware with HID support from legacy firmware. The old and current code both expose firmware version 0x0109; the internal EEPROM schema version is not a substitute for capability discovery. A configurator must not interpret the historical reserved bit as evidence of HID support.

When this field is absent, a configurator should offer only legacy XInput. Existing configurators can ignore the additive field. No hmkconf changes are included in this PR update.

Deterministic descriptor GUID

UUIDv5 is derived from the libhmk namespace URL, canonical keyboard target, normalized VID/PID, and xinput interface role. Different target definitions sharing VID/PID therefore receive different GUIDs, while repeated builds of the same target retain the same GUID.

This is an interface-class identifier, not a unique physical-device ID. Multiple units of the same target intentionally share it. The motivation is deterministic descriptors and stable interface-class identity; the earlier claim about forcing a new XInput driver binding on every build was too strong and has been removed. There is no claim here of fixing an observed Windows driver-binding bug.

USB compatibility

Interface counts, endpoint numbers, and descriptor lengths follow the selected API. HID uses a standard gamepad descriptor; XInput retains its vendor-specific descriptors. Only the selected implementation is active.

Validation

  • Full independent he60 STM32F446 build under Linux/WSL, ARM GCC 7.2.1, TinyUSB 0.20.0, strict project warnings: 38,844 bytes Flash / 14,420 bytes RAM, ELF and DFU-suffixed binary generated.
  • Exhaustive host tests over all 65,536 options words and v1.5 migrations, including byte-for-byte preservation of other settings, invalid values, and migration write failure.
  • Host tests passed with AddressSanitizer and UndefinedBehaviorSanitizer.
  • Five Python regression tests run the actual metadata generator with build inputs/file output mocked: stable UUID, distinct targets sharing VID/PID, normalized IDs, descriptor encoding, deterministic gzip, and capability metadata.
  • Reproduction commands are in tests/README.md. No physical keyboard was flashed for this review update; successful compilation/host tests do not claim USB hardware validation.

Part 1 is #25. RGB remains draft #27 and is not part of this diff.

Comment thread include/eeconfig.h Outdated
bool _unused0 : 1;
// Whether a standards-based HID gamepad interface is enabled. It is
// mutually exclusive with XInput and reuses a previously reserved bit.
bool hid_gamepad_enabled : 1;

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe just merge this with xinput_enabled and make it an 2-bit wide enum type (I see you defined gamepad_api_t anyway)? since they are mutually exclusive.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. separate booleans made an exclusive choice look like two independent switches. I replaced them with a two-bit gamepad_api_t gamepad_api field: 0 = disabled, 1 = XInput, 2 = HID. The existing two-byte options wire/storage layout is unchanged.

Reserved value 3 is rejected by SET_OPTIONS; malformed existing storage retains the previously documented XInput fallback. I also updated the migration and added exhaustive host tests over all 65,536 options words.

Comment thread scripts/metadata.py
"numAdvancedKeys": kb_json.keyboard.num_advanced_keys,
"numDynamicKeystrokeMaxBindings": kb_json.keyboard.num_dynamic_keystroke_max_bindings,
"numMacroNodes": kb_json.keyboard.num_macro_nodes,
"gamepadApis": ["xinput", "hid"],

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need this?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is capability discovery for configurators talking to both old and new firmware. Older firmware has no HID mode, and its old reserved option bit is not a reliable feature indicator. The current base and this PR also expose the same firmware version (0x0109); the EEPROM migration version is internal.

The intended rule is: if gamepadApis is absent, offer legacy XInput only; if it includes hid, the configurator can safely expose the HID choice. All builds of this implementation advertise both APIs, so the field is static now, but its absence is meaningful for older firmware. The existing hmkconf work consumes it this way; no hmkconf changes were made for this review update.

I kept the field and documented that contract in the README, with a generator regression test. If you prefer a common capability format instead, this could use that later; it does not need a broader capability framework in this PR.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to make a change to hmkconf in the end anyway, so I don't think this argument works. In general, the dev branch of the firmware is meant to be unstable, and hmkconf should use version mapping to figure out which features are available. If you cannot disable the HID gamepad in the firmware then it seems we are just sending constants that could have been hardcoded in hmkconf, right?>

Comment thread scripts/metadata.py Outdated
def ms_os_20_guid_def():
uuid = uuid4().hex.upper()
# A stable XInput interface identity avoids a new Windows device binding
# on every build while remaining unique for each VID/PID pair.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make sense but can you come up with a different scheme? wouldn't two different keyboards with the same VID/PID pair have the same UUID? also what's the implication of changing UUID every build?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right that VID/PID alone does not distinguish different keyboard definitions that reuse the same pair. I changed the UUIDv5 input to include the canonical keyboard target and the xinput interface role as well as normalized VID/PID, under the libhmk namespace URL. Different target definitions sharing VID/PID now get different GUIDs; rebuilding the same target retains its GUID. Regression tests cover that and hex-case normalization.

I also overstated the original motivation: I do not have evidence that a new GUID necessarily causes a new XInput driver binding on every build. This value identifies a device-interface class, not an individual physical keyboard. Multiple units may intentionally share it; Windows distinguishes device instances separately. The USB serial remains the per-unit identifier.

Keeping it stable gives deterministic descriptors and a stable interface-class identifier for applications that use it. I removed the unsupported driver-rebinding claim; this is not presented as a proven XInput driver fix. Microsoft's interface-class documentation and instance-ID documentation describe that distinction.

Comment thread src/migration.c
// v1.5 -> v1.6 Migration (HID gamepad option bit)
//--------------------------------------------------------------------+

static bool v1_6_global_config_func(uint8_t *dst, const uint8_t *src) {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we decide to make the change suggested above, make sure to update the migration function below too.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated alongside the enum change. The v1.5-to-v1.6 migration constructs the selector from the old XInput bit 0 only and ignores historical bit 1. This preserves enabled/disabled XInput and prevents an old reserved bit from accidentally selecting HID.

The host test exercises every possible old options word and checks byte-for-byte preservation of the other option bits, calibration, and profile data. It also covers migration write failure and ensures already-current configurations are not migrated again. These tests pass with strict compiler warnings, ASan, and UBSan; the full he60 firmware build also passes.

Comment thread scripts/metadata.py
"numAdvancedKeys": kb_json.keyboard.num_advanced_keys,
"numDynamicKeystrokeMaxBindings": kb_json.keyboard.num_dynamic_keystroke_max_bindings,
"numMacroNodes": kb_json.keyboard.num_macro_nodes,
"gamepadApis": ["xinput", "hid"],

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to make a change to hmkconf in the end anyway, so I don't think this argument works. In general, the dev branch of the firmware is meant to be unstable, and hmkconf should use version mapping to figure out which features are available. If you cannot disable the HID gamepad in the firmware then it seems we are just sending constants that could have been hardcoded in hmkconf, right?>

Comment thread scripts/metadata.py
f"https://github.com/peppapighs/libhmk/"
f"{kb_json.usb.vid}/{kb_json.usb.pid}/xinput"
f"https://github.com/peppapighs/libhmk/keyboards/{keyboard}/usb/"
f"{int(kb_json.usb.vid, 16):04x}:{int(kb_json.usb.pid, 16):04x}/xinput"

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this really addresses my concern. A different keyboard of the same model would still share the same GUID right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants