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
[off-list ref] wrote:

quoted

Thank you  Benjamin,

On Tue, Aug 23, 2022 at 11:49:59AM +0200, Benjamin Tissoires wrote:

quoted

Hi Marcus,

[and sorry for the delay in the review of your patches]

On Mon, Aug 22, 2022 at 8:04 AM Marcus Folkesson
[off-list ref] wrote:

[...]

quoted

quoted

quoted

+       };
+
+       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.

quoted

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.

quoted

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

quoted

quoted

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

quoted

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.

quoted

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?

quoted

quoted

quoted

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

quoted

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/