mbox series

[v11,00/11] HID: nintendo

Message ID 20200317032928.546172-1-djogorchock@gmail.com (mailing list archive)
Headers show
Series HID: nintendo | expand

Message

Daniel Ogorchock March 17, 2020, 3:29 a.m. UTC
I removed the IMU patch for now, since I have some more work on it to do
before it's ready. This version fixes a bug with joy-con S-trigger
configuration and switches the pro controller's Dpad input to a hat.

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 (11):
  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

 MAINTAINERS                |    6 +
 drivers/hid/Kconfig        |   24 +
 drivers/hid/Makefile       |    1 +
 drivers/hid/hid-ids.h      |    4 +
 drivers/hid/hid-nintendo.c | 1665 ++++++++++++++++++++++++++++++++++++
 5 files changed, 1700 insertions(+)
 create mode 100644 drivers/hid/hid-nintendo.c

Comments

Benjamin Tissoires April 1, 2020, 4:27 p.m. UTC | #1
Hi Daniel,

On Tue, Mar 17, 2020 at 4:30 AM Daniel J. Ogorchock
<djogorchock@gmail.com> wrote:
>
> I removed the IMU patch for now, since I have some more work on it to do
> before it's ready. This version fixes a bug with joy-con S-trigger
> configuration and switches the pro controller's Dpad input to a hat.

Thanks a lot for the continuous effort. I just had a quick look
through the series, and nothing came up problematic. I have requested
a few tests from people I know, and I'll be happy to merge this for
v5.8. Unless Jiri says that 5.7 is OK, but I doubt Linus will be happy
getting a new driver now that hasn't spent a little time in
linux-next.

Cheers,
Benjamin

>
> 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 (11):
>   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
>
>  MAINTAINERS                |    6 +
>  drivers/hid/Kconfig        |   24 +
>  drivers/hid/Makefile       |    1 +
>  drivers/hid/hid-ids.h      |    4 +
>  drivers/hid/hid-nintendo.c | 1665 ++++++++++++++++++++++++++++++++++++
>  5 files changed, 1700 insertions(+)
>  create mode 100644 drivers/hid/hid-nintendo.c
>
> --
> 2.25.1
>
Jiri Kosina April 3, 2020, 1:16 p.m. UTC | #2
On Wed, 1 Apr 2020, Benjamin Tissoires wrote:

> > I removed the IMU patch for now, since I have some more work on it to do
> > before it's ready. This version fixes a bug with joy-con S-trigger
> > configuration and switches the pro controller's Dpad input to a hat.
> 
> Thanks a lot for the continuous effort. I just had a quick look through 
> the series, and nothing came up problematic. I have requested a few 
> tests from people I know, and I'll be happy to merge this for v5.8. 
> Unless Jiri says that 5.7 is OK, but I doubt Linus will be happy getting 
> a new driver now that hasn't spent a little time in linux-next.

Yeah, let's queue this (once you have positive testing results from your 
testers) for 5.8.

Thanks,
Bastien Nocera April 25, 2020, 1:01 p.m. UTC | #3
Hey Daniel,

On Mon, 2020-03-16 at 22:29 -0500, Daniel J. Ogorchock wrote:
> I removed the IMU patch for now, since I have some more work on it to
> do
> before it's ready. This version fixes a bug with joy-con S-trigger
> configuration and switches the pro controller's Dpad input to a hat.

I don't have a Nintendo Switch Pro controller, but I recently got an
8Bitdo "GBros" adapter along with a third-party GameCube adapter for
less than 20€, and tried out the "Switch" mode against your driver.

