Re: [PATCH 1/2] HID: nintendo: fix face button mappings

[Martino Fontana]

I made a PR: https://github.com/libsdl-org/SDL/pull/5755.

On 31/05/22 17:31, Roderick Colenbrander wrote:
> On Tue, May 31, 2022 at 8:19 AM Martino Fontana <tinozzo123@xxxxxxxxx> wrote:
> >   > Second, even if the patch was right it would be tricky to merge. The
> >   > problem is that a changed mapping breaks user spaces and in general
> >   > can't do this unless there is a really good reason. It just would
> >   > break existing applications and libraries (often e.g. SDL)
> >
> > The problem is that the userspace is already broken.
> > If, out of the box, you attempt to launch something that uses SDL (like
> > Wine, or Super Tux Kart), the mapping you'll get will be wrong (and not
> > visually, the buttons are literally swapped).
> > Right now, this can be worked around (it makes the mapping correct) by
> > setting an environment variable (which isn't a thing that a user is
> > supposed to be doing, and the patch will remove the need of it):
> > ```
> > SDL_GAMECONTROLLERCONFIG=050000007e0500000920000001800000,Nintendo
> > Switch Pro
> > Controller,platform:Linux,a:b0,b:b1,x:b3,y:b2,back:b9,guide:b11,start:b10,leftstick:b12,rightstick:b13,leftshoulder:b5,rightshoulder:b6,dpup:h0.1,dpdown:h0.4,dpleft:h0.8,dpright:h0.2,leftx:a0,lefty:a1,rightx:a2,righty:a3,lefttrigger:b7,righttrigger:b8,030000007e0500000920000011810000,Nintendo
> > Switch Pro
> > Controller,platform:Linux,a:b0,b:b1,x:b3,y:b2,back:b9,guide:b11,start:b10,leftstick:b12,rightstick:b13,leftshoulder:b5,rightshoulder:b6,dpup:h0.1,dpdown:h0.4,dpleft:h0.8,dpright:h0.2,leftx:a0,lefty:a1,rightx:a2,righty:a3,lefttrigger:b7,righttrigger:b8,060000007e0500000620000000000000,Nintendo
> > Switch Combined
> > Joy-Cons,platform:Linux,a:b0,b:b1,x:b3,y:b2,back:b9,guide:b11,start:b10,leftstick:b12,rightstick:b13,leftshoulder:b5,rightshoulder:b6,dpup:b14,dpdown:b15,dpleft:b16,dpright:b17,leftx:a0,lefty:a1,rightx:a2,righty:a3,lefttrigger:b7,righttrigger:b8,
> > ```
> > The weird thing however is that SDL should already uses a controller
> > database:
> > https://github.com/libsdl-org/SDL/blob/main/src/joystick/SDL_gamecontrollerdb.h.
> > If this problem is caused by an SDL bug (I don't know if it is), then
> > it's SDL that will need a patch, and the problem will be solved with
> > zero repercussions. I think that SDL should be investigated before
> > continuing to discuss this.
>
> It looks like SDL lacks the patch to deal with the upstream driver
> properly. The chicken and egg problem SDL there is between the kernel
> and SDL, is that SDL supports a device prior to there being a kernel
> driver (or there being enough penetration for one). Without a driver,
> many devices function with an often strange mapping. That's the
> mapping SDL has in its table (for hid-generic). When a kernel driver
> comes around with a different, but proper mapping there is an issue.
>
> At the time we first dealt with this for DualShock 3 / 4 devices. The
> trick used then was to patch the 'version' bit of the HID device with
> '0x8000'. This resulted in a different mapping line. To understand SDL
> uses a GUID as the index for the table, it is composed of
> vendor/product id, version id and other fiels. The patched version,
> resulted in a different GUID, which then allowed SDL2 to recognize the
> device properly as it would use a different mapping line.
>
> For the Switch controllers, it looks like no patched line was added.
> Someone would need to provide a patch.
>
> > If, however, SDL is working as intended (making the patch necessary) and
> > this patch is merged, it will make the mapping correct out of the box
> > but it will also break a few things:
> > - Those who were using this workaround will have to remove it (by the
> > way, the page on the Arch Wiki should be updated too:
> > https://wiki.archlinux.org/title/Gamepad#Using_hid-nintendo_with_SDL2_Games);
> > - Applications that use raw joydev/evdev mappings (like Dolphin
> > Emulator) will have to be reconfigured.
> > - Also, some applications use SDL2 mappings in a different way (for
> > example, PCSX2), so out of the box the mappings are applied and the
> > controller is mapped as expected. Merging the patch will break
> > applications like these (in PCSX2's case it will be temporary since its
> > database is updated weekly, but it will force the user to use the new
> > kernel version, otherwise they will be stuck with a wrong mapping).
> >
> > Whether it's a wise idea to merge this as soon as possible in order to
> > cause the least amount amount of breakage, is not my call.
> >
> >
> > On 13/05/22 21:58, Roderick Colenbrander wrote:
> > > Hi Max,
> > >
> > > Thanks for your patch, however I must say the patch is not correct for
> > > 2 reasons.
> > >
> > > Over the years different controllers have different layouts. The
> > > standard which this driver (as well as others such as
> > > hid-sony/hid-playstation) follow is the Linux gamepad standard (see
> > > Documentation/input/gamepad.rst). It stays away of the debate what is
> > > A/B/X/Y. It talks about North/west/.., (yes they are macros which map
> > > to A/B/X/Y). In case of the Switch it does mean things are flipped,
> > > but it was not meant to represent an Xbox controller. (Technically one
> > > could argue that the Xbox controller should be flipped as it was the
> > > SNES controller back in the days which introduced X/Y and the Switch
> > > is still consistent with that.)
> > >
> > > Second, even if the patch was right it would be tricky to merge. The
> > > problem is that a changed mapping breaks user spaces and in general
> > > can't do this unless there is a really good reason. It just would
> > > break existing applications and libraries (often e.g. SDL)
> > >
> > > Thanks,
> > > Roderick
> > >
> > > On Wed, May 11, 2022 at 8:12 PM Max Fletcher <fletcher0max@xxxxxxxxx> wrote:
> > > > Previously, A and B would match the Xbox layout, but X and Y were incorrectly swapped. This corrects it so that X and Y match the Xbox layout.
> > > >
> > > > Signed-off-by: Max Fletcher <fletcher0max@xxxxxxxxx>
> > > > ---
> > > >    drivers/hid/hid-nintendo.c | 10 +++++-----
> > > >    1 file changed, 5 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
> > > > index 2204de889739..7735971ede3f 100644
> > > > --- a/drivers/hid/hid-nintendo.c
> > > > +++ b/drivers/hid/hid-nintendo.c
> > > > @@ -1351,10 +1351,10 @@ static void joycon_parse_report(struct joycon_ctlr *ctlr,
> > > >                   input_report_key(dev, BTN_START, btns & JC_BTN_PLUS);
> > > >                   input_report_key(dev, BTN_THUMBR, btns & JC_BTN_RSTICK);
> > > >                   input_report_key(dev, BTN_MODE, btns & JC_BTN_HOME);
> > > > -               input_report_key(dev, BTN_WEST, btns & JC_BTN_Y);
> > > > -               input_report_key(dev, BTN_NORTH, btns & JC_BTN_X);
> > > > -               input_report_key(dev, BTN_EAST, btns & JC_BTN_A);
> > > > -               input_report_key(dev, BTN_SOUTH, btns & JC_BTN_B);
> > > > +               input_report_key(dev, BTN_X, btns & JC_BTN_Y);
> > > > +               input_report_key(dev, BTN_Y, btns & JC_BTN_X);
> > > > +               input_report_key(dev, BTN_B, btns & JC_BTN_A);
> > > > +               input_report_key(dev, BTN_A, btns & JC_BTN_B);
> > > >           }
> > > >
> > > >           input_sync(dev);
> > > > @@ -1578,7 +1578,7 @@ static const unsigned int joycon_button_inputs_l[] = {
> > > >
> > > >    static const unsigned int joycon_button_inputs_r[] = {
> > > >           BTN_START, BTN_MODE, BTN_THUMBR,
> > > > -       BTN_SOUTH, BTN_EAST, BTN_NORTH, BTN_WEST,
> > > > +       BTN_A, BTN_B, BTN_Y, BTN_X,
> > > >           BTN_TR, BTN_TR2,
> > > >           0 /* 0 signals end of array */
> > > >    };
> > > > --
> > > > 2.35.3