refactor(hid): split the HID++ device layer from the host backend - #772
Merged
Conversation
`async_hid::HidError` was part of this crate's public API — in `InventoryError`, `GestureError`, `HostSwitchError` and `ChannelPool::open` — which put "openlogi-hid uses async-hid" into its contract rather than its implementation. `openlogi-agent-core` had picked up a dev-dependency on async-hid purely to construct one of those errors in a test. Introduce `backend::BackendError`, a two-variant backend-agnostic error, and convert at the one place that owns the backend (`channel::transport`). Nothing above the transport names `async_hid` any more. Being local to this crate, `BackendError` also clears the orphan rule that forced `classify_hid_error` on `WriteError` and `PairingError`: both are now real `From` impls, so nine `.map_err(|e| classify_hid_error(&e))?` call sites collapse to plain `?`. No wire change: the three error enums touched are not `Serialize`, and `WriteError`/`PairingError` gained a conversion, not a variant.
`async_hid::DeviceId` and `DeviceInfo` were the crate's node-identity and
node-metadata types all the way up: cache and ledger keys, the registry, the
probe's per-node record, the standalone descriptor. Every one of those layers
is pure HID++ policy that has no business naming a HID stack.
Introduce `backend::{NodeId, NodeInfo}` as the value types those layers hold,
converted once in `channel::transport`. `NodeInfo` carries exactly the fields
anything above the transport reads — the intersection any HID backend can
supply. `device_identity` becomes `NodeInfo::identity`, where it belongs.
`NodeId` renders the backend's opaque id verbatim, so the identity string
raw-HID routes are addressed by is byte-identical to before.
Also moves `HotplugEvent` to `backend` and the async-hid watch bridge into
`channel::transport`, which lets `hid_backend()` stop being crate-visible.
`async_hid` now appears outside `channel::transport` in exactly two places,
both of which need the open/write seam that comes next: the `Device` handle
`prepare_nodes` consumes, and the `DeviceWriter` the Litra raw-write path uses.
Enumeration and opening reached the OS through free functions over a process-wide `static`, which is why the layers above could not be tested without hardware and why `async_hid::Device` had to travel up to `prepare_nodes` to be opened there. Add `backend::HidBackend`: enumerate, enumerate_hidpp, open_hidpp, open_raw_writer, watch. `NativeBackend` implements it over async-hid and is now the only place that names that crate. Opening is defined only for a node a previous enumerate returned, so the backend keeps that enumeration's OS handles instead of re-finding a node it cannot rebuild from a `NodeId`. Route policy moves off the backend and onto the trait: `open_route_channel` and `open_route_writer` (relocated out of `channel::transport`, where it never belonged) both take `&dyn HidBackend`. The Litra raw-write path goes through `backend::RawWriter` rather than an `async_hid::DeviceWriter`. The public free functions keep their signatures by passing `native_backend()`; injecting a backend into `Enumerator` and `ChannelPool` — the part that buys testability — is the next step. `async_hid` now appears outside `channel::transport` in zero places.
`Enumerator` and `ChannelPool` reached the host through a process-wide accessor, so neither could be pointed anywhere else — which is why the inventory tick, the largest piece of logic in this crate, had no test that ran it end to end. Both now hold an `Arc<dyn HidBackend>`; `Default` still resolves to the host's, so no caller changes. `ScriptedBackend` joins the existing scripted transport in the test-only module, and two tests pin behaviour that previously needed hardware to observe: a node that will not open settles as an unhealthy probe (so the ledger replays its last-good snapshot), while a node that opens but is not HID++ leaves the tick healthy. Conflating those two would make every host with an unrelated HID device retry forever. Verified the first test fails when the open-failure settle loop is short- circuited, so it is pinning behaviour and not the implementation's shape.
The enumerator held a `PathBuf` and called `std::fs` directly, so the one piece of state worth keeping across restarts was welded to a filesystem — a host detail sitting in the middle of protocol logic. Split it the way the HID stack is split: `persist` now owns only the persistable *form* (`ProbeCacheSnapshot`, the schema version, the Bolt-only rule) and the `ProbeCacheStore` port; `persist::file` owns the JSON-on-disk implementation. `Enumerator` holds an `Option<Arc<dyn ProbeCacheStore>>` and `persisted()` keeps meaning what it meant. The schema-version check moves into `into_entries`, next to the schema it guards, so a store only has to hand back bytes it managed to read.
Portability was an intention with nothing enforcing it. Now a compiler does: `wasm32-unknown-unknown` has no OS beneath it, so a crate that grows a filesystem, a randomness source or a thread stops building there and nowhere else. Added as a real CI job plus a `cargo xtask ci wasm` lane, with the crate list in xtask and the ci.yml drift test holding the two in step. Two things had to go for `openlogi-hidpp` to pass, both improvements on their own terms: - Message-listener handles were random `u32`s with a collision retry loop. They are keys into a per-channel map, so a counter is collision-free by construction — and `rand` leaves the crate entirely. - `EventEmitter::emit` used `send_blocking` on channels that are all unbounded, where it can only ever mean `try_send` with a deadlock hazard attached (it runs holding the sender lock). The list is one crate. `openlogi-core` cannot join it yet: its config half writes through `atomic-write-file`, which pulls `getrandom`. That is the next split, and the comment on `WASM_PORTABLE_CRATES` says so. Note what a check does not prove: `HidppChannel` still spawns an OS thread for its read loop, which compiles for wasm and would fail at runtime. Making that loop a task is separate work.
`openlogi-core` is described as data — the device model, the action catalogue, the binding types — but it also read and wrote the user's config, which dragged in `atomic-write-file` → `getrandom` and made the whole crate unbuildable anywhere without a filesystem. Gate that half: `config::file`, `paths` and `single_instance` are now behind `fs`, on by default so every consumer here is unaffected. What is left needs no host at all, and the wasm CI job checks that with a `--no-default-features` pass rather than trusting the crate description. Two fields stay in every build with a cfg'd `dead_code` allow rather than being gated out: `Config::ephemeral` and `DeviceConfig::gesture_owner` are read only by the load/save half, but they are part of the shape serde sees, and a struct whose fields depend on a feature is a struct with two shapes.
Every open HID++ channel spawned an OS thread and blocked it on a private executor. That costs a thread per device, hard-codes an executor choice into a protocol crate, and cannot exist at all on a single-threaded target. `from_raw_channel` now returns the channel *and* its reader future; the caller spawns it. `openlogi-hid` does that with `tokio::spawn` at both open sites, so the reader runs on the runtime that already exists rather than one of its own. The behavioural change worth knowing: `Drop` used to signal the reader and then join the thread, so the OS handle was closed by the time drop returned. It now signals and returns, and the handle closes when the task winds down — which is the reader's next poll, since it is parked in a `select!` on that very signal. The inventory's reopen path is unaffected either way: a retired node is deferred to a later tick (~2 s) *and* until its channel's last `Arc` is gone, neither of which was ever the thread join. Tests keep a thread, on purpose: they run on `futures::executor::block_on`, which polls one future, so `channel_with_reader` gives the reader an executor without pulling a runtime into this crate's dev-dependencies. That the library no longer imposes one is the point. Not runtime-tested on hardware — the close-timing change is the part that would want a real device.
The seam existed as a module inside the crate that implements it, so a second backend — WebHID under wasm, a scripted tree — would have had to depend on `openlogi-hid`, `async-hid` and every host binding to obtain the trait it means to implement. It is now its own crate, and CI's wasm job holds its manifest to having no host in it at all. Splitting it moved the orphan rule, which is the part worth reading twice. `impl From<BackendError> for WriteError` had been legal in `openlogi-hid` because `BackendError` was local there; now that it is not, the impls move to `openlogi-device`, which is where the layering wanted them anyway — the crate that owns the error owns its conversions, and `openlogi-core` still depends on no backend. That is why `openlogi-device` takes `openlogi-core` (with `fs` off): the first edge of the graph the rest of the layer will move along. Going the other way, `async_hid::HidError -> BackendError` and `&DeviceInfo -> NodeInfo` are now foreign-to-foreign in `openlogi-hid`, so they become named functions again. That is the rule agreeing with the design: an adapter belongs to the backend it adapts, not to the contract. `openlogi-hid` re-exports the contract as `backend`, so every existing path still resolves and no consumer changed. Probed the gate by pointing the new crate at `openlogi-core` with default features: it fails on `getrandom`, which is the regression class it is for.
Every route-addressed write reached for the process-wide native backend from inside, so none of them could be driven by anything else — the last thing pinning that layer to this host. They now take `&dyn HidBackend`, and `host` holds the twenty-one one-line wrappers that supply this machine's. Consumers are untouched: the crate root re-exports the route-addressed half from `host` and the channel-addressed `_on` half straight from `write`, which is the same split the `_on` suffix has been drawing by hand all along — one needs a backend to find a device, the other is handed one already open. `with_route`, the twelve-line function that was the write layer's entire contact with the OS, is now the only place a backend is threaded through.
The enumerator, the channel pool, pairing, the capture sessions, standalone discovery and hotplug each reached for the process-wide native backend from inside. None of them could be pointed at anything else, and none of them could move out of a crate that knows what a host is. They all take a backend now. `Enumerator` and `ChannelPool` lose the `Default` impls that were doing the reaching — a default that resolves to *this machine* is not a default, it is a decision — and `Enumerator::persisted`, which picked a filesystem path, moves to `host` beside the other host wiring. Six call sites changed, all in the agent, and each one now names its backend where it starts the work rather than inheriting it invisibly. Deleted `run_capture_session_with_registry` rather than giving it a backend too: nothing called it, it ignored the registry it was named for (`_registry`), and its own doc said the reuse it promised was still TBD.
Enumeration, probing, the write layer, capture sessions and pairing knew HID++ and nothing about a host, but lived in the crate that owns `async-hid`. They move to `openlogi-device`, which the wasm job now builds in full — so "portable" is a fact about that crate rather than a description of it. `openlogi-hid` is what is left: the `async-hid` transport, the Windows composite channel, macOS Input Monitoring, the file-backed probe cache, and `host`. It re-exports the device layer verbatim, so every consumer path still resolves and no consumer changed. Two things moved to where they belonged rather than where they were: - `channel::transport` was a submodule of the channel layer it is beneath. It is now `openlogi-hid`'s `transport`, and `openlogi-device::channel` names no HID stack at all. - The probe-cache round-trip test was checking `ProbeCacheSnapshot::of` and `into_entries` — schema, Bolt-only keys, the volatile battery reading — through a file, as if the file were the subject. Those assertions move to `persist`'s own tests; what stays in `openlogi-hid` is what the file store actually owns: a missing or torn file is a cold start, and saving creates the data dir on a first run. `ProbeCacheError::new` and `ProbeCacheSnapshot::is_empty` are new: a store implemented outside the crate that defines them needs both, which is the crate boundary doing its job.
Greptile SummaryThe PR separates the portable HID++ device layer from the host transport and adds a wasm portability gate.
Confidence Score: 3/5The PR is not yet safe to merge because the new wasm job still executes mutable third-party action references. The reader-lifetime defect has been fixed by joining the owned reader thread during channel destruction, but the wasm CI job continues to resolve third-party action tags dynamically, leaving its executed code vulnerable to upstream tag movement. Files Needing Attention: .github/workflows/ci.yml
|
| Filename | Overview |
|---|---|
| crates/openlogi-hidpp/src/channel.rs | Keeps the HID++ read loop on an owned thread and joins it during channel destruction, addressing the previously reported reopen overlap. |
| crates/openlogi-hid/src/transport.rs | Implements the host-specific async-hid backend beneath the new backend-neutral device layer. |
| crates/openlogi-device/src/backend.rs | Defines the backend-neutral node, writer, hotplug, and transport abstractions used by portable device logic. |
| .github/workflows/ci.yml | Adds the wasm portability job, but its third-party action references remain mutable as previously reported. |
| xtask/src/commands/ci/jobs.rs | Registers the wasm portability gate in the local CI runner and its job metadata. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
Agent[openlogi-agent-core] --> Host[openlogi-hid host API]
Host --> Device[openlogi-device]
Host --> Transport[async-hid transport]
Device --> Protocol[openlogi-hidpp]
Transport --> Hardware[Host HID devices]
Protocol --> Hardware
Reviews (2): Last reviewed commit: "refactor(hidpp): keep the read loop on a..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| configure_channel_sw_ids(&mut c); | ||
| // The channel is inert until its reader runs; dropping the | ||
| // channel is what ends the task. | ||
| tokio::spawn(reader); |
There was a problem hiding this comment.
When inventory evicts an unhealthy channel, the detached reader still owns the raw HID handle after HidppChannel::drop returns, while prepare_nodes immediately opens the same node again. This can direct replies to the retired handle and leave the replacement channel timing out or repeatedly cycling through eviction.
Knowledge Base Used:
- openlogi-hid: device inventory, pairing, and HID++ feature control
- openlogi-hidpp: HID++ protocol implementation
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v6 | ||
| - uses: dtolnay/rust-toolchain@stable |
There was a problem hiding this comment.
The new wasm job executes dtolnay/rust-toolchain@stable and, at line 254, Swatinem/rust-cache@v2 through mutable tags. Pin both third-party actions to reviewed commit SHAs so upstream tag movement cannot silently change the code executed by this CI gate.
How this was verified: Both changed workflow steps resolve third-party action tags at job startup rather than immutable commit SHAs.
Rule Used: Flag third-party action tag pinning as higher prio... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
`ci_yml_runs_what_this_runner_runs` built each job's plan and compared its argv, but the wasm job skips itself on a machine without the wasm32 std — so on CI's test runners, which install no such target, it planned no steps and the comparison panicked. It passed locally only because devenv installs the target. My-machine-green, which is the same trap as macOS-green. The wasm job joins the four jobs already excluded for choosing their invocation from what the machine has. What replaces the comparison is narrower and stronger: `wasm_checks_the_crates_ci_checks` asserts every crate declared portable appears in the workflow's command, so a crate added to the list but not to `ci.yml` fails a test rather than silently going unchecked. Verified by removing `openlogi-device` from the workflow command: the new test fails, naming the crate.
Reverts the reader-as-a-future half of "hand the read loop back as a future". The `rand` removal and the `try_send` fix from that commit stay; only the threading changes back. The join was not politeness. It is what makes the OS handle closed by the time `HidppChannel::drop` returns, and the review found the case my own note missed: the inventory path defers a reopen by a full tick, but `with_route` and `ChannelPool` do not — a caller that drops a channel and reopens the same route can have two opens of one node alive at once. This tree has been bitten by exactly that before; `SW_ID_LEASES` exists for it, and `RECEIVER_PROBE_BUDGET`'s doc describes the macOS failure where replies reach only one of two open handles. What the change bought was a thread per channel and a step toward running on a single-threaded target. Neither is worth an unverifiable regression on the path that ships, and neither is needed by the split this PR is about. It belongs in its own change, where it can be tried against real hardware. The wasm job's caveat goes back to naming `std::thread::spawn`, which is once again the concrete example of something that compiles for wasm and fails when run.
davidbudnick
approved these changes
Aug 21, 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
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
Splits the HID++ stack in two:
openlogi-deviceholds everything that knows the protocol and nothing about a host,openlogi-hidis that layer wired to this machine. A second backend — a scripted device tree in tests, WebHID under wasm — can now exist without depending onasync-hidor any platform binding.The split is enforced rather than described. A new CI job builds the portable crates for
wasm32-unknown-unknown, a target with no OS under it, so a crate that grows a filesystem, a randomness source or a thread fails there and nowhere else.No consumer changed except the agent, which now names its backend where it starts work instead of inheriting it invisibly. The CLI's calls are untouched —
openlogi_hid::set_dpi(&route, dpi)still reads the same.Changes
openlogi-device(new) — the HID++ device layer.HidBackend(enumerate / enumerate_hidpp / open_hidpp / open_raw_writer / watch) plusBackendError,NodeId,NodeInfo,HotplugEvent,RawWriter; and above them enumeration policy, the probe, the write layer, capture sessions and pairing — all handed a backend rather than reaching for one. Its manifest lists no host dependency, which is what the wasm job checks.openlogi-hid— down to nine files, all of them the host's: theasync-hidtransport (channel::transportwas a submodule of the layer it sits beneath; it is nowtransport), the Windows composite channel, macOS Input Monitoring, the file-backed probe cache, andhost, which supplies this machine's backend to the entry points that need one. Re-exports the device layer verbatim.openlogi-hidpp— message-listener handles become a counter (they are map keys; a collision retry loop was solving a problem that does not exist), which dropsrand.EventEmitter::emitusestry_sendon channels that are all unbounded, where blocking could only ever mean the same call with a deadlock hazard attached.openlogi-core— reading and writing the user config moves behind anfsfeature, on by default. Everything else is data and needs no host.xtask/ci.yml— thewasm (portable crates)job, its crate list, and the drift test that keeps the two in step.Notes for review
impl From<BackendError> for WriteErrorwas legal whileBackendErrorwas local toopenlogi-hid; it now lives inopenlogi-device, which owns the error. Going the other way,async_hid::HidError -> BackendErrorbecame foreign-to-foreign and is a named function again — the rule agreeing that an adapter belongs to the backend it adapts.with_routeandChannelPooldo not, so dropping a channel and reopening the same route could have left two opens of one node alive — the failureSW_ID_LEASESandRECEIVER_PROBE_BUDGET's doc both exist for. Removing the thread is worth doing against real hardware, in its own change.run_capture_session_with_registryis deleted rather than given a backend: nothing called it, it ignored the registry it was named for, and its doc said the reuse it promised was still TBD.check.std::thread::spawnandtokio::timeboth compile for wasm and fail when run, so this gates dependencies, not execution. The job's caveat says so.ci.ymlpanicked on CI's test runners while passing locally. It now joins the four jobs already excluded for that reason, and a narrower test asserts every crate declared portable appears in the workflow command.Testing
Full local gate on the rebased tree:
All green. Wire format unchanged — no serde variant moved, so
PROTOCOL_VERSIONis untouched.Two tests were added for behaviour that previously needed hardware to observe: a node that will not open settles as an unhealthy probe so the ledger replays its last-good snapshot, while a node that opens but is not HID++ leaves the tick healthy. Verified the first fails when the open-failure settle loop is short-circuited, so it pins behaviour rather than shape.
Not runtime-tested on hardware. The reader-lifetime change above is the part that matters there: plug in a Bolt receiver and a direct BLE device, confirm both still enumerate, that DPI/SmartShift writes land, and that unplugging and replugging a receiver reopens it cleanly.
tests (linux)andclippy(ubuntu) not run locally — this host has no musl target, so Linuxcfgis CI's alone to check.