When plugging it in via USB:
[  593.228423] input: Nintendo Co., Ltd. Pro Controller as /devices/pci0000:00/0000:00:14.0/usb3/3-1/3-1:1.0/0003:057E:2009.0007/input/input34
[  593.229859] hid-generic 0003:057E:2009.0007: input,hidraw4: USB HID v1.11 Joystick [Nintendo Co., Ltd. Pro Controller] on usb-0000:00:14.0-1/input0
[  593.317011] nintendo 0003:057E:2009.0007: hidraw4: USB HID v81.11 Joystick [Nintendo Co., Ltd. Pro Controller] on usb-0000:00:14.0-1/input0
[  595.476014] nintendo 0003:057E:2009.0007: Failed to set baudrate; ret=-110
[  595.485961] nintendo 0003:057E:2009.0007: probe - fail = -110
[  595.486019] nintendo: probe of 0003:057E:2009.0007 failed with error -110

After pairing it with Bluetooth:
[ 3974.268521] nintendo 0005:057E:2009.0009: unknown main item tag 0x0
[ 3974.271295] nintendo 0005:057E:2009.0009: hidraw4: BLUETOOTH HID v80.01 Gamepad [Pro Controller] on 44:85:00:1e:ab:f8
[ 3976.494269] nintendo 0005:057E:2009.0009: controller MAC = 98:B6:E9:42:A8:DD
[ 3977.036391] nintendo 0005:057E:2009.0009: Failed to set home LED dflt; ret=-110
[ 3977.036402] nintendo 0005:057E:2009.0009: Failed to create leds; ret=-110
[ 3977.049328] nintendo 0005:057E:2009.0009: probe - fail = -110
[ 3977.564465] leds 0005:057E:2009.0009:home: Setting an LED's brightness failed (-110)
[ 3978.076440] leds 0005:057E:2009.0009:player4: Setting an LED's brightness failed (-110)
[ 3978.589367] leds 0005:057E:2009.0009:player3: Setting an LED's brightness failed (-110)
[ 3979.101347] leds 0005:057E:2009.0009:player2: Setting an LED's brightness failed (-110)
[ 3979.612308] leds 0005:057E:2009.0009:player1: Setting an LED's brightness failed (-110)
[ 3979.612977] nintendo: probe of 0005:057E:2009.0009 failed with error -110

I'm guessing the LED and baudrate setting failures should probably be
non-fatal. Let me know if there's anything else you'd like me to test.

For Fedora users who want to try out this v11 version of the driver:
https://koji.fedoraproject.org/koji/taskinfo?taskID=43730136
(note that the build will be removed in a bit less than a week)

