Conversation
… channels PR AprilNEA#522 made input capture reuse an inventory-owned HID++ channel instead of opening its own, but the channel's single leased software id still assumed one exclusive consumer. Capture's session setup (device version probe, root getFeature calls for reprog controls/thumbwheel/wireless status) and inventory's own probing then correlate requests on the exact same (device, feature, function, software id) key on a shared channel, so one queues behind the other's in-flight request — occasionally past either side's own timeout. Each side reads that as the other's channel having died: inventory retires the channel it judges dead, and capture's session ends unexpectedly and re-arms, matching AprilNEA#1128's reports. Leases a second software id per shared channel (best-effort; falls back to the single-id behavior when the pool is nearly exhausted) and gives capture's `Device`/`RootFeature` construction a `_secondary` constructor that stamps requests with it, so its correlation keys no longer collide with inventory's own probe traffic. Fixes AprilNEA#1128
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…y software id The secondary-software-id fix for AprilNEA#1128 covered capture's session-setup calls (device version probe, root getFeature for reprog controls/thumbwheel/wireless status) but left arm_controls_into's CID-level ReprogControlsV4/Thumbwheel traffic — and its capture_restore rollback counterpart — on the primary id, so that traffic could still collide with inventory's own reprog-controls probing on a shared channel. Adds `new_secondary` constructors for both wrapper types (delegating to the underlying hidpp features' own `new_secondary`, now generated by the `#[derive(Feature)]` macro for every creatable feature) and switches arm_controls_into/capture_restore to them. WirelessDeviceStatusFeature gets a `new_secondary` too, identical to `new`, since it only listens for broadcasts and has no outgoing correlation key to stamp.
…ects new_secondary's doc comment linked to FeatureEndpoint::new_secondary, which is pub(crate) — resolvable only under --document-private-items, so the public rustdoc job CI runs would have failed on it.
2 tasks
15 tasks
|
Hi! Did some tophat here in my machine, the problem is gone, everything seems to be working fine! Debug output: |
Contributor
Author
|
Thanks for testing on real hardware — great confirmation. Really appreciate you verifying the actual fix instead of just your own patch. |
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
correlation-key collision: both consumers share the channel's single leased
software id, so capture's session-setup calls (device version probe, root
getFeaturefor reprog controls/thumbwheel/wireless status) andinventory's own probing correlate on the exact same
(device, feature, function, software_id)key. One queues behind the other's in-flightrequest on the wire, occasionally past either side's own 5s timeout —
inventory then retires the channel it judges dead, and capture's session
ends unexpectedly and re-arms on a fresh channel, matching the reports.
the single-id behavior when the pool is nearly exhausted) and gives a
second, distinct in-process consumer a
_secondaryconstructor that stampsits requests with it, so its correlation keys stop colliding with the
channel's other consumer.
Changes
crates/openlogi-hidpp:SwIdPolicy::Leasedcarries an optional secondaryleased id;
HidppChannel::get_secondary_sw_idreads it (falling back tothe primary id when absent).
FeatureEndpoint/RootFeature/Deviceeachgain a
new_secondary/new_inner-backed constructor that stamps requestswith the secondary id.
protocol::determine_version_secondarymirrorsdetermine_versionfor the version-probe request the secondaryDeviceconstructor sends.
crates/openlogi-hid:configure_channel_sw_idsleases a second idalongside the existing primary one for every opened channel.
crates/openlogi-device:session/gesture.rs's capture session setup nowconstructs its
Device/RootFeaturevia the_secondaryconstructors,since its channel may be inventory-owned per fix(agent): reuse inventory channels for input capture #522.
#[derive(Feature)]macro (openlogi-hidpp-derive) now generates anew_secondaryfor every creatable feature, not justRoot/Device.ReprogControlsV4/Thumbwheel(crates/openlogi-device) gain their ownnew_secondarydelegating to it, andarm_controls_into's CID-leveldivert/read traffic plus its
capture_restorerollback counterpart now usethem — closing the gap noted below.
WirelessDeviceStatusFeature(hand-written,not derive-based) gets a
new_secondarytoo, identical tonew, since itonly listens for broadcasts and has no outgoing correlation key to stamp.
Testing
cargo test -p openlogi-hidpp --lib channel::— includes a new test,a_secondary_leased_sw_id_does_not_queue_behind_the_primary_ids_in_flight_request,that reproduces the collision with a parked-write mock transport (a request
under the primary id stays in flight; a same-header request under the
primary id would queue behind it, per the pre-existing
a_request_waits_while_the_same_header_is_in_flighttest) and proves arequest stamped with the secondary id reaches the wire immediately instead.
Verified this test fails without the fix (secondary id falling back to the
primary one) and passes with it.
cargo fmt --all -- --checkRUSTFLAGS="-D warnings" cargo clippy --workspace --all-targets -- -D warningsRUSTFLAGS="-D warnings" cargo test --workspaceRUSTDOCFLAGS="-D warnings" cargo doc --workspace --no-deps --document-private-items --exclude openlogi-ui --exclude openlogi-desktop --exclude openlogi-overlay --exclude openlogi-agentopenlogi-hidpp-derive,openlogi-hidpp,openlogi-device,openlogi-agent,openlogi-cli,openlogi,openlogi-hid,openlogi-agent-core,openlogi-desktop) —cargo fmt --all -- --check,cargo clippy -p <those> --all-targets -- -D warnings,cargo test -p <those>; full-workspace clippy/rustdoc covered bythe mandatory pre-push hook.
timing this workstation cannot reproduce; the fix and test target the
correlation-key mechanics directly, per the codebase's existing
a_request_waits_while_the_same_header_is_in_flightprecedent for the sameclass of bug (that one found cross-matched replies; [Bug]: Version 0.6.24 and newer: button mapping not working #1128's leased-id fix
turned that into contention/timeouts instead, which is what this closes).
Known remaining gap (fixed)
arm_controls_intoinsession/gesture.rsused to constructReprogControlsV4/ThumbwheelFeaturevia their standard (primary-id)CreatableFeature::new, so their CID-level divert/read traffic could stillcollide with inventory's own reprog-controls probing on a shared channel. A
follow-up commit closes this: both wrappers gained their own
new_secondary,arm_controls_intoand itscapture_restorerollbackcounterpart now use them, and
WirelessDeviceStatusFeaturegained anew_secondarytoo (see Changes above).Fixes #1128