Skip to content

feat: add wired G-series keyboard RGB control#29

Open
davidbudnick wants to merge 1 commit into
AprilNEA:masterfrom
davidbudnick:feat/keyboard-support
Open

feat: add wired G-series keyboard RGB control#29
davidbudnick wants to merge 1 commit into
AprilNEA:masterfrom
davidbudnick:feat/keyboard-support

Conversation

@davidbudnick
Copy link
Copy Markdown
Contributor

@davidbudnick davidbudnick commented Jun 1, 2026

Adds wired Logitech G-series keyboard support (G513, G512, G610, G810, G815, G Pro…).

  • Detects wired G keyboards via the 0xff43 HID++ usage page (Bolt/Unifying was 0xff00-only)
  • Solid-colour per-key RGB over 0x8080, wired to the lighting panel (swatches, on/off, brightness slider)
  • All connected devices shown in the menu-bar tray, with stable ordering + refresh hysteresis so flaky scans don't reshuffle
  • CLI: openlogi diag lighting <RRGGBB>

Screenshots

Screenshot 2026-05-31 at 11 04 28 PM Screenshot 2026-05-31 at 11 04 00 PM

@davidbudnick davidbudnick force-pushed the feat/keyboard-support branch from 110612a to ec1abeb Compare June 1, 2026 04:12
Copy link
Copy Markdown

@pullfrog pullfrog Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

The on-wire path hardcodes a G513-specific feature index, so the broader "G-series" claim in the PR description is unlikely to hold on non-G513 keyboards. A few stale/misleading docstrings to clean up alongside it.

Reviewed changes — initial review of the wired G-series keyboard RGB feature: HID++ enumeration, a per-key 0x8080 write path, a new LightingPanel, a generic non-pointer device_view, refresh hysteresis, multi-row tray, and the diag lighting CLI.

  • Discover G-series HID nodesenumerate_hidpp_devices now also matches the (0xff43, 0x0602) vendor page+usage pair (crates/openlogi-hid/src/transport.rs).
  • Per-key lighting writerset_keyboard_color sends raw 0x12 "set group keys" reports + a 0x5a commit through a direct HID writer (crates/openlogi-hid/src/write.rs).
  • Typed PerKeyLightingV0 feature0x8080 wrapper added in crates/openlogi-hid/src/lighting.rs but never called.
  • diag lighting CLI — solid color command and a --spam brute-force probe over candidate 0x8080/0x8070 framings.
  • Lighting config — per-device { enabled, color, brightness } persisted in config.toml (crates/openlogi-core/src/config.rs).
  • LightingPanel + device_view — swatches, on/off pill, release-only brightness slider; keyboards (and direct-path Unknown) get lighting, other non-pointer kinds get an "isn't available yet" note (crates/openlogi-gui/src/components/...).
  • AppState::commit_lighting — saves config and pushes via hardware::set_lighting_in_background, which spawns a thread + current-thread tokio runtime per write.
  • Carousel + refresh hysteresisMISS_THRESHOLD = 3 keeps a device across a few missed scans; sort_device_list is shared by initial build and merge so order is stable (state.rs, state/devices.rs).
  • Multi-row trayset_device_statusset_device_lines with up to MAX_DEVICE_ROWS = 8 pre-allocated NSMenuItems (platform/tray.rs, platform/status_item.rs).

⚠️ "G-series" scope vs. hardcoded 0x8080 feature index