Cheers
Roderick Colenbrander April 26, 2020, 8:42 p.m. UTC | #4
On Sat, Apr 25, 2020 at 6:01 AM Bastien Nocera <hadess@hadess.net> wrote:
>
> Hey Daniel,
>
> On Mon, 2020-03-16 at 22:29 -0500, Daniel J. Ogorchock wrote:
> > I removed the IMU patch for now, since I have some more work on it to
> > do
> > before it's ready. This version fixes a bug with joy-con S-trigger
> > configuration and switches the pro controller's Dpad input to a hat.
>
> I don't have a Nintendo Switch Pro controller, but I recently got an
> 8Bitdo "GBros" adapter along with a third-party GameCube adapter for
> less than 20€, and tried out the "Switch" mode against your driver.
>
> When plugging it in via USB:
> [  593.228423] input: Nintendo Co., Ltd. Pro Controller as /devices/pci0000:00/0000:00:14.0/usb3/3-1/3-1:1.0/0003:057E:2009.0007/input/input34
> [  593.229859] hid-generic 0003:057E:2009.0007: input,hidraw4: USB HID v1.11 Joystick [Nintendo Co., Ltd. Pro Controller] on usb-0000:00:14.0-1/input0
> [  593.317011] nintendo 0003:057E:2009.0007: hidraw4: USB HID v81.11 Joystick [Nintendo Co., Ltd. Pro Controller] on usb-0000:00:14.0-1/input0
> [  595.476014] nintendo 0003:057E:2009.0007: Failed to set baudrate; ret=-110
> [  595.485961] nintendo 0003:057E:2009.0007: probe - fail = -110
> [  595.486019] nintendo: probe of 0003:057E:2009.0007 failed with error -110
>
> After pairing it with Bluetooth:
> [ 3974.268521] nintendo 0005:057E:2009.0009: unknown main item tag 0x0
> [ 3974.271295] nintendo 0005:057E:2009.0009: hidraw4: BLUETOOTH HID v80.01 Gamepad [Pro Controller] on 44:85:00:1e:ab:f8
> [ 3976.494269] nintendo 0005:057E:2009.0009: controller MAC = 98:B6:E9:42:A8:DD
> [ 3977.036391] nintendo 0005:057E:2009.0009: Failed to set home LED dflt; ret=-110
> [ 3977.036402] nintendo 0005:057E:2009.0009: Failed to create leds; ret=-110
> [ 3977.049328] nintendo 0005:057E:2009.0009: probe - fail = -110
> [ 3977.564465] leds 0005:057E:2009.0009:home: Setting an LED's brightness failed (-110)
> [ 3978.076440] leds 0005:057E:2009.0009:player4: Setting an LED's brightness failed (-110)
> [ 3978.589367] leds 0005:057E:2009.0009:player3: Setting an LED's brightness failed (-110)
> [ 3979.101347] leds 0005:057E:2009.0009:player2: Setting an LED's brightness failed (-110)
> [ 3979.612308] leds 0005:057E:2009.0009:player1: Setting an LED's brightness failed (-110)
> [ 3979.612977] nintendo: probe of 0005:057E:2009.0009 failed with error -110
>
> I'm guessing the LED and baudrate setting failures should probably be
> non-fatal. Let me know if there's anything else you'd like me to test.
>
> For Fedora users who want to try out this v11 version of the driver:
> https://koji.fedoraproject.org/koji/taskinfo?taskID=43730136
> (note that the build will be removed in a bit less than a week)
>
> Cheers

I really wonder how a device like this should be handled. It looks
like the device can also handle a bunch of other classic Nintendo
controllers.

Is there anyway of detection this adapter? It feels nasty to have to
hack in quirks for this device...

Thanks,
Roderick
Bastien Nocera April 26, 2020, 9:14 p.m. UTC | #5
On Sun, 2020-04-26 at 13:42 -0700, Roderick Colenbrander wrote:
> 
<snip>
> I really wonder how a device like this should be handled. It looks
> like the device can also handle a bunch of other classic Nintendo
> controllers.
> 
> Is there anyway of detection this adapter? It feels nasty to have to
> hack in quirks for this device...

The end game isn't very different from how we handle XBox 360, or
PS3/PS4 "clone" devices.

Those devices (the adapters) work on the actual Nintendo Switch
hardware, so should probably work with the driver that handles the same
type of hardware on Linux.
Roderick Colenbrander April 26, 2020, 10:31 p.m. UTC | #6
On Sun, Apr 26, 2020 at 2:14 PM Bastien Nocera <hadess@hadess.net> wrote:
>
> On Sun, 2020-04-26 at 13:42 -0700, Roderick Colenbrander wrote:
> >
> <snip>
> > I really wonder how a device like this should be handled. It looks
> > like the device can also handle a bunch of other classic Nintendo
> > controllers.
> >
> > Is there anyway of detection this adapter? It feels nasty to have to
> > hack in quirks for this device...
>
> The end game isn't very different from how we handle XBox 360, or
> PS3/PS4 "clone" devices.
>
> Those devices (the adapters) work on the actual Nintendo Switch
> hardware, so should probably work with the driver that handles the same
> type of hardware on Linux.
>

(resend in plain text)

I agree it probably makes sense to handle in this driver. I'm worried
about the application level implications. We have been doing a lot of
work e.g. on Android to try to make gamepads consistent. It is weird
to have a "Switch controller" with different features as applications
make assumptions and don't expect there to be multiple versions of a
particular controller. Any button mapping files would potentially be
wrong for those too, certain features are not there (e.g. no sensors
or no lights or rumble) or if they are the behaviour is different
(e.g. HD rumble vs a classic rumble motor).

