Skip to content

Implement CH9350L UART protocol into kvm-serial#40

Merged
sjmf merged 45 commits into
mainfrom
feature/13-ch9350L-support
May 10, 2026
Merged

Implement CH9350L UART protocol into kvm-serial#40
sjmf merged 45 commits into
mainfrom
feature/13-ch9350L-support

Conversation

@sjmf

@sjmf sjmf commented May 10, 2026

Copy link
Copy Markdown
Owner

Closes #13.

Summary

  • Adds CH9350Comm covering all four CH9350L working states (0/1 paired with descriptor handshake, 2 BIOS+rel mouse, 3 BIOS+abs mouse, 4 HID Digitizers), wired through DataCommManager with selectors on the CLI (--ch9350 / --ch9350-state) and in the GUI (with settings persistence)
  • Documents the protocol from empirical capture (docs/CH9350L_PROTO.md) and adds a per-chip user guide (docs/CH9350L_GUIDE.md); docs/CH9329_GUIDE.md and docs/CH9329_PROTO.md added for parity now that the family has two members
  • 309 unit tests passing across all groups, including descriptor-announce, state transitions, LED echo, multi-frame fan-out for relative-mode large deltas, and drag-preserves-held-button regression coverage

A handful of non-obvious empirical findings (state-0/1 abs is silently dropped by the chip; state-3/4 X/Y range is 10-bit not 16-bit; state-0/1/2 inherits target-side pointer acceleration) — see the issue's closing comment and the §Divergences section of the protocol doc.

Test plan

  • Confirm pytest tests/ passes locally (309 passed, 4 skipped on my run)
  • Confirm black --check is clean
  • State 0/1: smoke test typing + relative-drag against real hardware
  • State 2: smoke test typing + relative mouse against real hardware
  • State 3: smoke test typing + absolute mouse + drag against real hardware
  • State 4: smoke test against single-monitor target (multi-monitor not yet verified — covered in issue closing comment)
  • GUI: protocol + state selectors persist across restarts
  • CLI: python -m kvm_serial.control --ch9350 --ch9350-state 3 /dev/cu.usbserial-XXXX works

sjmf and others added 30 commits May 2, 2026 23:07
docs/CH9350L_PROTO.md has empirically verified frame formats for State 0/1, state machine, CTR_SUM formula, SER byte discussion
Bidirectional sniff (2026-05-03) confirmed the 0x81 Device Connection
Frame as the load-bearing piece previously missing from emulate. State-1
transition is gated on UC ack of all announced PIDs, not the first 0x12.
0x86 fires at both attach and disconnect (datasheet documents only the
disconnect case). Updates ch9350_poc.py to send captured HID descriptors
on attach; key/mouse forwarding now works end-to-end. Documents protocol
in docs/CH9350L_PROTO.md, including divergences from datasheet §4.1, §4.3,
§4.6, §4.8, and §3.2.


The 0x12 keep-alive's STATUS byte is bit-encoded for per-port USB enumeration on the target host (bits 0,1) plus a UART-link bit (bit 2); 0x07 is the only "all green" state.

State-1 entry is a UART-internal PID-ack and does not imply HID forwarding works — STATUS == 0x07 does.

Target-side replug recovery requires replaying the full attach sequence (0x86 → 0x80 ×2 → 0x89 → 0x81 ×N); 0x81 retransmit alone leaves the UC at STATUS=0x04.

