Re: [RESEND PATCH 1/2] HID: Add driver for RC Simulator Controllers

[Marcus Folkesson <marcus.folkesson@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 10706 bytes --]

Hi Benjamin,

Thank you!
I have a few questions regarding the report descriptor, please see
below.

On Tue, Aug 23, 2022 at 06:43:47PM +0200, Benjamin Tissoires wrote:
> On Tue, Aug 23, 2022 at 5:13 PM Marcus Folkesson
> <marcus.folkesson@gmail.com> wrote:
> >
> > Thank you  Benjamin,
> >
> > On Tue, Aug 23, 2022 at 11:49:59AM +0200, Benjamin Tissoires wrote:
> > > Hi Marcus,
> > >
> > > [and sorry for the delay in the review of your patches]
> > >
> > > On Mon, Aug 22, 2022 at 8:04 AM Marcus Folkesson
> > > <marcus.folkesson@gmail.com> wrote:

[...]

> > > > +       };
> > > > +
> > > > +       priv->input = input;
> > > > +       return input_register_device(priv->input);
> > > > +}
> > >
> > > You are basically rewriting hid-input.c, which is suboptimal.
> >
> > Ouch. I will have a look at hid-input, thanks.
> >
> > >
> > > I guess the report descriptor provided by these devices are basically
> > > useless, and so you have to parse the reports yourself in the
> > > raw_event callback.
> >
> > Yep.
> >
> > >
> > > But instead of manually doing that, why not overwrite the report
> > > descriptor (with .rdesc_fixup) and declare here all of the data that
> >  Do you mean .report_fixup?
> 
> yes, sorry :/
> 
> >
> > > needs to be exported. You could remove basically everything in this
> > > driver by just providing a fixed report descriptor.
> >
> > What you are aiming for is to fixup the report descriptor and let the
> > generic hid-raw driver handle the rest, or do I get you wrong?
> 
> yep, exactly
> 
> >
> > How is the report mapped to certain events then?
> 
> Have a look at hid_configure_usage in hid-input.c [3]. Most of HID
> events are mapped to input events with a one to one mapping.
> 
> >
> > I do read at [1] but it is not obvious how to put it together.
> > Most drivers I've looked at that is using .report_fixup just fix broken
> > reports. I guess these reports are not "broken", just.. odd?
> 
> Have a look at [2], lots of full report descriptors :)
> 
> And in your case, the reports are incomplete, not odd.

Got it.
I've parsed [4] the report descriptor for VRC2, and I guess it does not looks
that good:

0x05, 0x01,        // Usage Page (Generic Desktop Ctrls)
0x09, 0x00,        // Usage (Undefined)
0xA1, 0x01,        // Collection (Application)
0x15, 0x00,        //   Logical Minimum (0)
0x25, 0x01,        //   Logical Maximum (1)
0x75, 0x01,        //   Report Size (1)
0x05, 0x09,        //   Usage Page (Button)
0x19, 0x01,        //   Usage Minimum (0x01)
0x29, 0x3F,        //   Usage Maximum (0x3F)
0x95, 0x40,        //   Report Count (64)
0x81, 0x02,        //   Input (Data,Var,Abs,No Wrap,Linear,Preferred State,No Null Position)
0xC0,              // End Collection

The Usage should rather be Joystick than undefined, the other
fields does also looks wrong to me.

The data for each axis (WHEEL and GAS) is 11 bit long (Logical maximum
2047 ?), how does the report descriptor map to the actual data in terms
of offset, order and length?

