Skip to content

fix(logitech): correct PRO X 2 battery queries and offline detection #593

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ChrisLauinger77 wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix(logitech): correct PRO X 2 battery queries and offline detection #593

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions lib/devices/logitech_gpro_x2_lightspeed.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -311,6 +311,17 @@ class LogitechGProX2Lightspeed : public protocols::LogitechCenturionProtocol {
};
}

protected:
protocols::CenturionOptions centurionOptions() const override
{
return {
.exact_direct_length = true,
.detect_connection_events = true,
.probe_offline_on_timeout = true,
.lookup_features_by_id = true,
};
}

private:
enum class EqualizerBackend {
AdvancedParametric,
Expand Down
31 changes: 20 additions & 11 deletions lib/devices/protocols/logitech_centurion_protocol.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,8 @@ struct CenturionOptions {
bool exact_direct_length = false;
// Report a bridge "headset disconnected" event as DeviceError::deviceOffline.
bool detect_connection_events = false;
// After a bridge ACK followed by a read timeout, query the bridge's
// connection status once and report deviceOffline if the headset is gone.
// After a bridge rejection or an ACK followed by a read timeout, query the
// bridge's connection status once and report deviceOffline if the headset is gone.
bool probe_offline_on_timeout = false;
// Look up each feature index with Root.getFeature when first needed,
// instead of enumerating every feature of the dongle and the headset.
Expand Down Expand Up @@ -573,7 +573,7 @@ class LogitechCenturionProtocol : public HIDDevice {
if (!read_result) {
if (ack_received && options.probe_offline_on_timeout
&& read_result.error().code == DeviceError::Code::Timeout) {
return probeOfflineAfterTimeout(device_handle, read_result.error());
return probeOfflineAfterFailure(device_handle, read_result.error());
}
return read_result.error();
}
Expand All @@ -584,6 +584,15 @@ class LogitechCenturionProtocol : public HIDDevice {
}

const auto& reply = *payload_result;
// The PRO X 2 can reject a bridge request before acknowledging it
// when no headset is connected. Verify the connection status rather
// than assuming that every rejection means the headset is offline.
if (options.probe_offline_on_timeout && reply.size() >= 4
&& reply[0] == 0xFF && reply[1] == *centurion_bridge_index_
&& reply[2] == static_cast<uint8_t>(BRIDGE_SEND_FRAGMENT_FN | SOFTWARE_ID)) {
return probeOfflineAfterFailure(device_handle,
DeviceError::protocolError("Centurion bridge rejected the request"));
}
if (reply.size() < 2 || reply[0] != *centurion_bridge_index_) {
continue;
}
Expand Down Expand Up @@ -622,21 +631,21 @@ class LogitechCenturionProtocol : public HIDDevice {

auto timeout = DeviceError::timeout("Timed out waiting for Centurion bridge response");
if (options.probe_offline_on_timeout) {
return probeOfflineAfterTimeout(device_handle, timeout);
return probeOfflineAfterFailure(device_handle, timeout);
}
return timeout;
}

/**
* @brief Tell "headset off" apart from a slow reply after a bridge timeout.
* @brief Distinguish a disconnected headset from other bridge failures.
*
* With the headset off, the dongle still acknowledges bridge requests, but
* nothing answers them. Ask the dongle's bridge directly whether a headset
* is connected; keep the original timeout if the answer is unclear.
* A disconnected dongle may reject a bridge request or acknowledge it with
* no subsequent reply. Ask the bridge directly whether a headset is
* connected; keep the original error if the answer is unclear.
*/
[[nodiscard]] Result<std::vector<uint8_t>> probeOfflineAfterTimeout(
[[nodiscard]] Result<std::vector<uint8_t>> probeOfflineAfterFailure(
hid_device* device_handle,
const DeviceError& timeout) const
const DeviceError& failure) const
{
auto status = sendCenturionRequest(device_handle, *centurion_bridge_index_, 0x00, {}, true);
if (status && isBridgeStatusDisconnected(*status)) {
Expand All @@ -645,7 +654,7 @@ class LogitechCenturionProtocol : public HIDDevice {
if (!status && status.error().code == DeviceError::Code::DeviceOffline) {
return status.error();
}
return timeout;
return failure;
}

static auto buildDirectPayloadVector(uint8_t feature_index, uint8_t function, std::span<const uint8_t> params)
Expand Down
116 changes: 114 additions & 2 deletions tests/test_protocols.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1386,6 +1386,12 @@ class TestableProX2 : public LogitechGProX2Lightspeed {
[[nodiscard]] auto getHIDInterface() const -> HIDInterface& override { return hid; }
};

// Keep coverage of the shared protocol's opt-out behavior.
class TestableLegacyProX2 : public TestableProX2 {
protected:
protocols::CenturionOptions centurionOptions() const override { return {}; }
};

static constexpr std::array<uint8_t, 1> PRO_X2_PREFIX { 0x51 };

static std::vector<uint8_t> centurionFrame(std::span<const uint8_t> prefix, std::vector<uint8_t> payload)
Expand Down Expand Up @@ -1853,7 +1859,7 @@ void testCenturionProX2DefaultOptionsUnchanged()
std::cout << " Testing Logitech PRO X2 traffic with default Centurion options..." << std::endl;

{
TestableProX2 dev;
TestableLegacyProX2 dev;
queueCenturionDiscovery(dev.hid, PRO_X2_PREFIX);
queueBridgeReply(dev.hid, PRO_X2_PREFIX, 0x02, { 0x2f, 0x32, 0x00 });

Expand All @@ -1873,7 +1879,7 @@ void testCenturionProX2DefaultOptionsUnchanged()
}
{
// A power-off event is skipped, and a timeout is returned without a probe.
TestableProX2 dev;
TestableLegacyProX2 dev;
queueCenturionDiscovery(dev.hid, PRO_X2_PREFIX);
dev.hid.replies.push_back(centurionFrame(PRO_X2_PREFIX, { 0x03, 0x11 }));
dev.hid.replies.push_back(centurionFrame(PRO_X2_PREFIX, { 0x03, 0x00, 0x00, 0x00 }));
Expand All @@ -1887,6 +1893,109 @@ void testCenturionProX2DefaultOptionsUnchanged()
std::cout << " [OK] Logitech PRO X2 traffic with default Centurion options verified" << std::endl;
}

void testCenturionProX2BatteryWire()
{
std::cout << " Testing Logitech PRO X2 battery lookup on the wire..." << std::endl;

TestableProX2 dev;
dev.hid.replies.push_back(centurionFrame(PRO_X2_PREFIX, { 0x00, 0x01, 0x03, 0x00, 0x01 }));
queueBridgeReply(dev.hid, PRO_X2_PREFIX, 0x00, { 0x02, 0x00, 0x03 });
queueBridgeReply(dev.hid, PRO_X2_PREFIX, 0x02, { 0x2f, 0x32, 0x00 });

auto battery = dev.getBattery(nullptr);
ASSERT_TRUE(battery.hasValue(), "PRO X2 battery should succeed with direct feature lookup");
ASSERT_EQ(47, battery->level_percent, "PRO X2 battery level");
ASSERT_EQ(BATTERY_AVAILABLE, battery->status, "PRO X2 battery status");
ASSERT_EQ(3u, dev.hid.writes.size(), "Two lookups and one battery request");
assertFrame(dev.hid.writes[0], { 0x51, 0x05, 0x00, 0x00, 0x01, 0x00, 0x03 },
"PRO X2 bridge lookup uses the exact direct length");
assertFrame(dev.hid.writes[1], { 0x51, 0x0a, 0x00, 0x03, 0x11, 0x00, 0x05, 0x00, 0x00, 0x01, 0x01, 0x04 },
"PRO X2 battery feature lookup goes through the bridge");
assertFrame(dev.hid.writes[2], { 0x51, 0x08, 0x00, 0x03, 0x11, 0x00, 0x03, 0x00, 0x02, 0x01 },
"PRO X2 battery request uses the resolved feature index");

queueBridgeReply(dev.hid, PRO_X2_PREFIX, 0x02, { 0x30, 0x32, 0x02 });
auto charging = dev.getBattery(nullptr);
ASSERT_TRUE(charging.hasValue(), "Cached PRO X2 battery request should succeed");
ASSERT_EQ(BATTERY_CHARGING, charging->status, "PRO X2 charging state");
ASSERT_EQ(4u, dev.hid.writes.size(), "Cached indexes avoid further lookups");
ASSERT_TRUE(dev.hid.replies.empty(), "All replies consumed");

std::cout << " [OK] Logitech PRO X2 battery lookup on the wire verified" << std::endl;
}

void testCenturionProX2Offline()
{
std::cout << " Testing Logitech PRO X2 offline detection..." << std::endl;

{
// An already powered-off headset can disappear during feature lookup.
TestableProX2 dev;
dev.hid.replies.push_back(centurionFrame(PRO_X2_PREFIX, { 0x00, 0x01, 0x03, 0x00, 0x01 }));
dev.hid.replies.push_back(centurionFrame(PRO_X2_PREFIX, { 0x03, 0x11 }));
dev.hid.replies.push_back(centurionFrame(PRO_X2_PREFIX, { 0x03, 0x00, 0x00, 0x00 }));

auto battery = dev.getBattery(nullptr);
ASSERT_TRUE(battery.hasError(), "Disconnect during lookup should fail");
ASSERT_TRUE(battery.error().code == DeviceError::Code::DeviceOffline, "Disconnect event means offline");
ASSERT_EQ(2u, dev.hid.writes.size(), "Offline lookup sends no legacy battery request");
ASSERT_TRUE(dev.hid.replies.empty(), "Disconnect event consumed");
}
for (bool connected : { false, true }) {
TestableProX2 dev;
dev.hid.replies.push_back(centurionFrame(PRO_X2_PREFIX, { 0x00, 0x01, 0x03, 0x00, 0x01 }));
queueBridgeReply(dev.hid, PRO_X2_PREFIX, 0x00, { 0x02, 0x00, 0x03 });
dev.hid.replies.push_back(centurionFrame(PRO_X2_PREFIX, { 0x03, 0x11 }));
dev.hid.replies.push_back(DeviceError::timeout("No reply from the headset"));
dev.hid.replies.push_back(centurionFrame(PRO_X2_PREFIX, { 0x03, 0x11 })); // stale ACK must be skipped
dev.hid.replies.push_back(centurionFrame(PRO_X2_PREFIX, { 0x03, 0x01, 0x03, 0xe8, 0x00, static_cast<uint8_t>(connected), 0x01 }));

auto battery = dev.getBattery(nullptr);
ASSERT_TRUE(battery.hasError(), "Unanswered battery query should fail");
ASSERT_TRUE(battery.error().code == (connected ? DeviceError::Code::Timeout : DeviceError::Code::DeviceOffline),
"Only a disconnected headset should turn a timeout into offline");
ASSERT_EQ(4u, dev.hid.writes.size(), "Two lookups, battery query and one probe; no legacy fallback");
assertFrame(dev.hid.writes.back(), { 0x51, 0x03, 0x00, 0x03, 0x01 },
"PRO X2 status probe uses the exact direct length");
ASSERT_TRUE(dev.hid.replies.empty(), "All probe replies consumed");

// A later successful request uses the cached index and recovers normally.
queueBridgeReply(dev.hid, PRO_X2_PREFIX, 0x02, { 0x30, 0x32, 0x00 });
auto recovered = dev.getBattery(nullptr);
ASSERT_TRUE(recovered.hasValue(), "Battery query should recover after an offline or timeout result");
ASSERT_EQ(48, recovered->level_percent, "Recovered battery level");
ASSERT_EQ(5u, dev.hid.writes.size(), "Recovery reuses the cached feature index");
ASSERT_TRUE(dev.hid.replies.empty(), "Recovery replies consumed");
}

std::cout << " [OK] Logitech PRO X2 offline detection verified" << std::endl;
}

void testCenturionProX2BridgeRejection()
{
std::cout << " Testing Logitech PRO X2 bridge rejection..." << std::endl;

for (bool connected : { false, true }) {
TestableProX2 dev;
dev.hid.replies.push_back(centurionFrame(PRO_X2_PREFIX, { 0x00, 0x01, 0x03, 0x00, 0x01 }));
dev.hid.replies.push_back(centurionFrame(PRO_X2_PREFIX, { 0xff, 0x03, 0x21, 0x0b })); // unrelated function
// Captured PRO X 2 rejection and bridge status while disconnected.
dev.hid.replies.push_back(centurionFrame(PRO_X2_PREFIX, { 0xff, 0x03, 0x11, 0x0b }));
dev.hid.replies.push_back(centurionFrame(PRO_X2_PREFIX, { 0x03, 0x01, 0x03, 0x7a, 0x00, static_cast<uint8_t>(connected), 0x01 }));

auto battery = dev.getBattery(nullptr);
ASSERT_TRUE(battery.hasError(), "Rejected feature lookup should fail");
ASSERT_TRUE(battery.error().code == (connected ? DeviceError::Code::ProtocolError : DeviceError::Code::DeviceOffline),
"A bridge rejection means offline only when the status confirms disconnection");
ASSERT_EQ(3u, dev.hid.writes.size(), "Bridge lookup, rejected feature lookup and one status probe");
ASSERT_EQ(2u, dev.hid.writes_at_read[2], "Unrelated rejection must not trigger a status probe");
assertFrame(dev.hid.writes.back(), { 0x51, 0x03, 0x00, 0x03, 0x01 }, "Direct bridge status probe");
ASSERT_TRUE(dev.hid.replies.empty(), "All replies consumed without waiting for a timeout");
}

std::cout << " [OK] Logitech PRO X2 bridge rejection verified" << std::endl;
}

// ============================================================================
// Test Runner
// ============================================================================
Expand Down Expand Up @@ -1952,6 +2061,9 @@ void runAllProtocolTests()
runTest("Logitech G522 Equalizer Encoding", testG522EqualizerEncoding);
runTest("Logitech G522 Equalizer Commands", testG522EqualizerCommands);
runTest("Logitech PRO X2 Default Centurion Options", testCenturionProX2DefaultOptionsUnchanged);
runTest("Logitech PRO X2 Battery Wire", testCenturionProX2BatteryWire);
runTest("Logitech PRO X2 Offline Detection", testCenturionProX2Offline);
runTest("Logitech PRO X2 Bridge Rejection", testCenturionProX2BridgeRejection);
runTest("Logitech PRO X2 Equalizer Info Cache", testLogitechProX2EqualizerInfoRequiresDescriptor);
runTest("Logitech PRO X2 EQ Quantization", testLogitechProX2OnboardEqCoefficientQuantization);
runTest("Logitech PRO X2 Onboard EQ Payload", testLogitechProX2OnboardEqPayloadBuilding);
Expand Down
Loading