Validated end-to-end against the PoC's reattach trigger.
… (towards #13)

State 2 (S1=HIGH, S0=LOW) now verified: keyboard 0x01 frames, relative mouse 0x02 frames, 0x80 LED-feedback channel from LC→UC, and UC 0x12 keep-alive flowing throughout.

Absolute mouse 0x04 confirmed *not* forwarded in state 2 (CH9329 abs-mouse reports silently dropped while keyboard from the same device passes through).

Renamed STARTUP_STATUS decoder to LED_STATUS in the PoC to match the frame's actual role.
…asheet (towards #13)

States 3 (S1=LOW, S0=HIGH) and 4 (S1=LOW, S0=LOW) are on-the-wire identical to state 2 except they emit 0x04 absolute-mouse frames; the difference between them is entirely in what the UC's built-in HID descriptor advertises to the target (HID Mouse vs HID Digitizers, per datasheet §3.4–3.5). All three alternative-mode dipswitch configurations advertise a BIOS keyboard on the target side, making them suitable for legacy / pre-boot interfacing.

Also documents the 0x10 VID/PID-modification command available in states 2/3/4.
…ures (towards #13)

States 2, 3 and 4 (S1/S0 dipswitch combinations) all advertise a USB HID
Boot-protocol keyboard, but differ in mouse semantics: state 2 pairs it
with a relative mouse (the legacy/UEFI-CSM-compatible combination), state
3 with absolute mouse, and state 4 adds HID Digitizers for Windows
multi-monitor use. State 3 and state 4 are on-the-wire identical at the
UART layer; the difference is in the UC's built-in USB descriptor.
…wards #13)

Consolidate a thicket of repeated claims (0x86/0x80/0x89 attach sequence stated in 4 places, STATUS=0x07/0x04 explained 4 times, PID-ack-as-state-1 trigger in 3) into single canonical descriptions with cross-references.

Replace the split CMD payload table in §Frame Structure with a unified table covering direction, length, and per-state applicability for every opcode including 0x10 (VID/PID modify) which was previously missing.

Tighten claims to match the actual evidence base: 0x89 generalised across state-0/1 (3 instances during descriptor announce) vs state-2/3/4 (single startup instance); STATUS=0xFF documented as "UC fully unknown" covering both startup transient and target-replug; the speculative "UC enumerates a default device" inference flagged as likely-mechanism rather than fact.

Drop date stamps throughout — provenance now lives in the gist links, not in watermarks that age poorly.
…sor motion

The defining feature of state 4 is that the UC identifies on the target USB bus as a HID Digitizers device class (graphics-tablet/pen input), not as a regular mouse: clear that up.

Real S4 LC emits 0x04 abs-mouse frames  with sub-pixel ~50 ms deltas during continuous motion. An emulator that fires one isolated frame per move command produces no visible cursor movement on the target even though the report does reach the target's USB host (screen un-dims).

The possible mechanism is that HID Digitizers / abs-mouse drivers treat isolated reports as glitches and only act on a sustained stream. Any S3/4 LC integration that wants cursor motion can burst frames per move event, mimicking the real LC's poll-driven cadence.
"Digitizer" kept as-is since it's the formal USB HID device class name.
DataComm becomes an abstract base for chip-family protocol implementations, with abstract send_scancode/release matching the surface every *op caller already uses. Existing CH9329 packet building moves into CH9329Comm. BaseOp.__init__ takes a comm_cls kwarg (default CH9329Comm) so future code paths can plug in CH9350Comm without further changes.

Tests patching DataComm in baseop / mouse paths now patch CH9329Comm.

test_communication.py TestDataComm renamed to TestCH9329Comm to be honest about what it covers; CH9350Comm will get its own suite. The ValueError raised for a malformed packet header now reads "CH9329 packet header..." to match.
…rds #13)

DataComm gains abstract send_mouse_absolute / send_mouse_relative methods; CH9329Comm implements them with the spec-correct frame layouts:
  abs (cmd 0x04): 02 btn xL xH yL yH wheel  (7 bytes)
  rel (cmd 0x05): 01 btn dx  dy  wheel       (5 bytes; 1-byte signed)

mouseop.on_move / on_click / on_scroll become forwarders.
 * on_move and on_click are equivalent to the previous implementation.

on_scroll is a behaviour fix: previous code had dx/dy as 2-byte big-endian values into a 5-byte payload, which CH9329 interpreted as: `01 dx_hi dx_lo dy_hi dy_lo  →  btn=dx_hi, dx=dx_lo, dy=dy_hi, wheel=dy_lo`. Vertical scrolls (dy=±1) accidentally landed the value in the wheel slot and worked. Horizontal scrolls (dx≠0) moved the cursor sideways and, for negative dx, asserted spurious button bits: both now fixed. Horizontal scroll itself isn't recoverable: CH9329's relative-mouse frame has only one wheel byte (vertical), so dx is dropped at the mouseop layer.

Tests rebalanced to match the new layering. test_mouse.py asserts on the high-level API calls (mouseop's actual contract) rather than raw bytes; its on_move case shrinks from four nameless tuples to one, since mouseop no longer does the scaling. The four monitor-edge cases (centre, -x, -y, both negative) move to TestCH9329Comm.test_send_mouse_absolute, where the 4096+dx wrap actually lives. New test_send_mouse_relative covers the 5-byte relative frame including signed-byte clamping.
CH9329Comm and its _signed_byte helper move from communication.py into a new kvm_serial/utils/ch9329.py. communication.py keeps the DataComm ABC and list_serial_ports. CH9350Comm will land alongside in ch9350.py.

baseop.py and tests/utils/test_communication.py updated to import from the new module path. No behaviour change.
Move TestCH9329Comm from tests/utils/test_communication.py to a new tests/utils/test_ch9329.py, mirroring the production-side split. The remaining test_communication.py keeps the (currently all-skipped) list_serial_ports tests under TestListSerialPorts; they're left as a TODO marker rather than deleted, since list_serial_ports itself remains in communication.py and warrants coverage once mocked correctly. That's a separate issue to raise.
CH9350Comm wraps the CH9350L's "simple" working states: fixed-format LC→UC frames with no descriptor handshake, no checksum, no per-frame counter.

State 2 (BIOS keyboard + relative mouse) is the legacy BIOS / UEFI CSM compatible path; states 3 and 4 (absolute mouse, optionally HID Digitizers) target modern OS hosts. State 0/1 (paired-mode descriptor handshake) raises ValueError at construction: TODO.

The class translates between the DataComm abstract API and each state's native frame:
  - send_scancode/release: HEADER + 0x01 + 8-byte HID boot report. Identical across all three states.
  - send_mouse_absolute: states 3/4 emit 0x04 frames with x/y scaled to the chip's 16-bit absolute space. State 2 has no abs path on the wire; calls are translated to a relative delta from the previous absolute position (each axis clamped to ±127 px per frame, which pynput's event rate covers in practice).
  - send_mouse_relative: state 2 emits a native 0x02 frame. States 3/4 re-emit the last known absolute position with the new button/wheel bytes — so clicks and scroll events reach the target via the absolute frame, without mouseop needing to know which state is active.

Tests cover wire format per state, cross-state mouse translation, state-3/4 wire equivalence, and constructor rejection of state 0/1/5.
Ports the PoC's LowerComputer state machine into CH9350Comm:
 - start() spawns rx + tx-maintenance daemon threads.
 - Initial attach: 0x86 -> 0x80 x2 -> 0x89 -> 0x81 xN; tx loop heartbeats every ~1 s and retransmits any 0x81 with an un-acked PID.
 - rx loop dispatches to _handle_frame, gating state-0 -> state-1 on full PID-ack and triggering reattach replay on PID drop.
 - send_* emit framed 0x83/0x88 payloads with per-SER counters; abs mouse translates to relative deltas (default descriptor is rel).

Default descriptors / PIDs baked in from real-LC captures; constructor accepts overrides. State 1 is internal-only (constructor rejects it).

11 new tests cover frame builders, _parse_frames, state-machine transitions, short-frame handling, and start/stop no-op for states 2/3/4.

Limitation: LED echo (0x80 0x3N) for states 2/3 isn't wired up yet: the rx loop only runs in state 0. Threading + live I/O still to hardware-test.
The CH9350L mirrors the target host's keyboard LED state back to the source keyboard via 0x80 0x3N (low nibble = NumLk/CapsLk/ScrLk bits). Wire that up in the simple modes:

 - start() now spawns the rx thread for states 2/3/4 too (no tx maintenance is needed- there's no handshake, just LED feedback).
 - _handle_frame's state-2/3/4 branch echoes the UC's 0x12 byte-4 LED state on change; never echoes the 0xFF pre-enum sentinel.

Two new tests cover the echo path (change-only, FF-skip) and verify the simple-mode early-return keeps state-0 PID logic from running in state 2.
DataComm: added no-op start()/stop() lifecycle hooks (CH9350Comm overrides; CH9329Comm no-ops). BaseOp's comm_cls annotation widens to Callable[[Serial], DataComm]: callers pass a class directly or a lambda/partial when constructor args are needed (e.g. CH9350 state). __init__ calls hid_serial_out.start() and the default cleanup() calls stop() so any background threads exit cleanly.

KeyboardListener and MouseListener: pass comm_cls through to the underlying op; the keyboard runner wraps op.run() in try/finally so cleanup runs on the listener thread.

kvm-control gains a mutually-exclusive --ch9329 / --ch9350 protocol flag (default CH9329; --ch9329 is imperative-only) plus --ch9350-state in {0, 2, 3, 4}, defaulting to 2 (legacy-BIOS / UEFI CSM compatible). Without --ch9350 the CH9329 path is unchanged.

6 new tests: BaseOp comm_cls invocation, BaseOp cleanup, CLI defaults, --ch9350 --ch9350-state=3 producing a CH9350Comm in state 3, --ch9329 parsing as a no-op, and --ch9329 + --ch9350 erroring as mutex. Existing baseop and keyboard tests updated for the new kwarg.
Introduce DataCommManager as a singleton owning the DataComm instance and the input-listener lifecycle for a kvm-serial run. Two MouseOp / KeyboardOp instances would otherwise each instantiate their own CH9350Comm, racing for the UART, running the descriptor handshake twice, and emitting frames with desynchronised counters.

DataCommManager(serial_port, comm_cls=CH9329Comm) constructs the comm via the callable and registers itself as the singleton. Subsequent BaseOp.__init__ calls fetch self.hid_serial_out via DataCommManager.get().comm; ownership of start()/stop() is the manager's, never BaseOp's. attach() registers listeners; start() runs comm-then-listeners, stop() runs listeners-then-comm (best-effort), join() blocks on the keyboard listener (which owns the exit key) when present, otherwise the first attached.

KeyboardListener / MouseListener drop the comm kwarg; the underlying op fetches the shared comm via the manager. control.py collapses start_threads/join_threads/stop_threads onto manager calls and grows a mutually-exclusive --ch9329 / --ch9350 protocol flag (default CH9329; --ch9329 is imperative-only) plus --ch9350-state in {0, 2, 3, 4}, defaulting to 2 (legacy-BIOS / UEFI CSM compatible).

Tests:
  - new tests/backend/test_manager.py (12 tests) covers init guard, get/reset, attach, start/stop ordering, exception-safe stop, and join's keyboard-preference fallback.
  - tests/backend/implementations/conftest.py autouse-installs a manager wrapping a MagicMock comm for every op test in the subtree.
  - root conftest pre-mocks usb/usb.core (with USBError as a real Exception class) and pre-imports kvm_serial.backend.keyboard + .implementations.pyusbop, anchoring them in sys.modules before later patch.dict("sys.modules", ...) blocks can evict them and leave parent-attribute paths resolving to stale modules.
  - test_baseop / test_mouse / test_control updated for the new signatures.
Add a Protocol submenu under Options: CH9329 (default) plus CH9350L state 0/1 paired, state 2 BIOS, state 3 abs mouse, state 4 Digitizers.
Selecting an item updates protocol_var / ch9350_state_var and re-runs __init_serial; the choice persists in .kvm_settings.ini.

__init_serial now stops any active DataCommManager before reopening the serial port, constructs a fresh manager bound to _build_comm_cls() (CH9329Comm or a state-bound CH9350Comm lambda), starts it (so the CH9350 state-0 handshake / state-2/3/4 LED-echo rx threads spin up), and instantiates QtOp / MouseOp which fetch the shared comm via DataCommManager.get(). closeEvent stops the manager so background threads exit cleanly.

5 new GUI tests cover the protocol-default load path, restoring ch9350 + state, invalid-state fallback, and _build_comm_cls returning the correct callable for each protocol. _setup_mock_menus now mocks protocol_menu too; the existing save-settings test was updated for the two new keys.
Co-authored-by: Copilot <copilot@github.com>
…dant copy in repo (towards #6)

- Introduced `generate_docs_index.py` to create the documentation homepage from README.
- Updated `.gitignore` to exclude generated `docs/index.md`.
- Modified `mkdocs.yml` to include a new navigation item for supported devices.
- Removed old `docs/index.md` file as it will now be generated dynamically.
Add project root to sys.path for local imports when kvm.py or control.py are run as scripts (__name__ == "__main__")

Co-authored-by: Copilot <copilot@github.com>
…ds 13)

Co-authored-by: Copilot <copilot@github.com>
)

The UC silently drops LEN=0x0A absolute mouse frames in state 0/1
despite valid descriptor handshakes. Verified with PoC-generated frames
under multiple descriptor variants and with a real CH9329 chip as the
LC's USB-host source. State 3/4 (UC-owned descriptors, cmd 0x04 frames)
is the only working absolute-mouse path on this hardware.

- §Mouse LEN=0x0A: warning callout pointing to §Divergences
- §State Machine: capability note on relative-only forwarding
- §Divergences: full evidence and interpretation
Reverts the main body of 0c501ed. State-0/1 absolute mouse on the
CH9350L UC is silently dropped by the chip's frame-forwarder
regardless of the announced descriptor — proven empirically with
PoC frames and a real CH9329 chip as the LC USB-host source. See
the documentation update in 3307d4f for the full evidence.

Removed:
  - COMPOSITE_MOUSE_DESC constant
  - MOU_ABS_SER, MOU_ABS_RID class constants
  - _send_state01_absolute_frame method
  - control.py / kvm.py wiring that passed the composite descriptor

Preserved from 0c501ed: the descriptor-section structural comments in
DEFAULT_KBD_DESC (Keyboard / System Control / Consumer Control) and
DEFAULT_MOUSE_DESC. Those document what each chunk of the hex blob
corresponds to and are useful independently of the absolute-mouse
question they were authored alongside.
sjmf added 9 commits May 10, 2026 13:57
…towards #13)

Two refinements to send_mouse_absolute behaviour in states 0/1/2 (the
relative-only modes per docs/CH9350L_PROTO.md §Divergences):

1. Fan out large deltas across multiple consecutive frames instead of
   clamping to ±127 in a single frame and losing displacement. A
   teleport from off-window or a focus event with no preceding move
   would otherwise leave the cursor short of the target by up to one
   axis-clamp per event. Equivalent in spirit to the on-chip
   pen-to-mouse conversion a Wacom-style device does in firmware --
   diff successive absolute positions, emit relative deltas, fan out
   when needed. Frame count = ceil(max(|dx|, |dy|) / 127); wheel rides
   only the first chunk so a single scroll request isn't multiplied.

2. Add a CH9350Comm.supports_absolute_mouse property surfacing whether
   this comm forwards absolute reports natively (states 3/4) or
   translates to relative deltas (states 0/1/2). Lets application
   code surface mode info or pick UX accordingly without duplicating
   the state-to-capability mapping.

Tests cover both: _split_relative_delta unit tests for chunk bounds and
sum-preservation across positive/negative axes; integration tests
asserting state-0 and state-2 wire frames fan out correctly with the
right cmd byte and the wheel-on-first-chunk behaviour; property tests
for each supported state and the internal state-0 -> state-1 promotion.
- Add docs/CH9329_GUIDE.md: hardware sourcing, DIY wiring (with 5 V
  backfeed warning), physical setup, GUI/CLI usage, troubleshooting
- Add docs/CH9350L_GUIDE.md: dipswitch configuration (SEL/S0/S1/BAUD),
  working state selection guide, RS-485 vs TTL UART section (with
  voltage damage warning), physical setup, GUI/CLI usage, state 0/1
  handshake explanation, troubleshooting
- Add docs/img/ch9350l-front.jpg and ch9350l-back.jpg (from issue #13)
- Add docs/img/mini-uart.jpg (CH9329 DIY module photo)
- Update README: signpost both guides from Kit List and Script Usage;
  remove --video/-x (removed feature); trim prose now covered by guides
State 3/4 absolute mouse was scaling source pixel coords to 0..0xFFFF
(16-bit) on the assumption that the chip's built-in absolute descriptor
declared a full 16-bit logical range. Empirically, the chip declares
0..4095 (12-bit) -- matching the CH9329's identical wire format -- and
sending values up to 65535 produced classic mod-4096 wrap behaviour on
the target host: a tiny on-host motion teleported the cursor wildly,
crossing the screen and back as values crossed 4096 boundaries.

Confirmed by comparison against the working CH9329 path
(kvm_serial/utils/ch9329.py:96), which scales to 0..4095 and works
correctly with the same wire frame format.

Centralised as a class constant CH9350Comm._ABS_MAX = 0x0FFF; both the
absolute-emit path (send_mouse_absolute) and the relative-re-emit path
in state 3/4 (send_mouse_relative, which carries position via the
absolute frame) now use it. The 16-bit LE wire field is unchanged --
upper 4 bits are unused / Const padding per the chip's descriptor.

Tests updated to reflect the new wire bytes.
When tracking a relative-mode magnitude issue ("crossing 0.25 of the
view window crosses the whole screen of the remote") it's useful to
see exactly what scene coordinates the comm layer is receiving, what
camera resolution is in scope, and what dx/dy the abs-to-rel
translation produces per call.

Logs at DEBUG level only -- silent by default; enabled via
logging.getLogger("kvm_serial.utils.ch9350").setLevel(logging.DEBUG)
or the application's existing log-level controls.
Previous fix (c27f82d) assumed the chip's built-in absolute mouse
descriptor declared a 12-bit range (0..4095), matching the CH9329's.
Empirical re-test on the same hardware shows that's not right: sending
4095 lands the cursor at ~25% across a 1920x1080 target, implying the
chip declares ~16383 (14-bit) and we're still 4x short.

Bumping _ABS_MAX from 0x0FFF to 0x3FFF. The X/Y wire fields are 16-bit
LE regardless; upper 2 bits remain unused / Const padding per the
chip's descriptor.

Also adds a debug log line in the state-3/4 native-absolute path
mirroring the existing one in state-0/1/2: shows scene xy, camera wh,
and the scaled chip coords with the active _ABS_MAX. Useful for any
follow-up calibration if a different board declares a different range.
Empirical sweep across a 1920x1080 target reveals a periodic
clip-sweep-clip-sweep pattern with period 2048 in nx: cursor sweeps
fully on nx 0..1023, clamps to the left edge for nx 1024..2047, sweeps
again on nx 2048..3071, and so on. That's the signature of an 11-bit
signed field in the chip's built-in HID descriptor (LogicalMin=-1024,
LogicalMax=+1023, RepSize=11) with bit 10 as the sign bit and bits 11+
silently ignored.

Effective usable range from the LC is 0..1023, not the previous
guesses of 0..0xFFFF (16-bit) or 0..0x3FFF (14-bit). Setting
_ABS_MAX = 0x03FF and updating tests to match.

This is empirically calibrated against one CH9350L board and may
differ between revisions; if a future board declares a different range
the constant is now isolated and easy to override. The state-3/4
debug log added previously gives the data needed for any such
re-calibration.
The 0x04 frame's X/Y fields are 16-bit on the wire but the chip's
USB-side descriptor declares an 11-bit signed range. Effective usable
range from the LC is 0..1023 (10-bit), not 0..0xFFFF or even the
CH9329's 0..4095 -- the matching wire format does not imply a matching
declared range.

Two updates:
  - §States 2/3/4: extends the "0x04 format verified" bullet with the
    range note and the periodic clip-sweep diagnostic signature.
  - §Divergences: full entry recording the symptom, the prior
    assumption, the observation, and the cross-chip difference.
States 0/1 and 2 ship relative deltas, so the cursor on the target
inherits the OS's pointer-acceleration curve: slow drags track the
source faithfully, fast drags overshoot. This is a property of
relative-mouse-over-anything, not a kvm-serial bug -- but worth
flagging in the user guide so the next user noticing the asymmetry
doesn't go hunting for it in the code path the way we did.

State-selection bullets: extends the state 2 / 0/1 entries with a
one-line note on the acceleration inheritance, and the state 3 entry
with a corresponding "no acceleration" callout (since reports carry
positions, not deltas).

Troubleshooting: new entry "Cursor lags behind on fast drags" with
mitigations -- switch to state 3, or disable acceleration on Windows /
macOS / Linux.
A drag is mouse-down -> on_move(s) while held -> mouse-up. MouseOp.on_move
was always sending buttons=0, so the very first move event during a drag
released the held button on the target -- drag failed on first motion in
every state (0/1, 2, 3/4 alike).

MouseOp now tracks held buttons in a bitmask:
  - on_click sets the bit on press, clears it on release; bits are
    independent so e.g. holding LEFT while pressing RIGHT correctly
    yields 0x03 and releasing LEFT alone leaves 0x02 held.
  - on_move and on_scroll pass the current bitmask through so drags
    and (uncommon) button-held scroll preserve target-side button state.

Tests cover the regression (drag preserves the bit through multiple
on_move events) and the multi-button-held bitmask path.
@sjmf sjmf linked an issue May 10, 2026 that may be closed by this pull request
@sjmf sjmf self-assigned this May 10, 2026
@codecov

codecov Bot commented May 10, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 89.15009% with 60 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.60%. Comparing base (5a6e37b) to head (dd0b53a).

Files with missing lines Patch % Lines
kvm_serial/kvm.py 67.85% 27 Missing ⚠️
kvm_serial/utils/ch9350.py 94.33% 18 Missing ⚠️
kvm_serial/control.py 56.66% 13 Missing ⚠️
kvm_serial/backend/manager.py 96.36% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #40      +/-   ##
==========================================
+ Coverage   81.37%   83.60%   +2.22%     
==========================================
  Files          19       22       +3     
  Lines        1745     2220     +475     
  Branches      222      299      +77     
==========================================
+ Hits         1420     1856     +436     
- Misses        325      364      +39     
Flag Coverage Δ
unittests 83.60% <89.15%> (+2.22%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sjmf

sjmf commented May 10, 2026

Copy link
Copy Markdown
Owner Author

Code Review

Overview

This is a substantial feature PR that adds full CH9350L support. It introduces a new CH9350Comm class covering all four working states, extracts CH9329Comm into its own file, makes DataComm properly abstract, introduces a DataCommManager singleton for lifecycle management, wires protocol selection into both the CLI and GUI, and adds comprehensive documentation. 309 tests cover the new code. The quality is high throughout.


Architecture

  • The ABC refactor on DataComm is clean and the interface is well-defined. start()/stop() as optional no-op hooks is the right pattern.
  • DataCommManager as a singleton works well for the CLI/GUI use case. The reset() test hook with best-effort stop() is thoughtful.
  • The _build_comm_cls() factory pattern (both in control.py and kvm.py) keeps protocol selection decoupled from instantiation cleanly.
  • Moving CH9329-specific code to ch9329.py was overdue and the split is clean.

Correctness Issues

Minor — docstring/reality mismatch in communication.py:

The send_mouse_absolute abstract docstring says:

CH9350 state 3/4: 0..0xFFFF

But ch9350.py has _ABS_MAX = 0x03FF (1023). The extensive comment in ch9350.py explaining the 10-bit empirical calibration is correct; the ABC docstring just never got updated. Should be 0..0x3FF.

Minor — wheel not clamped in state 0/1 mouse frame:

In _build_state01_mou_frame:

data = bytes([self.MOU_RID, btn & 0xFF, dx_b, dy_b, wheel & 0xFF])

dx_b and dy_b are clamped to ±127 before the & 0xFF, but wheel is not. A large wheel delta would silently wrap rather than clamp. The state-2 path in _send_relative_frame has the same pattern. In practice MouseOp passes small values, but it's an inconsistency with the documented −127..+127 range for signed fields.


Code Quality

  • comm_manager: object | None = None in kvm.py is typed as object rather than DataCommManager | None. The # type: ignore[attr-defined] comments on call sites confirm the pain — a TYPE_CHECKING import of DataCommManager would clean this up without creating a circular import at runtime.
  • The _PROTOCOL_OPTIONS constant has ("CH9329", "ch9329", 0) with state=0 for CH9329 — the zero is ignored in _protocol_label, but it could be confused with CH9350L state 0. A sentinel value (e.g. -1) or just not using the tuple slot for CH9329 would be clearer.
  • The _run_attach_sequence hardcoded sleeps (0.25, 0.23, 1.0) are empirically derived and the protocol doc explains why, but the in-code comment is sparse. A single inline note referencing the proto doc section would help future readers.

Threading

  • self.state is written in _handle_frame (rx thread) and read in _build_state01_kbd_frame / send_scancode (caller thread). CPython's GIL makes this safe in practice, but under an alternative Python implementation this would be a race. A threading.Lock around state transitions would be belt-and-suspenders. Not blocking.
  • _send_locked correctly serialises all writes via _tx_lock. The time.sleep(0.05) calls in _announce_descriptors happen between lock acquisitions rather than inside them — that's the right approach.
  • _rx_loop breaks on any serial exception rather than trying to distinguish shutdown-related errors from transient ones. This is intentional and the reattach logic handles reconnect via the tx thread, so it's fine.

UX / Defaults

  • CLI --ch9350-state defaults to 2 (relative mouse, BIOS-compatible), and the GUI ch9350_state_var also defaults to 2. The guide clearly recommends state 3 for desktop use. This is a conservative choice — state 2 works in more environments — but it does mean most desktop users will immediately need to change it. Worth considering whether state 3 as default (with a note about BIOS fallback) would reduce friction.
  • _on_protocol_selected calls __init_serial() immediately on every menu click, consistent with how baud rate changes work today — fine, but worth noting that an accidental click triggers a full reconnect/handshake.

Documentation

The three new doc files are thorough and accurate. Specific highlights:

  • The §Divergences section in CH9350L_PROTO.md documenting where the WCH datasheet diverges from empirical behaviour is exactly the right thing to preserve; this would be very hard to reconstruct later.
  • The RS-485 section and 5V power warning in the guides are safety-critical and well-placed.
  • generate_docs_index.py is simple and correct.

One nit: CH9329_GUIDE.md has arrows in a wiring diagram inside a fenced code block. The project convention is to avoid Unicode glyphs in source files — these should use -> for consistency (no rendering impact inside a code block, but consistent with the convention).


Test Coverage

309 tests is solid. Coverage of descriptor announce, state transitions, LED echo, multi-frame fan-out for large deltas, and the drag-preserves-held-button regression is exactly what you'd want for a state-machine protocol implementation. The conftest.py scoping of the DataCommManager mock to the implementations/ subtree is well-explained with a clear rationale comment.


Summary

This is a well-executed PR. The two actionable correctness items are the 0xFFFF docstring (trivial fix) and the wheel clamping gap in state 0/1 mouse frames (small fix). Everything else is minor. The protocol documentation alone justifies the PR — it captures empirical findings that would otherwise live only in commit messages. Ready to merge after addressing the docstring inconsistency and optionally the wheel clamping.


Review generated by Claude Sonnet 4.6

sjmf and others added 6 commits May 10, 2026 19:51
_ABS_MAX is 0x03FF (10-bit, 0..1023); the docstring said 0xFFFF.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
_build_state01_mou_frame clamped dx/dy but applied only & 0xFF to
wheel, which would silently wrap large values rather than clamp them.
Consistent with the signed-byte encoding used everywhere else.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…#13)

Avoids the circular runtime import while giving type checkers (and
readers) the correct type, removing the need for # type: ignore on
the stop() call.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
state=0 was ambiguous with CH9350L state 0. Also guard the
ch9350_state_var assignment in _on_protocol_selected so selecting
CH9329 does not clobber the remembered CH9350L working state.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…r paths (towards #13)

Covers _parse_frames partial-buffer and unknown-cmd branches, start/stop
lifecycle (state-0 tx thread spawn, double-start guard), heartbeat,
send_announce, announce_descriptors, maybe_retransmit_descriptors, and
run_attach_sequence. Also covers the swallowed-exception paths in
DataCommManager.reset() and stop(). ch9350.py: 73% -> 92%.
manager.py: 93% -> 100%.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@sjmf sjmf added the enhancement New feature or request label May 10, 2026
@sjmf sjmf added this to the 1.5.6 milestone May 10, 2026
@sjmf

sjmf commented May 10, 2026

Copy link
Copy Markdown
Owner Author

Minor cleanup done. Ready to merge.

@sjmf sjmf merged commit 7130cc0 into main May 10, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support for CH9350L module

1 participant