Ideally we would do some kind of "fixup" to change the device name and
or replace the device ids to at least separate them.

Roderick
Bastien Nocera April 27, 2020, 8:56 a.m. UTC | #7
On Sun, 2020-04-26 at 15:31 -0700, Roderick Colenbrander wrote:
> On Sun, Apr 26, 2020 at 2:14 PM Bastien Nocera <hadess@hadess.net>
> wrote:
> > On Sun, 2020-04-26 at 13:42 -0700, Roderick Colenbrander wrote:
> > <snip>
> > > I really wonder how a device like this should be handled. It
> > > looks
> > > like the device can also handle a bunch of other classic Nintendo
> > > controllers.
> > > 
> > > Is there anyway of detection this adapter? It feels nasty to have
> > > to
> > > hack in quirks for this device...
> > 
> > The end game isn't very different from how we handle XBox 360, or
> > PS3/PS4 "clone" devices.
> > 
> > Those devices (the adapters) work on the actual Nintendo Switch
> > hardware, so should probably work with the driver that handles the
> > same
> > type of hardware on Linux.
> > 
> 
> (resend in plain text)
> 
> I agree it probably makes sense to handle in this driver. I'm worried
> about the application level implications. We have been doing a lot of
> work e.g. on Android to try to make gamepads consistent. It is weird
> to have a "Switch controller" with different features as applications
> make assumptions and don't expect there to be multiple versions of a
> particular controller. Any button mapping files would potentially be
> wrong for those too, certain features are not there (e.g. no sensors
> or no lights or rumble) or if they are the behaviour is different
> (e.g. HD rumble vs a classic rumble motor).
> 
> Ideally we would do some kind of "fixup" to change the device name
> and
> or replace the device ids to at least separate them.

All those would be detectable at runtime. I'm not sure that it's ever a
good idea to presume that a particular VID/PID combination will have,
say, rumble and LEDs available when the driver can answer those
questions.

For example, I'm not sure that those controllers have either features
(though I'm not certain they identify as Switch Pro controllers, but
for the sake of argument):
https://store.nintendo.com/super-nintendo-entertainment-system-controller.html
https://store.nintendo.com/nintendo-entertainment-system-controllers.html

Cheers
Daniel Ogorchock May 22, 2020, 7:11 p.m. UTC | #8
Hi Bastien,

Apologies for the late reply. This thread sneaked past me somehow. If
we want to handle clone controllers with partial protocol
implementations, is it preferable to present them identically to
userspace, with non-existent functionality being no-ops? Or would it
be better to just not create the interfaces for missing functionality
(e.g. not create the led_classdevs for controllers without LEDs)? I
assume the latter makes more sense, since it doesn't lie to userspace.
Though it could potentially make the driver code messier.

Thanks,
Daniel

