keyboard_states: use HashMap #216
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ConcurrentCrab
force-pushed
the
udev_async
branch
from
64c0db7 to
c74a43e
Compare
keyboard_stream_map and keyboard_states have to be kept in sync, but beacause of states being a vector, it had different semantics during removal. If 2 devices were added and the first was removed then the vector shifts the second device to the front but the map obviously did not, leading to the collections becoming out of sync. This cause out-of-bounds errors on the states vector when accessing it using stream_map's keys. This patch changes keyboard_states to be a map as well, and both maps are indexed using the udev input path as a key. A string key is not ideal, but is close to being the only unique ID for a connected device (physical path is not made serialised in many udev events for reasons unknown.), besides being easily accessible from both disjoint APIs being used here, evdev and tokio_udev. An additional concern may be that tokio's StreamMap manages its own members and removes streams upon completion, but: a) We don't index into stream_map at all, only consume from it, so this will never be an issue b) the corresponding entry in states will be removed as well upon the udev remove event
ConcurrentCrab
force-pushed
the
udev_async
branch
from
c74a43e to
ee173d6
Compare
|
Force-pushed some tweaks with logging and error handling. |
Shinyzenith
approved these changes
Jul 19, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
keyboard_stream_map and keyboard_states have to be kept in sync, but beacause of states being a vector, it had different semantics during removal. If 2 devices were added and the first was removed then the vector shifts the second device to the front but the map obviously did not, leading to the collections becoming out of sync. This cause out-of-bounds errors on the states vector when accessing it using stream_map's keys.
This patch changes keyboard_states to be a map as well, and both maps are indexed using the udev input path as a key. A string key is not ideal, but is close to being the only unique ID for a connected device (physical path is not made serialised in many udev events for reasons unknown.), besides being easily accessible from both disjoint APIs being used here, evdev and tokio_udev.
An additional concern may be that tokio's StreamMap manages its own members and removes streams upon completion, but:
a) We don't index into stream_map at all, only consume from it,
so this will never be an issue
b) the corresponding entry in states will be removed as well upon
the udev remove event