fix(keyboard): re-strike a shared HID usage so a roll types both keys - #1079
Open
snowyukitty wants to merge 2 commits into
Open
fix(keyboard): re-strike a shared HID usage so a roll types both keys#1079snowyukitty wants to merge 2 commits into
snowyukitty wants to merge 2 commits into
Conversation
Two matrix positions can map to one HID usage - `<` and its SHIFTED() twin `>`, or the same keycode bound twice. Each press takes its own report slot, but the report deduplicates held keycodes, so rolling from one to the other leaves the usage continuously down. A host emits a character on a usage's absent->present transition, so the second key produces nothing and `<><` arrives as `<<`. The deduplication is deliberate and stays: without it, releasing the first of two owners clears a usage the second still holds. Both halves have to hold at once. So a press that re-strikes a usage another position already holds now sends one extra report first, the ordinary report with only that usage suppressed. Every other held usage and every held modifier stay present, and the usage still remains down until the last owner releases it. Detection keys on ownership rather than on the modifier set, so two positions bound to the same plain keycode also roll into two characters, while a real Shift tapped during a hold does not re-strike anything. build_keyboard_report keeps its signature and behaviour for every existing caller. One extra report per re-strike; no other report count changes. Closes rmk-rs#809
Size Report
|
Collaborator
|
Please simplify and clean the code. The current code is low-quality |
`build_keyboard_report` takes the suppressed usage as a parameter instead of delegating to a second builder, and `send_keyboard_report_with_resolved_modifiers` calls the suppressing send instead of repeating the send-and-yield pair. Two added helpers rather than four. Comments are cut back to the density of the surrounding file, here and in the scenario header, which was longer than any of its neighbours. No behaviour change: same condition, same report stream, and the same test counts on every RMK_TEST_FEATURESETS row.
Author
|
Simplified, thanks.
Against the base this branch was cut from, Behaviour is unchanged, and the test counts on every If you had a different shape in mind — no new function at all, or the ownership count folded into |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #809.
The defect
Two matrix positions can map to the same HID usage. On the reporter's board
<is
KC_NONUS_BSLASHand>isLSFT(KC_NONUS_BSLASH)— different keys, oneusage, differing only by the shift modifier.
State tracking is already right.
register_keycodegives each position its ownreport slot, so
held_keycodesholdsNonusBackslashtwice with two distinctowners. The report is where they merge (
keyboard.rs:1886):A USB HID host emits a character on the transition of a usage from absent to
present in the keycode array. A usage that is already present and merely gains
a modifier produces nothing. So rolling
<into>sends:One make for two intended keystrokes. Roll
<into>and type<again andthe host receives
<<, which is the<<in the report.Why the obvious repair is not available
The deduplication is deliberate.
fix(keyboard): deduplicate held keycodes in the keyboard reportadded it along withhid_reports::shared_keycode_roll_keeps_second_key, whose comment states theintent: the shared usage stays down until the last holder releases it. Before
that, releasing the first of two owners cleared a usage the second owner still
held — a dropped key.
These are two halves of one problem. Removing the deduplication brings back the
dropped key; keeping only the deduplication keeps the lost character. Both have
to hold at once.
The change
A press that gives an already-held usage an additional owner now sends one
extra report first: the ordinary report with only that usage suppressed. The
normal report follows, so the host observes a fresh absent→present transition.
Everything else stays. The suppressed report keeps every other held usage and
every held modifier — it is never an all-clear report — and the usage still
remains down until the last owner releases it.
The decision comes from counting how many report slots hold that usage on
behalf of a registered key event, immediately before and immediately after
registration. That is an observation of what registration did rather than a
prediction of what it would do, which matters in three places:
held_keycodes, so it can never re-strike ausage — holding a key and tapping
Shiftis one activation plus anindependent modifier;
SHIFTED()pair does, so they roll into two characters too;synthesises nothing and follows the existing rollover behaviour, rather than
emitting a stroke for a key the report cannot track.
build_keyboard_reportkeeps its signature and behaviour for every existingcaller: it now delegates with
HidKeyCode::Nosuppressed, which is never in areport anyway. The extra report goes through
send_reportlike any other. Noallocation, no feature gate, no new configuration, no change to
KeyboardReport's shape or the six-key array.One extra report per re-strike is the intended cost. No other report count
changes.
Tests
rmk/tests/scenarios/shared_usage_restrike.toml, twelve cases, plusshared_keycode_roll_keeps_second_keyupdated rather than replaced — itssubject, that the usage stays down while an owner holds it, is exactly the half
this must preserve, so only the new transition report is inserted into its
expectations and its comment now states both halves.
On this branch all five
RMK_TEST_FEATURESETSrows pass:vial,host_lock,_no_usb,steno,passkey_entryrynk,_ble,split,async_matrix,storagedongle,_ble,storagedongle,vial,_ble,storageOn unmodified
mainwith the same test files, therynk,_ble,split,…rowgives 561 passed, 8 failed. The eight are the ones that assert the new
transition:
shared_keycode_roll_keeps_second_key,rolled_shared_usage_types_both_keys,reverse_roll_shifted_first,rolled_same_keycode_two_positions_types_twice,shared_usage_survives_first_release,transition_preserves_other_held_keys,transition_preserves_held_modifiers,plain_keycode_restrike_preserves_an_unrelated_held_key.The rest pass on
maintoo, and they are what keeps the change honest ratherthan just proving the symptom: each names a case where a stroke must not be
synthesised, so a rule that fired too eagerly would break them.
held_key_plus_tapped_real_shift_types_onceandmodifier_only_change_sends_no_transition— a modifier arriving or leavingwhile a usage is held changes the modifier byte and nothing else;
sequential_typing_is_unchanged— the already-correct<><stream fromthe report is byte-identical after the change;
held_key_sends_no_extra_reports— one key held sends exactly one make andone break, so host auto-repeat is not chopped;
an_unregistered_seventh_press_synthesises_no_stroke— with the six-key arrayfull, a seventh press mapped to a held usage registers nothing, gains no
owner, and emits no extra report.
They do not cover every possible false positive; they cover those four classes.
cargo fmt --all -- --checkon nightly andcargo clippy … --all-targets -- -D warningsare both clean.A limit of this validation
The transition report and the make that follows are two consecutive reports,
intended to become two interrupt transfers over USB or two notifications over
BLE HID. A link that coalesced or dropped one would hide the make again.
The scenario harness asserts the report stream, so it proves the firmware
emits both. It does not observe an actual transfer on either transport, so it
proves nothing about delivery over USB or BLE, and I have not tested that on
hardware.