Re: [PATCH 2/4] winebus.sys: Process quirky DualSense bluetooth reports.

[Roderick Colenbrander]

On Sun, Jan 23, 2022 at 3:48 PM Rémi Bernon  wrote:
> On 1/22/22 21:01, Roderick Colenbrander wrote:
> > On Fri, Jan 21, 2022 at 2:53 PM Rémi Bernon  wrote:
> > > Hi Arek,
> > >
> > > On 1/21/22 17:10, Arkadiusz Hiler wrote:
> > > > Signed-off-by: Arkadiusz Hiler
> > > >
> > > > dlls/winebus.sys/bus_udev.c | 32 ++++++++++++++++++++++++++++++++
> > > >    1 file changed, 32 insertions(+)
> > > >
> > > > diff --git a/dlls/winebus.sys/bus_udev.c b/dlls/winebus.sys/bus_udev.c
> > > > index 2269f5b904e..c290cea6f4f 100644
> > > > --- a/dlls/winebus.sys/bus_udev.c
> > > > +++ b/dlls/winebus.sys/bus_udev.c
> > > > @@ -117,6 +117,7 @@ static inline struct base_device *impl_from_unix_device(struct unix_device *ifac
> > > >    }
> > > >
> > > > #define QUIRK_DS4_BT 0x1
> > > > +#define QUIRK_DUALSENSE_BT 0x2
> > > >
> > > > struct hidraw_device
> > > >    {
> > > > @@ -356,6 +357,34 @@ static void hidraw_device_read_report(struct unix_device *iface)
> > > >                buff[0] = 1;
> > > >            }
> > > > -
> > > >    /* The behavior of DualSense is very similar to DS4 described above with few exceptions.
> > > > -
> > > >     *
> > > > -
> > > >     * The report number #41 is used for the extended bluetooth input report. The report comes
> > > > -
> > > >     * with only one extra byte in front and the format is not exactly the same and we need to
> > > > -
> > > >     * shuffle a few bytes around.
> > > > -
> > > >     *
> > > > -
> > > >     * Small report:
> > > > -
> > > >     *   X  Y  Z  RZ  Buttons[3]  TriggerLeft  TriggerRight
> > > > -
> > > >     *
> > > > -
> > > >     * Extended report:
> > > > -
> > > >     *   Prefix X  Y  Z  Rz  TriggerLeft  TriggerRight  Counter  Buttons[3] ...
> > > > -
> > > >     */
> > > > -
> > > >    if ((impl->quirks & QUIRK_DUALSENSE_BT) && report_buffer[0] == 0x31 && size >= 11)
> > > > -
> > > >    {
> > > > -
> > > >        BYTE tmp;
> > > > -
> > > >        size = 10;
> > > > -
> > > >        buff += 1;
> > > > -
> > > > -
> > > >        buff[0] = 1; /* fake report #1 */
> > > > -
> > > > -
> > > >        tmp = buff[6];      /* Trigger Right */
> > > > -
> > > >        buff[5] = buff[8];  /* Buttons[0] */
> > > > -
> > > >        buff[6] = buff[9];  /* Buttons[1] */
> > > > -
> > > >        buff[7] = buff[10]; /* Buttons[2] */
> > > > -
> > > >        buff[8] = buff[5];  /* TriggerLeft */
> > > > -
> > > >        buff[9] = tmp;      /* TirggerRight */
> > > > -
> > > >    }
> > >
> > > You're writing to buff[5] and then reading it, probably you need to save
> > > both triggers first.
> >
> > Hi Arkadiusz,
> >
> > I'm not a big fan of this type of fixup here (and the same for DS4 to
> > be honest). The DS4/DualSense have unusual HID reports, but this type
> > of fixup causes issues for applications which properly parse the HID
> > data. For example various applications for example do use our
> > libScePad.dll for games on Windows. They go through the Windows HID
> > APIs. This breaks such applications.
> >
> > Thanks,
> >
> > Roderick Colenbrander
> > Sony Interactive Entertainment, LLC
>
> Hi Roderick,
>
> Thanks for giving some feedback. The fixups have been added for games
> and applications which do not expect to receive the extended reports,
> and only support the reports that are openly described in the HID
> descriptors.
>
> As Arek has found already, and as you say, some other applications or
> middlewares are explicitly requesting them, so we were considering
> adding some more logic to remove the fixups and send the extended
> reports in the same way as on Windows, when they are requested.
>
> I understand that these extended reports are triggered by some feature
> report requests, and this is automatically done on Linux by the kernel
> and evdev driver, to the contrary to what happens on Windows, or maybe
> there's something we've missed?

The DS4 / DualSense in Bluetooth can be switched to a different mode
in which they also report motion sensors, touchpad and other data. In
the default mode (not sure why it exists to be honest) it sends limit
data and in case of DS4 is actually not that responsive.

The calibration operations switches the device to this other mode.
Various applications including Chrome have their own HID parsing for
our devices (https://chromium.googlesource.com/chromium/src/+/refs/heads/main/device/game...).

Doing this type of fixup is not really the right thing for either
controller. It fixes some applications, while breaks others (e.g.
Chrome would break) as apps really need this extended data. Also other
data in the report would need transformation as it would be at the
wrong offsets.

Let me reply more to the email to Arek.
> Cheers,
>
> Rémi Bernon

Thanks,
Roderick