[Bug 51822] New: Simucube 2 TrueDrive: Doesn't recognize the steering wheel device

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=51822

Bug ID: 51822
           Summary: Simucube 2 TrueDrive: Doesn't recognize the steering
                    wheel device
           Product: Wine
           Version: 6.12
          Hardware: x86-64
                OS: Linux
            Status: UNCONFIRMED
          Severity: minor
          Priority: P2
         Component: hid
          Assignee: [e-mail]
          Reporter: [e-mail]
                CC: [e-mail]
   Regression SHA1: 51560aabcb259cb89659d96654d8ad38edbb528d
      Distribution: ArchLinux

Created attachment 70707
  --> https://bugs.winehq.org/attachment.cgi?id=70707
0001-hidparse.sys-Preserve-the-original-state-items.repor.patch, wine_6.18.log,
SC2_descr.txt

Simucube 2 is FFB steering wheel device using the HID PID specification for
FFB. Thus the support for it is included in the Linux kernel as part of the HID
specification. It still needs some patching of the usbhid kernel module, but in
general is entirely functional in Wine, as long as "DisableInput" and "Enable
SDL" are disabled in the Wine registry, and for the HIDRAW device is granted rw
user access.

TrueDrive is an application for controlling and fine tuning various parameters
of the steering wheel (using HidD/HidP calls), which also includes real time
animation of the steering wheel movement, buttons pushed, etc. The application
is not used/needed during gameplay. The TrueDrive version used is 2020.10. The
application can be downloaded from the manufacturer site:
https://granitedevices.com/wiki/Simucube_2_True_Drive_releases

The mentioned regression commit is the earliest in which the problem occurs.
I've investigated further, and found that state->items.report_count in the
parse_new_value_caps() function (hidclass.sys/descriptor.c) is not preserved
properly. While being used for building the alternate value array it's set to
1, and since "Report Count" is a global item, reset_local_items() at the end of
the function preserves its state. Thus if there are subsequent main items
(Input, Output, etc.) with more than one usages, and with no "Report Count"
specified in between, the parser would calculate wrong bit sizes, report len,
etc.

The particular problem was at the end of the descriptor. Here is an excerpt:

/*      Usage Page (FF00h),                     */
/*      Usage (01h),                            */
/*      Collection (Application),               */
/*          Usage (01h),                        */
/*          Report ID (107),                    */
/*          Report Size (8),                    */
/*          Report Count (60),                  */
/*          Logical Minimum (0),                */
/*          Logical Maximum (255),              */
/*          Output,                             */
/*          Usage (01h),                        */
/*          Report ID (108),                    */
/*          Input,                              */
/*      End Collection,                         */

The Input report 108 follows immediately the Output report 107, without Report
Count specified in between. In our case parse_new_value_caps() would set
erroneously Report Count to 1, after being called on the Output main item. Thus
the next Input report size would be calculated wrongly. This is a vendor
defined report, which they probably use in the process of recognizing the
device.

Since then the issue was mitigated a little bit with commit
7096c26a2e48089424fe90956c80c29617bce9f2, when button array value caps were
introduced, which stopped modifying state->items.report_count if the parsed
item is an array.

The issue is still present in the new hidparse.sys (main.c), where the parsing
code has been moved with commit a290c5bf7c9ddc92af56231693c6d8f00c3efd7b.
It is present in Output report 2. Here is an excerpt:

/*      Usage (5Ah),                            */
/*      Collection (Logical),                   */
/*          Report ID (2),                      */
            ...
/*          Report Count (2),                   */
/*          Output (Variable),                  */
/*          Usage (5Ch),                        */
/*          Usage (5Eh),                        */
/*          Unit (Seconds),                     */
/*          Unit Exponent (-3),                 */
/*          Logical Maximum (32767),            */
/*          Physical Maximum (32767),           */
/*          Report Size (16),                   */
/*          Output (Variable),                  */
/*          Physical Maximum (0),               */
/*          Unit,                               */
/*          Unit Exponent (0),                  */
/*      End Collection,                         */

