fix(deferred-review): descriptor caps, keyboard report-ID, remove locks, USB halt recovery#9
fix(deferred-review): descriptor caps, keyboard report-ID, remove locks, USB halt recovery#9ramseymcgrath wants to merge 2 commits into
Conversation
…ks, USB halt recovery - Increase descriptor storage caps (config/HID/string/BOS) for complex composites. - Add minimal keyboard report-ID parser (kb_layout.c) and use it in merge/phys/synth. - Remove dead g_lock_mask / TYPE_LOCK_* / Ferrum lock commands; update docs. - Restructure USB host interrupt-IN halt recovery: async CLEAR_FEATURE + DATA0 toggle reset. - Add USB host interrupt-OUT halt/error recovery + poll helper. - Implement USB device CLEAR/SET_FEATURE(ENDPOINT_HALT) and dTD error recovery. - Add kb_layout_test and update Makefile/AGENTS.md. make test-all passes for both Hurra and Ferrum builds.
There was a problem hiding this comment.
Pull request overview
This PR addresses several deferred review findings in Hurra v2’s firmware by expanding descriptor capture limits, improving HID keyboard handling (Report ID aware), removing dead “lock” commands/state, and hardening USB host/device halt recovery paths. It also updates the host-native test suite and build tooling to cover the new behavior.
Changes:
- Increased captured descriptor size caps and patched truncated descriptors to remain internally consistent during replay.
- Added keyboard Report-ID/layout parsing (
kb_layout.*) and updated merge/telemetry/synthetic keyboard send paths to use it. - Implemented/reshaped USB host + USB device endpoint halt/error recovery (including new interrupt-OUT polling) and removed legacy lock commands/state; expanded unit tests and build targets.
Reviewed changes
Copilot reviewed 22 out of 23 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/motion_test.c | Extends motion tests to validate multi-click press/release sequencing. |
| test/kb_layout_test.c | Adds host-native tests for the new HID keyboard layout parser. |
| test/humanize_test.c | Adds regression tests for residual pending behavior and EWMA reclamping. |
| src/usb_host.h | Declares new interrupt-OUT poll API and clarifies send semantics during recovery. |
| src/usb_host.c | Adds interrupt-OUT completion/halt/error handling and reworks interrupt-IN halt recovery to be async. |
| src/usb_device.c | Adds endpoint-targeted SET/CLEAR_FEATURE(ENDPOINT_HALT), dTD error recovery, and ep0 max-packet validation. |
| src/TF_Config.h | Enables SOF byte emission/expectation for TinyFrame framing. |
| src/main.c | Clamps HS bInterval shift and integrates interrupt-OUT polling into the main loop. |
| src/kmbox.h | Removes obsolete click-release scheduler API. |
| src/kmbox.c | Integrates click scheduler tick, keyboard layout parsing, and improves wheel/motion conservation behavior. |
| src/kb_layout.h | Introduces minimal HID keyboard report-descriptor layout extractor API. |
| src/kb_layout.c | Implements Report ID detection + boot-layout offset/length selection. |
| src/hurra.c | Removes dead lock frame types/listeners; retains catch/phys-mask. |
| src/humanize.c | Improves EWMA update safety, bounds interval estimate, and includes residuals in humanize_pending(). |
| src/ferrum.c | Removes lock commands and adds a getter-backed implementation for mask read behavior. |
| src/desc_capture.h | Doubles descriptor storage caps (config/HID report/string/BOS). |
| src/desc_capture.c | Patches truncated BOS/config/string/HID lengths for replay consistency; improves HID-report failure handling. |
| src/actions.h | Removes g_lock_mask, adds act_click_tick() and act_kb_mask_get(), updates phys-mask comment. |
| src/actions.c | Removes lock state, implements click scheduler tick, and adds keyboard mask getter. |
| Makefile | Adds dependency generation, map/memory reporting, HOST_CC override, kb_layout build/test, and test-all. |
| KMBOX_NET_FIRMWARE_REQUIREMENTS.md | Updates protocol/requirements docs to reflect lock removal and phys-mask semantics. |
| AGENTS.md | Adds an agent-focused project guide, build/test instructions, and architecture notes. |
| .gitignore | Ignores generated .d dependency files and linker .map files. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| intr_halt_count[index]++; | ||
| intr_halt_recovering[index] = true; | ||
|
|
||
| // Disable S-mask so HC won't touch this QH during re-prime | ||
| uint32_t saved_cap1 = qh->capabilities[1]; | ||
| qh->capabilities[1] = saved_cap1 & ~0xFFu; // zero S-mask | ||
| // Disable S-mask so HC won't touch this QH during recovery. | ||
| intr_saved_cap1[index] = qh->capabilities[1]; |
There was a problem hiding this comment.
Good catch — confirmed. intr_halt_recover() now early-returns false when usb_host_control_async_busy() before setting intr_halt_recovering, so we only enter the recovering state once CLEAR_FEATURE can actually be queued. When the pipe is busy the caller keeps the IN slot marked active (skipping the re-prime branch) and retries recovery on a later poll, so the qTD stays HALTED rather than re-priming against a still-STALLed endpoint. Fixed in d43fd03.
| // Finish a previous halt recovery. | ||
| if (__builtin_expect(intr_out_halt_recovering[index], 0)) { | ||
| if (usb_host_control_async_busy()) return; | ||
| intr_out_halt_recovering[index] = false; | ||
| ehci_qh_t *qh = &qh_intr_out[index]; | ||
| qh->capabilities[1] = intr_out_saved_cap1[index]; | ||
| qh->next = QTD_TERMINATE; | ||
| qh->alt_next = QTD_TERMINATE; | ||
| qh->token = QTD_TOKEN_ACTIVE | QTD_TOKEN_PID_OUT | | ||
| QTD_TOKEN_NBYTES(intr_out_maxpkt[index]) | QTD_TOKEN_CERR(3) | QTD_TOKEN_IOC; | ||
| set_qtd_buffers_small(qh->buffer, intr_out_buf[index]); | ||
| asm volatile("dsb" ::: "memory"); | ||
| intr_out_transfer_active[index] = true; | ||
| intr_out_prime_time[index] = millis(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Confirmed — re-arming an OUT qTD actually transmits, so this would re-send stale buffer contents. The recovery-finish path now restores the schedule and returns the slot to idle (token = 0, DATA0, intr_out_transfer_active = false) instead of re-arming; the next usb_host_interrupt_out_send() arms the next real packet. Fixed in d43fd03.
| // Cancel any in-flight click sequence so overlapping clicks don't interleave. | ||
| memset(&g_click_sched, 0, sizeof(g_click_sched)); | ||
|
|
There was a problem hiding this comment.
Confirmed — a new click on a different button could leave the previously scheduled button held. act_click() now releases the old button (re-injecting the cleared bitmap) when the prior sequence was mid-press and targets a different button, before zeroing the schedule. Fixed in d43fd03.
…covery - act_click(): release a still-pressed scheduled button before clearing the schedule so starting a new click on a different button can't leave the old button stuck down. - intr_halt_recover(): gate entry on !usb_host_control_async_busy() and return bool; if the async pipe is busy, keep the IN slot marked active (skipping the re-prime branch) and retry recovery later instead of latching "recovering" with no CLEAR_FEATURE ever queued. - usb_host_interrupt_out_poll(): on recovery-finish, return the OUT slot to idle (DATA0, inactive) instead of re-arming the qTD, which would re-transmit stale buffer contents without an explicit send. Both builds clean (Hurra + Ferrum), make test-all passes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Summary
Closes the remaining high-priority deferred findings from the agent code review.
Changes
src/kb_layout.c/.hto parse Report ID and boot-layout offsets; merge, physical telemetry, and synthetic send now use the parsed layout.g_lock_mask,TYPE_LOCK_*, and Ferrumlock_*handlers; updatedKMBOX_NET_FIRMWARE_REQUIREMENTS.mdandAGENTS.md.CLEAR_FEATURE(ENDPOINT_HALT)asynchronously and re-prime with DATA0 toggle reset; added interrupt-OUT halt/error recovery withusb_host_interrupt_out_poll().CLEAR/SET_FEATURE(ENDPOINT_HALT)and dTD error recovery for interrupt IN/OUT.test/kb_layout_test.c;make test-allpasses for both Hurra and Ferrum builds.Verification
make test-all✅Notes
The USB host/device halt-recovery paths are code-complete but still need real-hardware validation with a composite gaming device and, if possible, a Logitech HID++ device.