sidit77 / async-hid

A async Rust library for interacting with HID devices
MIT License
3 stars 2 forks source link

Hashing DeviceId #3

Closed zardini123 closed 4 months ago

zardini123 commented 6 months ago

Hi @sidit77! Thank you for this wonderful library.

I have been working to migrating one of my projects to using this. While migrating, I found DeviceId is not hashable.

My thought is, assuming there cannot be more than one unique device associated with the same DeviceId, a DeviceId represents a unique device. If a DeviceId represents a unique device, we can hash DeviceId to quickly know if an identical device is inside a hashing collection, such as a HashSet.

My reason why having DeviceId available in a hashing collection would be useful is for doing set operations on HID devices. For example, we can get a set of "unused" devices by taking the set difference of a set of "used" devices and all devices. This task can be done easily using a HashSet.

Research into Implementation

DeviceId is a tuple struct of BackendDeviceId. BackendDeviceId is whatever type that is set by the platform being used, as defined in src/backend/mod.rs.

https://github.com/sidit77/async-hid/blob/fc7a832ec965c2441b4a477bb6cd0d103ea898e7/src/lib.rs#L92-L95

hidraw

BackendDeviceId is a PathBuf which has Hash impl (source)

https://github.com/sidit77/async-hid/blob/fc7a832ec965c2441b4a477bb6cd0d103ea898e7/src/backend/hidraw/mod.rs#L163

iohidmanager

BackendDeviceId is a RegistryEntryId which is a async-hid tuple struct of a u64. Could derive Hash probably with a simple #derive change.

https://github.com/sidit77/async-hid/blob/fc7a832ec965c2441b4a477bb6cd0d103ea898e7/src/backend/iohidmanager/mod.rs#L192

https://github.com/sidit77/async-hid/blob/fc7a832ec965c2441b4a477bb6cd0d103ea898e7/src/backend/iohidmanager/service.rs#L64-L66

winrt

BackendDeviceId is a windows::core::HSTRING. Does not have a Hash impl directly, but could be converted to a OsString (which is hashable) using HSTRING's to_os_string function. Not sure if there is any losses or performance issues with this idea. A wrapper type would probably be required to implement Hash that calls to_os_string upon hashing, unless there is a more pragmatic idea.

https://github.com/sidit77/async-hid/blob/fc7a832ec965c2441b4a477bb6cd0d103ea898e7/src/backend/winrt/mod.rs#L172

zardini123 commented 6 months ago

I'd be interested in contributing changes once there is agreement on the general ideas 😄

sidit77 commented 6 months ago

My thought is, assuming there cannot be more than one unique device associated with the same DeviceId, a DeviceId represents a unique device.

I'm not sure how true this remains once you start plugging devices in and out. Irc on Linux the device id essentially boils down to some path like /dev/hid0 or /dev/hid1 and so forth. I don't know if that number monotonically increases or not. Probably worth doing some research.

On Windows I'm fairly certain that the id is actually unique.

The same goes for MacOS if I read the documentation correctly:

The entry ID returned by IORegistryEntryGetRegistryEntryID can be used to identify a registry entry across all tasks. A registry entry may be looked up by its entryID by creating a matching dictionary with IORegistryEntryIDMatching() to be used with the IOKit matching functions. The ID is valid only until the machine reboots.

BackendDeviceId is a windows::core::HSTRING. Does not have a Hash impl directly, but could be converted to a OsString (which is hashable) using HSTRING's to_os_string function. Not sure if there is any losses or performance issues with this idea. A wrapper type would probably be required to implement Hash that calls to_os_string upon hashing, unless there is a more pragmatic idea.

I don't think it's necessary to convert the HSTRING to an OsString. If you look at the hash implementation of OsString you'll see that it essentially just calls self.encoded_bytes().hash(hasher). It should be easy to do the same with HSTRING directly by calling self.as_wide().hash(hasher) instead.

But yeah, I think we would have to create a wrapper type for that.

zardini123 commented 6 months ago

Probably worth doing some research.

Would you think doing this research would be required before implementing Hash to DeviceId? I personally do not have the implements to test something like this, especially in a Linux context.

If we associate a unique DeviceId to be a unique device, and we assume there could be platforms where a DeviceId could be associated with two different devices, then would the property of equivalence not work either? How would you be able to differentiate those two devices? Personally at that point I would just notate the inconsistency of the platform in the documentation but keep the convenience of equivalence and hashing.

But yeah, I think we would have to create a wrapper type for that.

Sure thing. What would the naming convention for a type like this be?

sidit77 commented 6 months ago

I don't think that this is a blocker either. The id struct is opaque and platform dependent anyways so changing it later if/when it becomes a problem shouldn't even be a breaking change.

What would the naming convention for a type like this be?

HashableHSTRING? It's an internal implementation detail of the Windows backend so I don't think it really matters tbh as long as it's somewhat reasonable.

zardini123 commented 6 months ago

I realized that if Hash could be implemented in HSTRING directly, we wouldn't have to worry with an intermediate type. So I sent out a PR to windows-rs with a HSTRING hash impl. Once that is accepted, then changes can be made here to hash HSTRING directly.

The PR: https://github.com/microsoft/windows-rs/pull/2924

It seems the maintainers of windows-rs accept very quickly and release cycle is every couple days, so we wouldn't have to wait long.