On Mon, Apr 27, 2020 at 3:56 AM Bastien Nocera <hadess@hadess.net> wrote:
>
> On Sun, 2020-04-26 at 15:31 -0700, Roderick Colenbrander wrote:
> > On Sun, Apr 26, 2020 at 2:14 PM Bastien Nocera <hadess@hadess.net>
> > wrote:
> > > On Sun, 2020-04-26 at 13:42 -0700, Roderick Colenbrander wrote:
> > > <snip>
> > > > I really wonder how a device like this should be handled. It
> > > > looks
> > > > like the device can also handle a bunch of other classic Nintendo
> > > > controllers.
> > > >
> > > > Is there anyway of detection this adapter? It feels nasty to have
> > > > to
> > > > hack in quirks for this device...
> > >
> > > The end game isn't very different from how we handle XBox 360, or
> > > PS3/PS4 "clone" devices.
> > >
> > > Those devices (the adapters) work on the actual Nintendo Switch
> > > hardware, so should probably work with the driver that handles the
> > > same
> > > type of hardware on Linux.
> > >
> >
> > (resend in plain text)
> >
> > I agree it probably makes sense to handle in this driver. I'm worried
> > about the application level implications. We have been doing a lot of
> > work e.g. on Android to try to make gamepads consistent. It is weird
> > to have a "Switch controller" with different features as applications
> > make assumptions and don't expect there to be multiple versions of a
> > particular controller. Any button mapping files would potentially be
> > wrong for those too, certain features are not there (e.g. no sensors
> > or no lights or rumble) or if they are the behaviour is different
> > (e.g. HD rumble vs a classic rumble motor).
> >
> > Ideally we would do some kind of "fixup" to change the device name
> > and
> > or replace the device ids to at least separate them.
>
> All those would be detectable at runtime. I'm not sure that it's ever a
> good idea to presume that a particular VID/PID combination will have,
> say, rumble and LEDs available when the driver can answer those
> questions.
>
> For example, I'm not sure that those controllers have either features
> (though I'm not certain they identify as Switch Pro controllers, but
> for the sake of argument):
> https://store.nintendo.com/super-nintendo-entertainment-system-controller.html
> https://store.nintendo.com/nintendo-entertainment-system-controllers.html
>
> Cheers
>
Pierre-Loup A. Griffais July 22, 2020, 6:54 p.m. UTC | #9
Hi Daniel,

Sorry for hijacking this branch of the thread (it's the last one that 
survived my inbox) - it seems like merging this driver as-is would break 
Steam, according to user reports.

Is there any mechanism built into this hid_nintendo patch series to duck 
out of the way if userland directly opens the underlying hidraw device? 
That's what hid_steam does to coexist peacefully with userspace drivers 
(Steam being one of them, but not the only one).

Thanks,
  - Pierre-Loup

On 5/22/20 12:11 PM, Daniel Ogorchock wrote:
> Hi Bastien,
> 
> Apologies for the late reply. This thread sneaked past me somehow. If
> we want to handle clone controllers with partial protocol
> implementations, is it preferable to present them identically to
> userspace, with non-existent functionality being no-ops? Or would it
> be better to just not create the interfaces for missing functionality
> (e.g. not create the led_classdevs for controllers without LEDs)? I
> assume the latter makes more sense, since it doesn't lie to userspace.
> Though it could potentially make the driver code messier.
> 
> Thanks,
> Daniel
> 
> On Mon, Apr 27, 2020 at 3:56 AM Bastien Nocera <hadess@hadess.net> wrote:
>>
>> On Sun, 2020-04-26 at 15:31 -0700, Roderick Colenbrander wrote:
>>> On Sun, Apr 26, 2020 at 2:14 PM Bastien Nocera <hadess@hadess.net>
>>> wrote:
>>>> On Sun, 2020-04-26 at 13:42 -0700, Roderick Colenbrander wrote:
>>>> <snip>
>>>>> I really wonder how a device like this should be handled. It
>>>>> looks
>>>>> like the device can also handle a bunch of other classic Nintendo
>>>>> controllers.
>>>>>
>>>>> Is there anyway of detection this adapter? It feels nasty to have
>>>>> to
>>>>> hack in quirks for this device...
>>>>
>>>> The end game isn't very different from how we handle XBox 360, or
>>>> PS3/PS4 "clone" devices.
>>>>
>>>> Those devices (the adapters) work on the actual Nintendo Switch
>>>> hardware, so should probably work with the driver that handles the
>>>> same
>>>> type of hardware on Linux.
>>>>
>>>
>>> (resend in plain text)
>>>
>>> I agree it probably makes sense to handle in this driver. I'm worried
>>> about the application level implications. We have been doing a lot of
>>> work e.g. on Android to try to make gamepads consistent. It is weird
>>> to have a "Switch controller" with different features as applications
>>> make assumptions and don't expect there to be multiple versions of a
>>> particular controller. Any button mapping files would potentially be
>>> wrong for those too, certain features are not there (e.g. no sensors
>>> or no lights or rumble) or if they are the behaviour is different
>>> (e.g. HD rumble vs a classic rumble motor).
>>>
>>> Ideally we would do some kind of "fixup" to change the device name
>>> and
>>> or replace the device ids to at least separate them.
>>
>> All those would be detectable at runtime. I'm not sure that it's ever a
>> good idea to presume that a particular VID/PID combination will have,
>> say, rumble and LEDs available when the driver can answer those
>> questions.
>>
>> For example, I'm not sure that those controllers have either features
>> (though I'm not certain they identify as Switch Pro controllers, but
>> for the sake of argument):
>> https://store.nintendo.com/super-nintendo-entertainment-system-controller.html
>> https://store.nintendo.com/nintendo-entertainment-system-controllers.html
>>
>> Cheers
>>
> 
>
Daniel Ogorchock July 22, 2020, 7:22 p.m. UTC | #10
Hi Pierre-Loup,

