Skip to content

Track state of connections, seats, and devices#21

Open
glamberson wants to merge 1 commit into
ids1024:masterfrom
lamco-admin:track-pressed-keys-buttons
Open

Track state of connections, seats, and devices#21
glamberson wants to merge 1 commit into
ids1024:masterfrom
lamco-admin:track-pressed-keys-buttons

Conversation

@glamberson

Copy link
Copy Markdown
Contributor

Implements the state tracking requested in #15: equivalents of
eis_client_state, ei_state, eis_seat_state, ei_seat_state,
eis_device_state, and ei_device_state from libei/libeis.

Changes

Three new enums in event.rs, re-exported from request.rs:

  • ConnectionStateConnected / Disconnected
  • SeatStateActive / Closed
  • DeviceStatePaused / Resumed / Closed

Each 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)

  • Connection state set to Disconnected on EOF / disconnect.
  • Seat state set to Closed when the seat's Destroyed event fires.
  • Device state follows Resumed / Paused / Closed lifecycle.
  • Tracks pressed_keys, pressed_buttons, and down_touch_ids on each
    device, clearing them on pause (matching libei semantics).

Server side (EisRequestConverter)

  • Connection state derived from the existing AtomicBool.
  • Seat and device state tracked through add_seat / remove,
    resumed / paused / remove calls.
  • pressed_keys, pressed_buttons, and down_touch_ids tracked on
    each device, cleared on pause or remove.

Design notes

  • Consolidated into three enums rather than six, since the client and
    server perspectives share the same state values.
  • Input state (pressed keys/buttons/touches) included because libei ties
    this tracking to the pause/resume lifecycle -- they are inherently
    coupled.
  • All new public methods documented; # Panics sections added where
    .lock().unwrap() is used, per clippy missing_panics_doc.

Closes #15

@glamberson

Copy link
Copy Markdown
Contributor Author

Rebased onto master now that #22 and #23 are in, and adapted to the changes there:

  • The handler extraction that was part of this branch is gone, since Handle Release requests for devices and interfaces #23 already extracted the keyboard/button request handlers. The pressed key/button tracking now lives in those existing handlers instead.
  • The device Release arm sets DeviceState::Closed when queueing DeviceClosed, matching libeis where eis_device_closed_by_client() updates the state before the server application calls remove. Device::remove() also sets it, so the state is correct on both legs of the two-step close.
  • On the client side, the Destroyed handling combines the state update with the interface cleanup from Remove destroyed interfaces from device #22.

The diff against master is additive apart from the widened re-export.

Comment thread src/event.rs Outdated
Comment on lines 486 to 496
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,

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we add this, presumably pressed_keys shouldn't be updated until the Frame event.

@ids1024

ids1024 commented Jun 15, 2026

Copy link
Copy Markdown
Owner

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 reis?

Without marking the enums as #[non_exhaustive], adding a variant would be a breaking API change. Though reis doesn't really have a stable API yet anyway.

pressed_keys, pressed_buttons, and down_touch_ids, meanwhile, isn't something libei/libeis has, right? It's not necessarily bad to offer something useful like that regardless, though there may be some potential to misuse it. (For instance, when processing a stream of events, these accessors won't provide the state at the time of the event being processed, if new events have caused changes.) And then it's also not ideal that these require cloning a HashSet.

Given we leave it up to the caller of the library to use something like xkbcommon to track the keyboard state, maybe it's best to let the caller managed its own pressed_keys, etc. That might make it easier to make sure it's being used correctly, could avoid the extra clone, and probably doesn't make application code all that much more complicated.

@glamberson glamberson force-pushed the track-pressed-keys-buttons branch from 96a9bb7 to 6340b39 Compare June 15, 2026 23:01
@glamberson

Copy link
Copy Markdown
Contributor Author

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 down_touch_ids field stays, since it's still needed for the EIS_MAX_TOUCHES enforcement.

I kept the lifecycle state enums and marked them #[non_exhaustive] as you suggested. That's the opinionated part of the change. I'd like Connection::state(), Seat::state(), and Device::state() as a small queryable surface for an out-of-band session-health check (for example, whether a device's resumed or the connection's still up) that runs outside the event loop. In that setting the caller just wants the latest known state, so the timing mismatch that makes the pressed-key tracking unsafe doesn't apply here. The same information can be derived from the resumed, paused, and disconnected events, but a lock-guarded enum on the handle that reis already owns seemed like the cleaner primitive.

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.

@ids1024

ids1024 commented Jun 16, 2026

Copy link
Copy Markdown
Owner

Yeah, it's probably fine with #[non_exhaustive] to make these public, as long as there's a use for them.

@ids1024

ids1024 commented Jun 16, 2026

Copy link
Copy Markdown
Owner

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.

Consolidated into three enums rather than six, since the client and server perspectives share the same state values.

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,
};

ei_state/ei_client_state can be reduced to 3 states here since the event/request converters are only after a complete handshake (so any extra validation there needs to be done separately), but otherwise we probably want all the states libei has, so we can ensure we validate everything in the same way?

@ids1024 ids1024 mentioned this pull request Jun 16, 2026
ids1024 added a commit that referenced this pull request Jun 16, 2026
I think #21 /
#15 and any extra checks can be
considered a non-breaking change added in a 0.7.x version, even if it
causes errors for invalid uses of the protocol.
@glamberson

Copy link
Copy Markdown
Contributor Author

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.
@glamberson glamberson force-pushed the track-pressed-keys-buttons branch from 6340b39 to 2f28447 Compare June 16, 2026 12:16
@glamberson

Copy link
Copy Markdown
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Track state of connections and devices in higher-level request/event helpers

2 participants