Linux hotplug support#341
Conversation
|
Decision: APPROVE Issues: none Evidence:
Review Details
|
|
Nice! I'll try to get some Kodi time this week for review and testing. |
|
Other than the minor issues the code looks great. If I get a chance I'll test on Linux, but we might merge with just your own testing success. |
|
Glad to here it. In the mean time, I'll continue long term testing. |
|
Actually, I did some real spammy testing and I was able to cause it to crash twice from the same action, but it's inconsistent and took a lot. It specifically crashed after I switched from the linux driver to the udev driver and then power cycled my controller, and on connect kodi crashed. The logs don't show the error, but that last lines were If it crashed after mapping the controller, i am not sure if it's from my changes, but I am going to do some more testing to determine if this is somehow from my changes. |
|
After some more replications I determined it can crash on both the linux and udev drivers. It also not limited to just after switching. At this point the issue was a bit beyond me so I threw claude at the problem, so take what comes next with a grain of salt, but claude is surprisingly good at debugging. After giving claude the crash stack trace it determined that this was not a bug in the new hotplug feature, but a race condition in main kodi, but the hotplug feature made it much easier to hit. Now if this is a core issue then this is not the correct place to report a bug, but I want a second opinion on if this really is the issue and not my hotplug code. This is claude's description and patch made. After 10 minute of trying to replicate with this patch i was unable to replicate the issue. patch: |
|
After some more testing and digging into this crash myself I have concluded it was in fact a race condition in kodi, and not an issue with my changes. Hotplugging just made it much easier to cause the crash. I have created a PR for a fix over there PR for the issue. In other news that issue resulted in some serious stress testing of the my hotplug functionality and it held up quite well. |
|
Thanks for getting the other fixes in. Is this good to go? |
|
This has had plenty of testing on my part. Yeah it's good to go. |
There was a problem hiding this comment.
Pull request overview
Adds Linux hotplug support so joystick connect/disconnect events trigger immediate rescans (removing the current periodic-scan delay) for both the legacy Linux joystick driver (inotify on /dev/input) and the udev-based driver (udev monitor).
Changes:
- Add background monitor threads to Linux (
inotify) and udev (udev_monitor) joystick interfaces that callCJoystickManager::SetChanged(true)+TriggerScan()on hotplug events. - Add
eventfd-based shutdown signaling for both monitor threads to unblockpoll()during deinit. - Link against
Threads::Threadsin CMake.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/api/udev/JoystickInterfaceUdev.h | Adds thread/stop state members and monitor thread method declaration. |
| src/api/udev/JoystickInterfaceUdev.cpp | Starts/stops udev monitor thread; polls udev fd and triggers joystick rescans on add/remove. |
| src/api/linux/JoystickInterfaceLinux.h | Adds init/deinit + monitor thread state for inotify-based hotplug. |
| src/api/linux/JoystickInterfaceLinux.cpp | Implements inotify watcher thread for /dev/input and triggers rescans on js* changes. |
| CMakeLists.txt | Adds Threads dependency and links Threads::Threads. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
kodiai responseGuilty as charged on the typo. Fixed all issues: (1) Typo: Evidence:
Ready for merge. |
Watch /dev/input for linux driver Use udev_monitor_get_fd for udev driver Fallback to regular polling if watch fails
|
Fixes are up. |
|
@kodiai please review |
|
Decision: APPROVE Issues: none Evidence:
|
|
Adding to LE: LibreELEC/LibreELEC.tv#11503 |
Adds hotplug support to the two Linux drivers. This removes the up to 5 second delay from controller connect to inputs being registered.
For the Linux variant I used inotify_add_watch to watch the /dev/input directory. Any joystick files changes will trigger a rescan.
For the udev variant, I implemented the built in monitoring system in udev.
Both will create a background thread that listen to events in the relevant file descriptors, and new events may result in
CJoystickManager::Get().SetChanged(true); CJoystickManager::Get().TriggerScan();getting called.
Why add this? Thanks to the better wayland HDR support added in Piers alphas, I have been able to move from windows to Linux on my main content machine. Windows has the quick hot plug support and moving to Linux where it does not exist was quite obvious, and when the controller powers off regularly across and in the middle of TV episodes it is tedious having to wait where I use to not, and I dearly missed the quick response I had on windows.
I have tested both of these methods on my Arch machine.