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

From: Benjamin Tissoires
Date: Tue Aug 30 2022 - 08:45:39 EST

Next message: Andy Shevchenko: "Re: [jic23-iio:testing 124/129] drivers/iio/accel/msa311.c:993:24: warning: format specifies type 'unsigned char' but the argument has type 'unsigned int'"
Previous message: Andrew Lunn: "Re: [RFC Patch net-next v3 1/3] net: dsa: microchip: use dev_ops->reset instead of exit in ksz_switch_register"
In reply to: Marcus Folkesson: "Re: [RESEND PATCH 1/2] HID: Add driver for RC Simulator Controllers"
Next in thread: Marcus Folkesson: "Re: [RESEND PATCH 1/2] HID: Add driver for RC Simulator Controllers"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Thu, Aug 25, 2022 at 8:44 AM Marcus Folkesson
<marcus.folkesson@xxxxxxxxx> wrote:
>
> 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:
[...]
>
> 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.

yep, as you saw already :)

>
> 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?

You probably already got it, but it all depends on how much data is
used on the wire:
- if you have 2 bytes used per each value, then the report size will
be 16 and you set the logical max to 2047
- if the values are packed, then the report size is 11, and then you
probably have to add padding *after* the 22 bits.

>
> >
> > >
> > >
> > > >
> > > > > +
> > > > > +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?

No, this is actually easy to describe. You say that there is one usage
of "something" which has a report size of 1 bit, and then you have
another usage of "something else" with the same report size.

But usually you have to add padding after to make up to 8 bits (so 6
bits in that case).

I was referring to the case where you are parsing the same bit on the
wire, and give a different usage based if you have received an odd or
an even number of reports. In that case, we probably need to use move
this bit to a const field in the original report descriptor and say
that the data is now not const:

- initial report (completely random example):
X (2 bytes) | Y (2 bytes) | button this_or_that (1 bit, depending of
odd or even received reports) | 7 bits of padding
- we can declare it as:
X (2 bytes) | Y (2 bytes) | button this (1 bit) | button that (1
bit) | 6 bits of padding

and have raw_event do the translation for the button "this_or_that"
that would populate the correct bit in the translated report.

Cheers,
Benjamin

>
> 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/

Next message: Andy Shevchenko: "Re: [jic23-iio:testing 124/129] drivers/iio/accel/msa311.c:993:24: warning: format specifies type 'unsigned char' but the argument has type 'unsigned int'"
Previous message: Andrew Lunn: "Re: [RFC Patch net-next v3 1/3] net: dsa: microchip: use dev_ops->reset instead of exit in ksz_switch_register"
In reply to: Marcus Folkesson: "Re: [RESEND PATCH 1/2] HID: Add driver for RC Simulator Controllers"
Next in thread: Marcus Folkesson: "Re: [RESEND PATCH 1/2] HID: Add driver for RC Simulator Controllers"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]