Repository navigation
feat(cli): add openlogi backlight effect for HID++ 0x1982's predefined effects - #1397
fahadjibransheikh wants to merge 2 commits into
Conversation
|
|
Addressed all three findings in b327e4f:
All green: |
c34a183 to
9eabddd
Compare
…d effects The Backlight feature (0x1982, version 3) already models the predefined effects Logi Options+ exposed — Static, None, Breathing, Contrast, Reaction, Random, Waves — via set_backlight_effect / SetBacklightConfig.effect, but nothing above the protocol crate called either. set_backlight_enabled deliberately sends the 0xff "do not change" sentinel for effect on every write, which is why a user's Options+-configured effect kept running untouched under OpenLogi: it was never read or written, just left alone. Add get_backlight_effect / set_backlight_effect to the device write layer (same shape as the existing get_backlight / set_backlight_enabled), and a new `openlogi backlight effect <name>` CLI subcommand. Persistent, like on/off — setBacklightConfig writes to non-volatile memory. BacklightEffect is a new openlogi-core type but is not added to BacklightState: that struct crosses the agent<->GUI tarpc IPC (read_backlight in openlogi-ipc), so adding a field there is an append-only wire change needing a PROTOCOL_VERSION bump and the ipc crate's golden wire_format test — out of scope for a CLI-only feature nothing on the IPC side consumes yet. get_backlight_effect is a separate, small read instead. CLI only for now, matching the existing backlight on/off precedent. The GUI's Lighting panel is gated on the RGB `lighting` capability (0x8070/0x8080) and deliberately excludes backlight-only keyboards, so a GUI treatment needs its own capability flag and panel — left as a natural follow-up, not built speculatively.
Two review findings on the backlight-effect PR: - run_effect (CLI) didn't print the note set_backlight_enabled's caller already does when the keyboard is in TemporaryManual mode (level set via the keyboard's own backlight keys) — setBacklightConfig can't write that mode back, so it silently lands in Automatic. Same note, same trigger, now on both paths. - effect_from_firmware silently collapsed an unmodelled future BacklightEffect variant to Static, the same fallback shape as mode_from_firmware/status_from_firmware. Those two are read-only display values; effect_from_firmware isn't anymore — run_effect compares its result against the requested effect to confirm a write applied, so a silent Static fallback could make that check report success for the wrong effect. Made it fallible instead, reusing the same WriteError::UnsupportedResponse this file already returns for an unrecognized wire byte one layer down. mode_from_firmware/ status_from_firmware are unchanged — they stay display-only fallbacks.
9eabddd to
cd01cba
Compare
| .set_backlight_config(SetBacklightConfig { | ||
| enabled: current.enabled, |
There was a problem hiding this comment.
Effect changes undo backlight off
If set_backlight_effect reads enabled = true and a concurrent backlight off command finishes before the effect write, enabled: current.enabled turns the keyboard back on and saves that state. with_route does not lock the read-and-write pair, and separate CLI commands can open separate channels.
Serialize both backlight writers for the same device across processes so changing an effect cannot overwrite a newer enabled setting.
Knowledge Base Used: Device integration stack
There was a problem hiding this comment.
Valid in theory, but I'd rather not fix it in this PR. Backlight writes come only from the CLI (the agent and GUI never touch 0x1982), so this needs two openlogi backlight commands racing on the same keyboard within milliseconds. setBacklightConfig has a do-not-change value for the effect byte but not for enabled, so the only real fix is a cross-process lock per device. Nothing in the write layer has one today, and every read-then-write settings path would need the same treatment. Happy to open a follow-up issue if you want that lock across the board.
There was a problem hiding this comment.
That’s fair. The race is real, but addressing it correctly requires a per-device cross-process lock around every read-then-write backlight operation, including the existing enable/disable path—not a narrowly scoped change for this new effect command.
Given that 0x1982 is currently written only by the CLI and conflicting commands would need to overlap, this is reasonable follow-up work rather than a blocker for this PR. I’m withdrawing this finding.
Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.
Summary
Backlightfeature (0x1982, version 3) already models the predefined effects Logi Options+ exposed — Static, None, Breathing, Contrast, Reaction, Random, Waves — viaBacklightFeature::set_backlight_effect/SetBacklightConfig.effectinopenlogi-hidpp, but nothing above the protocol crate called either.set_backlight_enableddeliberately sends the0xff"do not change" sentinel foreffecton every write — that's why a keyboard's Options+-configured effect kept running untouched after switching to OpenLogi: it was never read or written, just left alone.openlogi backlight effect <name>(persistent, same ason/off—setBacklightConfigwrites to non-volatile memory).Changes
crates/openlogi-core/src/hid/backlight.rs: newBacklightEffectenum (7 variants), same derive set and doc style as the neighboringBacklightMode/BacklightStatus. Not added toBacklightState— that struct crosses the agent↔GUI tarpc IPC (read_backlightinopenlogi-ipc), so adding a field there means an append-only wire change, aPROTOCOL_VERSIONbump, and the ipc crate's goldenwire_formattest, for a feature nothing on the IPC side consumes yet.get_backlight_effectis a separate, small read instead of folding into that struct.crates/openlogi-device/src/write/backlight.rs:effect_from_firmware/effect_to_firmware(mirrors the existingmode_from_firmware/mode_to_firmware, same#[non_exhaustive]-source fallback pattern),get_backlight_effect,set_backlight_effect(same shape as the existingset_backlight_enabled— reads current config, writes every field back unchanged excepteffect). Two new round-trip tests.crates/openlogi-device/src/{backlight,lib,write}.rs,crates/openlogi-hid/src/{host,lib}.rs: re-export plumbing for the two new functions/type through the existing layering.crates/openlogi-cli/src/cmd/backlight.rs:BacklightAction::Effect { effect: EffectArg }, aclap::ValueEnumEffectArg,run_effect(prints before/after state + effect line, bails if the read-back doesn't match what was requested, same as the existingon/offmismatch check).crates/openlogi-cli/src/lib.rs: new parse test.No GUI changes — the Lighting panel is gated on the RGB
lightingcapability (0x8070/0x8080) and deliberately excludes backlight-only keyboards (see that capability's own doc comment inopenlogi-core::device), so a GUI treatment needs its own capability flag and panel. Left as a natural follow-up, not built speculatively.Testing
cargo fmt --all -- --check— cleancargo clippy -p openlogi-core -p openlogi-device -p openlogi-cli --all-targets -- -D warnings— cleancargo test -p openlogi-device backlight— 7 passed, including the 2 new round-trip testscargo test -p openlogi-cli— 95 passed, including the new parse testopenlogi backlight effect breathing --device "MX Mechanical"— read-back confirmedeffect: breathing, independently re-confirmed with a separateopenlogi backlight status. Keyboard's actual prior effect wasreaction(discovered via read-back, not assumed) — restored toreactionafterward, left exactly as found.