[Bug 51822] New: Simucube 2 TrueDrive: Doesn't recognize the steering wheel device
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: wine-bugs(a)winehq.org Reporter: logos128(a)gmail.com CC: rbernon(a)codeweavers.com 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). -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=51822 joaopa <jeremielapuree(a)yahoo.fr> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jeremielapuree(a)yahoo.fr --- Comment #1 from joaopa <jeremielapuree(a)yahoo.fr> --- 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. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=51822 Zebediah Figura <z.figura12(a)gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version|6.12 |6.18 --- Comment #2 from Zebediah Figura <z.figura12(a)gmail.com> --- (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. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=51822 --- Comment #3 from Ivo Ivanov <logos128(a)gmail.com> --- (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. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=51822 --- Comment #4 from Rémi Bernon <rbernon(a)codeweavers.com> --- Thanks, this looks correct indeed I'll try to add some tests as well and upstream a fix. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=51822 --- Comment #5 from Ivo Ivanov <logos128(a)gmail.com> --- (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. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=51822 --- Comment #6 from Rémi Bernon <rbernon(a)codeweavers.com> --- (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... -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=51822 --- Comment #7 from Ivo Ivanov <logos128(a)gmail.com> --- (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. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=51822 Ivo Ivanov <logos128(a)gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |FIXED Status|UNCONFIRMED |RESOLVED --- Comment #8 from Ivo Ivanov <logos128(a)gmail.com> --- Tested with 5a8dcb062793fbb68997e1b54ebc2666a2b2834d, and can confirm the issue is fixed. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=51822 Ivo Ivanov <logos128(a)gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Fixed by SHA1| |d97426be026e16f8dddcc2a003b | |f19c477668183 -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=51822 Alexandre Julliard <julliard(a)winehq.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |CLOSED --- Comment #9 from Alexandre Julliard <julliard(a)winehq.org> --- Closing bugs fixed in 6.19. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=51822 Gijs Vermeulen <gijsvrm(a)gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |regression -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
participants (1)
-
WineHQ Bugzilla