Skip to content

Commit 1d046e2

Browse files
ChrisLauinger77ChrisLauinger77
committed
Improve Logitech G PRO X 2 battery reporting and Centurion protocol robustness
Refines the Logitech G PRO X 2 implementation by adding proper tracking for battery query duration and expanding charging state detection. It also introduces a size limit check for Centurion bridge messages, improves equalizer info caching logic, and adds comprehensive unit tests for these protocol behaviors.
1 parent dde5c9a commit 1d046e2

3 files changed

Lines changed: 110 additions & 35 deletions

File tree

‎lib/devices/logitech_gpro_x2_lightspeed.hpp‎

Lines changed: 41 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,10 @@ class LogitechGProX2Lightspeed : public protocols::LogitechCenturionProtocol {
7171

7272
std::optional<EqualizerInfo> getEqualizerInfo() const override
7373
{
74+
if (!has_cached_equalizer_info_) {
75+
return std::nullopt;
76+
}
77+
7478
return EqualizerInfo {
7579
.bands_count = cached_band_count_,
7680
.bands_baseline = 0,
@@ -82,6 +86,10 @@ class LogitechGProX2Lightspeed : public protocols::LogitechCenturionProtocol {
8286

8387
std::optional<ParametricEqualizerInfo> getParametricEqualizerInfo() const override
8488
{
89+
if (!has_cached_equalizer_info_) {
90+
return std::nullopt;
91+
}
92+
8593
return ParametricEqualizerInfo {
8694
.bands_count = cached_band_count_,
8795
.gain_base = 0.0f,
@@ -113,6 +121,7 @@ class LogitechGProX2Lightspeed : public protocols::LogitechCenturionProtocol {
113121

114122
Result<BatteryResult> getBattery(hid_device* device_handle) override
115123
{
124+
auto centurion_start_time = std::chrono::steady_clock::now();
116125
if (auto centurion_battery = sendCenturionFeatureRequest(
117126
device_handle,
118127
static_cast<uint16_t>(protocols::CenturionFeature::CenturionBatterySoc),
@@ -124,7 +133,9 @@ class LogitechGProX2Lightspeed : public protocols::LogitechCenturionProtocol {
124133
}
125134

126135
battery_result->raw_data = *centurion_battery;
127-
battery_result->query_duration = std::chrono::milliseconds { 0 };
136+
auto centurion_end_time = std::chrono::steady_clock::now();
137+
battery_result->query_duration = std::chrono::duration_cast<std::chrono::milliseconds>(
138+
centurion_end_time - centurion_start_time);
128139
return *battery_result;
129140
}
130141

@@ -252,26 +263,14 @@ class LogitechGProX2Lightspeed : public protocols::LogitechCenturionProtocol {
252263
return DeviceError::invalidParameter("Device only supports presets 0-4");
253264
}
254265

255-
const std::array<float, 5>* preset_values = nullptr;
256-
switch (preset) {
257-
case 0:
258-
preset_values = &PRESET_FLAT;
259-
break;
260-
case 1:
261-
preset_values = &PRESET_BASS_BOOST;
262-
break;
263-
case 2:
264-
preset_values = &PRESET_TEAM_CHAT;
265-
break;
266-
case 3:
267-
preset_values = &PRESET_SHOOTER;
268-
break;
269-
case 4:
270-
preset_values = &PRESET_MOBA;
271-
break;
272-
default:
273-
return DeviceError::invalidParameter("Device only supports presets 0-4");
274-
}
266+
static constexpr std::array<const std::array<float, 5>*, EQUALIZER_PRESETS_COUNT> PRESET_VALUES {
267+
&PRESET_FLAT,
268+
&PRESET_BASS_BOOST,
269+
&PRESET_TEAM_CHAT,
270+
&PRESET_SHOOTER,
271+
&PRESET_MOBA,
272+
};
273+
const auto* preset_values = PRESET_VALUES[preset];
275274

276275
EqualizerSettings settings;
277276
settings.bands.assign(preset_values->begin(), preset_values->end());
@@ -404,6 +403,7 @@ class LogitechGProX2Lightspeed : public protocols::LogitechCenturionProtocol {
404403
}
405404

406405
auto charging_state = packet.size() >= 3 ? packet[2] : 0;
406+
// Centurion battery replies use states 1 and 2 for charging; the legacy packet parser only treats 0x02 as charging.
407407
auto status = (charging_state == 1 || charging_state == 2)
408408
? BATTERY_CHARGING
409409
: BATTERY_AVAILABLE;
@@ -472,8 +472,21 @@ class LogitechGProX2Lightspeed : public protocols::LogitechCenturionProtocol {
472472
return buildOnboardEqPayload(slot, converted);
473473
}
474474

475+
static std::vector<uint16_t> quantizeOnboardEqCoefficientsForTest(const std::array<double, 5>& coeffs)
476+
{
477+
return quantizeOnboardEqCoefficients(coeffs);
478+
}
479+
475480
private:
476481

482+
void cacheEqualizerInfo(const AdvancedEqDescriptor& descriptor) const
483+
{
484+
cached_band_count_ = static_cast<int>(descriptor.bands.size());
485+
cached_gain_min_ = static_cast<int>(descriptor.gain_min);
486+
cached_gain_max_ = static_cast<int>(descriptor.gain_max);
487+
has_cached_equalizer_info_ = true;
488+
}
489+
477490
static constexpr std::array<uint8_t, 2> buildPlaybackEqSelector(uint8_t slot)
478491
{
479492
return { PLAYBACK_DIRECTION, slot };
@@ -523,7 +536,7 @@ class LogitechGProX2Lightspeed : public protocols::LogitechCenturionProtocol {
523536
for (size_t i = 0; i < coeffs.size(); ++i) {
524537
auto q_value = static_cast<int64_t>(std::llround(coeffs[i] * scales[i]));
525538
q_value = std::clamp<int64_t>(q_value, -(1LL << 31), (1LL << 31) - 1);
526-
q_value &= 0xFFFFFF00;
539+
q_value &= ~INT64_C(0xFF);
527540
words.push_back(static_cast<uint16_t>((q_value >> 16) & 0xFFFF));
528541
words.push_back(static_cast<uint16_t>(q_value & 0xFFFF));
529542
}
@@ -677,9 +690,7 @@ class LogitechGProX2Lightspeed : public protocols::LogitechCenturionProtocol {
677690
return DeviceError::protocolError("Advanced EQ band response was empty");
678691
}
679692

680-
cached_band_count_ = static_cast<int>(descriptor.bands.size());
681-
cached_gain_min_ = static_cast<int>(descriptor.gain_min);
682-
cached_gain_max_ = static_cast<int>(descriptor.gain_max);
693+
cacheEqualizerInfo(descriptor);
683694
return descriptor;
684695
}
685696

@@ -732,9 +743,7 @@ class LogitechGProX2Lightspeed : public protocols::LogitechCenturionProtocol {
732743
return DeviceError::protocolError("Onboard EQ band response was empty");
733744
}
734745

735-
cached_band_count_ = static_cast<int>(descriptor.bands.size());
736-
cached_gain_min_ = static_cast<int>(descriptor.gain_min);
737-
cached_gain_max_ = static_cast<int>(descriptor.gain_max);
746+
cacheEqualizerInfo(descriptor);
738747
return descriptor;
739748
}
740749

@@ -789,9 +798,10 @@ class LogitechGProX2Lightspeed : public protocols::LogitechCenturionProtocol {
789798
return {};
790799
}
791800

792-
mutable int cached_band_count_ = 5;
793-
mutable int cached_gain_min_ = -12;
794-
mutable int cached_gain_max_ = 12;
801+
mutable bool has_cached_equalizer_info_ = false;
802+
mutable int cached_band_count_ = 0;
803+
mutable int cached_gain_min_ = 0;
804+
mutable int cached_gain_max_ = 0;
795805
};
796806

797807
} // namespace headsetcontrol

‎lib/devices/protocols/logitech_centurion_protocol.hpp‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -35,8 +35,10 @@ class LogitechCenturionProtocol : public HIDDevice {
3535
static constexpr uint8_t SOFTWARE_ID = 0x01;
3636
static constexpr uint8_t BRIDGE_SEND_FRAGMENT_FN = 0x10;
3737
static constexpr uint8_t BRIDGE_MESSAGE_EVENT_FN = 0x10;
38+
static constexpr int POLL_ATTEMPTS = 8;
3839
static constexpr size_t MAX_SINGLE_BRIDGE_PAYLOAD = 56;
3940
static constexpr size_t MAX_CONTINUATION_PAYLOAD = 60;
41+
static constexpr size_t MAX_BRIDGE_SUB_MESSAGE_SIZE = 0x0FFF;
4042

4143
[[nodiscard]] Result<std::vector<uint8_t>> sendCenturionRequest(
4244
hid_device* device_handle,
@@ -51,7 +53,7 @@ class LogitechCenturionProtocol : public HIDDevice {
5153
return write_result.error();
5254
}
5355

54-
for (int attempt = 0; attempt < 8; ++attempt) {
56+
for (int attempt = 0; attempt < POLL_ATTEMPTS; ++attempt) {
5557
std::array<uint8_t, FRAME_SIZE> response {};
5658
auto read_result = this->readHIDTimeout(device_handle, response, hsc_device_timeout);
5759
if (!read_result) {
@@ -110,6 +112,11 @@ class LogitechCenturionProtocol : public HIDDevice {
110112
}
111113

112114
public:
115+
static constexpr bool isBridgeSubMessageSizeSupported(size_t sub_message_size)
116+
{
117+
return sub_message_size <= MAX_BRIDGE_SUB_MESSAGE_SIZE;
118+
}
119+
113120
static constexpr auto buildCenturionFrame(std::span<const uint8_t> payload, uint8_t flags = 0x00)
114121
-> std::array<uint8_t, FRAME_SIZE>
115122
{
@@ -189,8 +196,6 @@ class LogitechCenturionProtocol : public HIDDevice {
189196

190197
return std::vector<uint8_t>(reply_data.begin() + 7, reply_data.end());
191198
}
192-
193-
protected:
194199
private:
195200
[[nodiscard]] Result<void> ensureCenturionFeaturesDiscovered(hid_device* device_handle) const
196201
{
@@ -308,6 +313,9 @@ class LogitechCenturionProtocol : public HIDDevice {
308313

309314
auto sub_message = buildBridgeSubMessageVector(sub_feature_index, function, params);
310315
const size_t sub_message_size = sub_message.size();
316+
if (!isBridgeSubMessageSizeSupported(sub_message_size)) {
317+
return DeviceError::invalidParameter("Centurion bridge message exceeds the 12-bit size limit");
318+
}
311319

312320
std::vector<uint8_t> bridge_prefix {
313321
*centurion_bridge_index_,
@@ -352,7 +360,7 @@ class LogitechCenturionProtocol : public HIDDevice {
352360
}
353361

354362
bool ack_received = false;
355-
for (int attempt = 0; attempt < 8; ++attempt) {
363+
for (int attempt = 0; attempt < POLL_ATTEMPTS; ++attempt) {
356364
std::array<uint8_t, FRAME_SIZE> response {};
357365
auto read_result = this->readHIDTimeout(device_handle, response, hsc_device_timeout);
358366
if (!read_result) {

‎tests/test_protocols.cpp‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -340,6 +340,11 @@ void testLogitechProX2CenturionBatteryParsing()
340340
{
341341
std::cout << " Testing Logitech PRO X2 Centurion battery parsing..." << std::endl;
342342

343+
std::array<uint8_t, 3> charging_v1 { 42, 42, 0x01 };
344+
auto charging_v1_result = LogitechGProX2Lightspeed::parseCenturionBatteryResponse(charging_v1);
345+
ASSERT_TRUE(charging_v1_result.hasValue(), "Centurion charging state 0x01 should parse successfully");
346+
ASSERT_EQ(BATTERY_CHARGING, charging_v1_result->status, "Centurion charging state 0x01 should map to charging");
347+
343348
std::array<uint8_t, 3> charging { 87, 87, 0x02 };
344349
auto charging_result = LogitechGProX2Lightspeed::parseCenturionBatteryResponse(charging);
345350
ASSERT_TRUE(charging_result.hasValue(), "Centurion charging response should parse successfully");
@@ -395,6 +400,55 @@ void testCenturionBridgeResponseParsing()
395400
std::cout << " [OK] Logitech Centurion bridge response parsing verified" << std::endl;
396401
}
397402

403+
void testCenturionBridgeMessageSizeLimit()
404+
{
405+
std::cout << " Testing Logitech Centurion bridge message size limit..." << std::endl;
406+
407+
std::vector<uint8_t> params_at_limit(0x0FFF - 3, 0x5A);
408+
auto message_at_limit = protocols::LogitechCenturionProtocol::buildBridgeSubMessageVector(0x04, 0x10, params_at_limit);
409+
ASSERT_EQ(0x0FFF, static_cast<int>(message_at_limit.size()), "Bridge sub-message should reach the 12-bit size limit");
410+
ASSERT_TRUE(protocols::LogitechCenturionProtocol::isBridgeSubMessageSizeSupported(message_at_limit.size()),
411+
"Bridge sub-message at the 12-bit size limit should be accepted");
412+
413+
std::vector<uint8_t> params_over_limit(0x1000 - 3, 0x5A);
414+
auto message_over_limit = protocols::LogitechCenturionProtocol::buildBridgeSubMessageVector(0x04, 0x10, params_over_limit);
415+
ASSERT_EQ(0x1000, static_cast<int>(message_over_limit.size()), "Bridge sub-message should exceed the 12-bit size limit by one byte");
416+
ASSERT_TRUE(!protocols::LogitechCenturionProtocol::isBridgeSubMessageSizeSupported(message_over_limit.size()),
417+
"Bridge sub-message above the 12-bit size limit should be rejected");
418+
419+
std::cout << " [OK] Logitech Centurion bridge message size limit verified" << std::endl;
420+
}
421+
422+
void testLogitechProX2EqualizerInfoRequiresDescriptor()
423+
{
424+
std::cout << " Testing Logitech PRO X2 equalizer info cache behavior..." << std::endl;
425+
426+
LogitechGProX2Lightspeed device;
427+
ASSERT_TRUE(!device.getEqualizerInfo().has_value(), "Equalizer info should be unavailable before the descriptor is read");
428+
ASSERT_TRUE(!device.getParametricEqualizerInfo().has_value(), "Parametric equalizer info should be unavailable before the descriptor is read");
429+
430+
std::cout << " [OK] Logitech PRO X2 equalizer info cache behavior verified" << std::endl;
431+
}
432+
433+
void testLogitechProX2OnboardEqCoefficientQuantization()
434+
{
435+
std::cout << " Testing Logitech PRO X2 onboard EQ coefficient quantization..." << std::endl;
436+
437+
auto words = LogitechGProX2Lightspeed::quantizeOnboardEqCoefficientsForTest({
438+
0.0,
439+
-100.0 / 1073741824.0,
440+
0.0,
441+
0.0,
442+
0.0,
443+
});
444+
445+
ASSERT_EQ(10, static_cast<int>(words.size()), "Quantized coefficient block should contain 10 words");
446+
ASSERT_EQ(0xFFFF, static_cast<int>(words[2]), "Negative coefficients should preserve sign extension in the upper word");
447+
ASSERT_EQ(0xFF00, static_cast<int>(words[3]), "Negative coefficients should still clear the low byte");
448+
449+
std::cout << " [OK] Logitech PRO X2 onboard EQ coefficient quantization verified" << std::endl;
450+
}
451+
398452
void testLogitechProX2OnboardEqPayloadBuilding()
399453
{
400454
std::cout << " Testing Logitech PRO X2 onboard EQ payload building..." << std::endl;
@@ -669,6 +723,9 @@ void runAllProtocolTests()
669723
runTest("Logitech PRO X2 Centurion Battery", testLogitechProX2CenturionBatteryParsing);
670724
runTest("Logitech Centurion Frame Building", testCenturionFrameBuilding);
671725
runTest("Logitech Centurion Bridge Parsing", testCenturionBridgeResponseParsing);
726+
runTest("Logitech Centurion Bridge Size Limit", testCenturionBridgeMessageSizeLimit);
727+
runTest("Logitech PRO X2 Equalizer Info Cache", testLogitechProX2EqualizerInfoRequiresDescriptor);
728+
runTest("Logitech PRO X2 EQ Quantization", testLogitechProX2OnboardEqCoefficientQuantization);
672729
runTest("Logitech PRO X2 Onboard EQ Payload", testLogitechProX2OnboardEqPayloadBuilding);
673730

674731
std::cout << "\n=== SteelSeries Protocol ===" << std::endl;

0 commit comments

Comments
 (0)