[gdevenyi]

Implements light color support as agreed in #572.

Changes made

Following your answers in #572, point by point:

1. Separate capability. CAP_LIGHT_COLOR, CLI --light-color RRGGBB with a leading # accepted, appended at the end of CAPABILITIES_XLIST and of CAPABILITY_DESCRIPTORS. HSC_CAP_LIGHT_COLOR = 24 is mirrored in hsc_capability_t, and master's per-capability static_asserts from 97e74b5 check it, so the version of those asserts this PR used to carry has been dropped.
2. Struct in the variant. LightColorSettings { uint8_t r, g, b; } in FeatureParam; HIDDevice::setLightColor(hid_device*, const LightColorSettings&); descriptor min/max nullopt, value hint <RRGGBB>. C API is hsc_set_light_color(h, r, g, b), C++ is Headset::setLightColor(). The int-vs-struct rule is written down in ADDING_A_CAPABILITY.md.
3. All zones. Zone count stays private to the device. 000000 is valid.
4. Effects untouched; LightsResult.mode as is.

-l with --light-color: the color request is last in the request table, so it runs after -l and wins. Documented in --help, the README usage section, and at the request table.

Output: actions carry color: "#rrggbb" in JSON, YAML and ENV, and value packs 0xRRGGBB. The value > 0 guard wasn't only in output_data.hpp — YAML and ENV each had their own inline copy, so fixing the one line would still have lost black in two formats. The rule now lives once in ActionData::hasValue(). It still omits 0 for other actions, since the equalizer returns success(0) to mean "no value"; -l 0 output is unchanged. api_version goes to 1.9: the capability list grows by one, and actions gain the color field.

Also: test device implements it; ADDING_A_CAPABILITY.md says new capabilities append only and must be mirrored in the C header; LIBRARY_USAGE.md, ADDING_A_DEVICE.md and AGENTS.md list the capability. AGENTS.md's list had also fallen behind, so it now includes sidetone status, the ANC family, the two microphone statuses and headset volume; README table regenerated with --readme-helper.

Corsair Virtuoso XT/SE

Split off as you asked. This PR is now only the plumbing plus the test device, and no hardware advertises CAP_LIGHT_COLOR yet. The Virtuoso implementation is parked on gdevenyi:virtuoso-light-color until there's a hold or a hardware-mode store. When it comes back, brightness will only be raised if it reads 0.

Testing

• Unit suite green; clang-format 18 clean over every changed file.
• New tests: the hex parser; ActionData's value rule; the C and C++ API through the test device; CLI output in every format, including black and -l ordering. Reverting hasValue() to value > 0 fails the ActionData test, and doing so only in the YAML writer fails the CLI test.

Checklist

• I adjusted the README (if needed)
• For new features in HeadsetControl: I discussed it beforehand in Issues or Discussions and adhered to the wiki — RGB colour support: no capability can express a colour today #572

🤖 Generated with Claude Code

[Copilot]

🟡 Changes recommended

Unresolved moderate findings remain in API versioning and Virtuoso error and connection handling.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds CAP_LIGHT_COLOR with CLI, C/C++ APIs, serialization, documentation, tests, and Corsair Virtuoso XT/SE support.

Changes:

• Adds RGB parsing, capability plumbing, and public APIs.
• Implements color output across JSON, YAML, and ENV.
• Adds Corsair lighting control and test coverage.

Unresolved findings concern API versioning and Virtuoso error, cleanup, connectivity, and target-resolution handling.

File summaries

File	Summary
tests/test_utilities.cpp	Tests RGB parsing
tests/test_output_formats.cpp	Tests color and value serialization
tests/test_library_api.cpp	Tests C/C++ APIs
tests/test_cli_output.cpp	Tests CLI output and ordering
README.md	Documents color support and device behavior
lib/utility.hpp	Declares the color parser
lib/utility.cpp	Implements color parsing
lib/result_types.hpp	Adds light color result types
lib/headsetcontrol.hpp	Adds the C++ color API
lib/headsetcontrol.cpp	Implements the C++ color API
lib/headsetcontrol_c.h	Adds the C API and capability enum
lib/headsetcontrol_c.cpp	Implements the C API and enum checks
lib/feature_handlers.hpp	Registers color handling
lib/devices/hid_device.hpp	Adds the device color method
lib/devices/headsetcontrol_test.hpp	Adds test-device support
lib/devices/corsair_virtuoso_xt.hpp	Implements Virtuoso lighting control
lib/device.hpp	Adds capability and RGB parameter plumbing
lib/capability_descriptors.hpp	Adds capability metadata
docs/LIBRARY_USAGE.md	Documents library APIs
docs/ADDING_A_DEVICE.md	Updates capability guidance
docs/ADDING_A_CAPABILITY.md	Documents capability implementation rules
cli/output/output.cpp	Emits structured color fields
cli/output/output_data.hpp	Adds color-aware value handling
cli/main.cpp	Parses and schedules color actions
AGENTS.md	Updates capability guidance

