feat: add extended mouse forwarding#169
Merged
Conversation
Contributor
WalkthroughAdds an extended HID mouse (1-byte buttons, signed 16-bit X/Y, signed 8-bit wheel/pan), maps more BTN_* codes, buffers/coalesces relative events at SYN_REPORT, chunks/clamps movements into HID-sized reports with fractional accumulation, updates relay retry logic, and extends Linux/Windows capture and test harnesses for new scenarios. Changes
Sequence Diagram(s)sequenceDiagram
participant Input as Input Device / evdev
participant Parser as evdev.py (parse → x,y,mwheel,pan)
participant Relay as DeviceRelay (buffer & SYN coalesce)
participant HID as ExtendedMouse (chunking, fractional accum)
participant Gadget as USB Gadget / hidraw
Input->>Parser: raw REL_/BTN_ events
Parser->>Relay: parsed deltas (x,y,mwheel,pan) + button changes
Relay->>Relay: accumulate until SYN_REPORT
Relay->>HID: send chunked move/press actions (via retry wrapper)
HID->>Gadget: write 7‑byte HID reports (may be multiple)
Gadget-->>HID: write result / BlockingIOError (retry)
HID-->>Relay: success / failure (relay may abort)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/bluetooth_2_usb/relay.py (2)
231-238: Docstring return type is stale.The docstring at line 235-236 mentions "Mouse object" but the method now returns
ExtendedMouse | None.📝 Proposed doc fix
def get_mouse(self) -> ExtendedMouse | None: """ Get the Mouse gadget. - :return: A Mouse object, or None if not initialized - :rtype: Mouse | None + :return: An ExtendedMouse object, or None if not initialized + :rtype: ExtendedMouse | None """ return self._gadgets["mouse"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bluetooth_2_usb/relay.py` around lines 231 - 238, The docstring for get_mouse() is out of date: it says "A Mouse object" and uses :rtype: Mouse | None, but the method returns ExtendedMouse | None; update the docstring in get_mouse (and the :rtype/:return description) to reference ExtendedMouse instead of Mouse so the docstring matches the actual return type.
915-950: Consider extracting shared retry/error logic with_process_event_with_retry.Both methods share identical retry loop structure, delay, and exception handling for
BlockingIOErrorandBrokenPipeError. A shared helper could reduce duplication, but given this is a Raspberry Pi HID gadget with specific failure modes, keeping them separate for clarity is also reasonable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bluetooth_2_usb/relay.py` around lines 915 - 950, The retry and exception handling in _process_mouse_delta_with_retry is identical to other handlers — extract the shared loop into a helper _process_event_with_retry(action: Callable[..., Any], action_name: str, *args, **kwargs) that encapsulates the for-loop, BlockingIOError handling (incrementing self._hid_write_retries and sleeping), BrokenPipeError handling (incrementing self._hid_write_failures, logging the detailed message, clearing self._relaying_active if set, and returning), and generic Exception handling (incrementing self._hid_write_failures and logging); then replace the body of _process_mouse_delta_with_retry to call _process_event_with_retry(lambda: self._gadget_manager.get_mouse().move(x,y,wheel,pan), "mouse.move", ) while ensuring you still raise/handle the "mouse gadget not initialized" case inside the action or check before calling the helper so behavior remains identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bluetooth_2_usb/extended_mouse.py`:
- Around line 47-49: The move method currently casts pan to int which drops
fractional hi-res pan values coming from get_mouse_movement; change the logic in
move (the pan variable in def move) to preserve fractional parts across
reporting cycles: keep pan as a float, compute the integer amount to send each
iteration (using sign-aware truncation, e.g.,
int(math.copysign(math.floor(abs(pan)), pan)) or extracting the integer part
while preserving sign), subtract that sent integer from pan to retain the
fractional remainder, and use the integer amount when emitting REL_HWHEEL_HI_RES
reports so slow/high-resolution horizontal scroll accumulates correctly across
calls.
In `@src/bluetooth_2_usb/test_harness_capture_windows.py`:
- Around line 493-502: The code currently returns early when RI_MOUSE_WHEEL or
RI_MOUSE_HORIZONTAL_WHEEL are set, dropping lLastX/lLastY and preventing
coalesced REL_* steps; instead, detect wheel flags from raw_mouse.ulButtons
(button_flags), extract the wheel value (use ctypes.c_short((raw_mouse.ulButtons
>> 16) & 0xFFFF).value for both vertical and horizontal as needed), append the
appropriate wheel report bytes to reports (like you already do for pan) but do
NOT return immediately—remove the early returns in the RI_MOUSE_WHEEL and
RI_MOUSE_HORIZONTAL_WHEEL branches so the function continues to process
lLastX/lLastY and other flags before returning the combined reports. Ensure you
still only append wheel reports when the extracted wheel value is nonzero.
---
Nitpick comments:
In `@src/bluetooth_2_usb/relay.py`:
- Around line 231-238: The docstring for get_mouse() is out of date: it says "A
Mouse object" and uses :rtype: Mouse | None, but the method returns
ExtendedMouse | None; update the docstring in get_mouse (and the :rtype/:return
description) to reference ExtendedMouse instead of Mouse so the docstring
matches the actual return type.
- Around line 915-950: The retry and exception handling in
_process_mouse_delta_with_retry is identical to other handlers — extract the
shared loop into a helper _process_event_with_retry(action: Callable[..., Any],
action_name: str, *args, **kwargs) that encapsulates the for-loop,
BlockingIOError handling (incrementing self._hid_write_retries and sleeping),
BrokenPipeError handling (incrementing self._hid_write_failures, logging the
detailed message, clearing self._relaying_active if set, and returning), and
generic Exception handling (incrementing self._hid_write_failures and logging);
then replace the body of _process_mouse_delta_with_retry to call
_process_event_with_retry(lambda:
self._gadget_manager.get_mouse().move(x,y,wheel,pan), "mouse.move", ) while
ensuring you still raise/handle the "mouse gadget not initialized" case inside
the action or check before calling the helper so behavior remains identical.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bf5c8be2-ca87-4a6d-a33b-d0dba38bd0b8
📒 Files selected for processing (12)
docs/host-relay-loopback.mdsrc/bluetooth_2_usb/evdev.pysrc/bluetooth_2_usb/extended_mouse.pysrc/bluetooth_2_usb/gadget_config.pysrc/bluetooth_2_usb/hid_layout.pysrc/bluetooth_2_usb/relay.pysrc/bluetooth_2_usb/test_harness_capture.pysrc/bluetooth_2_usb/test_harness_capture_windows.pysrc/bluetooth_2_usb/test_harness_common.pysrc/bluetooth_2_usb/test_harness_inject.pytests/test_relay.pytests/test_test_harness.py
Owner
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/bluetooth_2_usb/relay.py (1)
1052-1064: Note:move_mousenow appears unused in the coalescing flow.With
RelEventevents handled via_accumulate_mouse_movement+_flush_pending_mouse_movement, this function isn't called fromDeviceRelay. It's still reachable throughrelay_event()for external callers. If this is intentional API preservation, consider adding a brief docstring note; otherwise it could be removed in a follow-up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bluetooth_2_usb/relay.py` around lines 1052 - 1064, The move_mouse function appears unused by the internal coalescing flow (_accumulate_mouse_movement and _flush_pending_mouse_movement) but is still reachable via relay_event and may be intended as a public compatibility API; either add a brief docstring note on move_mouse clarifying it's preserved for external callers/legacy API (mention DeviceRelay and relay_event as callers) or remove the function in a follow-up cleanup if truly unused — update the docstring on move_mouse to state its preserved/legacy purpose and intended callers (relay_event/DeviceRelay) so future maintainers know why it remains.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/bluetooth_2_usb/relay.py`:
- Around line 1052-1064: The move_mouse function appears unused by the internal
coalescing flow (_accumulate_mouse_movement and _flush_pending_mouse_movement)
but is still reachable via relay_event and may be intended as a public
compatibility API; either add a brief docstring note on move_mouse clarifying
it's preserved for external callers/legacy API (mention DeviceRelay and
relay_event as callers) or remove the function in a follow-up cleanup if truly
unused — update the docstring on move_mouse to state its preserved/legacy
purpose and intended callers (relay_event/DeviceRelay) so future maintainers
know why it remains.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ef1e26a1-a65a-4bbc-828e-5256177c5fca
📒 Files selected for processing (12)
docs/host-relay-loopback.mdsrc/bluetooth_2_usb/evdev.pysrc/bluetooth_2_usb/extended_mouse.pysrc/bluetooth_2_usb/gadget_config.pysrc/bluetooth_2_usb/hid_layout.pysrc/bluetooth_2_usb/relay.pysrc/bluetooth_2_usb/test_harness_capture.pysrc/bluetooth_2_usb/test_harness_capture_windows.pysrc/bluetooth_2_usb/test_harness_common.pysrc/bluetooth_2_usb/test_harness_inject.pytests/test_relay.pytests/test_test_harness.py
Owner
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/bluetooth_2_usb/hid_layout.py (1)
80-88: AC Pan HID descriptor bytes are functionally correct.The web search confirmed 0x0238 is the correct usage code for AC Pan (Consumer page 0x0C). The bytes parse correctly as a valid HID descriptor. However, the extended 2-byte usage item (0x0A 0x38 0x02) crosses the declared two-byte row boundary, violating the stated formatting goal on lines 9–10 and making the item boundaries harder to audit at a glance. Consider restructuring lines 81–82 to keep HID items within two-byte rows for consistency with the rest of the descriptor.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bluetooth_2_usb/hid_layout.py` around lines 80 - 88, The AC Pan extended usage item (bytes 0x0A, 0x38, 0x02) currently crosses the two-byte row boundary; update the HID descriptor in hid_layout.py so that the entire extended usage item is kept on a single row (i.e., group 0x0A, 0x38, 0x02 together) and then resume the existing two-byte row grouping for subsequent items (like 0x15, 0x81, 0x7F, etc.) so item boundaries are auditable and consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/bluetooth_2_usb/hid_layout.py`:
- Around line 80-88: The AC Pan extended usage item (bytes 0x0A, 0x38, 0x02)
currently crosses the two-byte row boundary; update the HID descriptor in
hid_layout.py so that the entire extended usage item is kept on a single row
(i.e., group 0x0A, 0x38, 0x02 together) and then resume the existing two-byte
row grouping for subsequent items (like 0x15, 0x81, 0x7F, etc.) so item
boundaries are auditable and consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 40a2f1fd-8e8f-474d-8ba3-fb9b94f4a68c
📒 Files selected for processing (13)
docs/host-relay-loopback.mdsrc/bluetooth_2_usb/args.pysrc/bluetooth_2_usb/evdev.pysrc/bluetooth_2_usb/extended_mouse.pysrc/bluetooth_2_usb/gadget_config.pysrc/bluetooth_2_usb/hid_layout.pysrc/bluetooth_2_usb/relay.pysrc/bluetooth_2_usb/test_harness_capture.pysrc/bluetooth_2_usb/test_harness_capture_windows.pysrc/bluetooth_2_usb/test_harness_common.pysrc/bluetooth_2_usb/test_harness_inject.pytests/test_relay.pytests/test_test_harness.py
16 tasks
Owner
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/test_test_harness.py (1)
587-624: Add Windows button Raw Input coverage here.These new
_mouse_event_to_reports()tests cover wheel/pan and 16-bit motion, but they still never assert anyRI_MOUSE_*_BUTTON_(DOWN|UP)conversion. SinceMOUSE_BUTTON_STEPSis now part ofmouse/combo, a button down/up case here would catch Windows-only regressions before they turn into harness timeouts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_test_harness.py` around lines 587 - 624, Add a test case exercising Windows raw input button down/up conversion to _mouse_event_to_reports: create a RAWMOUSE with ulButtons set to an RI_MOUSE_*_BUTTON_DOWN (and another for _UP) plus a 16-bit button mask (e.g., 0x0001 << 16 or 0xFFFF << 16) and assert the produced reports include the expected button press/release report bytes alongside any wheel/pan/motion reports; reference the _mouse_event_to_reports function, RAWMOUSE struct, RI_MOUSE_*_BUTTON_DOWN/UP constants and MOUSE_BUTTON_STEPS to locate where to add the new assertions so Windows-only regressions are caught.src/bluetooth_2_usb/hid_layout.py (1)
149-177: Preserveconfigfs_report_lengthwhen cloning from an existing gadget device.If
base_deviceis already aGadgetHidDevice, omitting the override currently drops itsconfigfs_report_length. Keeping inheritance behavior consistent with other fields avoids accidental fallback to descriptor-length semantics in future clones.Suggested patch
- configfs_report_length=configfs_report_length, + configfs_report_length=( + getattr(base_device, "configfs_report_length", None) + if configfs_report_length is None + else configfs_report_length + ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bluetooth_2_usb/hid_layout.py` around lines 149 - 177, When cloning a GadgetHidDevice, the configfs_report_length field is being set directly to the passed configfs_report_length argument which drops the base device's value when the argument is None; update the factory/clone call (the classmethod that returns cls(...), which constructs a GadgetHidDevice from base_device) to preserve inheritance by using base_device.configfs_report_length when configfs_report_length is None (i.e., set configfs_report_length = base_device.configfs_report_length if the param is None else the provided param).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bluetooth_2_usb/test_harness_capture_windows.py`:
- Around line 493-511: The _mouse_event_to_reports function currently only emits
wheel/X/Y reports and ignores RI_MOUSE_*_BUTTON_DOWN/UP flags, causing missing
button-state reports; update _mouse_event_to_reports to maintain a persistent
mouse button state (e.g., module-level or self.mouse_button_state) and map
RI_MOUSE_LEFT_BUTTON_DOWN/UP, RI_MOUSE_RIGHT_BUTTON_DOWN/UP,
RI_MOUSE_MIDDLE_BUTTON_DOWN/UP and any extra RI_MOUSE_* button flags into the
first byte of each emitted report (bitmask per button), updating that stored
state when a DOWN sets the bit and UP clears it; include the current accumulated
button-state byte as the first byte when constructing every report (including
wheel, horizontal wheel, X and Y reports) so button press/release events produce
the expected reports for the matcher.
---
Nitpick comments:
In `@src/bluetooth_2_usb/hid_layout.py`:
- Around line 149-177: When cloning a GadgetHidDevice, the
configfs_report_length field is being set directly to the passed
configfs_report_length argument which drops the base device's value when the
argument is None; update the factory/clone call (the classmethod that returns
cls(...), which constructs a GadgetHidDevice from base_device) to preserve
inheritance by using base_device.configfs_report_length when
configfs_report_length is None (i.e., set configfs_report_length =
base_device.configfs_report_length if the param is None else the provided
param).
In `@tests/test_test_harness.py`:
- Around line 587-624: Add a test case exercising Windows raw input button
down/up conversion to _mouse_event_to_reports: create a RAWMOUSE with ulButtons
set to an RI_MOUSE_*_BUTTON_DOWN (and another for _UP) plus a 16-bit button mask
(e.g., 0x0001 << 16 or 0xFFFF << 16) and assert the produced reports include the
expected button press/release report bytes alongside any wheel/pan/motion
reports; reference the _mouse_event_to_reports function, RAWMOUSE struct,
RI_MOUSE_*_BUTTON_DOWN/UP constants and MOUSE_BUTTON_STEPS to locate where to
add the new assertions so Windows-only regressions are caught.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8269d14f-5df3-4844-8b43-efb923cc3830
📒 Files selected for processing (13)
docs/host-relay-loopback.mdsrc/bluetooth_2_usb/args.pysrc/bluetooth_2_usb/evdev.pysrc/bluetooth_2_usb/extended_mouse.pysrc/bluetooth_2_usb/gadget_config.pysrc/bluetooth_2_usb/hid_layout.pysrc/bluetooth_2_usb/relay.pysrc/bluetooth_2_usb/test_harness_capture.pysrc/bluetooth_2_usb/test_harness_capture_windows.pysrc/bluetooth_2_usb/test_harness_common.pysrc/bluetooth_2_usb/test_harness_inject.pytests/test_relay.pytests/test_test_harness.py
Owner
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bluetooth_2_usb/evdev.py`:
- Around line 1291-1294: The horizontal wheel handling currently adds both
REL_HWHEEL and REL_HWHEEL_HI_RES into _pending_rel_pan causing doubled pan;
modify the handler in evdev event processing to prefer the hi-res event: when
REL_HWHEEL_HI_RES is received set pan to input_event.value/120 and mark a flag
(e.g., _seen_rel_hwheel_hi_res) for the current SYN frame, and when REL_HWHEEL
is received ignore it if that flag is set for this frame; reset the flag at SYN
(where relay.py accumulates and flushes _pending_rel_pan) so low-res fallback is
only used when no hi-res event arrived in that SYN frame.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 656a7755-3b11-4d44-8378-f2d928373678
📒 Files selected for processing (13)
docs/host-relay-loopback.mdsrc/bluetooth_2_usb/args.pysrc/bluetooth_2_usb/evdev.pysrc/bluetooth_2_usb/extended_mouse.pysrc/bluetooth_2_usb/gadget_config.pysrc/bluetooth_2_usb/hid_layout.pysrc/bluetooth_2_usb/relay.pysrc/bluetooth_2_usb/test_harness_capture.pysrc/bluetooth_2_usb/test_harness_capture_windows.pysrc/bluetooth_2_usb/test_harness_common.pysrc/bluetooth_2_usb/test_harness_inject.pytests/test_relay.pytests/test_test_harness.py
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bluetooth_2_usb/test_harness_capture_windows.py`:
- Around line 559-610: The _mouse_event_to_reports function currently emits
wheel/pan reports before coalesced X/Y, causing the shared mouse sequence to see
pan -> x -> y; change the emission order so any lLastX and lLastY reports are
appended before RI_MOUSE_WHEEL or RI_MOUSE_HORIZONTAL_WHEEL reports (or combine
into a single report containing button state, X bytes, Y bytes, and wheel/pan
bytes) to preserve the coalesced tail; reference symbols:
_mouse_event_to_reports, lLastX, lLastY, RI_MOUSE_WHEEL,
RI_MOUSE_HORIZONTAL_WHEEL, and _mouse_button_state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 05bfbb5c-0c37-4213-bb1f-f271daf8cc4f
📒 Files selected for processing (7)
docs/host-relay-loopback.mdsrc/bluetooth_2_usb/extended_mouse.pysrc/bluetooth_2_usb/relay.pysrc/bluetooth_2_usb/test_harness_capture_windows.pysrc/bluetooth_2_usb/test_harness_common.pytests/test_relay.pytests/test_test_harness.py
🚧 Files skipped from review as they are similar to previous changes (4)
- src/bluetooth_2_usb/extended_mouse.py
- docs/host-relay-loopback.md
- tests/test_relay.py
- tests/test_test_harness.py
Owner
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bluetooth_2_usb/test_harness_capture.py (1)
233-254:⚠️ Potential issue | 🟠 MajorValidate button state on motion packets too.
Lines 233-250 only compare
buttonswhen the packet has no motion. That means a button press piggybacked onto a movement chunk is accepted even if the scenario expects that button transition later, so misordered or sticky button bits can slip through capture. Motion packets should still be checked against the current_button_state; only the step advancement needs to wait for the zero-motion button report.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bluetooth_2_usb/test_harness_capture.py` around lines 233 - 254, The code only validates button bits when rel_x, rel_y, wheel and pan are all zero; add a check for button state on packets that contain motion by comparing the incoming buttons value against the current self._button_state and raising CaptureMismatchError if they differ. Locate the motion branch (where rel_x, rel_y, wheel, pan are not all zero) and insert a validation using self._button_state (and use self._apply_button_step / self.expected_button_steps and self.button_index only for determining expected future transitions), ensuring only the zero-motion path advances self.button_index while motion packets still assert buttons == self._button_state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/host-relay-loopback.md`:
- Around line 158-163: Update the docs/host-relay-loopback.md section that
instructs running the mouse_buttons_intrusive scenario to add a Windows-specific
caveat: note that src/bluetooth_2_usb/test_harness_capture_windows.py rejects
scenarios containing BTN_FORWARD, BTN_BACK and BTN_TASK with EXIT_PREREQUISITE
so the mouse_buttons_intrusive path will immediately fail on Windows backends
that do not surface all eight mouse-button bits; recommend running the scenario
only on non-Windows/backends that support all eight bits or provide an
alternative scenario for Windows.
In `@src/bluetooth_2_usb/test_harness_capture.py`:
- Around line 195-201: The current __post_init__ groups expected_rel_steps into
_pending_rel_remaining_by_code, losing cross-axis ordering; change it to
preserve sequence (e.g., store an ordered list like _pending_rel_remaining:
list[tuple[int,int]] populated from expected_rel_steps in __post_init__) and
update the REL-event matcher logic to consume from that ordered list
front-to-back, only allowing relaxed matching across axes within the same
multi-axis report (i.e., when a single input report contains multiple REL_*
events, permit matching those out-of-order among themselves but never allow a
REL from a later expected position in the global sequence to match before an
earlier one). Ensure all places that referenced _pending_rel_remaining_by_code
are updated to use the ordered structure and that the same change is applied to
the analogous logic noted in the second block (the matcher that builds/consumes
pending relative steps).
---
Outside diff comments:
In `@src/bluetooth_2_usb/test_harness_capture.py`:
- Around line 233-254: The code only validates button bits when rel_x, rel_y,
wheel and pan are all zero; add a check for button state on packets that contain
motion by comparing the incoming buttons value against the current
self._button_state and raising CaptureMismatchError if they differ. Locate the
motion branch (where rel_x, rel_y, wheel, pan are not all zero) and insert a
validation using self._button_state (and use self._apply_button_step /
self.expected_button_steps and self.button_index only for determining expected
future transitions), ensuring only the zero-motion path advances
self.button_index while motion packets still assert buttons ==
self._button_state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d0951eb1-6c23-4de5-9f41-c994b0d1bf7a
📒 Files selected for processing (13)
docs/host-relay-loopback.mdsrc/bluetooth_2_usb/args.pysrc/bluetooth_2_usb/evdev.pysrc/bluetooth_2_usb/extended_mouse.pysrc/bluetooth_2_usb/gadget_config.pysrc/bluetooth_2_usb/hid_layout.pysrc/bluetooth_2_usb/relay.pysrc/bluetooth_2_usb/test_harness_capture.pysrc/bluetooth_2_usb/test_harness_capture_windows.pysrc/bluetooth_2_usb/test_harness_common.pysrc/bluetooth_2_usb/test_harness_inject.pytests/test_relay.pytests/test_test_harness.py
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bluetooth_2_usb/test_harness_capture.py (1)
398-413:⚠️ Potential issue | 🟡 MinorAccept the unnumbered 5-byte pan layout too.
The new pan-aware branch only recognizes the compact format when it arrives as
0x02 + payload. A valid unnumberedbuttons,x,y,wheel,panreport still falls through toNone, so the capture harness will reject that layout as an unexpected mouse report.🩹 Proposed fix
if len(report) == 8 and report[0] == 0x02: payload = report[1:] elif len(report) == 7: payload = report elif len(report) == 6 and report[0] == 0x02: payload = report[1:] elif len(report) == 5 and report[0] == 0x02: payload = report[1:] + elif len(report) == 5: + payload = report elif len(report) == 4 and report[0] == 0x02: payload = report[1:]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bluetooth_2_usb/test_harness_capture.py` around lines 398 - 413, The code path that extracts payload from incoming 'report' only accepts the 5-byte pan layout when it is prefixed with 0x02; update the branch that sets 'payload' (the if/elif chain handling len(report) == 8/7/6/5/4/3) to also accept an unnumbered 5-byte report by adding an elif len(report) == 5: payload = report case (or merge it with the existing 5-byte check) so the unnumbered buttons,x,y,wheel,pan layout is treated as valid and not returned as None.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/bluetooth_2_usb/test_harness_capture.py`:
- Around line 398-413: The code path that extracts payload from incoming
'report' only accepts the 5-byte pan layout when it is prefixed with 0x02;
update the branch that sets 'payload' (the if/elif chain handling len(report) ==
8/7/6/5/4/3) to also accept an unnumbered 5-byte report by adding an elif
len(report) == 5: payload = report case (or merge it with the existing 5-byte
check) so the unnumbered buttons,x,y,wheel,pan layout is treated as valid and
not returned as None.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c69b0dce-80a3-46dd-900e-9eb9e02a0b97
📒 Files selected for processing (6)
docs/host-relay-loopback.mdsrc/bluetooth_2_usb/evdev.pysrc/bluetooth_2_usb/extended_mouse.pysrc/bluetooth_2_usb/test_harness_capture.pytests/test_relay.pytests/test_test_harness.py
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/host-relay-loopback.md
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bluetooth_2_usb/test_harness_capture.py (1)
217-256:⚠️ Potential issue | 🟠 MajorValidate mouse button state on motion reports too.
The new zero-motion gate means
buttonsis only checked for button-only packets. Any movement packet with a stale or missing button byte now slips through, even though the HID report always carries current button state and the Windows helper already emits that state on motion packets. This can hide relay bugs where a press/release is coalesced with movement or the button bit stays latched after release.🔧 Suggested direction
if rel_events: self._apply_rel_report(rel_events) - if rel_x == 0 and rel_y == 0 and wheel == 0 and pan == 0: - if self.button_index >= len(self.expected_button_steps): - if buttons == 0: - return - raise CaptureMismatchError( - f"Unexpected extra mouse button report {report.hex(sep=' ')}" - ) - - if not self.rel_complete: - if buttons != 0: - raise CaptureMismatchError( - "Mouse button report arrived before movement" - ) - return - - expected = self.expected_button_steps[self.button_index] - expected_buttons = self._apply_button_step(expected) - if buttons != expected_buttons: - raise CaptureMismatchError( - f"Unexpected mouse button report {report.hex(sep=' ')}; expected {expected.describe()}" - ) - self.button_index += 1 + if buttons != self._button_state: + if self.button_index >= len(self.expected_button_steps): + raise CaptureMismatchError( + f"Unexpected mouse button bits in report {report.hex(sep=' ')}" + ) + if not self.rel_complete: + raise CaptureMismatchError("Mouse button report arrived before movement") + + expected = self.expected_button_steps[self.button_index] + expected_buttons = self._apply_button_step(expected) + if buttons != expected_buttons: + raise CaptureMismatchError( + f"Unexpected mouse button report {report.hex(sep=' ')}; expected {expected.describe()}" + ) + self.button_index += 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bluetooth_2_usb/test_harness_capture.py` around lines 217 - 256, Validate the mouse button bits for every parsed report (buttons, rel_x, rel_y, wheel, pan), not only for zero-motion reports: after parsing the report but before returning/applying rel events, check if self.button_index >= len(self.expected_button_steps) and ensure buttons == 0 (raise the existing "Unexpected extra mouse button report" error if not); otherwise if not self.rel_complete enforce that buttons == 0 (raise "Mouse button report arrived before movement" if violated); when a button step is expected compute expected_buttons via expected = self.expected_button_steps[self.button_index] and expected_buttons = self._apply_button_step(expected) and compare buttons to expected_buttons (raise the existing "Unexpected mouse button report ...; expected ..." error if they differ) — preserving the existing error messages and increments of self.button_index when matched, but perform these checks for both motion and zero-motion packets (i.e., move the button-validation logic out of the zero-motion-only branch so it runs before/alongside the rel_events/_apply_rel_report handling).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/bluetooth_2_usb/test_harness_capture.py`:
- Around line 217-256: Validate the mouse button bits for every parsed report
(buttons, rel_x, rel_y, wheel, pan), not only for zero-motion reports: after
parsing the report but before returning/applying rel events, check if
self.button_index >= len(self.expected_button_steps) and ensure buttons == 0
(raise the existing "Unexpected extra mouse button report" error if not);
otherwise if not self.rel_complete enforce that buttons == 0 (raise "Mouse
button report arrived before movement" if violated); when a button step is
expected compute expected_buttons via expected =
self.expected_button_steps[self.button_index] and expected_buttons =
self._apply_button_step(expected) and compare buttons to expected_buttons (raise
the existing "Unexpected mouse button report ...; expected ..." error if they
differ) — preserving the existing error messages and increments of
self.button_index when matched, but perform these checks for both motion and
zero-motion packets (i.e., move the button-validation logic out of the
zero-motion-only branch so it runs before/alongside the
rel_events/_apply_rel_report handling).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2458531a-7cbe-42a6-bc36-a2494c24836b
📒 Files selected for processing (3)
src/bluetooth_2_usb/test_harness_capture.pysrc/bluetooth_2_usb/test_harness_capture_windows.pytests/test_test_harness.py
✅ Files skipped from review due to trivial changes (1)
- src/bluetooth_2_usb/test_harness_capture_windows.py
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bluetooth_2_usb/test_harness_capture_windows.py`:
- Around line 524-526: The helper _mouse_i16_bytes clamps input incorrectly at
-32767, losing the valid int16 lower bound; update the clamping to allow the
full signed 16-bit range by using -32768 as the minimum (e.g., clamp to between
-32768 and 32767) and then call to_bytes(2, "little", signed=True) as before so
-32768 is preserved in the little-endian int16 encoding.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3411cc61-1c37-428c-8ff8-a526d3958439
📒 Files selected for processing (3)
src/bluetooth_2_usb/test_harness_capture.pysrc/bluetooth_2_usb/test_harness_capture_windows.pytests/test_test_harness.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/bluetooth_2_usb/test_harness_capture.py
- tests/test_test_harness.py
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/test_test_harness.py (2)
127-132: Make the unknown-scenario assertion less order-sensitive.This hard-coded list will fail on the next scenario addition or a harmless registration reorder, even if
get_scenario()still reports the right error. Build the expected names fromSCENARIOSor only assert the stable prefix / missing scenario name.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_test_harness.py` around lines 127 - 132, The test test_invalid_scenario_name_is_reported_cleanly currently asserts a hard-coded list of scenario names making it brittle; change it to build the expected portion of the error dynamically from the registered SCENARIOS (or assert only the stable parts). Specifically, update the assertion for get_scenario("nope") to either (a) construct the "Expected one of: ..." substring by joining the keys/names from SCENARIOS (e.g., sorted(SCENARIOS.keys())) so the message reflects current registrations, or (b) only assert the stable prefix "Unknown scenario 'nope'." and that the error mentions "Expected one of" without enumerating names; locate this in the test_invalid_scenario_name_is_reported_cleanly test and adjust the assertRaisesRegex accordingly.
20-36: Use module-qualified calls for functions when the module reloads during tests.After
importlib.reload(test_harness_capture_windows)in the test's finally block, the imported aliases_mouse_event_to_reportsand_reset_mouse_button_stateremain bound to the pre-reload function objects. Subsequent test methods will exercise stale code, potentially masking bugs if behavior changes on reload. Replace direct calls with module-qualified calls liketest_harness_capture_windows._mouse_event_to_reports().Also applies to: 856-867
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_test_harness.py` around lines 20 - 36, The tests import private functions like _mouse_event_to_reports and _reset_mouse_button_state by name which become stale after importlib.reload(test_harness_capture_windows); change the tests to call the functions via the module object (e.g. test_harness_capture_windows._mouse_event_to_reports(...) and test_harness_capture_windows._reset_mouse_button_state(...)) instead of the imported aliases, and apply the same module-qualified replacement for other private symbols used (e.g. _stable_device_identity, _extract_device_identities, _device_matches_candidate, _keyboard_event_to_report) including the other occurrences noted (lines ~856-867).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/test_test_harness.py`:
- Around line 127-132: The test test_invalid_scenario_name_is_reported_cleanly
currently asserts a hard-coded list of scenario names making it brittle; change
it to build the expected portion of the error dynamically from the registered
SCENARIOS (or assert only the stable parts). Specifically, update the assertion
for get_scenario("nope") to either (a) construct the "Expected one of: ..."
substring by joining the keys/names from SCENARIOS (e.g.,
sorted(SCENARIOS.keys())) so the message reflects current registrations, or (b)
only assert the stable prefix "Unknown scenario 'nope'." and that the error
mentions "Expected one of" without enumerating names; locate this in the
test_invalid_scenario_name_is_reported_cleanly test and adjust the
assertRaisesRegex accordingly.
- Around line 20-36: The tests import private functions like
_mouse_event_to_reports and _reset_mouse_button_state by name which become stale
after importlib.reload(test_harness_capture_windows); change the tests to call
the functions via the module object (e.g.
test_harness_capture_windows._mouse_event_to_reports(...) and
test_harness_capture_windows._reset_mouse_button_state(...)) instead of the
imported aliases, and apply the same module-qualified replacement for other
private symbols used (e.g. _stable_device_identity, _extract_device_identities,
_device_matches_candidate, _keyboard_event_to_report) including the other
occurrences noted (lines ~856-867).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dd20ffc3-9594-4bee-85bb-57392bc4d4ef
📒 Files selected for processing (2)
src/bluetooth_2_usb/test_harness_capture_windows.pytests/test_test_harness.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/bluetooth_2_usb/test_harness_capture_windows.py
quaxalber
changed the title
This was referenced Apr 28, 2026
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.
Summary
report_lengthto 8 on the mouse gadget.Pi/URB findings
report_lengthcaused every 7-byte mouse interrupt-IN completion to be followed by an extra 0-byte completion.Validation
venv/bin/black --check src testsvenv/bin/ruff check src testsvenv/bin/python -m unittest discover -s tests -vvenv/bin/python -m compileall src testspython -m buildsmoketest.sh --verbose,debug.sh --duration 10,bluetoothctl show,btmgmt infoeac464c8d2c020b87f693c2093a3021e4593ba98capture harness:combopassed withkeyboard_steps_seen=6,mouse_rel_steps_seen=11,mouse_button_steps_seen=4;mouse_fastpassed withmouse_rel_steps_seen=8Closes #11.
Closes #167.
Closes #168.
Summary by CodeRabbit
New Features
Documentation
Refactor
Tests