On Wed, Jul 22, 2020 at 1:54 PM Pierre-Loup A. Griffais
<pgriffais@valvesoftware.com> wrote:
>
> Hi Daniel,
>
> Sorry for hijacking this branch of the thread (it's the last one that
> survived my inbox) - it seems like merging this driver as-is would break
> Steam, according to user reports.
>
> Is there any mechanism built into this hid_nintendo patch series to duck
> out of the way if userland directly opens the underlying hidraw device?
> That's what hid_steam does to coexist peacefully with userspace drivers
> (Steam being one of them, but not the only one).>
> Thanks,

I have run into the same issue of Steam/kernel fighting over the device.
I opened an issue describing it here:
https://github.com/ValveSoftware/steam-for-linux/issues/6651.

I'd been telling people to use firejail as a temporary workaround to prevent
steam from seeing the hidraw device. Note that hid-nintendo sets the most
significant bit of the evdev's version number to allow userspace applications
to discern it from the default hid device. There's no current mechanism in
the driver to yield to userspace using hidraw, but I can look at what
hid-steam is currently doing to accomplish that.

I guess the downside to that method is that any other process listening to
the controller's evdev events would cease to receive them (maybe a voice
chat program using one of the buttons as a push-to-talk hotkey or something
similar).

Does steam use hidraw for the sony dualshock controllers as well? If so, is
hid-sony doing anything special for that usecase?

As an additional note to anyone following this thread: I have a newer patchset
I need to submit which has a couple fixes for issues people have found while
testing (sets a missing power supply flag and improves bluetooth connection
stability). I will probably hold off on submitting that until we
figure out the right
solution to the hidraw issue.

Thanks,
Daniel

>   - Pierre-Loup
>
Bastien Nocera July 22, 2020, 7:47 p.m. UTC | #11
On Wed, 2020-07-22 at 14:22 -0500, Daniel Ogorchock wrote:
> 
> <snip>
> I will probably hold off on submitting that until we
> figure out the right
> solution to the hidraw issue.

Any chance you could make your work available as a stand-alone driver?
Something like this:
https://github.com/hadess/retrode
would make it easy to test in-development versions of the driver until
it lands (I might want to give a try to the optional features for the
GBros adapter as we discussed a couple of months ago).

Cheers
Daniel Ogorchock July 22, 2020, 8:03 p.m. UTC | #12
Hi Bastien,

On Wed, Jul 22, 2020 at 2:47 PM Bastien Nocera <hadess@hadess.net> wrote:
>
> On Wed, 2020-07-22 at 14:22 -0500, Daniel Ogorchock wrote:
> >
> > <snip>
> > I will probably hold off on submitting that until we
> > figure out the right
> > solution to the hidraw issue.
>
> Any chance you could make your work available as a stand-alone driver?
> Something like this:
> https://github.com/hadess/retrode
> would make it easy to test in-development versions of the driver until
> it lands (I might want to give a try to the optional features for the
> GBros adapter as we discussed a couple of months ago).
>
> Cheers
>