Review details

Suppressed comments (3)

cli/output/output.cpp:103

• Adding action.color changes the structured output schema, but API_VERSION remains 1.5 (line 29). docs/LIBRARY_USAGE.md:97-104 defines minor-version increments for additions, so clients cannot distinguish this schema from the old 1.5 format; bump the version and update the corresponding documentation/fixtures.

        action.color  = std::format("#{:02x}{:02x}{:02x}", c.r, c.g, c.b);

lib/devices/corsair_virtuoso_xt.hpp:133

• When the headset is charging, this new branch returns BATTERY_CHARGING, but the CLI's --connected implementation in cli/main.cpp:1209-1210 only treats BATTERY_AVAILABLE as connected. Consequently headsetcontrol --connected reports false for a connected Virtuoso while it is charging; the connectivity check should accept both available and charging statuses.

        auto status = BATTERY_AVAILABLE;
        if (auto charge_state = readProperty(device_handle, resolved->target, PROP_BATTERY_STATUS);
            charge_state && *charge_state == CHARGE_STATE_CHARGING) {
            status = BATTERY_CHARGING;

lib/devices/corsair_virtuoso_xt.hpp:453

• resolveTarget treats every failed readProperty as evidence that the candidate was wrong and discards the error. If both probes hit an HID, permission, or protocol failure, the caller is incorrectly told that the headset is offline; only a silent timeout/offline result should trigger the alternate probe, while other errors should be propagated.

        for (const uint8_t candidate : { hinted, alternate }) {
            if (auto level = readProperty(
                    device_handle, candidate, PROP_BATTERY_LEVEL, TARGET_PROBE_TIMEOUT_MS)) {
                return ResolvedTarget { .target = candidate, .battery_level = *level };
            }

• Files reviewed: 25/25 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.

[gdevenyi]

Fixed in d79a316. SoftwareModeGuard gained dismiss(), and the color path now takes a guard like every other write, dismissing it only once the frame is written. A successful color stays in software mode, so the frame stays visible; a failed brightness or frame write hands the headset back to hardware mode.

test_corsair_virtuoso.cpp covers both halves with a mock: a clean write takes six requests and never restores hardware mode, and a failed frame write still closes the handle and then restores it. Dropping the guard fails the failure case, and dropping dismiss() fails the success case. On the XT, --light-color still leaves the headset in software mode.

[gdevenyi]

Fixed — in #570 (8feff12), since that's where this code comes from, merged in here. The previous review there settled for a comment; this replaces the comment with a fix.

Every request now goes through transact(), which throws away anything already queued (non-blocking reads, bounded) before writing. Nothing read before a request goes out can be its reply, so it only ever discards stale replies and the volume events readReply() skipped anyway. The handle commands for the lighting frame use it too. A battery read still takes about 0.2 s on the XT.

The test mock makes each report readable only after a given number of writes, which is what distinguishes a stale reply from a real one. With a stale charge-state reply queued before getBattery(), the level comes out right; without the discard, the stale 1 is read as the battery level and the test fails.

[gdevenyi]

Addressing the three findings Copilot suppressed in its summary:

API version — fixed, now 1.6 (d79a316). Action entries gained color, and 4.1.0 already shipped API 1.5, so this can't fold into 1.5 the way #560's sidetone field did before that release. Following #549: the constant gets its note, and the LIBRARY_USAGE.md example and serializer fixture follow.

resolveTarget() swallowing errors — fixed, in #570 (8feff12) and merged here. Only two probe failures now mean "wrong target": silence, and "no such property", which is a receiver answering for itself with no headset behind it. Anything else — a failed HID write, an unexpected status — is returned as is instead of becoming "offline". The mock tests check all three cases, including that treating only silence as "wrong target" would regress a receiver with its headset off from "offline" to "not supported".

--connected reporting a charging headset as disconnected — real, but not changed here. It's not specific to this PR: checkDeviceConnected() has treated only BATTERY_AVAILABLE as connected since the C version (info.status != BATTERY_AVAILABLE → false), and that applies to all 22 device files that report BATTERY_CHARGING. The Virtuoso only meets it now because #570 made it report charging at all. Counting charging as connected is a one-line change, but it alters the meaning of --connected for every device, so I'd rather not slip it into a light-color PR. @Sapd — happy to send it separately if you agree charging should count as connected.

[Sapd]

Plumbing is good - matches #572 point for point. Output is byte-identical to master for every non-color action across all formats, so the hasValue() change is safe. API bump follows what #549 did.

The per-entry static_asserts are worth keeping: they catch two entries having their values swapped, which a count check wouldn't. (Your description is wrong on one detail - deleting HSC_CAP_NOISE_FILTER fails with "undeclared identifier", not the sync message.)

I don't want to advertise light color on the Virtuoso yet, and it's worse than the ~60s revert:

1. Any later command kills it. setSidetone, setInactiveTime and setLights all install a SoftwareModeGuard, and restoring hardware mode swaps the frame for the headset's own effect - your own finding. So --light-color ff0000 then -s 50 a minute later and the color is gone.
2. setLightColor writes PROP_BRIGHTNESS = BRIGHTNESS_MAX unconditionally, and brightness persists. -l 0 today plus --light-color tomorrow leaves the stock effect at full brightness, with the earlier "lights off" silently undone.

The README table can't say "works for a minute unless you run another command". Please split the Virtuoso commit off - I'll take it once there's a hold or a hardware-mode store. If you'd rather keep it, at minimum read brightness and only raise it from 0.

My fault in #572 on this: "color implies on" assumed on was stateless. On this headset it's a persisted brightness value. Should have said raise it only if it reads 0, or leave it to the device.

-f can't be the hold either - processFeatureRequests only runs requests still marked FEATURE_NOT_PROCESSED and nothing resets that in the loop, so follow mode re-prints instead of re-querying. Opened separately, not your problem.

[Sapd]

On --connected: yes, charging should count. A headset on the cable is plainly there, and the other three states all mean we couldn't talk to it:

BATTERY_UNAVAILABLE  off or not paired
BATTERY_HIDERROR     HID error
BATTERY_TIMEOUT      no reply

So the condition becomes

status == BATTERY_AVAILABLE || status == BATTERY_CHARGING

Send it separately, agreed it doesn't belong in this PR. It changes --connected for the 18 device files that can report charging, so it wants a line in the release notes.

[gdevenyi]

Split as you asked, and rebased onto master, so this is a force-push. The PR is now a single commit (b964690): the plumbing plus the test device, with no hardware advertising CAP_LIGHT_COLOR. It no longer stacks on #570.

• Virtuoso: parked on gdevenyi:virtuoso-light-color with no PR. When it comes back with a hold or a hardware-mode store, it will read brightness and only raise it from 0. That also covers the stale -l 0 you pointed out.
• static_assert: you were right about the description: deleting HSC_CAP_NOISE_FILTER fails with "undeclared identifier", not the sync message. Master now has its own per-capability asserts (97e74b5), so I dropped mine and HSC_CAP_LIGHT_COLOR = 24 goes through yours. I've corrected the description.
• Rebase: CAP_LIGHT_COLOR is appended after CAP_HEADSET_VOLUME, the handler is registered under CAPABILITYTYPE_ACTION, and API_VERSION goes to 1.9 with a line in the history comment. The LIBRARY_USAGE.md example and the fixture follow. The README table is regenerated. The AGENTS.md capability list had fallen behind master, so it now has the ANC family, the microphone statuses and headset volume as well.
• --connected: sent separately as --connected now reports a charging headset as connected #609.

Unit suite green and clang-format 18 clean. --test-device --light-color "#00ff80" -o json gives api_version 1.9, value 65408 and color #00ff80.

[Sapd]

All three points are addressed, and the asserts in master cover HSC_CAP_LIGHT_COLOR = 24 - I set it to 25 locally and the build stops at headsetcontrol_c.cpp:254.

Checked here:

• hasValue() is the only part that touches existing behaviour. 92 action blocks compared against master across json/yaml/env/standard, every action at 0 included - byte-identical. Breaking it either way is caught: back to value > 0 fails "Black should keep its value", always-true fails "Zero value without a color should be omitted".
• Parser rejects fff, gggggg, 0xff0000, ff00000, ##ff0000, " ff0000", +ff000, empty; takes ff0000, FF0000, 000000, #00ff80.
• Request order holds regardless of command-line order: --light-color ff0000 -l 0 applies lights first, color second.
• README table matches --readme-helper, clang-format 18 clean, no new warnings, suite green.

Merging. Title trimmed - it still advertised the Virtuoso support you parked.

The AGENTS.md backfill and the "appended, never inserted - the values are C ABI" note are worth more than the capability itself; that list had been wrong since CAP_ANC.