| Message ID | 20210911173639.5688-1-djogorchock@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | HID: nintendo | expand |
On Sat, 11 Sep 2021, Daniel J. Ogorchock wrote: > Rebased onto Linus' tree (sha 926de8c4326c14fcf35f1de142019043597a4fac) > Depends on Roderick's patch to add the player LED defines: > https://patchwork.kernel.org/project/linux-input/patch/20210908165539.3102929-3-roderick.colenbrander@sony.com/ I just got Ack for the joydev part from Dmitry. v16 is now queued in hid.git#for-5.16/nintendo Thanks,
On Tue, 19 Oct 2021, Jiri Kosina wrote: > > Rebased onto Linus' tree (sha 926de8c4326c14fcf35f1de142019043597a4fac) > > Depends on Roderick's patch to add the player LED defines: > > https://patchwork.kernel.org/project/linux-input/patch/20210908165539.3102929-3-roderick.colenbrander@sony.com/ > > I just got Ack for the joydev part from Dmitry. > > v16 is now queued in hid.git#for-5.16/nintendo Benjamin noticed that I pushed wrong version of the branch -- the one that still doesn't contain the LED_FUNCTION_PLAYER[1-5] defines, which I've had staged here locally, waiting for Pavel's Ack (which is taking time, unfortunately). So please ignore this branch for now, I'll push v2 once that situation is cleared out. CCing Pavel as well here to make him aware of the issues this is causing all over the place (see .e.g my mail [1] from yesterday). [1] https://lore.kernel.org/all/nycvar.YFH.7.76.2110181739310.12554@cbobk.fhfr.pm/
On Tue, Oct 19, 2021 at 11:44 AM Jiri Kosina <jikos@kernel.org> wrote: > > On Tue, 19 Oct 2021, Jiri Kosina wrote: > > > > Rebased onto Linus' tree (sha 926de8c4326c14fcf35f1de142019043597a4fac) > > > Depends on Roderick's patch to add the player LED defines: > > > https://patchwork.kernel.org/project/linux-input/patch/20210908165539.3102929-3-roderick.colenbrander@sony.com/ > > > > I just got Ack for the joydev part from Dmitry. > > > > v16 is now queued in hid.git#for-5.16/nintendo > > Benjamin noticed that I pushed wrong version of the branch -- the one that > still doesn't contain the LED_FUNCTION_PLAYER[1-5] defines, which I've had > staged here locally, waiting for Pavel's Ack (which is taking time, > unfortunately). > > So please ignore this branch for now, I'll push v2 once that situation is > cleared out. > > CCing Pavel as well here to make him aware of the issues this is causing > all over the place (see .e.g my mail [1] from yesterday). > > [1] https://lore.kernel.org/all/nycvar.YFH.7.76.2110181739310.12554@cbobk.fhfr.pm/ > I am also jumping on this to raise a concern I recently had with all of the work we do for HID devices in the kernel regarding game controllers. Foreword: this is definitely not a NACK on the series, but more trying to open a discussion to take a step back. The SDL developers recently took a hard turn in how they are handling game controllers on Linux: libsdl now directly talks to hidraw or libusb to handle the controllers in user space, bypassing the kernel processing. Which means that a game working on a recent SDL release already has the PS5 player LEDs ready for instance. I think that part of this work was the integration of the steam client database, which already supports a lot of fix ups for game controllers. So I am starting to wonder if we are not adding dead code in the kernel, because both steam and SDL will disable the input handling of the controllers when they open the hidraw node (IIRC about what was done in this series). I know that Android/Chrome is interested in having actual input nodes created, but are they the sole users of those now? What is the benefit of having game controllers in the kernel? I am opening the discussion and we probably want to bring in the SDL folks here too. But I'd like to have some sort of confirmation that these series which are adding game controllers are not just adding dead code. Cheers, Benjamin
Hi Benjamin, On Tue, Oct 19, 2021 at 6:19 AM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > On Tue, Oct 19, 2021 at 11:44 AM Jiri Kosina <jikos@kernel.org> wrote: > > > > On Tue, 19 Oct 2021, Jiri Kosina wrote: > > > > > > Rebased onto Linus' tree (sha 926de8c4326c14fcf35f1de142019043597a4fac) > > > > Depends on Roderick's patch to add the player LED defines: > > > > https://patchwork.kernel.org/project/linux-input/patch/20210908165539.3102929-3-roderick.colenbrander@sony.com/ > > > > > > I just got Ack for the joydev part from Dmitry. > > > > > > v16 is now queued in hid.git#for-5.16/nintendo > > > > Benjamin noticed that I pushed wrong version of the branch -- the one that > > still doesn't contain the LED_FUNCTION_PLAYER[1-5] defines, which I've had > > staged here locally, waiting for Pavel's Ack (which is taking time, > > unfortunately). > > > > So please ignore this branch for now, I'll push v2 once that situation is > > cleared out. > > > > CCing Pavel as well here to make him aware of the issues this is causing > > all over the place (see .e.g my mail [1] from yesterday). > > > > [1] https://lore.kernel.org/all/nycvar.YFH.7.76.2110181739310.12554@cbobk.fhfr.pm/ > > > > I am also jumping on this to raise a concern I recently had with all > of the work we do for HID devices in the kernel regarding game > controllers. > > Foreword: this is definitely not a NACK on the series, but more trying > to open a discussion to take a step back. > > The SDL developers recently took a hard turn in how they are handling > game controllers on Linux: libsdl now directly talks to hidraw or > libusb to handle the controllers in user space, bypassing the kernel > processing. > Which means that a game working on a recent SDL release already has > the PS5 player LEDs ready for instance. > > I think that part of this work was the integration of the steam client > database, which already supports a lot of fix ups for game > controllers. > > So I am starting to wonder if we are not adding dead code in the > kernel, because both steam and SDL will disable the input handling of > the controllers when they open the hidraw node (IIRC about what was > done in this series). > > I know that Android/Chrome is interested in having actual input nodes > created, but are they the sole users of those now? > What is the benefit of having game controllers in the kernel? > > I am opening the discussion and we probably want to bring in the SDL > folks here too. But I'd like to have some sort of confirmation that > these series which are adding game controllers are not just adding > dead code. > > Cheers, > Benjamin > I agree that interactions with sdl are in a strange state where it essentially bypasses kernel control over the controller. I'm not really a fan of that technique when there's a specific kernel driver, since it removes the ability for multiple applications to share the input (e.g. if I wanted to bind one of the buttons on my controller as a push-to-talk key in voip software while still using the controller in a game). I understand why this technique was used, since it allows for predictable cross-platform code from the sdl perspective. The patch series follows the example of the hid-sony driver in modifying the version field to allow userspace to detect the use of the kernel driver, which gives userspace the option of behaving differently if a kernel hid driver is in use for a game controller. That said, I don't believe kernel game controller drivers are amounting to dead code, since there are userspace applications using evdev rather than libsdl for their controller inputs. A couple examples specific to hid-nintendo: https://github.com/DanielOgorchock/joycond https://github.com/joaorb64/joycond-cemuhook I know that several people use these kernel drivers alongside game console emulators with evdev backends (e.g. dolphin, higan, etc.). I'd personally like to see fewer cases of applications taking sole control over the input devices, maybe keying off the version field change mentioned above to determine their behavior. I understand that is a tougher sell when the userspace driver implementation predates the kernel driver's existence. - Daniel Ogorchock
On Tue, 19 Oct 2021, Jiri Kosina wrote: > Benjamin noticed that I pushed wrong version of the branch -- the one that > still doesn't contain the LED_FUNCTION_PLAYER[1-5] defines, which I've had > staged here locally, waiting for Pavel's Ack (which is taking time, > unfortunately). > > So please ignore this branch for now, I'll push v2 once that situation is > cleared out. > > CCing Pavel as well here to make him aware of the issues this is causing > all over the place (see .e.g my mail [1] from yesterday). > > [1] https://lore.kernel.org/all/nycvar.YFH.7.76.2110181739310.12554@cbobk.fhfr.pm/ As we got Ack for the LED_FUNCTION_PLAYER[1-5] patch, this has now been revived and version that actually builds pushed to hid.git. Thanks,
Rebased onto Linus' tree (sha 926de8c4326c14fcf35f1de142019043597a4fac) Depends on Roderick's patch to add the player LED defines: https://patchwork.kernel.org/project/linux-input/patch/20210908165539.3102929-3-roderick.colenbrander@sony.com/ Version 16 changes: - Use latest LED_FUNCTION_PLAYERX defines for player/home leds Version 15 changes: - fixed minor oversight in how rumble rate limiting was being handled Version 14 changes: - Use proper LED classdev name scheme - Prevent situations where a missed zero amplitude rumble packet would leave the controller stuck vibrating until a timeout - Introduce a max rate at which subcommands or rumble packets can be sent to the controller. This reduces bluetooth disconnects. - Send rumble packets immediately after receiving input reports. This reduces bluetooth disconnects (similar technique was already used for subcommands). Version 13 changes: - Switched to using the dedicated rumble data message type, rather than constantly resending the rumble enabled subcommand. This more closely resembles how the console itself handles rumble data. - Applied revisions based on Silvan Jegen's feedback on v12. Version 12 changes: - Added support for reading user calibration from the controller's SPI flash (written when someone calibrates the controller on the Nintendo switch). - Added patch to prevent sending rumble subcommands when no effect is being played. This turned out to drastically improve bluetooth connection reliability. - Set the battery description to POWER_SUPPLY_TYPE_BATTERY (was missing in previous revisions due to oversight). This fixes problems with desktop environments not handling the controller batteries properly. - Reintroduced IMU patch with improvements to documentation, packet drop handling, and increased precision for gyro readings. Also now blacklists the IMU input dev from joydev like hid-sony. Version 11 changes: - Removed IMU patch for now, since it has some issues to work out. - Fixed bug introduced in v10 which led to the joy-cons' S-triggers not being configured as an input. - Changed the pro controller's d-pad input from buttons to a hat to be more in line with other controller drivers. Version 10 changes: - Removed duplicate reporting of one of the triggers that Billy noticed - The joy-cons now only report having the buttons they actually have (they used to register the input devices with the buttons of the other joy-con as well). - The input device is now created after the LEDs/power supply. - The removed state handling bool has been removed, instead opting to add a new controller state (removed). - Eliminated a 1 second delay when probing a USB controller. - Added support for the IMU. This mostly consisted of merging in some work provided by Carl. I'm not incredibly familiar with proper gyro/accelerometer handling in linux, so this might need some tweaking. Preliminary tests in evtest show the gyro/accel values being reported. - Added support for the joy-con USB charging grip. Version 9 changes: - Fixed compiler errors on gcc versions older than 8.2 - Set input device's uniq value to the controller's MAC address Version 8 changes: - Corrected the handshaking protocol with USB pro controllers. A handshake now occurs both prior and after the baudrate set. This doesn't appear to have a noticeable difference, but it more accurately follows documentation found online. - Fixed potential race condition which could lead to a slightly longer delay sending subcommands in rare circumstances. - Moved the rumble worker to its own workqueue, since it can block. This prevents it from having a negative impact on the default kernel workqueue. It also prevents dropped subcommands due to something else blocking the kernel workqueue. The benefit is most obvious when using multiple controllers at once, since the controller subcommand timings are very picky. - Added a patch to set the most significant bit of the hid hw version. Roderick had mentioned needing to probably do this awhile ago, but I had forgotten about it until now. This is the same thing hid-sony does. It allows SDL2 to have different mappings for the hid-nintendo driver vs the default hid mappings. Version 7 changes: - Changed name to hid-nintendo to fit modern naming conventions - Removed joycon_ctlr_destroy(), since it wasn't needed an could potentially invalidate a mutex while it could be in use on other threads - Implemented minor code improvements suggested by Silvan - The driver now waits to send subcommands until after receiving an input report. This significantly reduces dropped subcommands. - Reduced the number of error messages when disconnecting a controller. Version 6 changes: - Improved subcommand sending reliabilty - Decreased rumble period to 50ms - Added rumble queue to avoid missing ff_effects if sent too quickly - Code cleanup and minor refactoring - Added default analog stick calibration Version 5 changes: - Removed sysfs interface to control motor frequencies. - Improved rumble reliability by using subcommands to set it. - Changed mapping of the SL/SR triggers on the joy-cons to map to whichever triggers they lack (e.g. a left joycon's sl/sr map to TR and TR2). This allows userspace to distinguish between the normal and S triggers. - Minor refactors Version 4 changes: - Added support for the Home button LED for the pro controller and right joy-con - Changed name from hid-switchcon to hid-joycon - Added rumble support - Removed ctlr->type and use hdev->product instead - Use POWER_SUPPLY_CAPACITY_LEVEL enum instead of manually translating to capacity percentages - Misc. minor refactors based on v3 feedback Version 3 changes: - Added led_classdev support for the 4 player LEDs - Added power_supply support for the controller's battery - Made the controller number mutex static - Minor refactoring/style fixes based on Roderick's feedback from v2 Version 2 changes: - Switched to using a synchronous method for configuring the controller. - Removed any pairing/orientation logic in the driver. Every controller now corresponds to its own input device. - Store controller button data as a single u32. - Style corrections Daniel J. Ogorchock (16): HID: nintendo: add nintendo switch controller driver HID: nintendo: add player led support HID: nintendo: add power supply support HID: nintendo: add home led support HID: nintendo: add rumble support HID: nintendo: improve subcommand reliability HID: nintendo: send subcommands after receiving input report HID: nintendo: reduce device removal subcommand errors HID: nintendo: patch hw version for userspace HID mappings HID: nintendo: set controller uniq to MAC HID: nintendo: add support for charging grip HID: nintendo: add support for reading user calibration HID: nintendo: add IMU support HID: nintendo: improve rumble performance and stability HID: nintendo: ratelimit subcommands and rumble HID: nintendo: prevent needless queueing of the rumble worker MAINTAINERS | 6 + drivers/hid/Kconfig | 24 + drivers/hid/Makefile | 1 + drivers/hid/hid-ids.h | 4 + drivers/hid/hid-nintendo.c | 2319 ++++++++++++++++++++++++++++++++++++ drivers/input/joydev.c | 10 + 6 files changed, 2364 insertions(+) create mode 100644 drivers/hid/hid-nintendo.c