> 
> >
> >
> > >
> > > > +
> > > > +static int rcsim_raw_event(struct hid_device *hdev,
> > > > +                              struct hid_report *report,
> > > > +                              u8 *raw_data, int size)
> > > > +{
> > > > +       struct rcsim_priv *priv = hid_get_drvdata(hdev);
> > > > +       u16 value;
> > > > +
> > > > +       switch (priv->controller) {
> > > > +       case PHOENIXRC:
> > > > +               if (size != PHOENIXRC_DSIZE)
> > > > +                       break;
> > > > +
> > > > +               /* X, RX, Y and RY, RUDDER and THROTTLE are sent every time */
> > > > +               input_report_abs(priv->input, ABS_X, raw_data[2]);
> > > > +               input_report_abs(priv->input, ABS_Y, raw_data[0]);
> > > > +               input_report_abs(priv->input, ABS_RX, raw_data[4]);
> > > > +               input_report_abs(priv->input, ABS_RY, raw_data[3]);
> > > > +               input_report_abs(priv->input, ABS_RUDDER, raw_data[5]);
> > > > +               input_report_abs(priv->input, ABS_THROTTLE, raw_data[6]);
> > > > +
> > > > +               /* Z and RZ are sent every other time */
> > > > +               if (priv->alt)
> > > > +                       input_report_abs(priv->input, ABS_Z, raw_data[7]);
> > > > +               else
> > > > +                       input_report_abs(priv->input, ABS_RZ, raw_data[7]);
> > > > +
> > > > +               priv->alt ^= 1;
> > > > +               break;
> > > > +       case VRC2:
> > > > +               if (size != VRC2_DSIZE)
> > > > +                       break;
> > > > +               value = (raw_data[1] << 8 | raw_data[0]) & GENMASK(10, 0);
> > > > +               input_report_abs(priv->input, ABS_GAS, value);
> > > > +               value = (raw_data[3] << 8 | raw_data[2]) & GENMASK(10, 0);
> > > > +               input_report_abs(priv->input, ABS_WHEEL, value);
> > > > +               break;
> > > > +       case REALFLIGHT:
> > > > +               if (size != REALFLIGHT_DSIZE)
> > > > +                       break;
> > > > +               input_report_abs(priv->input, ABS_X, raw_data[2]);
> > > > +               input_report_abs(priv->input, ABS_Y, raw_data[1]);
> > > > +               input_report_abs(priv->input, ABS_RX, raw_data[5]);
> > > > +               input_report_abs(priv->input, ABS_RY, raw_data[3]);
> > > > +               input_report_abs(priv->input, ABS_MISC, raw_data[4]);
> > > > +               input_report_key(priv->input, BTN_A,
> > > > +                               raw_data[7] & REALFLIGHT_BTN_A);
> > > > +               input_report_key(priv->input, BTN_B,
> > > > +                               raw_data[7] & REALFLIGHT_BTN_B);
> > > > +               break;
> > > > +       case XTRG2FMS:
> > > > +               if (size != XTRG2FMS_DSIZE)
> > > > +                       break;
> > > > +
> > > > +               /* X, RX, Y and RY are sent every time */
> > > > +               value = FIELD_GET(XTRG2FMS_X_HI, raw_data[3]);
> > > > +               value = (value << 8) | raw_data[1];
> > > > +               input_report_abs(priv->input, ABS_X, value);
> > > > +
> > > > +               value = FIELD_GET(XTRG2FMS_Y_HI, raw_data[3]);
> > > > +               value = (value << 8) | raw_data[2];
> > > > +               input_report_abs(priv->input, ABS_Y, value);
> > > > +
> > > > +               value = FIELD_GET(XTRG2FMS_RX_HI, raw_data[3]);
> > > > +               value = (value << 8) | raw_data[0];
> > > > +               input_report_abs(priv->input, ABS_RX, value);
> > > > +
> > > > +               value = FIELD_GET(XTRG2FMS_RY_HI, raw_data[3]);
> > > > +               value = (value << 8) | raw_data[4];
> > > > +               input_report_abs(priv->input, ABS_RY, value);
> > > > +
> > > > +               /* Z, RZ, RUDDER and THROTTLE are sent every other time */
> > > > +               value = FIELD_GET(XTRG2FMS_ALT1_HI, raw_data[7]);
> > > > +               value = (value << 8) | raw_data[6];
> > > > +               if (priv->alt)
> > > > +                       input_report_abs(priv->input, ABS_Z, value);
> > > > +               else
> > > > +                       input_report_abs(priv->input, ABS_RUDDER, value);
> > > > +
> > > > +               value = FIELD_GET(XTRG2FMS_ALT2_HI, raw_data[7]);
> > > > +               value = (value << 8) | raw_data[5];
> > > > +               if (priv->alt)
> > > > +                       input_report_abs(priv->input, ABS_RZ, value);
> > > > +               else
> > > > +                       input_report_abs(priv->input, ABS_THROTTLE, value);
> > > > +
> > > > +               priv->alt ^= 1;
> > > > +               break;
> > > > +       case ORANGERX:
> > > > +               if (size != ORANGERX_DSIZE)
> > > > +                       break;
> > > > +               input_report_abs(priv->input, ABS_X, raw_data[0]);
> > > > +               input_report_abs(priv->input, ABS_Y, raw_data[2]);
> > > > +               input_report_abs(priv->input, ABS_RX, raw_data[3]);
> > > > +               input_report_abs(priv->input, ABS_RY, raw_data[1]);
> > > > +               input_report_abs(priv->input, ABS_RUDDER, raw_data[5]);
> > > > +               input_report_abs(priv->input, ABS_THROTTLE, raw_data[6]);
> > > > +               break;
> > > > +       };
> > > > +
> > > > +       input_sync(priv->input);
> > > > +       return 0;
> > > > +}
> > > > +
> > > > +static int rcsim_probe(struct hid_device *hdev, const struct hid_device_id *id)
> > > > +{
> > > > +       struct device *dev = &hdev->dev;
> > > > +       struct rcsim_priv *priv;
> > > > +       int ret;
> > > > +
> > > > +       if (!hid_is_using_ll_driver(hdev, &usb_hid_driver))
> > > > +               return -ENODEV;
> > >
> > > You are not accessing anything in the USB stack, so there is no need
> > > to prevent regression tests that could inject uhid devices to your
> > > drivers.
> >
> > Ok, thanks.
> >
> > >
> > > Cheers,
> > > Benjamin
> > >
> >
> > Best regards,
> > Marcus Folkesson
> >
> > [1] https://www.usb.org/hid
> >
> 
> If you need help in writing report descriptors, I can give you some,
> but the easiest might be for you to start from the report descriptor
> in hid-sony.c. I used to have a tool to dynamically write a report
> descriptor, but I'm not sure it still works...

I think at least some advice would be great :-)

The VRC2 would be the most simple of those, it only has 2 axis with
resolution of 11-bit.
If you have time, would you please give some advice what a report descriptor would look
like and I could probably come up with something for the others.

> 
> FYI, I just re-read rcsim_raw_event() and there is stuff that would
> require more than just a report descriptor fixup (the fact that some
> data is sent every other report is not good and will need some manual
> handling though).


Is the fact that more than one button share the same
byte hard to describe in the report?

  value = FIELD_GET(XTRG2FMS_ALT1_HI, raw_data[7]);
  value = (value << 8) | raw_data[6];

...
  value = FIELD_GET(XTRG2FMS_ALT2_HI, raw_data[7]);
  value = (value << 8) | raw_data[5];


> 
> Cheers,
> Benjamin
> 

Best regards
Marcus Folkesson

> [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/hid/hid-uclogic-rdesc.c
> [3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/hid/hid-input.c#n817
[4] https://eleccelerator.com/usbdescreqparser/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]