I've been keeping my latest patches in this tree:
https://github.com/DanielOgorchock/linux/commits/ogorchock
nicman23 has been maintaining a dkms module for it here:
https://github.com/nicman23/dkms-hid-nintendo
as well as an aur package here:
https://aur.archlinux.org/packages/hid-nintendo-dkms/
if either of those are helpful.

Thanks,
Daniel
Colenbrander, Roderick Aug. 1, 2020, 7:59 p.m. UTC | #13
> I'd been telling people to use firejail as a temporary workaround to prevent
> steam from seeing the hidraw device. Note that hid-nintendo sets the most
> significant bit of the evdev's version number to allow userspace applications
> to discern it from the default hid device. There's no current mechanism in
> the driver to yield to userspace using hidraw, but I can look at what
> hid-steam is currently doing to accomplish that.
> 
> I guess the downside to that method is that any other process listening to
> the controller's evdev events would cease to receive them (maybe a voice
> chat program using one of the buttons as a push-to-talk hotkey or something
> similar).

> Does steam use hidraw for the sony dualshock controllers as well? If so, is
> hid-sony doing anything special for that usecase?

In hid-sony we are not doing anything like hid-steam is doing (no virtual extra hidraw device).

I just don't know how I feel about mixing of evdev and hidraw. It just means userspace drivers are doing things behind the back of the real driver (unless you add a virtual hidraw driver like hid-steam). For Sony devices I'm not a big fan as our devices are very complex. Our devices tend to use a single output report (same report id) for rumble, audio and other types of data. When we will expose audio properly, a user space application doing hidraw will mess up audio streams and other behavior (e.g. power settings are also in the same hid report).

I don't know a good way yet. Has the usage of EVIOCGRAB been explored? If I recall it was intended to claim exclusive access to a device at least for input. It is a way of avoiding of some of the issues for simpler devices.

Virtual devices could work too, but I really dislike the extra complexity to hid drivers. Just in Sony case our driver for future devices will already be quite complex. If we really want virtual device support, I don't think it would scale to add it to every driver. Maybe there would be a generic way in the HID driver in which drivers can blacklist certain reports or hook into just certain requests or so.

Thanks,
Roderick
Daniel Ogorchock Aug. 4, 2020, 5:59 a.m. UTC | #14
Thanks for the input Roderick.

> In hid-sony we are not doing anything like hid-steam is doing (no virtual extra hidraw device).
>
> I just don't know how I feel about mixing of evdev and hidraw. It just means userspace drivers are doing things behind the back of the real driver (unless you add a virtual hidraw driver like hid-steam). For Sony devices I'm not a big fan as our devices are very complex. Our devices tend to use a single output report (same report id) for rumble, audio and other types of data. When we will expose audio properly, a user space application doing hidraw will mess up audio streams and other behavior (e.g. power settings are also in the same hid report).
>
> I don't know a good way yet. Has the usage of EVIOCGRAB been explored? If I recall it was intended to claim exclusive access to a device at least for input. It is a way of avoiding of some of the issues for simpler devices.
>

I've had some success using EVIOCGRAB in joycond:
https://github.com/DanielOgorchock/joycond/blob/3969af9dcdc2b8199716ec08220df5d9ef7cfc6a/include/phys_ctlr.h#L46
It takes sole-control of the individual joy-con evdevs so it can
re-expose them as a combined uinput device without having the original
devices usable by other applications.

I forgot to note that using the joy-cons (wirelessly or using the
charging grip) with steam seems to work fine. I think steam is just
using the evdev in that case. That leads me to suspect steam would
work fine with the pro controller evdev itself if the matching rules
it uses internally are altered to check the evdev's version number as
well (which hid-nintendo alters for distinction).

I guess the downside to using hid-nintendo as-is with steam is lack of
gyro/accel support compared to steam's hidraw driver. Maybe it's best
for me to finish up the gyro patch before getting this driver merged
in that case. Several people have been successfully using the
in-progress patch with the dolphin emulator. I need to clean things up
with it though based on some of Roderick's previous feedback.

Thanks,
Daniel