Skip to content

Add PartialEq, Eq and Hash to DeviceInfo #154

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add PartialEq, Eq and Hash to DeviceInfo #154

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,7 @@ impl HidApi {
}

#[allow(dead_code)]
#[derive(Clone, PartialEq)]
#[derive(Clone, PartialEq, Eq, Hash)]
enum WcharString {
String(String),
#[cfg_attr(all(feature = "linux-native", target_os = "linux"), allow(dead_code))]
Expand All @@ -322,7 +322,7 @@ impl From<WcharString> for Option<String> {

/// The underlying HID bus type.
#[repr(C)]
#[derive(Copy, Clone, Debug)]
#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)]
pub enum BusType {
Unknown = 0x00,
Usb = 0x01,
Expand All @@ -336,7 +336,7 @@ pub enum BusType {
/// Note: Methods like `serial_number()` may return None, if the conversion to a
/// String failed internally. You can however access the raw hid representation of the
/// string by calling `serial_number_raw()`
#[derive(Clone)]
#[derive(Clone, PartialEq, Eq, Hash)]
Copy link
Owner

Choose a reason for hiding this comment

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

I am not sure if its a good idea to implement Eq operator directly for this struct.
Out of interest, what is you usecase for it?

Copy link
Author

@alvaro-cuesta alvaro-cuesta May 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

You mean Eq in particular or both PartialEq + Eq?

For Eq in particular I think the equality relation is total so you're supposed to implement Eq, right? So just semantics, no particular use case.

For rationale on PartialEq see comment in PR description. I'm already using it (and Hash through HashSet) to track devices over time.

Is there any other way to uniquely identify a device? Am I supposed to just compare all relevant fields individually?

Copy link
Author

@alvaro-cuesta alvaro-cuesta May 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Actually I think I'm using strict Eq too through HashSet::difference (bound on its impl Iterator).

Copy link
Owner

@ruabmbua ruabmbua May 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

My concern with implementing Eq (as an operator overload) is, that it is unclear what device equality means. If we implement it for the DeviceInfo struct, it just means that one instance is equal in terms of data representation to the

I fear people might misinterpret that as a way to check if two devices are the same, but due to differences in the platforms it might not quite reflect reality. Even if you check all the platform implementations now and see no problem, you have to take newer versions into account. Can you guarantee that all new fields we may want to add in the future will not break this assumption?

A much better way to do this would be an explicit method with documentation, that can be used to identify if two devices are the same. But even that is kind of hard to pull off, since two different applications might have different expectations about device equality.

USB is a bit of a pain in that regard in general, with devices having cloned serial numbers, certain hubs screwing up usb descriptors ....

Copy link
Owner

Choose a reason for hiding this comment

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

Maybe we want multiple methods. One of them for the more general case of trying to providing equality between actual physical devices. (e.g. detecting that the same keyboard was plugged into a different port, but you can still tell its the same).

Another that just looks at the topology / ports etc...

Copy link
Author

@alvaro-cuesta alvaro-cuesta May 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Great insights!

I didn't consider the "physical device" perspective -- I was thinking in hidapi hid_device_info terms, in particular because I've e.g. learnt not to trust S/N (it's "" for my particular device)...

The problem with having explicit methods instead of implementing traits is the consumer would still have to newtype if they want to use the struct for e.g. HashSet, but I see your point.

Just thinking out loud here: a solution that won't force consumer to newtype around the desired method might be to split some of the fields into a inner physical-device-related struct, so that consumer could use that one for Partial/Eq/Hash, or the whole DeviceInfo, as desired. That'd be a breaking change though, and I'm not sure it addresses your future-proofing concerns.

Would you still accept if I rework this PR only for WcharString and BusType, so I can easily newtype DeviceInfo for now? I think those should be safe, right?

Copy link
Author

@alvaro-cuesta alvaro-cuesta May 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

For reference: libusb/hidapi#291 implemented stable paths in hidapi libusb backend but signal11/hidapi#405 doesn't seem like it's been addressed in libusb/hidapi (and I guess just depends on whatever the underlying hidraw driver does). No idea about other backends' guarantees (I can only confirm Windows keep the path stable for device+USB port across reconnections).

This means equality in DeviceInfo would only make semantic sense if it is meant to have strictly the same meaning as hid_device_info in hidapi, i.e. identify a particular HID path+device (as in "connected device" this is what DeviceInfo is, am I right?), but cannot tell anything about physical devices or even reconnections of the same device on same USB port (would be nice to document this non-guarantee regardless of outcome).

DeviceInfo equality could still be useful for e.g. tracking this "connected device" across time. I think this would be needed for Windows hotplug which only reports that there are new devices, but not which (at least with WM_DEVICECHANGE, unsure about CM_Register_Notification), also might be useful for 1+2 here so you can rule out the duplicate. Whether that warrants implementing the traits is debatable.

pub struct DeviceInfo {
path: CString,
vendor_id: u16,
Expand Down