Conversation
42c3e6a to
5542b39
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The updated device header introduces at least one build-breaking include issue and a behavior in setLights(false) that can leave the device stuck in software mode with undesirable side effects.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR reworks the Corsair Virtuoso XT/SE device implementation to use Corsair’s Bragi property protocol on the vendor HID report, expanding support beyond battery reporting to additional device capabilities.
Changes:
- Reimplemented Virtuoso XT/SE communication using Bragi property GET/SET framing and reply filtering.
- Added support for
CAP_SIDETONE,CAP_INACTIVE_TIME, andCAP_LIGHTS, including software-mode bracketing for persistent settings writes. - Updated README capability matrix and Corsair sidetone note to reflect XT/SE behavior.
File summaries
| File | Description |
|---|---|
| README.md | Updates device capability table and Corsair sidetone guidance for Virtuoso XT/SE. |
| lib/devices/corsair_virtuoso_xt.hpp | Implements Bragi protocol framing, target resolution, and adds sidetone/lights/inactive-time capabilities (plus battery read rework). |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
5f23a5c to
01796de
Compare
|
Opened #572 in Ideas to discuss RGB colour support, since the Virtuoso XT's lighting hardware does full RGB (I cycled the zones red/green/blue while reverse-engineering it) but no capability in the project can express a colour. This PR stays as-is — static white for |
The Virtuoso XT/SE speaks Corsair's Bragi protocol, the same one corsair_void_v2w.hpp uses, but framed on HID report 0x02 of the vendor collection instead of an unnumbered report. Reworking the device around that protocol replaces the single undocumented battery command with the documented property table and adds three capabilities: * CAP_SIDETONE - property 0x46 (enable) and 0x47 (volume, 0-1000) * CAP_INACTIVE_TIME - property 0x0d (enable) and 0x0e (timeout in ms) * CAP_LIGHTS - lighting handle 0x00, resource 0x01 Battery now reads BRAGI_BATTERY_LEVEL (0x0f, tenths of a percent) and reports charging from BRAGI_BATTERY_STATUS (0x10). The previous command returned a byte that stayed frozen at one value across a full charge cycle, while 0x0f tracked the level and matched the charge state; properties 0x11 and 0x12 decoding to the exact vendor and product IDs confirm the property numbering. Writes are refused with status 0x09 unless the headset is switched into software mode (property 0x03 = 2). Settings written that way persist, so those writes are bracketed by a scope guard that hands the headset straight back to hardware mode rather than parking it in software mode. Lighting is the exception and deliberately stays in software mode. It is a block of data pushed through the open/write/close handle sequence, and hardware mode drives the LEDs from the effect the headset runs itself, which paints straight over the frame we wrote - restoring it makes "lights off" do nothing at all. The headset drops back to hardware mode by itself within a few minutes of the host going quiet, and the frame it was last given survives that. Because the capability is only on/off, "on" paints every zone static white rather than restoring the effect that was running before; the protocol offers no way to read that effect back and replay it. A wireless receiver relays commands to the headset paired with it (target 0x09) while a wired headset answers for itself (target 0x08). The registry shares one instance across product IDs and only records the ID it last matched on, so the target is confirmed against the device rather than trusted - otherwise a receiver and a wired headset plugged in together break each other. Tested on a Virtuoso XT in both wireless (0x0a64) and wired (0x0a62) modes: battery level and charging state, sidetone across its range and off, inactive time set/disabled/restored - each verified by reading the property back - and lights toggled both ways through the CLI and confirmed visually. The SE product IDs are untested and ride along on the same framing. Note that iCUE's equalizer and spatial audio are host-side DSP applied to the PC audio stream, not device settings, so they are out of scope here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BHCVMmAAeV5WGP7oYZsk3T
01796de to
01ef3fd
Compare
Sapd
left a comment
There was a problem hiding this comment.
Taking this one. The battery rework fixes a real bug. What 4.1.0 ships is
{ 0x02, 0x00 }
- report ID and target, no command, no property. The byte we print is 0x3b = 59, which is why #567's reporter and the author of #568 both got exactly 59%. 0x0f is what corsair_void_v2w.hpp reads too.
#568 is the same protocol. Closed it and asked its author to run this branch on his SE - that's the gate for the SE PIDs. CI is green.
Blocking:
-
PROP_BRIGHTNESS = 1000 is written unconditionally (line 238), before the on/off branch.
-l 0sets brightness to max and then paints black. On Corsair's other Bragi devices 0x02 is the persisted hardware-mode brightness, so this likely also overwrites whatever the user set in iCUE.Compare ckb-next led_bragi.c updatergb_bragi(): it writes 0x02 = 0 when the frame is all black and 1000 otherwise - "being able to just switch them off even in hw mode is really nice". I think that's the whole feature for a boolean:
-l 0= software mode, brightness 0, guard back to hardware mode;-l 1= brightness 1000. Off survives a power cycle, on brings back the user's own effect instead of static white, and the headset is never left in software mode. Please test that on the XT. If it holds, drop writeLighting() from this PR - it comes back with #572. -
Header (46-49) and the setLights comment state the idle drop-back as fact. Measured on one XT - say so.
Minor:
- resolveTarget() reads 0x0f and getBattery() reads it again. Return the probed value.
- 300 ms probe: a headset slower than that is reported offline, and its late reply is the first thing the next readReply() sees - there's no property echo to catch it. Worth a comment.
- writeLighting(): ckb-next closes and reopens on open status 0x03. Only relevant if the frame path stays.
- Sidetone: checked it survives a power cycle, or only the mode switch?
Replied on #572.
Review feedback on Sapd#570. setLights() wrote brightness 1000 unconditionally and then painted a static white or black frame, which left the headset in software mode and replaced the user's own lighting effect with white. It now does what ckb-next's updatergb_bragi() does for Corsair's other Bragi devices: switch to software mode, write brightness 0x02 as 0 or 1000, and hand the headset back to hardware mode through the same scope guard as every other write. Checked on the XT: -l 0 turns the LEDs off and they stay off in hardware mode, -l 1 brings back the headset's own effect rather than white, and brightness 0 survives a power cycle. writeLighting() and its handle constants are gone from this PR; they come back with light colour support (Sapd#572). The last session's evidence against this approach was invalid. Two of the three runs crashed before writing anything, and the one that did never entered software mode, so its writes were refused. Also from review: - resolveTarget() already reads the battery level to find a target that answers, so it now returns that value and getBattery() no longer asks twice. - Document the 300 ms probe timeout's sharp edge: a slower headset is reported offline, and because replies carry no property ID its late reply can be taken by the next read for the same target and command. - State what was measured, and on what. Sidetone, the sleep timer and brightness all survived a power cycle on the one XT tested; the drop back to hardware mode after a few idle minutes was seen on that unit only, and nothing relies on it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BHCVMmAAeV5WGP7oYZsk3T
|
Thanks — all addressed in 9f54a74, added as a new commit rather than a force-push so the delta is reviewable. 1. Brightness. Your design holds on the XT.
I should own why I didn't land on this myself: last session I had "evidence" that brightness doesn't gate the LEDs in hardware mode. Going back through it, two of the three runs crashed before writing anything (the hidraw node had moved when the headset changed transport), and the one that did run never entered software mode, so every write was refused. My probe also wasn't matching the command echo, which is why it "restored" brightness to 35500. It was worthless, and your reading of 2. Measured on one XT. The header no longer mentions the idle drop-back at all — nothing depends on it now that lights restore hardware mode. The guard's comment says it was seen on one unit and not relied on, and the power-cycle persistence is attributed the same way. Minor:
|
The frame path removed from Sapd#570 comes back here, as agreed there. The color is painted through handle 0 / resource 1 as a planar frame for three zones (every red byte, then green, then blue). Opening a handle that is already open returns status 0x03 on the XT - checked - so, as ckb-next does, the handle is closed and reopened once. Tested by leaving the lighting handle open and running --light-color: it succeeds and leaves the handle closed. The handle is also closed when the frame write fails, so one bad run does not block the next. Two things set a color apart from the other writes, both checked on the XT: - Brightness gates a painted frame as well as the headset's own effect. With brightness 0 a green frame stayed dark; raising brightness showed it. A color therefore raises brightness, which is also what "color implies on" asks for: `-l 0 --light-color 00ff00` in one call ends up green. - Returning to hardware mode replaces the frame with the headset's own effect at once, so a color write stays in software mode instead of using the guard. That makes -l 1 the way back: after a color it restores the user's effect. The color is temporary on this headset. With nothing talking to it, the headset drops back to hardware mode on its own and its own effect returns: it was still in software mode 45 s after --light-color and back in hardware mode by 90 s. Vendor software gets a lasting color by staying connected. I looked for a way to store a hardware-mode color and did not find a safe one. A read-only sweep of every 16-bit property and resource ID found 33 properties and 6 resources, all below 0x100. The only plausible stores are resources 0x14 (6 bytes) and 0x2a (5 bytes), and both refuse reads, so their contents cannot be backed up; nothing writes to them. The limitation is documented in the device header and in a README note. Black (000000) is a valid color and turns the zones dark. README table regenerated; the Virtuoso gains the light color column. Tested through the CLI and confirmed visually, wired (0x0a62): green after -l 0 in the same call, black goes dark, -l 1 restores the headset's own effect. Wireless (0x0a64, target 0x09): magenta. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BHCVMmAAeV5WGP7oYZsk3T
Two findings from Copilot's review of the stacked Sapd#580, both in code this PR introduces. A reply that arrives after its request timed out stayed queued, and replies carry no property or handle ID - only who sent them and the command they answer - so the next request of the same kind would take it for its own. The last review settled for a comment on this; it is fixed instead. Every request now goes through transact(), which throws away anything already queued before writing. Reports read before a request is sent cannot be its reply, so this only ever drops stale replies and the volume events readReply() skipped anyway. It is bounded, so a device that never stops sending cannot stall a request, and a battery read still takes about 0.2 s. resolveTarget() treated every failed probe as "wrong target" and ended with "offline", so a failed HID write or an unexpected status was reported as the headset being powered off. Only two failures now mean wrong target: silence, and "no such property", which is a receiver answering for itself with no headset behind it. Anything else is returned as is. tests/test_corsair_virtuoso.cpp adds a mock whose reports only become readable after a given number of writes, which is what distinguishes a stale reply from a real one. It covers battery over both targets, a stale reply queued before the request, a receiver with no headset (offline, not unsupported), and a HID write failure (propagated, and the other target is not probed). Removing the discard fails the stale-reply test; treating every probe failure as wrong target fails the HID error test; treating only silence as wrong target fails the receiver test. Checked on the XT over wireless: battery, sidetone, sleep timer and lights, each read back. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BHCVMmAAeV5WGP7oYZsk3T
From Copilot's review. setLightColor() switched to software mode and then returned on a failed brightness or frame write without switching back, since the color path deliberately has no guard. A failed color left the headset in software mode with nothing worth keeping on its LEDs. SoftwareModeGuard now has dismiss(): the color path takes a guard like every other write and dismisses it only once the frame is written, so success stays in software mode and failure is handed back. A mock test covers both: a clean write takes six requests and never restores hardware mode, and a failed frame write still closes the handle and then restores it. Without the guard the failure case fails; without dismiss() the success case does. On the XT, --light-color still leaves the headset in software mode. API_VERSION goes to 1.6. Action entries gained "color", and 4.1.0 already shipped 1.5, so this cannot fold into 1.5 the way Sapd#560's sidetone field did before that release. The LIBRARY_USAGE.md example and the serializer fixture follow, as in Sapd#549. Also brings in Sapd#570's fixes for stale replies and probe errors, which the light color path picks up through transact(). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BHCVMmAAeV5WGP7oYZsk3T
|
One more commit here, 8feff12, from Copilot's review of the stacked #580. Both findings were in code this PR introduces:
Both come with mock tests in a new |
Changes made
The Virtuoso XT/SE turns out to speak Corsair's Bragi property protocol — the same one
corsair_void_v2w.hppalready uses — just framed on HID report0x02of the vendor collection (Usage-Page0xff42) instead of an unnumbered report. Reworking the device around that protocol replaces the single battery command with the documented property table and adds three capabilities:CAP_SIDETONE0x46(enable) +0x47(volume, 0–1000 in steps of 10)CAP_INACTIVE_TIME0x0d(enable) +0x0e(timeout in ms)CAP_LIGHTS0x02— 0 or 1000Packet framing —
[0]report ID0x02,[1]target,[2]command (0x01SET /0x02GET),[3]property,[5..]little-endian value. Replies come back on report0x01with the status in[3](0x00ok,0x05no such property,0x09write refused).Software mode. Writes are refused with status
0x09unless the headset is switched into software mode (property0x03= 2). Every write — lights included — is bracketed by an RAII guard that hands the headset straight back to hardware mode.Lights follow ckb-next's
updatergb_bragi(): brightness is the headset's own persisted setting and gates whatever effect it runs, so-l 0writes 0 and-l 1writes 1000. The user's own lighting effect comes back on-l 1, and off survives a power cycle. Setting a colour is a separate capability, in #572.Wireless vs wired. A receiver relays commands to the headset paired with it (target
0x09); a wired headset answers for itself (target0x08). Asking the wrong one is silent rather than an error. The registry shares one instance across product IDs and only records the ID it last matched on, so the product ID is used as a hint and then confirmed against the device — without that, a receiver and a wired headset plugged in at the same time break each other. (Plugging the USB-C cable into a Virtuoso XT doesn't just charge it; the headset re-enumerates as the wired product ID.)Battery now reads
BRAGI_BATTERY_LEVEL(0x0f, tenths of a percent) and reports charging fromBRAGI_BATTERY_STATUS(0x10). The command 4.1.0 ships,{ 0x02, 0x00 }, is a report ID and a target with no command or property, and the byte it prints is a constant0x3b= 59 — which is why every report of this headset reads exactly 59%. Properties0x11and0x12decoding to exactly0x1b1cand0x0a62confirm the property numbering matches ckb-next's Bragi table.Testing
On a Virtuoso XT, in both wireless (
0x0a64) and wired (0x0a62) modes:1) after every writeThe SE product IDs (
0x0a3e,0x0a3d) are untested here — that's gated on #568's author running this branch on his SE.Out of scope
iCUE's equalizer and spatial audio are host-side DSP applied to the PC audio stream rather than device settings, so they aren't reachable from a HID tool. Light colour is #572.
Checklist
Existing capabilities on a device that already had an entry, so no new feature to discuss.
🤖 Generated with Claude Code
https://claude.ai/code/session_01BHCVMmAAeV5WGP7oYZsk3T