The PR description lists G512, G513, G610, G810, G815, G Pro and similar, but the on-wire writer assumes the same 0x8080 feature index (0x0c) on every model. HID++ assigns feature indices per device at enumeration time — the G513 happens to land at 0x0c, other G-series keyboards generally do not. On a mismatched keyboard set_keyboard_color will still appear to succeed (raw output reports don't return an error for an unknown feature index), but no LEDs will change, so the failure mode is silent.

The fix shape that matches what crates/openlogi-hid/src/lighting.rs already gestures at is to resolve the 0x8080 feature index dynamically through the HID++ channel before sending the raw 0x12 frames — same pattern the DPI / SmartShift paths use.

Technical details
# "G-series" scope vs. hardcoded 0x8080 feature index

## Affected sites
- `crates/openlogi-hid/src/write.rs:164-198``rep[2] = 0x0c` for both the `0x12 setGroupKeys` frames and the `0x11 commit`. No per-device feature lookup.
- `crates/openlogi-hid/src/lighting.rs:1-86``PerKeyLightingV0` wrapper exists (`CreatableFeature::ID = 0x8080`) but is unused; `set_keyboard_color` bypasses the typed channel path entirely.
- PR description in #29 — claims G512/G513/G610/G810/G815/G Pro.

## Required outcome
- `set_keyboard_color` (and `spam_lighting`) resolves the runtime feature index of `0x8080` (and `0x8070` for `spam_lighting`'s `V5`/`V6` variants) per device, instead of using `0x0c` / `0x0d` literals; OR the PR's scope claim is narrowed in the description, code comments and the CLI help text to the models the hardcoded index has actually been validated on.

## Suggested approach
- Open a `HidppChannel` via `transport::open_hidpp_channel` for the route, call the existing feature-table lookup (the same one `dump_features` uses) to resolve `0x8080`'s index, then send the raw `0x12 / 0x11` frames with that index. The typed wrapper in `lighting.rs` can host the resolved index so the raw writes and the typed `set_solid_color` / `commit` stay consistent.
- The fallback for keyboards that expose `0x8070` (`ColorLedEffects`) but not `0x8080` is a follow-up — at minimum, fail loudly (`WriteError::FeatureUnsupported`) when neither feature is present rather than silently writing to a nonexistent index.

## Open questions for the human
- Has this been tested on anything other than a G513? If so, which models, and did they reuse feature index `0x0c`?
- Is the intent to ship the hardcoded path for G513-only first and broaden later, or to claim multi-model support up front? The PR description, the module comment in `lighting.rs`, and the actual implementation currently disagree.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

Comment thread crates/openlogi-hid/src/write.rs Outdated
Comment thread crates/openlogi-hid/src/write.rs Outdated
Comment thread crates/openlogi-hid/src/write.rs Outdated
Comment thread crates/openlogi-gui/src/hardware.rs Outdated
Comment thread crates/openlogi-core/src/config.rs
Comment thread crates/openlogi-hid/src/lighting.rs Outdated
Comment thread crates/openlogi-gui/src/state/devices.rs
Copy link
Copy Markdown

@pullfrog pullfrog Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes — incremental review of ec1abeb against the previously-reviewed 110612a. The delta is cargo fmt only (line wraps in cli/lighting.rs, core/config.rs, gui/components/device_view.rs, gui/components/lighting_panel.rs, gui/platform/tray.rs, gui/state.rs, gui/state/devices.rs, hid/transport.rs, hid/write.rs) — no behavioural changes. Findings from the prior review still stand.

Pullfrog  | View workflow run | Using Claude Opus𝕏

@davidbudnick davidbudnick force-pushed the feat/keyboard-support branch from ec1abeb to d6ca00f Compare June 1, 2026 04:14
Copy link
Copy Markdown

@pullfrog pullfrog Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes — incremental review of d6ca00f against the previously-reviewed ec1abeb. Two-file delta touching gui/hardware.rs and gui/state.rs; no other code paths moved. The stale set_lighting_in_background docstring is now accurate. Other findings from the initial review (110612a) — hardcoded 0x0c feature index, misleading set_keyboard_color docstring, missing brightness clamp on deserialize — still stand.

  • Rewrote set_lighting_in_background docstring — drops the "not wired yet" claim and describes the actual HID++ write path (crates/openlogi-gui/src/hardware.rs:120-125).
  • set_lighting_in_background now borrows Lighting — signature changed from Lighting to &Lighting, eliminating the lighting.clone() previously needed at the call site.
  • Reordered commit_lighting — the background HID++ write is now dispatched before the config mutation + save_atomic, so the device sees the new colour without waiting on disk I/O on the main thread (crates/openlogi-gui/src/state.rs:362-367).

Pullfrog  | Fix it ➔View workflow run | Using Claude Opus𝕏

@davidbudnick davidbudnick force-pushed the feat/keyboard-support branch from d6ca00f to 78a87b5 Compare June 1, 2026 04:19
Copy link
Copy Markdown

@pullfrog pullfrog Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes — incremental review of 78a87b5 against the previously-reviewed d6ca00f. This force-push consolidates the branch into one commit and resolves every open concern from the initial review: dynamic HID++ feature-index resolution, accurate docstring, brightness clamp on deserialize, debug-gated demo keyboard, and deletion of the unused typed wrapper.

  • Resolve 0x8080 PerKeyLighting index per deviceset_keyboard_color now opens an HidppChannel, calls device.root().get_feature(0x8080), and uses the returned info.index for both the 0x12 setGroupKeys frames and the 0x11 commit. Also drops the hardcoded 0xff device index in favour of route.device_index() (crates/openlogi-hid/src/write.rs:163-233).
  • Rewrote set_keyboard_color docstring — replaced the stale 0x8070 ColorLedEffects description with an accurate 0x8080 PerKeyLighting summary that also notes the runtime feature-index resolution (crates/openlogi-hid/src/write.rs:152-162).
  • Clamp deserialized brightnessLighting::brightness now uses deserialize_with = "deserialize_brightness", which runs u8::deserialize(..)?.min(100) so a hand-edited config.toml can't feed out-of-range values into the c * brightness / 100 scaling (crates/openlogi-core/src/config.rs:131-172).
  • Gate OPENLOGI_DEMO_KEYBOARD behind cfg(debug_assertions) — both the env-var check in build_device_list and the demo_keyboard fn itself are now debug-only, so the dev scaffolding doesn't ship in release builds (crates/openlogi-gui/src/state/devices.rs:56-112).
  • Deleted the unused PerKeyLightingV0 wrappercrates/openlogi-hid/src/lighting.rs is gone and pub mod lighting; is removed from lib.rs, since set_keyboard_color now drives the resolve-then-raw-write flow directly.

Pullfrog  | View workflow run | Using Claude Opus𝕏

@davidbudnick davidbudnick force-pushed the feat/keyboard-support branch from 78a87b5 to d0fa77b Compare June 1, 2026 04:29
Copy link
Copy Markdown

@pullfrog pullfrog Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes — incremental review of d0fa77b against the previously-reviewed 78a87b5. The force-push touches only crates/openlogi-gui/src/state.rs (+23/-2): a new AppState::same_physical_device helper threaded through merge_inventory so a transiently-flaky direct-USB config_key can no longer spawn a duplicate carousel/tray entry for the same physical device.

  • Add same_physical_device helper — returns true when two DeviceRecords share a config_key, or when both carry the same route (so a momentarily-different config key for the same physical device collapses). crates/openlogi-gui/src/state.rs:308-314.
  • Route dedup in merge_inventory — the existing-device loop skips an existing when merged already contains the same physical device (collapses pre-existing duplicates), and the "match existing against fresh" lookup as well as the "append unseen fresh" filter both now use same_physical_device instead of raw config_key equality. crates/openlogi-gui/src/state.rs:265-301.

Route equality is the right discriminator here: DeviceRoute::Bolt { receiver_uid, slot } is unique per pairing, and DeviceRoute::Direct { vendor_id, product_id } already documents "two identical mice on one host are indistinguishable here, so the first match wins (acceptable for v0)" — the new dedup inherits that v0 limitation but doesn't introduce any new ambiguity. Miss-counting still keys by existing.config_key, which is safe because the lookup only fires when the existing record was not found in fresh.

Pullfrog  | View workflow run | Using Claude Opus𝕏

@davidbudnick davidbudnick force-pushed the feat/keyboard-support branch from d0fa77b to 6a86d98 Compare June 1, 2026 14:08
Copy link
Copy Markdown

@pullfrog pullfrog Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes — incremental review of 6a86d98 against the previously-reviewed d0fa77b. This force-push rebases the feature commit onto current master (0ca1442), absorbing PR #28's HIDPP_LONG_COLLECTIONS table and PR #37's device_order_key / DeviceStableId. The feature-level deltas are integration-only — no new runtime behavior.

  • Joined the new HIDPP collections tabletransport.rs drops the bespoke HIDPP_G_USAGE_PAGE / HIDPP_G_LONG_USAGE_ID constants and the hand-rolled filter; the wired G-series interface is now a third row (0xff43, 0x0602, false) in HIDPP_LONG_COLLECTIONS, with long_only = false since wired G keyboards expose both report widths. Tests updated for both is_hidpp_long_collection and is_long_only_collection.
  • Reused master's stable-ordering helperstate.rs swaps sort_device_list for device_order_key, and state/devices.rs deletes its own sort_device_list / route_sort_key. DeviceStableId and DeviceIdentity widened to pub(super) so merge_inventory can sort by them via merged.sort_by_key(device_order_key).
  • Backfilled DeviceRecord fields on the dev fixturedemo_keyboard() now sets serial_number: None and unit_id: [0; 4] to satisfy the new fields master added in PR #37 for identity-based ordering.

Pullfrog  | View workflow run | Using Claude Opus𝕏

Copy link
Copy Markdown
Owner

@AprilNEA AprilNEA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR — the overall direction looks good and CI is green, but I think a few correctness/UI issues should be fixed before merging. The main blockers are the lighting slider state drifting from the selected device/config, treating every Unknown non-pointer device as a keyboard, and accepting non-RRGGBB values in the CLI despite the documented contract.

Comment thread crates/openlogi-gui/src/components/lighting_panel.rs
Comment thread crates/openlogi-gui/src/components/device_view.rs Outdated
Comment thread crates/openlogi-cli/src/cmd/diag/lighting.rs
Comment thread crates/openlogi-hid/src/write.rs Outdated
Comment thread crates/openlogi-hid/src/lib.rs Outdated
@davidbudnick davidbudnick force-pushed the feat/keyboard-support branch from 93907bd to f254373 Compare June 1, 2026 23:24
Copy link
Copy Markdown

@pullfrog pullfrog Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes — incremental review of f254373 against the last-reviewed 6a86d98. This force-push incorporates all 5 concerns from AprilNEA's prior review of 93907bd: CLI hex validation, device-view keyboard heuristic, slider/state synchronisation, removal of the spam_lighting probe from the public API, and proper write-error propagation.

  • CLI hex validationdiag lighting now rejects non-RRGGBB inputs (crates/openlogi-cli/src/cmd/diag/lighting.rs).
  • DeviceView keyboard heuristic narrowedis_keyboard matches DeviceKind::Keyboard or (Unknown + DeviceRoute::Direct) instead of blanket-unconditionally treating Unknown as a keyboard (crates/openlogi-gui/src/components/device_view.rs).
  • Slider state synced with AppStateLightingPanel tracks last_brightness and re-syncs the SliderState thumb when the active device's brightness changes without fighting an in-progress drag (crates/openlogi-gui/src/components/lighting_panel.rs).
  • spam_lighting removed — the brute-force probe and its --spam CLI option are deleted; set_keyboard_color is the sole lighting write path, with every HID error propagated through WriteError (crates/openlogi-hid/src/write.rs, crates/openlogi-hid/src/lib.rs).

Pullfrog  | View workflow run | Using Big Pickle (free) (credentials for Anthropic not configured) | 𝕏

@davidbudnick
Copy link
Copy Markdown
Contributor Author

@AprilNEA Thanks for your comments they all should be addressed, please let me know if you find any other issues!

@davidbudnick davidbudnick requested a review from AprilNEA June 1, 2026 23:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants