Skip to content
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.

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

InputCommon/evdev: Sort devices by their /dev/ name for consistent ordering. #8569

Open
wants to merge 1 commit into
base: master
from

Conversation

@jordan-woyak
Copy link
Member

jordan-woyak commented Jan 18, 2020

Fixes: https://bugs.dolphin-emu.org/issues/11917

Unfortunately udev seems to enumerate devices in a seemingly random order.
This is especially problematic for multi-port controller adapters.
Users expect our device list ordering to match the physical ports on their adapter.

This PR sorts evdev devices based on their /dev/ name.
I cannot find any other more reliable means of sorting.

@jordan-woyak jordan-woyak force-pushed the jordan-woyak:evdev-device-order branch from 60a86d4 to edb1677 Jan 18, 2020
std::map<std::string, std::string> devnodes;

// Pad the first occurance of digits for natural sorting,
// so /dev/input/event9 sorts before /dev/input/event10.

This comment has been minimized.

Copy link
@Warepire

Warepire Jan 19, 2020

Would it make more sense to sort by /dev/input/by-id/ or /dev/input/by-path/ ? According to what I can gather /dev/input/eventX is by order of discovery.

This comment has been minimized.

Copy link
@jordan-woyak

jordan-woyak Jan 24, 2020

Author Member

Order of discovery is desired. The first attached device should take the first slot and so on. A second device should never pop in before the first. I've updated the comment to make that more clear.

@phire

This comment has been minimized.

Copy link
Member

phire commented Jan 19, 2020

According to what I can gather /dev/input/eventX is by order of discovery

I assume in the case of multi-controller adapters, the discovery order will be linear.

I cannot find any other more reliable means of sorting.

Have you tried /sys/class/inputNN/phys? You should be able to find the correct sysfs directory with udev_device_get_syspath().
I don't have any multi-input controller adaptors, but phys seems like it does what you want.

Another advantage is that it's stable by USB port and even USB hub ordering. Should have the same ordering on every single boot.

@jordan-woyak jordan-woyak force-pushed the jordan-woyak:evdev-device-order branch from edb1677 to 0c5dd90 Jan 24, 2020
@jordan-woyak

This comment has been minimized.

Copy link
Member Author

jordan-woyak commented Jan 24, 2020

@phire I mentioned in IRC, but I'll post here for everyone else. All 4 ports of my GC Adapter return the same "phys" string so it can't be used. It also wouldn't give us the discovery order.

@phire

This comment has been minimized.

Copy link
Member

phire commented Jan 24, 2020

This solution makes me uneasy; It's not a stable API, just a side effect of some internal kernel code.

  1. A future Linux kernel update could break the initialization order and event number ordering consistency.
  2. There are only 31 event device nodes. They will wrap around at some point.

It's really annoying that the phys strings are identical.

One thing I did susggest on irc was using phys and then appending the event id as a fallback for when the phys strings was identical.

@jordan-woyak

This comment has been minimized.

Copy link
Member Author

jordan-woyak commented Jan 24, 2020

@phire I do agree that relying on some probably unspecified kernel behavior isn't ideal but "phys" isn't want we want anyways. We don't want to sort by some irrelevant physical property. We do want discovery order.

The firstly attached PS4 controller should take the first slot and so on. A secondly attached PS4 controller should not jump to the first slot because its USB port or MAC address or something compares less than that of the other controller.

"Wrap around" is a problem but this should still handle the main situation I am trying to address.
If you completely fill, then remove some nodes creating fragmentation; and then attach a 4-port GC adapter, at least those four ports should be relatively sorted.

@phire

This comment has been minimized.

Copy link
Member

phire commented Jan 24, 2020

It's not an irrelevant physical property, it's the USB port tree node, meaning it stays stable based on what USB host/hub port it's been plugged into.

Referencing the original bug, the other usecase is identical controllers plugged into different USB ports. Enumeration order is less than ideal for that usecase as it's different depending on if the users plugged devices in, or rebooted with all the devices already plugged in.

While phys stays stable in both situations.

If you completely fill, then remove some nodes creating fragmentation; and then attach a 4-port GC adapter, at least those four ports should be relatively sorted.

Or the first two ports could end up at 30 and 31, and the second two ports could be end up in lower gaps.

@jordan-woyak

This comment has been minimized.

Copy link
Member Author

jordan-woyak commented Jan 24, 2020

phire, The USB port is stable but we do not care which port it's plugged in to! We want discovery order for the reason I stated above: A secondly attached controller should not jump to the first slot just because it's in a lower USB port number.

Rebooting will change event nodes no doubt but it's not unreasonable to assign controllers to different slots after an entire reboot.

Or the first two ports could end up at 30 and 31, and the second two ports could be end up in lower gaps.

That's not the behavior I've observed.

@phire

This comment has been minimized.

Copy link
Member

phire commented Jan 25, 2020

Oh, you are talking about runtime user plugging order, while the bug report you reference is talking about initialization order during boot and wanting stable controller assignments across boots.

These are different usecases.

@jordan-woyak

This comment has been minimized.

Copy link
Member Author

jordan-woyak commented Jan 25, 2020

The logic affects both situations.

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

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.