From 01ef3fd3023afd2686bc78d2f22755700c5686d7 Mon Sep 17 00:00:00 2001 From: "Gabriel A. Devenyi" Date: Sat, 5 Sep 2026 10:06:32 -0400 Subject: [PATCH 1/3] feat(corsair): add sidetone, lights and inactive time to Virtuoso XT/SE 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) Claude-Session: https://claude.ai/code/session_01BHCVMmAAeV5WGP7oYZsk3T --- README.md | 4 +- lib/devices/corsair_virtuoso_xt.hpp | 561 ++++++++++++++++++++++++---- 2 files changed, 500 insertions(+), 65 deletions(-) diff --git a/README.md b/README.md index a74e6d2..ba5184d 100644 --- a/README.md +++ b/README.md @@ -166,7 +166,7 @@ sudo udevadm control --reload-rules && sudo udevadm trigger | Logitech Zone Wired/Zone 750 | All | x | | | | | | x | x | | | | | | | | | | | | Corsair Headset Device | All | x | x | x | x | | | | | | | | | | | | | | | | Corsair Wireless V2 Headset Device | All | x | x | | | x | | | | | | | | | | | | | | -| Corsair Virtuoso XT/SE | All | | x | | | | | | | | | | | | | | | | | +| Corsair Virtuoso XT/SE | All | x | x | | x | x | | | | | | | | | | | | | | | SteelSeries Arctis (1/7X/7P) Wireless | All | x | x | | | x | | | | | | | | | | | | | | | SteelSeries Arctis (7/Pro) | All | x | x | | x | x | x | | | | | | | | | | | | | | SteelSeries Arctis 9 | All | x | x | | | x | x | | | | | | | | | | | | | @@ -199,7 +199,7 @@ sudo udevadm control --reload-rules && sudo udevadm trigger \* Only available on some product variants of that device. Sidetone status reading, for instance, is verified only for the SteelSeries Arctis Nova 7 Gen 2 (`1038:227e`). -> **Note:** Some Corsair headsets may need additional configuration - see [Adding a Corsair device](docs/ADDING_A_CORSAIR_DEVICE.md). Some headsets (HS80, HS70 wired, RGB Elite, Virtuoso) expose sidetone via ALSA mixer instead. +> **Note:** Some Corsair headsets may need additional configuration - see [Adding a Corsair device](docs/ADDING_A_CORSAIR_DEVICE.md). Some headsets (HS80, HS70 wired, RGB Elite, Virtuoso other than the XT/SE) expose sidetone via ALSA mixer instead. ## Usage diff --git a/lib/devices/corsair_virtuoso_xt.hpp b/lib/devices/corsair_virtuoso_xt.hpp index 96dd6b7..1b31cd6 100644 --- a/lib/devices/corsair_virtuoso_xt.hpp +++ b/lib/devices/corsair_virtuoso_xt.hpp @@ -2,8 +2,13 @@ #include "../result_types.hpp" #include "../utility.hpp" #include "corsair_device.hpp" +#include "device.hpp" #include #include +#include +#include +#include +#include #include using namespace std::string_view_literals; @@ -13,29 +18,55 @@ namespace headsetcontrol { /** * @brief Corsair Virtuoso XT / SE (Wireless + Wired) * - * Protocol reverse-engineered via hidraw probing on Linux. + * These headsets speak Corsair's "Bragi" property protocol, the same one used by + * the newer devices in corsair_void_v2w.hpp, but framed on HID report 0x02 of the + * vendor collection (Usage-Page 0xff42) rather than on an unnumbered report. * - * Battery request: send 0x02 0x00 on interface 3 (Usage-Page 0xff42) - * Response format (64 bytes): - * [0] = 0x01 (report ID) - * [1] = status flags (0xf0 = normal, TBD for charging) - * [2] = battery percentage (0-100) - * [3+] = zeros (unused) + * Request layout (64 bytes, zero padded): + * [0] = 0x02 report ID of the vendor OUT report + * [1] = target 0x09 = headset behind a receiver, 0x08 = this device + * [2] = command 0x01 = SET, 0x02 = GET + * [3] = property ID + * [4] = 0x00 + * [5..] = little-endian value (SET only) * - * Volume events are broadcast unsolicited: - * [0] = 0x0E - * [1] = 0x00 (down), 0x01 (up), 0x02 (fast up) + * Reply layout (64 bytes, report ID 0x01): + * [0] = 0x01 report ID of the vendor IN report + * [1] = 0x01 reply relayed from the headset (0x00 = from this device) + * [2] = command echo + * [3] = status 0x00 = ok, 0x05 = no such property, 0x09 = write refused + * [4..] = little-endian value + * + * Writes are refused (status 0x09) unless the headset has been switched into + * software mode (property 0x03 = 2). Settings written that way persist after + * switching back, so they are bracketed by a scope guard that hands the headset + * straight back to hardware mode instead of parking it in software mode. + * + * Lighting is not a property but a block of data pushed through the protocol's + * open/write/close handle sequence. The frame written that way sticks: the LEDs + * keep showing it even once the headset drops back to hardware mode, which it + * does on its own shortly after the host stops talking to it. What hardware mode + * does not do is resume whatever effect was running beforehand. + * + * The device also broadcasts unsolicited volume events on report 0x0e, which have + * to be skipped when looking for a reply. + * + * Note that plugging the USB-C cable into a Virtuoso XT does not just charge it: + * the headset re-enumerates as the wired product ID, and the receiver then reports + * that no headset is attached. * * Virtuoso XT - Wireless Product ID: 0x0a64 (receiver), Wired Product ID: 0x0a62 * Virtuoso SE - Wireless Product ID: 0x0a3e (receiver), Wired Product ID: 0x0a3d */ class CorsairVirtuosoXT : public CorsairDevice { public: + static constexpr uint16_t PID_XT_WIRELESS = 0x0a64; // Wireless receiver (Virtuoso XT) + static constexpr uint16_t PID_XT_WIRED = 0x0a62; // Wired USB (Virtuoso XT) + static constexpr uint16_t PID_SE_WIRELESS = 0x0a3e; // Wireless receiver (Virtuoso SE) + static constexpr uint16_t PID_SE_WIRED = 0x0a3d; // Wired USB (Virtuoso SE) + static constexpr std::array SUPPORTED_PRODUCT_IDS { - 0x0a64, // Wireless receiver (Virtuoso XT) - 0x0a62, // Wired USB (Virtuoso XT) - 0x0a3e, // Wireless receiver (Virtuoso SE / Slipstream) - 0x0a3d // Wired USB (Virtuoso SE) + PID_XT_WIRELESS, PID_XT_WIRED, PID_SE_WIRELESS, PID_SE_WIRED }; std::vector getProductIds() const override @@ -50,86 +81,490 @@ class CorsairVirtuosoXT : public CorsairDevice { constexpr int getCapabilities() const override { - return B(CAP_BATTERY_STATUS); + return B(CAP_BATTERY_STATUS) | B(CAP_SIDETONE) | B(CAP_INACTIVE_TIME) | B(CAP_LIGHTS); } - constexpr capability_detail getCapabilityDetail(enum capabilities cap) const override + constexpr capability_detail + getCapabilityDetail([[maybe_unused]] enum capabilities cap) const override { - switch (cap) { - case CAP_BATTERY_STATUS: - // Interface 3, Usage-Page 0xff42, Usage-ID 0x0001 - return { .usagepage = 0xff42, .usageid = 0x1, .interface_id = 3 }; - default: - return HIDDevice::getCapabilityDetail(cap); - } + // Interface 3, Usage-Page 0xff42, Usage-ID 0x0001 + return { .usagepage = 0xff42, .usageid = 0x1, .interface_id = 3 }; } Result getBattery(hid_device* device_handle) override { auto start_time = std::chrono::steady_clock::now(); - // Send battery status request: 0x02 0x00 - std::array request { 0x02, 0x00 }; - if (auto result = writeHID(device_handle, request); !result) { + auto target = resolveTarget(device_handle); + if (!target) { + return target.error(); + } + + // Reading does not need software mode, which keeps the headset from + // producing an audible pop just to report its battery level. + auto level = readProperty(device_handle, *target, PROP_BATTERY_LEVEL); + if (!level) { + return level.error(); + } + + // The level is reported in tenths of a percent. + if (*level > BATTERY_LEVEL_MAX) { + return DeviceError::protocolError( + std::format("Battery level out of range: {}", *level)); + } + + // Charge state is a separate property; treat it as advisory so that a + // firmware which does not implement it still yields a usable level. + auto status = BATTERY_AVAILABLE; + if (auto charge_state = readProperty(device_handle, *target, PROP_BATTERY_STATUS); + charge_state && *charge_state == CHARGE_STATE_CHARGING) { + status = BATTERY_CHARGING; + } + + auto duration = std::chrono::duration_cast( + std::chrono::steady_clock::now() - start_time); + + return BatteryResult { + .level_percent = static_cast(*level / 10), + .status = status, + .mic_status = MICROPHONE_UNKNOWN, + .query_duration = duration, + }; + } + + Result setSidetone(hid_device* device_handle, uint8_t level) override + { + // The headset stores the sidetone volume as 0-1000 in steps of 10. + const uint16_t mapped_level + = map(level, 0, 128, SIDETONE_DEVICE_MIN, SIDETONE_DEVICE_MAX); + const auto sidetone_value + = static_cast(round_to_multiples(mapped_level, 10)); + + auto target = resolveTarget(device_handle); + if (!target) { + return target.error(); + } + + if (auto result = writeProperty(device_handle, *target, PROP_MODE, MODE_SOFTWARE); + !result) { return result.error(); } + SoftwareModeGuard guard { *this, device_handle, *target }; - // Read the battery report (ID 0x01), skipping any unsolicited volume - // events (ID 0x0e) the device broadcasts and that may be queued ahead - // of the reply. - std::array response {}; - int attempt = 0; - while (true) { - auto read_result = readHIDTimeout(device_handle, response, hsc_device_timeout); - if (!read_result) { - return read_result.error(); - } - if (response[0] == 0x01) { - break; + // Level 0 switches sidetone off outright rather than turning it down. + if (auto result = writeProperty(device_handle, *target, PROP_SIDETONE_ENABLED, + level == 0 ? 0 : 1); + !result) { + return result.error(); + } + + if (level > 0) { + if (auto result + = writeProperty(device_handle, *target, PROP_SIDETONE_VOLUME, sidetone_value); + !result) { + return result.error(); } - if (++attempt >= MAX_READ_ATTEMPTS) { - return DeviceError::protocolError( - std::format("Unexpected report ID: 0x{:02x}", response[0])); + } + + return SidetoneResult { + .current_level = level, + .min_level = 0, + .max_level = 128, + .device_min = 0, + // The native range is 0-1000, which does not fit the single byte this + // struct exposes, so report it as a percentage instead. + .device_max = 100, + }; + } + + Result setInactiveTime(hid_device* device_handle, uint8_t minutes) override + { + if (minutes > MAX_INACTIVE_MINUTES) { + minutes = MAX_INACTIVE_MINUTES; + } + + auto target = resolveTarget(device_handle); + if (!target) { + return target.error(); + } + + if (auto result = writeProperty(device_handle, *target, PROP_MODE, MODE_SOFTWARE); + !result) { + return result.error(); + } + SoftwareModeGuard guard { *this, device_handle, *target }; + + if (auto result + = writeProperty(device_handle, *target, PROP_SLEEP_ENABLED, minutes == 0 ? 0 : 1); + !result) { + return result.error(); + } + + // The timeout itself is stored in milliseconds. + if (minutes > 0) { + const uint32_t timeout_ms = static_cast(minutes) * 60U * 1000U; + if (auto result + = writeProperty(device_handle, *target, PROP_SLEEP_TIMEOUT, timeout_ms); + !result) { + return result.error(); } } - auto duration = std::chrono::duration_cast( - std::chrono::steady_clock::now() - start_time); + return InactiveTimeResult { + .minutes = minutes, + .min_minutes = 0, + .max_minutes = MAX_INACTIVE_MINUTES, + }; + } - // Byte 1: status flags - // 0xf0 = normal / discharging (confirmed via observation) - // 0x00 = headset offline / not connected to receiver - // Other values TBD (charging state not yet reverse-engineered) - const uint8_t status_byte = response[1]; - const uint8_t battery_level = response[2]; + Result setLights(hid_device* device_handle, bool on) override + { + auto target = resolveTarget(device_handle); + if (!target) { + return target.error(); + } - if (status_byte == 0x00) { - return DeviceError::deviceOffline("Headset not connected to receiver"); + // No scope guard here, deliberately. Hardware mode drives the LEDs from the + // effect the headset runs itself, which paints straight over the frame + // written below - restoring it makes turning the lights off do nothing at + // all. Staying in software mode is what makes the frame stick. 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. + if (auto result = writeProperty(device_handle, *target, PROP_MODE, MODE_SOFTWARE); + !result) { + return result.error(); } - if (battery_level > 100) { - return DeviceError::protocolError( - std::format("Battery percentage out of range: {}", battery_level)); + if (auto result = writeProperty(device_handle, *target, PROP_BRIGHTNESS, BRIGHTNESS_MAX); + !result) { + return result.error(); } - return BatteryResult { - .level_percent = static_cast(battery_level), - .status = BATTERY_AVAILABLE, - .mic_status = MICROPHONE_UNKNOWN, - .raw_data = std::vector(response.begin(), response.end()), - .query_duration = duration, + // This capability is only on/off, so "on" paints every zone static white + // rather than restoring whatever effect the headset was running before - + // that effect is not something the protocol lets us read back and replay. + const uint8_t level = on ? 0xff : 0x00; + if (auto result = writeLighting(device_handle, *target, level, level, level); !result) { + return result.error(); + } + + return LightsResult { + .enabled = on, + .mode = on ? std::optional { "static" } : std::nullopt, }; } Result getCapabilityInfo(enum capabilities cap) override { - return HIDDevice::getCapabilityInfo(cap); + auto info = HIDDevice::getCapabilityInfo(cap); + if (!info) { + return info; + } + + switch (cap) { + case CAP_SIDETONE: + info->parameter + = CapabilityInfo::RangeParam { .min = 0, .max = 128, .step = 1, .units = "level" }; + break; + + case CAP_INACTIVE_TIME: + info->parameter = CapabilityInfo::RangeParam { + .min = 0, .max = MAX_INACTIVE_MINUTES, .step = 1, .units = "minutes" + }; + break; + + default: + break; + } + + return info; } private: - // Reports to skip (e.g. unsolicited volume events) before giving up on the - // battery reply + static constexpr uint8_t REPORT_ID_OUT = 0x02; + static constexpr uint8_t REPORT_ID_IN = 0x01; + + // A wireless receiver relays commands to the headset paired with it, whereas a + // wired headset answers for itself. Addressing the wrong one is not reported as + // an error - the device simply stays silent. + static constexpr uint8_t TARGET_HEADSET = 0x09; + static constexpr uint8_t TARGET_SELF = 0x08; + static constexpr uint8_t REPLY_FROM_HEADSET = 0x01; + static constexpr uint8_t REPLY_FROM_SELF = 0x00; + + static constexpr uint8_t BRAGI_SET = 0x01; + static constexpr uint8_t BRAGI_GET = 0x02; + static constexpr uint8_t BRAGI_CLOSE_HANDLE = 0x05; + static constexpr uint8_t BRAGI_WRITE_DATA = 0x06; + static constexpr uint8_t BRAGI_OPEN_HANDLE = 0x0d; + + // Handle and resource numbering follow ckb-next, which drives the LEDs on + // Corsair's other Bragi devices the same way. + static constexpr uint8_t LIGHTING_HANDLE = 0x00; + static constexpr uint8_t LIGHTING_RESOURCE = 0x01; + // The firmware expects a frame for three LEDs, stored one colour channel at a + // time: every red byte, then every green byte, then every blue byte. + static constexpr uint8_t LIGHTING_ZONES = 3; + static constexpr uint8_t LIGHTING_PAYLOAD_SIZE = LIGHTING_ZONES * 3; + static constexpr size_t LIGHTING_PAYLOAD_OFFSET = 8; + + static constexpr uint8_t STATUS_OK = 0x00; + static constexpr uint8_t STATUS_NO_PROPERTY = 0x05; + + static constexpr uint8_t PROP_BRIGHTNESS = 0x02; + static constexpr uint8_t PROP_MODE = 0x03; + static constexpr uint8_t PROP_SLEEP_ENABLED = 0x0d; + static constexpr uint8_t PROP_SLEEP_TIMEOUT = 0x0e; + static constexpr uint8_t PROP_BATTERY_LEVEL = 0x0f; + static constexpr uint8_t PROP_BATTERY_STATUS = 0x10; + static constexpr uint8_t PROP_SIDETONE_ENABLED = 0x46; + static constexpr uint8_t PROP_SIDETONE_VOLUME = 0x47; + + static constexpr uint16_t MODE_HARDWARE = 1; + static constexpr uint16_t MODE_SOFTWARE = 2; + + static constexpr uint32_t CHARGE_STATE_CHARGING = 1; + + static constexpr uint16_t BRIGHTNESS_MAX = 1000; + static constexpr uint32_t BATTERY_LEVEL_MAX = 1000; + static constexpr uint16_t SIDETONE_DEVICE_MIN = 0; + static constexpr uint16_t SIDETONE_DEVICE_MAX = 1000; + static constexpr uint8_t MAX_INACTIVE_MINUTES = 90; + + static constexpr size_t MSG_SIZE = 64; + // Unsolicited reports (volume events) to skip before giving up on a reply static constexpr int MAX_READ_ATTEMPTS = 8; + // Long enough for a reply from a device that is listening, short enough that + // asking the wrong target does not stall the command + static constexpr int TARGET_PROBE_TIMEOUT_MS = 300; + + /** + * @brief Restores hardware mode when leaving the scope of a write + * + * Settings written in software mode persist, so there is nothing to gain by + * keeping the headset there once the write is done - including a write that + * failed part way through. It drops back on its own after a few minutes of + * silence anyway; handing it back immediately just keeps that window short. + */ + class SoftwareModeGuard { + public: + SoftwareModeGuard(CorsairVirtuosoXT& device, hid_device* device_handle, uint8_t target) + : device_(device) + , device_handle_(device_handle) + , target_(target) + { + } + + SoftwareModeGuard(const SoftwareModeGuard&) = delete; + SoftwareModeGuard& operator=(const SoftwareModeGuard&) = delete; + SoftwareModeGuard(SoftwareModeGuard&&) = delete; + SoftwareModeGuard& operator=(SoftwareModeGuard&&) = delete; + + ~SoftwareModeGuard() + { + // Best effort; there is nothing useful to do if the restore fails. + static_cast( + device_.writeProperty(device_handle_, target_, PROP_MODE, MODE_HARDWARE)); + } + + private: + CorsairVirtuosoXT& device_; + hid_device* device_handle_; + uint8_t target_; + }; + + static constexpr uint8_t replySourceFor(uint8_t target) + { + return target == TARGET_SELF ? REPLY_FROM_SELF : REPLY_FROM_HEADSET; + } + + /** + * @brief Work out whether this device answers for itself or relays to a headset + * + * The registry hands out a single instance per device class and only records + * the product ID it last matched on, so that ID is a hint rather than an + * answer: it goes stale as soon as a wireless receiver and a wired headset are + * plugged in at the same time. The hint is therefore tried first, but only + * accepted once the device has answered on it. + * + * The battery level is used as the probe because a receiver answers identity + * properties for itself even when no headset is paired with it, and would + * otherwise look like a valid target. + */ + [[nodiscard]] Result resolveTarget(hid_device* device_handle) + { + const auto product_id = getMatchedProductId(); + const bool wired = product_id == PID_XT_WIRED || product_id == PID_SE_WIRED; + + const uint8_t hinted = wired ? TARGET_SELF : TARGET_HEADSET; + const uint8_t alternate = wired ? TARGET_HEADSET : TARGET_SELF; + + for (const uint8_t candidate : { hinted, alternate }) { + if (readProperty( + device_handle, candidate, PROP_BATTERY_LEVEL, TARGET_PROBE_TIMEOUT_MS)) { + return candidate; + } + } + + return DeviceError::deviceOffline("Headset not connected or powered off"); + } + + /** + * @brief Read a property + * + * @return The little-endian value, or an error if the device stays silent or + * the property is unknown to this firmware + */ + [[nodiscard]] Result readProperty( + hid_device* device_handle, uint8_t target, uint8_t property, int timeout_ms = 0) + { + std::array request { REPORT_ID_OUT, target, BRAGI_GET, property }; + if (auto result = writeHID(device_handle, request, MSG_SIZE); !result) { + return result.error(); + } + + auto response = readReply(device_handle, target, BRAGI_GET, + timeout_ms == 0 ? hsc_device_timeout : timeout_ms); + if (!response) { + return response.error(); + } + + const auto& data = *response; + if (data[3] == STATUS_NO_PROPERTY) { + return DeviceError::notSupported( + std::format("Property 0x{:02x} not supported by this firmware", property)); + } + if (data[3] != STATUS_OK) { + return DeviceError::protocolError( + std::format("Read of property 0x{:02x} failed with status 0x{:02x}", property, + data[3])); + } + + return static_cast(data[4]) | (static_cast(data[5]) << 8) + | (static_cast(data[6]) << 16) | (static_cast(data[7]) << 24); + } + + /** + * @brief Write a property + * + * Requires software mode; outside it the headset answers with status 0x09. + */ + [[nodiscard]] Result writeProperty( + hid_device* device_handle, uint8_t target, uint8_t property, uint32_t value) + { + std::array request { REPORT_ID_OUT, target, BRAGI_SET, property, 0x00, + static_cast(value & 0xFF), static_cast((value >> 8) & 0xFF), + static_cast((value >> 16) & 0xFF), static_cast((value >> 24) & 0xFF) }; + if (auto result = writeHID(device_handle, request, MSG_SIZE); !result) { + return result.error(); + } + + auto response = readReply(device_handle, target, BRAGI_SET, hsc_device_timeout); + if (!response) { + return response.error(); + } + + if ((*response)[3] != STATUS_OK) { + return DeviceError::protocolError( + std::format("Write of property 0x{:02x} rejected with status 0x{:02x}", property, + (*response)[3])); + } + return {}; + } + + /** + * @brief Paint every LED zone one colour + * + * Lighting is not a property but a block of data, so it goes through the + * open/write/close sequence the Bragi protocol uses for bulk transfers. The + * headset has to already be in software mode for the frame to be applied. + */ + [[nodiscard]] Result writeLighting( + hid_device* device_handle, uint8_t target, uint8_t red, uint8_t green, uint8_t blue) + { + std::array open_request { REPORT_ID_OUT, target, BRAGI_OPEN_HANDLE, + LIGHTING_HANDLE, LIGHTING_RESOURCE, 0x00 }; + if (auto result = sendLightingCommand(device_handle, target, open_request, + BRAGI_OPEN_HANDLE, "open lighting handle"); + !result) { + return result.error(); + } + + std::array write_request { REPORT_ID_OUT, target, BRAGI_WRITE_DATA, + LIGHTING_HANDLE, LIGHTING_PAYLOAD_SIZE, 0x00, 0x00, 0x00 }; + for (uint8_t zone = 0; zone < LIGHTING_ZONES; ++zone) { + write_request[LIGHTING_PAYLOAD_OFFSET + zone] = red; + write_request[LIGHTING_PAYLOAD_OFFSET + LIGHTING_ZONES + zone] = green; + write_request[LIGHTING_PAYLOAD_OFFSET + (2 * LIGHTING_ZONES) + zone] = blue; + } + auto write_result = sendLightingCommand( + device_handle, target, write_request, BRAGI_WRITE_DATA, "write lighting frame"); + + // Close the handle even if the frame was rejected, so a failure does not + // leave the transfer open and block the next one. + std::array close_request { REPORT_ID_OUT, target, BRAGI_CLOSE_HANDLE, + 0x01, LIGHTING_HANDLE }; + auto close_result = sendLightingCommand( + device_handle, target, close_request, BRAGI_CLOSE_HANDLE, "close lighting handle"); + + if (!write_result) { + return write_result.error(); + } + if (!close_result) { + return close_result.error(); + } + return {}; + } + + /** + * @brief Send one step of the lighting transfer and check that it was accepted + */ + [[nodiscard]] Result sendLightingCommand(hid_device* device_handle, uint8_t target, + std::span request, uint8_t command, std::string_view step) + { + if (auto result = writeHID(device_handle, request, MSG_SIZE); !result) { + return result.error(); + } + + auto response = readReply(device_handle, target, command, hsc_device_timeout); + if (!response) { + return response.error(); + } + if ((*response)[3] != STATUS_OK) { + return DeviceError::protocolError( + std::format("Failed to {} (status 0x{:02x})", step, (*response)[3])); + } + return {}; + } + + /** + * @brief Read the reply to a command, skipping unsolicited reports + * + * Volume events arrive on report 0x0e, and when a receiver is in play it also + * answers some commands on its own behalf, so both are filtered out here. + */ + [[nodiscard]] Result> readReply( + hid_device* device_handle, uint8_t target, uint8_t command, int timeout_ms) + { + std::array response {}; + for (int attempt = 0; attempt < MAX_READ_ATTEMPTS; ++attempt) { + if (auto result = readHIDTimeout(device_handle, response, timeout_ms); !result) { + // A headset that is powered off or out of range never answers. + if (result.error().code == DeviceError::Code::Timeout) { + return DeviceError::deviceOffline("Headset not connected or powered off"); + } + return result.error(); + } + + if (response[0] == REPORT_ID_IN && response[1] == replySourceFor(target) + && response[2] == command) { + return response; + } + } + + return DeviceError::protocolError(std::format( + "No reply to command 0x{:02x} after {} reports", command, MAX_READ_ATTEMPTS)); + } }; } // namespace headsetcontrol From 9f54a74dc5d22b18f0449340cdc41a53a5365c24 Mon Sep 17 00:00:00 2001 From: "Gabriel A. Devenyi" Date: Sun, 13 Sep 2026 22:35:08 -0400 Subject: [PATCH 2/3] corsair_virtuoso_xt: switch lights with brightness, not a painted frame Review feedback on #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 (#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 Claude-Session: https://claude.ai/code/session_01BHCVMmAAeV5WGP7oYZsk3T --- lib/devices/corsair_virtuoso_xt.hpp | 216 +++++++++------------------- 1 file changed, 68 insertions(+), 148 deletions(-) diff --git a/lib/devices/corsair_virtuoso_xt.hpp b/lib/devices/corsair_virtuoso_xt.hpp index 1b31cd6..d982f97 100644 --- a/lib/devices/corsair_virtuoso_xt.hpp +++ b/lib/devices/corsair_virtuoso_xt.hpp @@ -7,8 +7,6 @@ #include #include #include -#include -#include #include using namespace std::string_view_literals; @@ -42,11 +40,11 @@ namespace headsetcontrol { * switching back, so they are bracketed by a scope guard that hands the headset * straight back to hardware mode instead of parking it in software mode. * - * Lighting is not a property but a block of data pushed through the protocol's - * open/write/close handle sequence. The frame written that way sticks: the LEDs - * keep showing it even once the headset drops back to hardware mode, which it - * does on its own shortly after the host stops talking to it. What hardware mode - * does not do is resume whatever effect was running beforehand. + * Lights are switched with the brightness property (0x02) rather than by painting + * a frame. Brightness is the headset's own persisted setting and gates whatever + * effect it is running, so 0 turns the LEDs off and full brightness brings back + * the user's own effect. On the XT this was tested on, brightness 0 survived a + * power cycle. * * The device also broadcasts unsolicited volume events on report 0x0e, which have * to be skipped when looking for a reply. @@ -95,28 +93,24 @@ class CorsairVirtuosoXT : public CorsairDevice { { auto start_time = std::chrono::steady_clock::now(); - auto target = resolveTarget(device_handle); - if (!target) { - return target.error(); - } - - // Reading does not need software mode, which keeps the headset from - // producing an audible pop just to report its battery level. - auto level = readProperty(device_handle, *target, PROP_BATTERY_LEVEL); - if (!level) { - return level.error(); + // Resolving the target already reads the battery level. Neither that nor + // the charge state needs software mode, which keeps the headset from + // producing an audible pop just to report its battery. + auto resolved = resolveTarget(device_handle); + if (!resolved) { + return resolved.error(); } + const uint32_t level = resolved->battery_level; // The level is reported in tenths of a percent. - if (*level > BATTERY_LEVEL_MAX) { - return DeviceError::protocolError( - std::format("Battery level out of range: {}", *level)); + if (level > BATTERY_LEVEL_MAX) { + return DeviceError::protocolError(std::format("Battery level out of range: {}", level)); } // Charge state is a separate property; treat it as advisory so that a // firmware which does not implement it still yields a usable level. auto status = BATTERY_AVAILABLE; - if (auto charge_state = readProperty(device_handle, *target, PROP_BATTERY_STATUS); + if (auto charge_state = readProperty(device_handle, resolved->target, PROP_BATTERY_STATUS); charge_state && *charge_state == CHARGE_STATE_CHARGING) { status = BATTERY_CHARGING; } @@ -125,7 +119,7 @@ class CorsairVirtuosoXT : public CorsairDevice { std::chrono::steady_clock::now() - start_time); return BatteryResult { - .level_percent = static_cast(*level / 10), + .level_percent = static_cast(level / 10), .status = status, .mic_status = MICROPHONE_UNKNOWN, .query_duration = duration, @@ -140,19 +134,20 @@ class CorsairVirtuosoXT : public CorsairDevice { const auto sidetone_value = static_cast(round_to_multiples(mapped_level, 10)); - auto target = resolveTarget(device_handle); - if (!target) { - return target.error(); + auto resolved = resolveTarget(device_handle); + if (!resolved) { + return resolved.error(); } + const uint8_t target = resolved->target; - if (auto result = writeProperty(device_handle, *target, PROP_MODE, MODE_SOFTWARE); + if (auto result = writeProperty(device_handle, target, PROP_MODE, MODE_SOFTWARE); !result) { return result.error(); } - SoftwareModeGuard guard { *this, device_handle, *target }; + SoftwareModeGuard guard { *this, device_handle, target }; // Level 0 switches sidetone off outright rather than turning it down. - if (auto result = writeProperty(device_handle, *target, PROP_SIDETONE_ENABLED, + if (auto result = writeProperty(device_handle, target, PROP_SIDETONE_ENABLED, level == 0 ? 0 : 1); !result) { return result.error(); @@ -160,7 +155,7 @@ class CorsairVirtuosoXT : public CorsairDevice { if (level > 0) { if (auto result - = writeProperty(device_handle, *target, PROP_SIDETONE_VOLUME, sidetone_value); + = writeProperty(device_handle, target, PROP_SIDETONE_VOLUME, sidetone_value); !result) { return result.error(); } @@ -183,19 +178,20 @@ class CorsairVirtuosoXT : public CorsairDevice { minutes = MAX_INACTIVE_MINUTES; } - auto target = resolveTarget(device_handle); - if (!target) { - return target.error(); + auto resolved = resolveTarget(device_handle); + if (!resolved) { + return resolved.error(); } + const uint8_t target = resolved->target; - if (auto result = writeProperty(device_handle, *target, PROP_MODE, MODE_SOFTWARE); + if (auto result = writeProperty(device_handle, target, PROP_MODE, MODE_SOFTWARE); !result) { return result.error(); } - SoftwareModeGuard guard { *this, device_handle, *target }; + SoftwareModeGuard guard { *this, device_handle, target }; if (auto result - = writeProperty(device_handle, *target, PROP_SLEEP_ENABLED, minutes == 0 ? 0 : 1); + = writeProperty(device_handle, target, PROP_SLEEP_ENABLED, minutes == 0 ? 0 : 1); !result) { return result.error(); } @@ -204,7 +200,7 @@ class CorsairVirtuosoXT : public CorsairDevice { if (minutes > 0) { const uint32_t timeout_ms = static_cast(minutes) * 60U * 1000U; if (auto result - = writeProperty(device_handle, *target, PROP_SLEEP_TIMEOUT, timeout_ms); + = writeProperty(device_handle, target, PROP_SLEEP_TIMEOUT, timeout_ms); !result) { return result.error(); } @@ -219,39 +215,28 @@ class CorsairVirtuosoXT : public CorsairDevice { Result setLights(hid_device* device_handle, bool on) override { - auto target = resolveTarget(device_handle); - if (!target) { - return target.error(); + auto resolved = resolveTarget(device_handle); + if (!resolved) { + return resolved.error(); } + const uint8_t target = resolved->target; - // No scope guard here, deliberately. Hardware mode drives the LEDs from the - // effect the headset runs itself, which paints straight over the frame - // written below - restoring it makes turning the lights off do nothing at - // all. Staying in software mode is what makes the frame stick. 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. - if (auto result = writeProperty(device_handle, *target, PROP_MODE, MODE_SOFTWARE); + if (auto result = writeProperty(device_handle, target, PROP_MODE, MODE_SOFTWARE); !result) { return result.error(); } + SoftwareModeGuard guard { *this, device_handle, target }; - if (auto result = writeProperty(device_handle, *target, PROP_BRIGHTNESS, BRIGHTNESS_MAX); + // Brightness is the headset's own persisted setting rather than a frame we + // paint, so it gates whatever effect the headset is running: 0 switches the + // LEDs off, full brightness brings back the user's own effect. + if (auto result + = writeProperty(device_handle, target, PROP_BRIGHTNESS, on ? BRIGHTNESS_MAX : 0); !result) { return result.error(); } - // This capability is only on/off, so "on" paints every zone static white - // rather than restoring whatever effect the headset was running before - - // that effect is not something the protocol lets us read back and replay. - const uint8_t level = on ? 0xff : 0x00; - if (auto result = writeLighting(device_handle, *target, level, level, level); !result) { - return result.error(); - } - - return LightsResult { - .enabled = on, - .mode = on ? std::optional { "static" } : std::nullopt, - }; + return LightsResult { .enabled = on }; } Result getCapabilityInfo(enum capabilities cap) override @@ -292,21 +277,8 @@ class CorsairVirtuosoXT : public CorsairDevice { static constexpr uint8_t REPLY_FROM_HEADSET = 0x01; static constexpr uint8_t REPLY_FROM_SELF = 0x00; - static constexpr uint8_t BRAGI_SET = 0x01; - static constexpr uint8_t BRAGI_GET = 0x02; - static constexpr uint8_t BRAGI_CLOSE_HANDLE = 0x05; - static constexpr uint8_t BRAGI_WRITE_DATA = 0x06; - static constexpr uint8_t BRAGI_OPEN_HANDLE = 0x0d; - - // Handle and resource numbering follow ckb-next, which drives the LEDs on - // Corsair's other Bragi devices the same way. - static constexpr uint8_t LIGHTING_HANDLE = 0x00; - static constexpr uint8_t LIGHTING_RESOURCE = 0x01; - // The firmware expects a frame for three LEDs, stored one colour channel at a - // time: every red byte, then every green byte, then every blue byte. - static constexpr uint8_t LIGHTING_ZONES = 3; - static constexpr uint8_t LIGHTING_PAYLOAD_SIZE = LIGHTING_ZONES * 3; - static constexpr size_t LIGHTING_PAYLOAD_OFFSET = 8; + static constexpr uint8_t BRAGI_SET = 0x01; + static constexpr uint8_t BRAGI_GET = 0x02; static constexpr uint8_t STATUS_OK = 0x00; static constexpr uint8_t STATUS_NO_PROPERTY = 0x05; @@ -335,16 +307,23 @@ class CorsairVirtuosoXT : public CorsairDevice { // Unsolicited reports (volume events) to skip before giving up on a reply static constexpr int MAX_READ_ATTEMPTS = 8; // Long enough for a reply from a device that is listening, short enough that - // asking the wrong target does not stall the command + // asking the wrong target does not stall the command. A headset slower than + // this is reported offline, and its reply can still turn up afterwards: replies + // carry no property ID, so the next read on this handle for the same target + // and command would take that late reply for its own. static constexpr int TARGET_PROBE_TIMEOUT_MS = 300; /** * @brief Restores hardware mode when leaving the scope of a write * - * Settings written in software mode persist, so there is nothing to gain by - * keeping the headset there once the write is done - including a write that - * failed part way through. It drops back on its own after a few minutes of - * silence anyway; handing it back immediately just keeps that window short. + * Settings written in software mode persist - sidetone, the sleep timer and + * brightness all survived a power cycle on the XT this was tested on - so there + * is nothing to gain by keeping the headset there once the write is done, + * including a write that failed part way through. + * + * That XT also dropped back to hardware mode by itself after a few minutes + * without host traffic, but that was measured on one unit and nothing here + * relies on it. */ class SoftwareModeGuard { public: @@ -389,9 +368,15 @@ class CorsairVirtuosoXT : public CorsairDevice { * * The battery level is used as the probe because a receiver answers identity * properties for itself even when no headset is paired with it, and would - * otherwise look like a valid target. + * otherwise look like a valid target. The level is handed back so that + * getBattery() does not have to ask for it a second time. */ - [[nodiscard]] Result resolveTarget(hid_device* device_handle) + struct ResolvedTarget { + uint8_t target; + uint32_t battery_level; + }; + + [[nodiscard]] Result resolveTarget(hid_device* device_handle) { const auto product_id = getMatchedProductId(); const bool wired = product_id == PID_XT_WIRED || product_id == PID_SE_WIRED; @@ -400,9 +385,9 @@ class CorsairVirtuosoXT : public CorsairDevice { const uint8_t alternate = wired ? TARGET_HEADSET : TARGET_SELF; for (const uint8_t candidate : { hinted, alternate }) { - if (readProperty( + if (auto level = readProperty( device_handle, candidate, PROP_BATTERY_LEVEL, TARGET_PROBE_TIMEOUT_MS)) { - return candidate; + return ResolvedTarget { .target = candidate, .battery_level = *level }; } } @@ -472,71 +457,6 @@ class CorsairVirtuosoXT : public CorsairDevice { return {}; } - /** - * @brief Paint every LED zone one colour - * - * Lighting is not a property but a block of data, so it goes through the - * open/write/close sequence the Bragi protocol uses for bulk transfers. The - * headset has to already be in software mode for the frame to be applied. - */ - [[nodiscard]] Result writeLighting( - hid_device* device_handle, uint8_t target, uint8_t red, uint8_t green, uint8_t blue) - { - std::array open_request { REPORT_ID_OUT, target, BRAGI_OPEN_HANDLE, - LIGHTING_HANDLE, LIGHTING_RESOURCE, 0x00 }; - if (auto result = sendLightingCommand(device_handle, target, open_request, - BRAGI_OPEN_HANDLE, "open lighting handle"); - !result) { - return result.error(); - } - - std::array write_request { REPORT_ID_OUT, target, BRAGI_WRITE_DATA, - LIGHTING_HANDLE, LIGHTING_PAYLOAD_SIZE, 0x00, 0x00, 0x00 }; - for (uint8_t zone = 0; zone < LIGHTING_ZONES; ++zone) { - write_request[LIGHTING_PAYLOAD_OFFSET + zone] = red; - write_request[LIGHTING_PAYLOAD_OFFSET + LIGHTING_ZONES + zone] = green; - write_request[LIGHTING_PAYLOAD_OFFSET + (2 * LIGHTING_ZONES) + zone] = blue; - } - auto write_result = sendLightingCommand( - device_handle, target, write_request, BRAGI_WRITE_DATA, "write lighting frame"); - - // Close the handle even if the frame was rejected, so a failure does not - // leave the transfer open and block the next one. - std::array close_request { REPORT_ID_OUT, target, BRAGI_CLOSE_HANDLE, - 0x01, LIGHTING_HANDLE }; - auto close_result = sendLightingCommand( - device_handle, target, close_request, BRAGI_CLOSE_HANDLE, "close lighting handle"); - - if (!write_result) { - return write_result.error(); - } - if (!close_result) { - return close_result.error(); - } - return {}; - } - - /** - * @brief Send one step of the lighting transfer and check that it was accepted - */ - [[nodiscard]] Result sendLightingCommand(hid_device* device_handle, uint8_t target, - std::span request, uint8_t command, std::string_view step) - { - if (auto result = writeHID(device_handle, request, MSG_SIZE); !result) { - return result.error(); - } - - auto response = readReply(device_handle, target, command, hsc_device_timeout); - if (!response) { - return response.error(); - } - if ((*response)[3] != STATUS_OK) { - return DeviceError::protocolError( - std::format("Failed to {} (status 0x{:02x})", step, (*response)[3])); - } - return {}; - } - /** * @brief Read the reply to a command, skipping unsolicited reports * From 8feff1207bb6980561de8a9dfc5ab7b75993f49f Mon Sep 17 00:00:00 2001 From: "Gabriel A. Devenyi" Date: Mon, 14 Sep 2026 18:14:53 -0400 Subject: [PATCH 3/3] corsair_virtuoso_xt: discard stale replies, propagate real probe errors Two findings from Copilot's review of the stacked #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 Claude-Session: https://claude.ai/code/session_01BHCVMmAAeV5WGP7oYZsk3T --- lib/devices/corsair_virtuoso_xt.hpp | 79 +++++++--- tests/CMakeLists.txt | 1 + tests/test_corsair_virtuoso.cpp | 219 ++++++++++++++++++++++++++++ tests/test_runner.cpp | 3 + 4 files changed, 284 insertions(+), 18 deletions(-) create mode 100644 tests/test_corsair_virtuoso.cpp diff --git a/lib/devices/corsair_virtuoso_xt.hpp b/lib/devices/corsair_virtuoso_xt.hpp index d982f97..f323376 100644 --- a/lib/devices/corsair_virtuoso_xt.hpp +++ b/lib/devices/corsair_virtuoso_xt.hpp @@ -7,6 +7,7 @@ #include #include #include +#include #include using namespace std::string_view_literals; @@ -308,10 +309,11 @@ class CorsairVirtuosoXT : public CorsairDevice { static constexpr int MAX_READ_ATTEMPTS = 8; // Long enough for a reply from a device that is listening, short enough that // asking the wrong target does not stall the command. A headset slower than - // this is reported offline, and its reply can still turn up afterwards: replies - // carry no property ID, so the next read on this handle for the same target - // and command would take that late reply for its own. + // this is reported offline. static constexpr int TARGET_PROBE_TIMEOUT_MS = 300; + // Upper bound on queued reports thrown away before a request, so a device + // that never stops sending cannot stall it + static constexpr int MAX_STALE_REPORTS = 32; /** * @brief Restores hardware mode when leaving the scope of a write @@ -385,10 +387,20 @@ class CorsairVirtuosoXT : public CorsairDevice { const uint8_t alternate = wired ? TARGET_HEADSET : TARGET_SELF; for (const uint8_t candidate : { hinted, alternate }) { - if (auto level = readProperty( - device_handle, candidate, PROP_BATTERY_LEVEL, TARGET_PROBE_TIMEOUT_MS)) { + auto level + = readProperty(device_handle, candidate, PROP_BATTERY_LEVEL, TARGET_PROBE_TIMEOUT_MS); + if (level) { return ResolvedTarget { .target = candidate, .battery_level = *level }; } + + // Silence means nothing is listening on that target, and "no such + // property" is a receiver answering for itself with no headset behind + // it. Anything else - a failed HID write, an unexpected status - is a + // real fault, and reporting it as offline would hide it. + const auto code = level.error().code; + if (code != DeviceError::Code::DeviceOffline && code != DeviceError::Code::NotSupported) { + return level.error(); + } } return DeviceError::deviceOffline("Headset not connected or powered off"); @@ -403,12 +415,8 @@ class CorsairVirtuosoXT : public CorsairDevice { [[nodiscard]] Result readProperty( hid_device* device_handle, uint8_t target, uint8_t property, int timeout_ms = 0) { - std::array request { REPORT_ID_OUT, target, BRAGI_GET, property }; - if (auto result = writeHID(device_handle, request, MSG_SIZE); !result) { - return result.error(); - } - - auto response = readReply(device_handle, target, BRAGI_GET, + const std::array request { REPORT_ID_OUT, target, BRAGI_GET, property }; + auto response = transact(device_handle, target, request, BRAGI_GET, timeout_ms == 0 ? hsc_device_timeout : timeout_ms); if (!response) { return response.error(); @@ -437,14 +445,10 @@ class CorsairVirtuosoXT : public CorsairDevice { [[nodiscard]] Result writeProperty( hid_device* device_handle, uint8_t target, uint8_t property, uint32_t value) { - std::array request { REPORT_ID_OUT, target, BRAGI_SET, property, 0x00, - static_cast(value & 0xFF), static_cast((value >> 8) & 0xFF), + const std::array request { REPORT_ID_OUT, target, BRAGI_SET, property, + 0x00, static_cast(value & 0xFF), static_cast((value >> 8) & 0xFF), static_cast((value >> 16) & 0xFF), static_cast((value >> 24) & 0xFF) }; - if (auto result = writeHID(device_handle, request, MSG_SIZE); !result) { - return result.error(); - } - - auto response = readReply(device_handle, target, BRAGI_SET, hsc_device_timeout); + auto response = transact(device_handle, target, request, BRAGI_SET, hsc_device_timeout); if (!response) { return response.error(); } @@ -457,6 +461,45 @@ class CorsairVirtuosoXT : public CorsairDevice { return {}; } + /** + * @brief Send a request and wait for its reply + * + * Replies carry no property or handle ID, only who sent them and the command + * they answer. A reply that arrives after its request has already timed out + * would otherwise be taken by the next request of the same kind, so anything + * still queued is thrown away first. + */ + [[nodiscard]] Result> transact(hid_device* device_handle, + uint8_t target, std::span request, uint8_t command, int timeout_ms) + { + if (auto result = discardStaleReports(device_handle); !result) { + return result.error(); + } + if (auto result = writeHID(device_handle, request, MSG_SIZE); !result) { + return result.error(); + } + return readReply(device_handle, target, command, timeout_ms); + } + + /** + * @brief Throw away reports already waiting on the handle, without blocking + */ + [[nodiscard]] Result discardStaleReports(hid_device* device_handle) + { + std::array stale {}; + for (int discarded = 0; discarded < MAX_STALE_REPORTS; ++discarded) { + auto result = readHIDTimeout(device_handle, stale, 0); + if (!result) { + // An empty queue shows up as a timeout on a non-blocking read. + if (result.error().code == DeviceError::Code::Timeout) { + return {}; + } + return result.error(); + } + } + return {}; + } + /** * @brief Read the reply to a command, skipping unsolicited reports * diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 88ce025..aeff20c 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -15,6 +15,7 @@ set(TEST_SOURCES ${CMAKE_CURRENT_SOURCE_DIR}/test_library_api.cpp ${CMAKE_CURRENT_SOURCE_DIR}/test_protocols.cpp ${CMAKE_CURRENT_SOURCE_DIR}/test_steelseries_sidetone.cpp + ${CMAKE_CURRENT_SOURCE_DIR}/test_corsair_virtuoso.cpp ) # Export to parent scope diff --git a/tests/test_corsair_virtuoso.cpp b/tests/test_corsair_virtuoso.cpp new file mode 100644 index 0000000..12408a0 --- /dev/null +++ b/tests/test_corsair_virtuoso.cpp @@ -0,0 +1,219 @@ +#include "devices/corsair_virtuoso_xt.hpp" + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +namespace headsetcontrol::testing { + +class VirtuosoTestFailure : public std::runtime_error { +public: + explicit VirtuosoTestFailure(const std::string& message) + : std::runtime_error(message) + { + } +}; + +#define VIRTUOSO_ASSERT(condition, message) \ + do { \ + if (!(condition)) \ + throw VirtuosoTestFailure(std::string("Assertion failed: ") + (message)); \ + } while (false) + +/** + * @brief Mock HID interface that knows when each report arrives + * + * A report only becomes readable once a given number of requests have been + * written, which is what separates a stale reply (already queued before a + * request goes out) from the reply to that request. + */ +class VirtuosoMockHID final : public HIDInterface { +public: + struct Report { + size_t after_write; // readable once this many writes have happened + std::vector data; + }; + + std::deque reads; + std::vector> writes; + size_t fail_on_write = 0; // 1-based; 0 means never + + Result write(hid_device*, std::span data) override + { + writes.emplace_back(data.begin(), data.end()); + if (fail_on_write != 0 && writes.size() == fail_on_write) + return DeviceError::hidError("Simulated HID write error"); + return {}; + } + + Result write(hid_device* handle, std::span data, size_t size) override + { + std::vector padded(size); + std::copy_n(data.begin(), std::min(data.size(), size), padded.begin()); + return write(handle, padded); + } + + Result readTimeout(hid_device*, std::span data, int) override + { + if (reads.empty() || reads.front().after_write > writes.size()) + return DeviceError::timeout("Simulated timeout"); + + const auto report = std::move(reads.front()); + reads.pop_front(); + const size_t size = std::min(report.data.size(), data.size()); + std::copy_n(report.data.begin(), size, data.begin()); + return size; + } + + Result sendFeatureReport(hid_device* handle, std::span data) override + { + return write(handle, data); + } + + Result sendFeatureReport(hid_device* handle, std::span data, size_t size) override + { + return write(handle, data, size); + } + + Result getFeatureReport(hid_device*, std::span) override + { + return DeviceError::notSupported("Not used by this test"); + } + + Result getInputReport(hid_device*, std::span) override + { + return DeviceError::notSupported("Not used by this test"); + } + + /// Queue a GET/SET reply: [report 0x01][source][command][status][value LE] + void reply(size_t after_write, uint8_t source, uint8_t command, uint8_t status, uint32_t value = 0) + { + reads.push_back({ after_write, + { 0x01, source, command, status, static_cast(value & 0xFF), + static_cast((value >> 8) & 0xFF), static_cast((value >> 16) & 0xFF), + static_cast((value >> 24) & 0xFF) } }); + } +}; + +class TestableVirtuoso final : public CorsairVirtuosoXT { +public: + TestableVirtuoso(VirtuosoMockHID& hid, uint16_t product_id) + : hid_(hid) + { + setMatchedProductId(product_id); + } + +protected: + HIDInterface& getHIDInterface() const override { return hid_; } + +private: + VirtuosoMockHID& hid_; +}; + +constexpr uint16_t PID_WIRELESS = 0x0a64; +constexpr uint16_t PID_WIRED = 0x0a62; +constexpr uint8_t FROM_HEADSET = 0x01; +constexpr uint8_t FROM_SELF = 0x00; +constexpr uint8_t GET = 0x02; +constexpr uint8_t STATUS_OK = 0x00; +constexpr uint8_t STATUS_NO_PROP = 0x05; +constexpr uint32_t CHARGING = 1; +constexpr uint32_t DISCHARGING = 2; + +void testVirtuosoBattery() +{ + std::cout << " Testing battery over both targets..." << std::endl; + + // Wireless: the receiver relays to the headset behind it. + VirtuosoMockHID wireless; + TestableVirtuoso wireless_device(wireless, PID_WIRELESS); + wireless.reply(1, FROM_HEADSET, GET, STATUS_OK, 810); + wireless.reply(2, FROM_HEADSET, GET, STATUS_OK, CHARGING); + auto battery = wireless_device.getBattery(nullptr); + VIRTUOSO_ASSERT(battery.hasValue(), "wireless battery should succeed"); + VIRTUOSO_ASSERT(battery->level_percent == 81, "level is reported in tenths of a percent"); + VIRTUOSO_ASSERT(battery->status == BATTERY_CHARGING, "charge state 1 means charging"); + VIRTUOSO_ASSERT(wireless.writes.size() == 2, "the probed level should not be read a second time"); + VIRTUOSO_ASSERT(wireless.writes[0][1] == 0x09, "wireless requests target the headset"); + + // Wired: the headset answers for itself. + VirtuosoMockHID wired; + TestableVirtuoso wired_device(wired, PID_WIRED); + wired.reply(1, FROM_SELF, GET, STATUS_OK, 1000); + wired.reply(2, FROM_SELF, GET, STATUS_OK, DISCHARGING); + battery = wired_device.getBattery(nullptr); + VIRTUOSO_ASSERT(battery.hasValue(), "wired battery should succeed"); + VIRTUOSO_ASSERT(battery->level_percent == 100 && battery->status == BATTERY_AVAILABLE, + "wired battery should read 100% discharging"); + VIRTUOSO_ASSERT(wired.writes[0][1] == 0x08, "wired requests target the device itself"); + + std::cout << " OK battery over both targets" << std::endl; +} + +void testVirtuosoStaleReplyIsDiscarded() +{ + std::cout << " Testing a late reply is not taken for a new request's..." << std::endl; + + // A charge-state reply left over from an earlier request that timed out is + // already queued before getBattery() sends anything. Replies carry no + // property ID, so without discarding it the level probe would read 1 as the + // battery level. + VirtuosoMockHID hid; + TestableVirtuoso device(hid, PID_WIRELESS); + hid.reply(0, FROM_HEADSET, GET, STATUS_OK, CHARGING); + hid.reply(1, FROM_HEADSET, GET, STATUS_OK, 810); + hid.reply(2, FROM_HEADSET, GET, STATUS_OK, DISCHARGING); + + auto battery = device.getBattery(nullptr); + VIRTUOSO_ASSERT(battery.hasValue(), "battery should succeed"); + VIRTUOSO_ASSERT(battery->level_percent == 81, "the stale reply must not be read as the level"); + VIRTUOSO_ASSERT(battery->status == BATTERY_AVAILABLE, "the charge state must come from its own reply"); + + std::cout << " OK late reply discarded" << std::endl; +} + +void testVirtuosoTargetResolutionErrors() +{ + std::cout << " Testing which probe failures mean \"wrong target\"..." << std::endl; + + // A receiver with its headset off: the headset stays silent, and the + // receiver answers the battery probe for itself with "no such property". + // Both mean nothing is there, so the headset is offline. + VirtuosoMockHID receiver_only; + TestableVirtuoso receiver_device(receiver_only, PID_WIRELESS); + receiver_only.reply(2, FROM_SELF, GET, STATUS_NO_PROP); + auto offline = receiver_device.getBattery(nullptr); + VIRTUOSO_ASSERT(offline.hasError(), "no headset should fail"); + VIRTUOSO_ASSERT(offline.error().code == DeviceError::Code::DeviceOffline, + "a silent headset behind a receiver should be reported offline, not unsupported"); + VIRTUOSO_ASSERT(receiver_only.writes.size() == 2, "both targets should have been probed"); + + // A HID failure is a real fault and must not be dressed up as offline. + VirtuosoMockHID broken; + TestableVirtuoso broken_device(broken, PID_WIRELESS); + broken.fail_on_write = 1; + auto failure = broken_device.getBattery(nullptr); + VIRTUOSO_ASSERT(failure.hasError(), "a failed write should fail"); + VIRTUOSO_ASSERT(failure.error().code == DeviceError::Code::HIDError, + "a HID error during target resolution should be propagated"); + VIRTUOSO_ASSERT(broken.writes.size() == 1, "a HID error should not fall through to the other target"); + + std::cout << " OK target resolution errors" << std::endl; +} + +void runAllCorsairVirtuosoTests() +{ + std::cout << "\n=== Corsair Virtuoso Tests ===" << std::endl; + testVirtuosoBattery(); + testVirtuosoStaleReplyIsDiscarded(); + testVirtuosoTargetResolutionErrors(); + std::cout << " Corsair Virtuoso tests passed" << std::endl; +} + +} // namespace headsetcontrol::testing diff --git a/tests/test_runner.cpp b/tests/test_runner.cpp index f9555c8..bc0272c 100644 --- a/tests/test_runner.cpp +++ b/tests/test_runner.cpp @@ -27,6 +27,7 @@ void runAllStringEscapingTests(); void runAllLibraryApiTests(); void runAllProtocolTests(); void runAllSteelSeriesSidetoneTests(); +void runAllCorsairVirtuosoTests(); } int main() @@ -70,6 +71,8 @@ int main() headsetcontrol::testing::runAllSteelSeriesSidetoneTests(); + headsetcontrol::testing::runAllCorsairVirtuosoTests(); + std::cout << "\n====================================================================" << std::endl; std::cout << " All tests passed successfully! " << std::endl; std::cout << "====================================================================" << std::endl;