Add Logitech ASTRO A50 Gen 4 (9886:002c) - #578
Conversation
Sapd
left a comment
Sapd
left a comment
There was a problem hiding this comment.
Nice work. Builds clean here, tests pass, clang-format 18 clean, README matches --readme-helper, and it's already on current master. The test coverage is well beyond what device PRs usually come with - timeout recovery, handle reuse, reply length and non-finite EQ are all the right things to test.
One thing I want to change before merging: onConnectionClosed().
The doc on it says connection owners must call it before hid_close(), including callers using device implementations directly. That's a contract external embedders can't be made to honour, and right now it's load-bearing rather than advisory.
In sendRequest() the recovery branch has three early returns and none of them erase the pending entry - only the success path does:
if (!recovered) return recovered.error();
if (*recovered == 0) return DeviceError::timeout("...still waiting for the previous reply");
if (bad frame) return DeviceError::protocolError(...);
pending_replies_.erase(device_handle);
For a genuinely delayed reply that's right, and testAstroA50Gen4TimeoutRecovery shows it recovering once the late frame arrives. The problem is the other case: if someone closes a handle without the hook and the OS reuses the address, the new connection inherits a pending entry for a reply that never comes. *recovered == 0 every time, so every call after that fails permanently on healthy hardware - with a message that sends the user off to check their headset.
Please bound it instead of relying on the contract: expire the entry after a couple of failed recovery attempts, or timestamp it. Then a missed onConnectionClosed() costs one slow call rather than wedging the device, and the hook is an optimisation instead of a correctness requirement.
Minor:
HIDConnection::close()dereferencesdevice_guarded only byhandle_. The invariant holds today sincedevice_is set on every successful open, but it isn't stated anywhere anddevice_ &&is free.closeAllConnections()in headsetcontrol.cpp:152 is the same shape.- No
getSupportedPlatforms()override, so it advertises all platforms with only Linux tested. You do set usagepage/usageid (0xff32/0x0074) as well as interface_id 6, so Windows has what it needs - but #576 restricted itself for exactly this reason. Either is fine, just pick one deliberately. - "Settings are not saved to flash" belongs in the device header comment, not only the PR body - that's where the next person will look.
…behaviour Review follow-up for Sapd#578. After a timeout, sendRequest() previously kept the connection's pending entry until a frame was consumed, so a handle closed without onConnectionClosed() whose address was reused failed permanently. The recovery step is now a single bounded read whose outcome is discarded; the entry is cleared either way and the request proceeds. A missed hook costs one call that is up to a second slower, and the hook is documented as optional. Guard the hook calls on device_ in the CLI connection and the library's closeAllConnections(). State in the header that settings are never saved to flash and that hardware verification was on Linux only; all platforms stay declared. Tests cover a lost reply, a late reply consumed during recovery, a reused handle address without the hook, the hook skipping the recovery wait, and read errors or truncated frames during recovery, for both the timeout error and zero-byte read variants.
|
Fixed in 084a910. Recovery is now a single bounded read; the pending entry is dropped either way and the request proceeds. I went with one attempt rather than a counter, since every command here answers in milliseconds and a second attempt would only guard a delay the hardware hasn't shown. Kept all platforms declared, with the Linux-only testing noted in the header. |
2 participants
Changes made
Adds Logitech ASTRO A50 Gen 4 (
9886:002c) support in PC mode. Related: #207.Settings are not saved to flash.
Some replies carry no command identifier, so after a timeout the next request on that connection first waits up to one second for the outstanding reply and discards it. The optional
onConnectionClosed()hook lets connection owners skip that wait when a handle address is reused.Validation
Tested on Linux with the PlayStation/PC edition, including EQ read-back and restoration. Windows, macOS, and the Xbox/PC edition are untested.
Unit and integration tests pass, including delayed replies and handle reuse. clang-format 18 and whitespace checks pass.
Checklist