Track state of connections, seats, and devices#21
Conversation
bec4726 to
96a9bb7
Compare
|
Rebased onto master now that #22 and #23 are in, and adapted to the changes there:
The diff against master is additive apart from the widened re-export. |
| match state { | ||
| ei::keyboard::KeyState::Press => { | ||
| device.0.pressed_keys.lock().unwrap().insert(key); | ||
| } | ||
| ei::keyboard::KeyState::Released => { | ||
| device.0.pressed_keys.lock().unwrap().remove(&key); | ||
| } | ||
| } | ||
| self.queue_event(EiEvent::KeyboardKey(KeyboardKey { | ||
| device: device.clone(), | ||
| time: 0, |
There was a problem hiding this comment.
If we add this, presumably pressed_keys shouldn't be updated until the Frame event.
|
Adding the state enums is definitely good, as long as all the uses of them are correct. Given that's how libei works. Though libei doesn't seem to expose them publicly. Is there a particularly good reason to make these public, or can we just leave them out of the public API surface? How useful is this in practice in applications using Without marking the enums as
Given we leave it up to the caller of the library to use something like |
96a9bb7 to
6340b39
Compare
|
I appreciate the detailed review. I've split the PR into its two halves based on your feedback. I dropped the pressed-key, pressed-button, and touch tracking. You're right that libei and libeis don't offer it, and it's easy to misuse: The set updates as events come in, so a consumer that's processing a queued event can't rely on it to reflect the state as of that event, and every call clones the set. It's better left to the caller, which already tracks keyboard state through xkbcommon and can update its own set in step with the events it handles. The server's internal I kept the lifecycle state enums and marked them Further out, I'm interested in lightweight per-device metrics for performance tuning, such as event throughput and timing to drive adaptive encoding. That's a separate conversation, and I've kept it out of this change to keep the scope minimal. I've rebased the branch onto master. |
|
Yeah, it's probably fine with |
|
Something I didn't mention explicitly in #15 (since I wrote the issue more for my own reference) is that we should also use the state to have checks that messages are received or functions are called only in the appropriate state, and send protocol errors or log and return if it is incorrect. The hard part of this is carefully adding all of those checks that exist in libei and making sure they're all right. That's the role the state machine serves in libei/libeis, given the enum isn't public. But we should be able to add these things incrementally later; this validation can hopefully be considered a minor non-breaking change since it shouldn't affect any correct uses of the protocol.
The way libei/libei define these doesn't seem to be quite the same on client and server side, particularly the seat state seems different? Though I haven't carefully studied exactly how libei defines all of the states. enum ei_device_state {
/* Before the DeviceAddedDone was received */
EI_DEVICE_STATE_NEW,
EI_DEVICE_STATE_PAUSED,
EI_DEVICE_STATE_RESUMED,
EI_DEVICE_STATE_EMULATING,
/**
* Client removed the device, we no longer accept events from the
* client
*/
EI_DEVICE_STATE_REMOVED_FROM_CLIENT,
/**
* Server removed the device, we need to remove it ourselves now.
*/
EI_DEVICE_STATE_REMOVED_FROM_SERVER,
/**
* Device has been removed by both sides.
*/
EI_DEVICE_STATE_DEAD,
};enum eis_device_state {
EIS_DEVICE_STATE_NEW,
EIS_DEVICE_STATE_AWAITING_READY,
EIS_DEVICE_STATE_PAUSED,
EIS_DEVICE_STATE_RESUMED,
EIS_DEVICE_STATE_EMULATING,
EIS_DEVICE_STATE_CLOSED_BY_CLIENT,
EIS_DEVICE_STATE_DEAD,
};enum ei_seat_state {
EI_SEAT_STATE_NEW,
EI_SEAT_STATE_DONE,
EI_SEAT_STATE_REMOVED,
};enum eis_seat_state {
EIS_SEAT_STATE_PENDING,
EIS_SEAT_STATE_ADDED,
EIS_SEAT_STATE_BOUND,
EIS_SEAT_STATE_REMOVED_INTERNALLY, /* Removed internally but eis_seat_remove() may be called
*/
EIS_SEAT_STATE_REMOVED, /* Removed but still waiting for some devices to be removed */
EIS_SEAT_STATE_DEAD, /* Removed from our list */
};enum ei_state {
EI_STATE_NEW, /* No backend yet */
EI_STATE_BACKEND, /* We have a backend */
EI_STATE_CONNECTING, /* client requested connect */
EI_STATE_CONNECTED, /* server has sent connect */
EI_STATE_DISCONNECTING, /* in the process of cleaning up */
EI_STATE_DISCONNECTED,
};enum eis_client_state {
EIS_CLIENT_STATE_NEW, /* socket just handed over */
EIS_CLIENT_STATE_CONNECTING, /* client completed setup but hasn't been accepted yet */
EIS_CLIENT_STATE_CONNECTED, /* caller has done eis_client_connect */
EIS_CLIENT_STATE_REQUESTED_DISCONNECT, /* caller has disconnected */
EIS_CLIENT_STATE_DISCONNECTED,
};
|
|
Following up on your review. The more I dug in response to your comments and went back through the issues, the more I think this PR's shape is wrong rather than just incomplete. I built #21 the expedient way: I tracked each wrapper's state as events flowed and consolidated it into a few shared enums to get a lifecycle surface working quickly. But once I treated the state as a validation engine the way you framed it and went back to the protocol itself, the right shape came out quite different. The protocol doesn't define these states at all, so they're purely an internal modeling choice, and the per-side asymmetry that libei has matters in ways the consolidated version flattened. So I'd rather not patch this incrementally. I'm treating #21 as suspended and moving the design discussion back to #15, where I'll lay out the per-side model and where the validation actually has to live before I write the rework. I'll either repurpose this branch or open a fresh one once the shape is settled there. |
Add per-side lifecycle state enums that match libei's variant sets. The client side defines EiConnectionState, EiDeviceState, and EiSeatState, and the server side defines EisConnectionState, EisDeviceState, and EisSeatState. Each wrapper exposes its state through a state() query and updates it as protocol events flow through the converters. The enums keep only the states each converter can actually observe. The server device tracks AwaitingReady, Paused, Resumed, Emulating, ClosedByClient, and Dead. The client device tracks Paused, Resumed, Emulating, and RemovedFromServer, since the client converter does not model client-initiated release. Pre-done phases (New on the client device, Pending on the server seat) are omitted, and the connection enum is reduced to Connected and Disconnected on both sides. State-gated validation follows as a separate incremental change.
6340b39 to
2f28447
Compare
|
I've pushed the per-side rework to the branch. It replaces the consolidated three-enum approach with separate client and server enums that match libei's variant sets, with the state tracked through the converters. The full shape, what I dropped from libei and why, and the open question of how much should be public are in #15. If you'd rather a different shape, say so there and I'll amend before adding the validation checks. |
Implements the state tracking requested in #15: equivalents of
eis_client_state,ei_state,eis_seat_state,ei_seat_state,eis_device_state, andei_device_statefrom libei/libeis.Changes
Three new enums in
event.rs, re-exported fromrequest.rs:ConnectionState—Connected/DisconnectedSeatState—Active/ClosedDeviceState—Paused/Resumed/ClosedEach connection, seat, and device wrapper tracks its own state via
Mutex-guarded fields, updated automatically as protocol events arrive(client side) or as methods are called (server side).
Client side (
EiEventConverter)Disconnectedon EOF / disconnect.Closedwhen the seat'sDestroyedevent fires.Resumed/Paused/Closedlifecycle.pressed_keys,pressed_buttons, anddown_touch_idson eachdevice, clearing them on pause (matching libei semantics).
Server side (
EisRequestConverter)AtomicBool.add_seat/remove,resumed/paused/removecalls.pressed_keys,pressed_buttons, anddown_touch_idstracked oneach device, cleared on pause or remove.
Design notes
server perspectives share the same state values.
this tracking to the pause/resume lifecycle -- they are inherently
coupled.
# Panicssections added where.lock().unwrap()is used, per clippymissing_panics_doc.Closes #15