Since the Output main items are variable, state->items.report_count is still
being modified to 1. So the second Output item ends up with two usages, one 16
bit report, and one 0 bit report. This can be seen in the attached log file
wine_6.18.log. For Output report 2, the RCnt for Usage 0x5E is 0.
The following debug options were used for the log:
WINEDEBUG=+timestamp,+pid,+hid,+hidp,+hid_report,+plugplay. The Wine build is
6.18-107-gbcdb28a563d.
I've attached the Simucube 2 device descriptor in SC2_Descr.txt. Hope it can
help as a real life example of a HID PID device with multiple ReportIDs.

Attached also a patch which is fixing the issue (based on the latest master
aa629c4c7225166f4ee46476d98702df2e142711).

[Bug 51822] Simucube 2 TrueDrive: Doesn't recognize the steering wheel device

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=51822

joaopa  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

--- Comment #1 from joaopa  ---
Please, always report bug against the latest vanilla wine version. Currently,
it is wine-6.18. There was a lot of work in this area the last weeks. Surely
your bug is already fixed.

[Bug 51822] Simucube 2 TrueDrive: Doesn't recognize the steering wheel device

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=51822

Zebediah Figura  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
            Version|6.12                        |6.18

--- Comment #2 from Zebediah Figura  ---
(In reply to joaopa from comment #1)
> Please, always report bug against the latest vanilla wine version.
> Currently, it is wine-6.18. There was a lot of work in this area the last
> weeks. Surely your bug is already fixed.

Since the reporter mentioned 6.18 in the description, and specifically
referenced commits that postdate 6.12, I'm going to assume that marking the
version as 6.12 was an accident.

[Bug 51822] Simucube 2 TrueDrive: Doesn't recognize the steering wheel device

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=51822

--- Comment #3 from Ivo Ivanov  ---
(In reply to joaopa from comment #1)
> Please, always report bug against the latest vanilla wine version.
> Currently, it is wine-6.18. There was a lot of work in this area the last
> weeks. Surely your bug is already fixed.

Sorry for the confusion. The bug is still present, just not as severe as
before. Previously the application didn't recognize the device at all. Now it
recognizes it visibly, but the device descriptor is still not parsed properly,
as is visible by the provided log, which I mentioned in the comment. The log is
made with Wine 6.18-107-gbcdb28a563d two days earlier, but nothing has changed
in hidparse.sys since then.

The patch I provided is based on the latest master
(aa629c4c7225166f4ee46476d98702df2e142711).

(In reply to Zebediah Figura from comment #2)
> Since the reporter mentioned 6.18 in the description, and specifically
> referenced commits that postdate 6.12, I'm going to assume that marking the
> version as 6.12 was an accident.

Thanks for correcting the version. The report is for 6.18.

[Bug 51822] Simucube 2 TrueDrive: Doesn't recognize the steering wheel device

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=51822

--- Comment #4 from Rémi Bernon  ---
Thanks, this looks correct indeed I'll try to add some tests as well and
upstream a fix.

[Bug 51822] Simucube 2 TrueDrive: Doesn't recognize the steering wheel device

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=51822

--- Comment #5 from Ivo Ivanov  ---
(In reply to Rémi Bernon from comment #4)
> Thanks, this looks correct indeed I'll try to add some tests as well and
> upstream a fix.

Glad I can help! Pretty excited about the HID PID joystick implementation in
dinput :) Will test it thoroughly when it's ready.

I'd like to mention another similar issue with this device descriptor, which I
spotted while observing the log.
This is part of Output report 1:

/*          Usage (57h),                        */
/*          Collection (Logical),               */
/*              Usage (000A0001h),              */
/*              Usage (000A0002h),              */
/*              Unit (Degrees),                 */
/*              Unit Exponent (-2),             */
/*              Logical Minimum (0),            */
/*              Logical Maximum (180),          */
/*              Physical Minimum (0),           */
/*              Physical Maximum (36000),       */
/*              Unit,                           */
/*              Report Size (8),                */
/*              Report Count (1),               */
/*              Output (Variable),              */
/*              Unit Exponent (0),              */
/*              Unit,                           */
/*          End Collection,                     */

There are two ordered items Usages, but the Report Count is only 1.
hidparse.sys parses this as two usages, with the second one being with RCnt 0.
It's visible in the provided log, slightly above the previous issue. I believe
this is correct in this situation (RCnt 0). Don't know how this would affect
other parts of the logic though.
Anyway, there is this line of code in parse_new_value_caps()
(hidparse.sys/main.c):

if (!(is_array = HID_VALUE_CAPS_IS_ARRAY( &state->items )))
state->items.report_count -= usages_size - 1;

if usages_size is >= 3 and state->items.report_count is 1, then report_count
would be a negative value if it was a signed integer, otherwise it would wrap
around leading to undesired behavior. In my opinion this needs to be looked at
too.

The descriptor doesn't look correct though. I'll contact Granite Devices for
this issue, and hopefully they'll fix it themselves.

P.S. I have few more bugs (with patches) to report, mostly in hidclass.sys and
one in winebus.sys, which affect mostly such HID devices with multiple
reports/ReportIDs. Hopefully won't disturb you, or the other devs too much.

[Bug 51822] Simucube 2 TrueDrive: Doesn't recognize the steering wheel device

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=51822

--- Comment #6 from Rémi Bernon  ---
(In reply to Ivo Ivanov from comment #5)
> (In reply to Rémi Bernon from comment #4)
> > Thanks, this looks correct indeed I'll try to add some tests as well and
> > upstream a fix.
>
> Glad I can help! Pretty excited about the HID PID joystick implementation in
> dinput :) Will test it thoroughly when it's ready.
>
> I'd like to mention another similar issue with this device descriptor, which
> I spotted while observing the log.
> This is part of Output report 1:
>
> /*          Usage (57h),                        */
> /*          Collection (Logical),               */
> /*              Usage (000A0001h),              */
> /*              Usage (000A0002h),              */
> /*              Unit (Degrees),                 */
> /*              Unit Exponent (-2),             */
> /*              Logical Minimum (0),            */
> /*              Logical Maximum (180),          */
> /*              Physical Minimum (0),           */
> /*              Physical Maximum (36000),       */
> /*              Unit,                           */
> /*              Report Size (8),                */
> /*              Report Count (1),               */
> /*              Output (Variable),              */
> /*              Unit Exponent (0),              */
> /*              Unit,                           */
> /*          End Collection,                     */
>
> There are two ordered items Usages, but the Report Count is only 1.
> hidparse.sys parses this as two usages, with the second one being with RCnt
> 0. It's visible in the provided log, slightly above the previous issue. I
> believe this is correct in this situation (RCnt 0). Don't know how this
> would affect other parts of the logic though.
> Anyway, there is this line of code in parse_new_value_caps()
> (hidparse.sys/main.c):
>
> if (!(is_array = HID_VALUE_CAPS_IS_ARRAY( &state->items )))
> state->items.report_count -= usages_size - 1;
>
> if usages_size is >= 3 and state->items.report_count is 1, then report_count
> would be a negative value if it was a signed integer, otherwise it would
> wrap around leading to undesired behavior. In my opinion this needs to be
> looked at too.

Hmm indeed... maybe a few tests would show what Windows is really doing but in
any case we should probably make sure not to wrap around.

I didn't bother too much (yet) with arguably invalid report descriptors, as
Windows is already quite brittle in some cases (for instance I could see that
it doesn't like very much descriptors which skip some logical/physical
min/max).
> The descriptor doesn't look correct though. I'll contact Granite Devices for
> this issue, and hopefully they'll fix it themselves.
>
> P.S. I have few more bugs (with patches) to report, mostly in hidclass.sys
> and one in winebus.sys, which affect mostly such HID devices with multiple
> reports/ReportIDs. Hopefully won't disturb you, or the other devs too much.

Thanks! I will be happy to have a look, even before I finish sending the
patches, maybe there's some fixes that could be worth including early.

I couldn't test the PID code very extensively, as I don't have any device
actually supporting such kind of reports. Thanks to the HID test driver we can
confirm the behavior on Windows even without one, but maybe it's supposed to
differ a bit depending on the devices.

For instance, I can see that your device is using the "new effect block" /
"load block" / "free block" reports, which I initially intended to implement
too, but the tests showed that even when these reports are declared Windows
dinput only seems to be using the "set effect block" with its own effect
indexes.

Maybe I just couldn't figure the exact required report declaration to make it
use them, but I tried with a SideWinder Force Feedback 2 report descriptor,
which has them as well, and it's still not doing it.

So I just implemented the same behavior, and hopefully it'll work here. If it
doesn't then we'll have to implement these reports, it's a bit more verbose and
I still do not completely understand how they are supposed to work though.

Same goes with the "set condition" report, with the weird "type specific block
offset" and "type specific parameter offset", which seems to be simply ignored
on Windows, although required. The PID documentation is also pretty cryptic
about them...

[Bug 51822] Simucube 2 TrueDrive: Doesn't recognize the steering wheel device

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=51822

--- Comment #7 from Ivo Ivanov  ---
(In reply to Rémi Bernon from comment #6)
> Thanks! I will be happy to have a look, even before I finish sending the
> patches, maybe there's some fixes that could be worth including early.

OK, I can file separate bug reports for each patch, since they address
different commits, but this would be slower. Don't know if I'll be able to post
all of the bug reports today.
Or I can attach them here or send them by email. This would be much faster, as
they are already rebased to the latest master, and are pretty much self
explanatory. The most important one which benefits also controllers with single
reports, fixes (I hope:)) the input lag and skipping/jumping that some users
experience. It's severe with the Simucube 2 and probably the Fanatec ClubSport
Pedals, while using such apps with raw HID access.
> I couldn't test the PID code very extensively, as I don't have any device
> actually supporting such kind of reports. Thanks to the HID test driver we
> can confirm the behavior on Windows even without one, but maybe it's
> supposed to differ a bit depending on the devices.
>
> For instance, I can see that your device is using the "new effect block" /
> "load block" / "free block" reports, which I initially intended to implement
> too, but the tests showed that even when these reports are declared Windows
> dinput only seems to be using the "set effect block" with its own effect
> indexes.

I'll have to look the PID documentation, but in my opinion these are more
important for custom FFB forces in Driver Managed mode, which to my knowledge
aren't used by any modern game these days. The sims use mostly "constant" or
"periodic" effects, which are streamed to the device in the range of 60Hz
iRacing to ~600Hz Automobilista 2 or even higher BeamNG. At least this is what
can be measured through the hid-pidff driver in Linux (> 450Hz is really
possible only with futex2 in Wine).
Anyway, you could look the Linux PID driver in the usbhid module. I can confirm
that Simucube 2 works very well with this driver. Literally can't sense any
difference between Windows and Linux (with Wine) with SC2 while playing various
sims. The Linux driver supports only Device Managed mode.
> So I just implemented the same behavior, and hopefully it'll work here. If
> it doesn't then we'll have to implement these reports, it's a bit more
> verbose and I still do not completely understand how they are supposed to
> work though.
>
> Same goes with the "set condition" report, with the weird "type specific
> block offset" and "type specific parameter offset", which seems to be simply
> ignored on Windows, although required. The PID documentation is also pretty
> cryptic about them...

The Linux driver has all of them fully implemented - Set Effect Report, Set
Envelope Report, Set Condition Report, Set Periodic Report, Set Constant Force
Report, etc. IMO it's a pretty good reference for PID implementation. You can
find it in linux/drivers/hid/usbhid/hid-pidff.c.
In the past I have written few patches for it mostly to be able to run the SC2
on Linux. For example they require the "Start Delay" (0xa7) parameter, while
the SC2 doesn't advertise it in its descriptor, so it needed to be made
optional. There are few more parameters in the Condition report, that are
lacking from the SC2 descriptor, etc.

[Bug 51822] Simucube 2 TrueDrive: Doesn't recognize the steering wheel device

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=51822

Ivo Ivanov  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|UNCONFIRMED                 |RESOLVED

--- Comment #8 from Ivo Ivanov  ---
Tested with 5a8dcb062793fbb68997e1b54ebc2666a2b2834d, and can confirm the issue
is fixed.

[Bug 51822] Simucube 2 TrueDrive: Doesn't recognize the steering wheel device

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=51822

Ivo Ivanov  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
      Fixed by SHA1|                            |d97426be026e16f8dddcc2a003b
                   |                            |f19c477668183

[Bug 51822] Simucube 2 TrueDrive: Doesn't recognize the steering wheel device

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=51822

Alexandre Julliard  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #9 from Alexandre Julliard  ---
Closing bugs fixed in 6.19.

[Bug 51822] Simucube 2 TrueDrive: Doesn't recognize the steering wheel device

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=51822

Gijs Vermeulen  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |regression