[Bug 51873] New: Joy.cpl crashes on the FFB tab when the FFB device is selected in the drop down list
https://bugs.winehq.org/show_bug.cgi?id=51873 Bug ID: 51873 Summary: Joy.cpl crashes on the FFB tab when the FFB device is selected in the drop down list Product: Wine Version: 6.19 Hardware: x86-64 OS: Linux Status: UNCONFIRMED Severity: minor Priority: P2 Component: joy.cpl Assignee: wine-bugs(a)winehq.org Reporter: logos128(a)gmail.com CC: rbernon(a)codeweavers.com Distribution: ArchLinux Created attachment 70792 --> https://bugs.winehq.org/attachment.cgi?id=70792 joy_cpl_6.19_crash.log This happens when the wineprefix is configured through registry to use bus_udev / hidraw ("Enable SDL" = 0 and "DisableInput" = 1). After some debugging found out that the supported number of buttons in Joy.cpl is 32, while the device is reporting 128. In result state.rgbButtons (main.c/ff_input_thread()) is being evaluated outside of its bounds, which eventually leads to going through the check where the real crash happens in IDirectInputEffect_SetParameters():775. After fixing the out of bounds issue (attached a patch for that), now joy.cpl doesn't crash immediately, but after choosing the desired FFB effect and pushing a button on the wheel to start the test. After some more debugging found out that joy->effects[chosen_effect].effect (the first operand) is NULL, leading to the segmentation fault. So probably the lpVtbl is not initialized for some reason. Other parts of that structure looked OK IMO. Otherwise the tracking of the steering wheel on the first tab worked properly through HIDRAW, as well as the available buttons. So this is already very positive :) Have tried it also through the SDL bus ("Enable SDL" = 1), and it worked as expected. The FFB test also partially worked. The sine wave effect was functional, while the constant and conditional effect didn't react. Also while debugging the above issue saw that joy->num_buttons is incorrectly set to 134 buttons, while the device (Simucube 2 Sport) has 128. Appeared that the PID State input report which includes several usages with report count 1, is being parsed as a normal input report in dinput/joystick_hid.c/enum_objects(), and those parameters are counted as buttons. So attached a patch for this too. Do you have any hints how to debug dinput properly for the lpVtbl issue? The gdb script helps a lot BTW :) (The attached crash log is after the first patch applied, but it was similar before that.) -- 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=51873 --- Comment #1 from Ivo Ivanov <logos128(a)gmail.com> --- Created attachment 70793 --> https://bugs.winehq.org/attachment.cgi?id=70793 0001-joy.cpl-Fix-crash-in-FFB-test-tab-with-devices-which.patch -- 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=51873 --- Comment #2 from Ivo Ivanov <logos128(a)gmail.com> --- Created attachment 70794 --> https://bugs.winehq.org/attachment.cgi?id=70794 0001-dinput-Prevent-PID-Usage-Page-input-reports-being-pa.patch -- 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=51873 --- Comment #3 from Rémi Bernon <rbernon(a)codeweavers.com> --- Awesome, thanks! Yes, joy.cpl is brittle and easy to crash, and I didn't spend much time trying to fix it (yet?). For instance, I believe it's not properly checking the effect creation and assumes it succeeds as long as the effect type has been enumerated by EnumEffect / GetEffectInfo, even though it could fail depending on the effect parameters used. That may be what is happening for you. It is also very possible that joy.cpl doesn't pass the correct effect parameters structure for effect types other than periodic. I could only really test it with periodic effects. Regarding the buttons I considered raising the limit but then the layout was not very flexible. I'll have a look at your change, and see if we can maybe change the way it's presented to better support large number of buttons. -- 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=51873 --- Comment #4 from Ivo Ivanov <logos128(a)gmail.com> --- (In reply to Rémi Bernon from comment #3) Thanks! Forgot to add that the device names listed in HIDRAW (UDEV bus) mode include only the first words from the names - i.e. just "FANATEC" instead of "FANATEC ClubSport Pedals", or "Granite" instead of "Granite Devices Simucube 2 Sport". While in SDL bus mode they appear properly.
For instance, I believe it's not properly checking the effect creation and assumes it succeeds as long as the effect type has been enumerated by EnumEffect / GetEffectInfo, even though it could fail depending on the effect parameters used. That may be what is happening for you.
Indeed, seems this is the initial issue. I looked the code and during the initialization phase the enum_callback() is called to enumerate the devices, which calls ff_effects_callback() to create the effects. It calls CreateEffect, which would release the effect buffer and null the pointer in case of an error. I believe this is what happens here. The Initialization function probably passes, but the SetParameters looks like to cause the issue. I'll probably have to debug it further. The Simucube 2 doesn't include the Start Delay parameter from the Set Effect report, and strangely Negative Coefficient and Dead Band from the Condition report. On first site this shouldn't cause problems in the SetParameters function, but who knows. I have attached the Simucube 2 report descriptor for further reference.
Regarding the buttons I considered raising the limit but then the layout was not very flexible. I'll have a look at your change, and see if we can maybe change the way it's presented to better support large number of buttons.
I attached a new dinput patch with an updated description for the second issue from my initial report. Yesterday mistakenly wrote "Report Count" 1, while I meant of course "Report Size" 1. As everything with Report Size 1 is considered a button in hidparse.sys (HID_VALUE_CAPS_IS_BUTTON), when enumerating PID input reports in the joystick_hid.c' enum_objects() function, it counts those parameters as buttons. The result is total number of 134 buttons (caps.dwButtons) instead of 128. Here is an excerpt from the descriptor: /* Usage Page (PID), */ /* Usage (92h), */ /* Collection (Logical), */ /* Report ID (2), */ /* Usage (9Fh), */ /* Usage (A0h), */ /* Usage (A4h), */ /* Usage (A5h), */ /* Usage (A6h), */ /* Logical Minimum (0), */ /* Logical Maximum (1), */ /* Physical Minimum (0), */ /* Physical Maximum (1), */ /* Report Size (1), */ /* Report Count (5), */ /* Input (Variable), */ /* Report Count (3), */ /* Input (Constant, Variable), */ /* Usage (94h), */ /* Logical Minimum (0), */ /* Logical Maximum (1), */ /* Physical Minimum (0), */ /* Physical Maximum (1), */ /* Report Size (1), */ /* Report Count (1), */ /* Input (Variable), */ /* Usage (22h), */ /* Logical Minimum (1), */ /* Logical Maximum (40), */ /* Physical Minimum (1), */ /* Physical Maximum (40), */ /* Report Size (7), */ /* Report Count (1), */ /* Input (Variable), */ /* End Collection, */ -- 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=51873 Ivo Ivanov <logos128(a)gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #70794|0 |1 is obsolete| | --- Comment #5 from Ivo Ivanov <logos128(a)gmail.com> --- Created attachment 70803 --> https://bugs.winehq.org/attachment.cgi?id=70803 0001-dinput-Prevent-PID-Usage-Page-input-reports-being-pa.patch -- 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=51873 --- Comment #6 from Ivo Ivanov <logos128(a)gmail.com> --- Created attachment 70804 --> https://bugs.winehq.org/attachment.cgi?id=70804 SC2_descr.txt -- 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=51873 --- Comment #7 from Rémi Bernon <rbernon(a)codeweavers.com> --- (In reply to Ivo Ivanov from comment #4)
Regarding the buttons I considered raising the limit but then the layout was not very flexible. I'll have a look at your change, and see if we can maybe change the way it's presented to better support large number of buttons.
I attached a new dinput patch with an updated description for the second issue from my initial report. Yesterday mistakenly wrote "Report Count" 1, while I meant of course "Report Size" 1. As everything with Report Size 1 is considered a button in hidparse.sys (HID_VALUE_CAPS_IS_BUTTON), when enumerating PID input reports in the joystick_hid.c' enum_objects() function, it counts those parameters as buttons. The result is total number of 134 buttons (caps.dwButtons) instead of 128. Here is an excerpt from the descriptor:
/* Usage Page (PID), */ /* Usage (92h), */ /* Collection (Logical), */ /* Report ID (2), */ /* Usage (9Fh), */ /* Usage (A0h), */ /* Usage (A4h), */ /* Usage (A5h), */ /* Usage (A6h), */ /* Logical Minimum (0), */ /* Logical Maximum (1), */ /* Physical Minimum (0), */ /* Physical Maximum (1), */ /* Report Size (1), */ /* Report Count (5), */ /* Input (Variable), */ /* Report Count (3), */ /* Input (Constant, Variable), */ /* Usage (94h), */ /* Logical Minimum (0), */ /* Logical Maximum (1), */ /* Physical Minimum (0), */ /* Physical Maximum (1), */ /* Report Size (1), */ /* Report Count (1), */ /* Input (Variable), */ /* Usage (22h), */ /* Logical Minimum (1), */ /* Logical Maximum (40), */ /* Physical Minimum (1), */ /* Physical Maximum (40), */ /* Report Size (7), */ /* Report Count (1), */ /* Input (Variable), */ /* End Collection, */
Thanks, I send an updated patch today, with tests to validate that any input caps from the PID usage page should be ignored, including value caps (https://source.winehq.org/patches/data/217210) -- 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=51873 Julian Rüger <jr98(a)gmx.net> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jr98(a)gmx.net -- 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=51873 --- Comment #8 from Ivo Ivanov <logos128(a)gmail.com> --- I continued working on this and so far have fully functioning FFB device in HIDRAW mode in almost all tested games and apps. The FFB works very well like in Windows, and generally the setup of the wheel (Simucube 2 Sport) and the pedals (Fanatec Clubsport Pedals) in games doesn't need any additional tweaking, once the prefix is setup for hidraw, etc. Managed to capture some usb traffic from the WheelCheck app (from iRacing) to confirm that the usb data and the sequence of the reports is pretty similar between Windows 10 and Wine. Especially the direction field in the Set Effect Report, as well as other fields in the constant, periodic and condition reports. Needed to implement few additional reports - Create New Effect Report, PID Block Load Report and PID Block Free Report, which are required by the devices managing the Parameter Blocks on the device side. Also implemented Device Gain Report and the DIPROP_FFGAIN property as well. Added support for few additional properties, which are needed by various games and apps to function properly. For example the WheelCheck application needed DIPROP_PHYSICALRANGE to correctly calculate the top center of the steering wheel against its permitted range. This fixed the constant force test pulling the wheel hard to the left. Now it works just like in Windows, directing the wheel to the upper center and resisting any movement to left or right. Fixed as well few other issues. There are still few remaining issues, though. For example BeamNG doesn't have FFB due to the not implemented GetEffectStatus. They create the effect in an acquired state, and then strangely call GetEffectStatus, to find out if it's started. Lately they push constant reports through SetParameters (Download), without ever starting the effect, since the GetEffectStatus returns DIERR_UNSUPPORTED. Proton's joystick_sdl mitigate this issue with always returning DI_OK, even if the function doesn't support all devices. This way it may work depending on how the *status is initialized :) The WheelCheck application is also affected, as they use the function to update the state of the effects in real time. In the PID specification there is a PID State Input Report, which needs Get Input Report request through the control channel, as it is not sent freely on the Simucube 2 for example. I'll probably try to implement the functionality in the winebus.sys, but anyway the effect status can be implemented on the dinput part as well. Attached the patchset with the needed changes. Rebased on the latest master 6a072b98c100f38a61fad00b6c96c86b3445efac P.S. Just saw in the winehq patch list that you have worked on the issue with the not allowed single-axis effects with Cartesian coordinates, etc. Still leaving my patches on the issue for reference :) -- 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=51873 --- Comment #9 from Ivo Ivanov <logos128(a)gmail.com> --- Created attachment 71008 --> https://bugs.winehq.org/attachment.cgi?id=71008 dinput-additions-and-fixes.patch.tar.gz -- 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=51873 --- Comment #10 from Rémi Bernon <rbernon(a)codeweavers.com> --- Awesome! I'll have a look at the patches, thanks. About the GetEffectStatus issue and the PID state input report, I saw that problem too and I have some WIP patches (attached next), if you want to have a look -and feel free to make updates or rewrite then if you like- but I wanted to write some tests first and didn't find the time to do it yet. -- 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=51873 --- Comment #11 from Rémi Bernon <rbernon(a)codeweavers.com> --- Created attachment 71009 --> https://bugs.winehq.org/attachment.cgi?id=71009 WIP patches for PID input state report -- 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=51873 --- Comment #12 from Ivo Ivanov <logos128(a)gmail.com> --- (In reply to Rémi Bernon from comment #11)
Created attachment 71009 [details] WIP patches for PID input state report
Thanks! The patchset looks good. The SDL and lnxev parts look implemented well. My device (Simucube 2) though, doesn't send a single PID State Report on the INTERRUPT IN endpoint. It'll need the input report requested through the control endpoint with the HIDIOCGINPUT from the winebus.sys/bus_udev hidraw part. I thought that if there is no available report with the requested id from the last read through the INTERRUPT IN channel, then something like the Get Feature Report can be done in IOCTL_HID_GET_INPUT_REPORT in winebus.sys/main.c hid_internal_dispatch(), 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=51873 --- Comment #13 from Rémi Bernon <rbernon(a)codeweavers.com> --- I see. It could be possible to do it the same way as feature reports but the problem I'm seeing there is that IOCTL_HID_GET_INPUT_REPORT needs to be explicitly requested and I don't see how it would specify the event number it's interested in. Is HIDIOCGINPUT returning an error if there's no more input report for the matching id, or is it blocking until there's one? If it's the case, we'll maybe have to implement IOCTL_HID_GET_INPUT_REPORT in the same kind of way then, instead of succeeding every time and returning the last report seen. This would need to be the case for all the backend though, and maybe not completely straightforward. The client (dinput) will also need to make a dedicated asynchronous ioctl to read effect statuses, instead of reading them in the same way as the device state reports. Another possible approach would be to do like the SDL backend, where we would request HIDIOCGINPUT regularly and push the report in the same way as device state reports. That may be simpler from the client perspective as we wouldn't have to worry whether a device needs IOCTL_HID_GET_INPUT_REPORT requests or just ReadFile calls. -- 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=51873 --- Comment #14 from Rémi Bernon <rbernon(a)codeweavers.com> --- Sorry, effect number, not event. -- 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=51873 --- Comment #15 from Ivo Ivanov <logos128(a)gmail.com> --- (In reply to Rémi Bernon from comment #13)
It could be possible to do it the same way as feature reports but the problem I'm seeing there is that IOCTL_HID_GET_INPUT_REPORT needs to be explicitly requested and I don't see how it would specify the event number it's interested in.
I didn't think about the effect id. Yeah, looks like the most logical way to implement this from manufacturer POV is to send periodical Input reports on the INTERRUPT IN channel, and seems this is what the specification suggests. Otherwise they would have written this explicitly. There is no logic for such report to be implemented on the control endpoint, without a way to specify the input parameters as effect id, etc. If the authors of the specification wanted such a thing, they would have implemented it with feature reports in the same manner as Create New Effect -> PID Block Load sequence. I've looked again the usb capture data from the WheelCheck application on Windows 10, and there is no trace of any Input report through the control endpoint or any Input report with id 2 (the PID State Report id in Simucube 2) in the INTERRUPT IN endpoint. So MS doesn't use the control endpoint as well. IMO this is probably either overlooked by the device manufacturer, or a bug in the firmware. I'm not on the latest firmware, though. There are few iterations from the last few months, that mostly add support for new revisions of the device. I'll install the latest firmware, and let you know for the results. I plan to contact the manufacturer for the issue with the report descriptor we talked about a few weeks ago, so I can tell them about this issue too.
Is HIDIOCGINPUT returning an error if there's no more input report for the matching id, or is it blocking until there's one?
If it's the case, we'll maybe have to implement IOCTL_HID_GET_INPUT_REPORT in the same kind of way then, instead of succeeding every time and returning the last report seen. This would need to be the case for all the backend though, and maybe not completely straightforward.
I have read again the MS documentation about the IOCTL_HID_GET_INPUT_REPORT / HidD_GetInputReport. They seem to suggest that IOCTL_HID_GET_* / HidD_Get* routines should be used non regularly only to get the current state of the device. Something that is always mentioned for the control endpoint in the USB HID specification. They also always couple the info for IOCTL_HID_GET_INPUT_REPORT with IOCTL_HID_GET_FEATURE. For continuously obtaining Input reports, they suggest IRP_MJ_READ / ReadFile. So I would assume that IRP_MJ_READ uses the highly responsive INTERRUPT IN endpoint, while IOCTL_HID_GET_* uses the control endpoint. So, IMO to be more consistent with MS' documentation, we should implement IOCTL_HID_GET_INPUT_REPORT (using HIDIOCGINPUT) in the same manner as IOCTL_HID_GET_FEATURE. This way it (HidD_GetInputReport) would do exactly what the applications expect from it. And some apps might use it just because it supposedly uses the control endpoint, and not the INTERRUPT IN. It would also help to better and more easily test this particular issue, of course.
The client (dinput) will also need to make a dedicated asynchronous ioctl to read effect statuses, instead of reading them in the same way as the device state reports.
Another possible approach would be to do like the SDL backend, where we would request HIDIOCGINPUT regularly and push the report in the same way as device state reports. That may be simpler from the client perspective as we wouldn't have to worry whether a device needs IOCTL_HID_GET_INPUT_REPORT requests or just ReadFile calls.
IMO, the proposed patchset is the right way to implement this feature. If the state of the effect is unknown by the time GetEffectStatus is called, then there should be an alternative way to calculate this on the dinput side. It would need the effect start time, loop count (or infinity), duration and startdelay (if supported) to calculate the current state of the effect, taking into account if the device is acquired and all the other factors, etc. On every successful Start it would set the start time, and on every Stop, Unload, Reset, etc. it would zero it. Every successful SetParameters would set the other time parameters. Then it's pretty easy to calculate if the effect is currently playing or not. -- 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=51873 --- Comment #16 from Ivo Ivanov <logos128(a)gmail.com> --- Created attachment 71017 --> https://bugs.winehq.org/attachment.cgi?id=71017 dinput-additions-and-fixes.patch Rebased the patchset to the current master 97d6cfd7059fbe55fdd24a04e8d9133848328d4e Decided to save you some time, since there are a lot of merge conflicts with today's 'master' :) -- 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=51873 --- Comment #17 from Rémi Bernon <rbernon(a)codeweavers.com> --- Thanks, I had it rebased already and I started upstreaming the patches after tweaking them a bit. For the patch to only send modified reports, I'm not sure it's right, at least according to the conformance tests, although it could be some device specific behavior. According to the conformance tests, the generic PID driver always seems to send the reports, regardless of whether they are actually different from the previous one or not. Was it actually required for your device or just an optimization? -- 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=51873 --- Comment #18 from Ivo Ivanov <logos128(a)gmail.com> --- (In reply to Rémi Bernon from comment #17)
Thanks, I had it rebased already and I started upstreaming the patches after tweaking them a bit.
For the patch to only send modified reports, I'm not sure it's right, at least according to the conformance tests, although it could be some device specific behavior.
According to the conformance tests, the generic PID driver always seems to send the reports, regardless of whether they are actually different from the previous one or not.
I just realized this myself. Finally captured some usb traffic from my gaming machine, since till now used VMs to capture some traffic from Windows. They (MS) don't send all the reports, though. Just those that have been probably set through the dwFlags parameter of SetParameters, and seems the whole logic conforms to the SetParameters documentation. For example BeamNG sends one Set Effect Report with infinite duration, one Effect Operation Report (to start the report) and then starts pushing thousands of Set Constant Force Reports ~260Hz. On other side Project Cars 2 and Automobilista 2, which use the Madness physic engine, send one Effect Operation Report to start the effect, and then start pushing couples of Set Effect Report with infinite duration and Set Constant Force Report. The Set Effect Report is exactly the same through the whole session, and according to the MS documentation is not needed, since the dynamic effect parameters can be changed on the fly during the effect duration, as if the effect was started with the new values. Seems the PID specification follows this logic, or probably the device manufacturers follow it :) Automobilista 2 sends FFB reports with ~400Hz rate, which is doubled to 800Hz with unneeded Set Effect Reports, since they probably call SetParameters with something like DIEP_DURATION, DIEP_GAIN, DIEP_STARTDELAY, etc., in addition to the DIEP_TYPESPECIFICPARAMS. So, according to the usb traffic captures of the Simucube 2, the PID driver probably follows the logic of the SetParamers' dwFlags, and sends only the reports which are requested through the dwFlags.
Was it actually required for your device or just an optimization?
I have followed the logic of the Linux PID driver. For example Automobilista 2 on Proton/Wine sends (through joystick_sdl) only Set Constant Force reports with ~550Hz rate, which is 150Hz more than Windows. Even if on Windows the rate is ~800Hz, only half of it is useful for the device. They probably try to send more, but unfortunately reach the bus limits :) -- 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=51873 --- Comment #19 from Ivo Ivanov <logos128(a)gmail.com> --- Created attachment 71043 --> https://bugs.winehq.org/attachment.cgi?id=71043 Simucube_2_logs.tar.gz Attached logs with usb captured traffic from Simucube 2 on a Windows machine. There are two logs from Automobilista 2 and BeamNG. Added also two Wine logs (+dinput) from the same games. Just for a reference what's being called in dinput. BeamNG's wine log is from proton (joystick_sdl), since it's FFB is not yet functioning in Wine due to the not supported GetEffectStatus. It's evident from the captured data, that only the reports that are explicitly set through the flags operand in SetParameters, are being send by the PID driver. For example Automobilsta 2 calls repeatedly SetParameters with DIEP_TYPESPECIFICPARAMS + DIEP_GAIN (0x104). In result the PID driver sends Set Effect Report and Set Constant Force Report. They probably set the gain to it's max value every time, which results in 400Hz rate of not needed Set Effect Reports with exactly the same values. BeamNG calls SetParameters repeatedly with just DIEP_TYPESPECIFICPARAMS (0x100), which results in only Set Constant Reports being send by the PID driver. They send a lot of Set Constant Reports which are exactly the same without ever playing them. The first Start report command is on frame 21943. Also we can note that there are no envelope reports at all, since this parameter is not used by these games - they never set DIEP_ENVELOPE in any SetParameters call. Some handy filters for WireShark (for examining the usb traffic data): - filter only the Output reports - (usb.transfer_type == 0x1) && (usb.endpoint_address.direction == 0) && (usb.src == "host") - filter only the Input reports - (usb.transfer_type == 0x1) && (usb.endpoint_address.direction == 1) && (usb.dst == "host") -- 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=51873 --- Comment #20 from Ivo Ivanov <logos128(a)gmail.com> --- Created attachment 71044 --> https://bugs.winehq.org/attachment.cgi?id=71044 dinput-additions-and-fixes.patch Rebased the remaining patches to the current master be0684dad50ffbc93b3ded4fbfebf1d1e4690589 Slightly reworked the patch for sending only the modified reports. Added modified_envelope instead of modified_type_specific[1], etc. Also it should now pass the current dinput conformance tests. It still includes the checks for the modified values, with the exception of duration, which will set the modified_effect_update flag every time it is different from INFINITE. If you decide to make it fully compliant to the MS PID driver, then probably can set the three modified flags in accordance to the flags operand in SetParameters. -- 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=51873 Ivo Ivanov <logos128(a)gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #71017|0 |1 is obsolete| | Attachment #71044|0 |1 is obsolete| | --- Comment #21 from Ivo Ivanov <logos128(a)gmail.com> --- Created attachment 71063 --> https://bugs.winehq.org/attachment.cgi?id=71063 dinput-additions-and-fixes.patch Added two new patches to support the PID_USAGE_PARAMETER_BLOCK_OFFSET usage in the Set Condition Report of dinput and UDEV lnxev + SDL backends. Added also a patch for the UDEV lnxev and SDL backends, which makes their behavior similar to the MS PID driver, in relation to the output reports being sent only when the corresponding flag in SetParameters is set. The patch also makes those backends compatible with the modified reports patch. While looking at the backends, noted that the Logical min/max values of various usages as magnitude, offset, attack/fade level, pos/neg coefficient and saturation, deadband, ramp start/end, gain, etc., are being set to one byte max values. Thus the frontend scales them down to those ranges and sends them to the backends which pass them directly to ff_effect / SDL_HapticEffect. While those structures expect mostly 16bit signed/unsigned values in the range of -32767/+32767 or 0/65535. IMO this could be one of the reasons for non existent FFB in those backends. I'll look further when the time permits. Rebased the remaining patches to the current master b65ef71fc0a7044557e0ba530e3b95497644867c P.S. Saw that you already submitted the DIPROP_FFGAIN patch in winehq, so excluded it as well. From the other new reports probably the Block Free Report will need additional implementation in the backends, similar to the Device Gain Report. -- 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=51873 --- Comment #22 from Rémi Bernon <rbernon(a)codeweavers.com> --- (In reply to Ivo Ivanov from comment #21)
Created attachment 71063 [details] dinput-additions-and-fixes.patch
Added two new patches to support the PID_USAGE_PARAMETER_BLOCK_OFFSET usage in the Set Condition Report of dinput and UDEV lnxev + SDL backends. Added also a patch for the UDEV lnxev and SDL backends, which makes their behavior similar to the MS PID driver, in relation to the output reports being sent only when the corresponding flag in SetParameters is set. The patch also makes those backends compatible with the modified reports patch.
Thanks, is the MS PID driver actually using these reports with your device? I haven't yet found a way to have them used with the virtual HID driver we have for conformance testing. It would be really nice if we could test them too.
While looking at the backends, noted that the Logical min/max values of various usages as magnitude, offset, attack/fade level, pos/neg coefficient and saturation, deadband, ramp start/end, gain, etc., are being set to one byte max values. Thus the frontend scales them down to those ranges and sends them to the backends which pass them directly to ff_effect / SDL_HapticEffect. While those structures expect mostly 16bit signed/unsigned values in the range of -32767/+32767 or 0/65535. IMO this could be one of the reasons for non existent FFB in those backends. I'll look further when the time permits.
I just used the same kind of value range as some sample descriptors from the PID spec, or as the SideWinder HID descriptors that can be found on internet. This can definitely be modified to better match backend ranges and avoid unnecessary rounding (although SDL and evdev may have slightly different ranges).
Rebased the remaining patches to the current master b65ef71fc0a7044557e0ba530e3b95497644867c
Thanks, I have some of the patches split up locally that I'll send later, mainly about the properties which I instead moved to be handled directly in the base device class. I'll have a look and integrate your changes. -- 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=51873 --- Comment #23 from Ivo Ivanov <logos128(a)gmail.com> --- (In reply to Rémi Bernon from comment #22)
Thanks, is the MS PID driver actually using these reports with your device? I haven't yet found a way to have them used with the virtual HID driver we have for conformance testing. It would be really nice if we could test them too.
Yes, it needs the Create New Effect Report and PID Block Load Report to function properly, as it is Device Managed. I think most of the modern PID devices manage their block index on the device side. Those reports are visible in the usb traffic captures (Simucube_2_logs.tar.gz) attached to this bug report. IMO the virtual HID driver need to implement the PID Pool Feature Report (look the attached SC2_descr.txt - Usage (7Fh)), and set the Device Managed Pool usage there. With other words it needs to advertise itself as Device Managed. The Linux driver does just this to check if the device is Device Managed. Here (in Wine) we check for the existence of the Create New Effect Report, and assume that the device manages its blocks internally, since this and the other reports are used only by such devices as per specification.
I just used the same kind of value range as some sample descriptors from the PID spec, or as the SideWinder HID descriptors that can be found on internet. This can definitely be modified to better match backend ranges and avoid unnecessary rounding (although SDL and evdev may have slightly different ranges).
The ranges in the frontend dinput/joystick_hid.c are OK, IMO. They are similar to my device's report descriptor. I meant the "virtual" HID report descriptors in the UDEV lnxev and the SDL backends. Their logical ranges are too small, and are being passed without any rescaling to the ff_effect / SDL_HapticEffect structures. Unless I am overlooking something :) -- 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=51873 --- Comment #24 from Rémi Bernon <rbernon(a)codeweavers.com> --- (In reply to Ivo Ivanov from comment #23)
(In reply to Rémi Bernon from comment #22)
Thanks, is the MS PID driver actually using these reports with your device? I haven't yet found a way to have them used with the virtual HID driver we have for conformance testing. It would be really nice if we could test them too.
Yes, it needs the Create New Effect Report and PID Block Load Report to function properly, as it is Device Managed. I think most of the modern PID devices manage their block index on the device side. Those reports are visible in the usb traffic captures (Simucube_2_logs.tar.gz) attached to this bug report.
IMO the virtual HID driver need to implement the PID Pool Feature Report (look the attached SC2_descr.txt - Usage (7Fh)), and set the Device Managed Pool usage there. With other words it needs to advertise itself as Device Managed. The Linux driver does just this to check if the device is Device Managed.
Ah I see, I'll try that. Thanks!
Here (in Wine) we check for the existence of the Create New Effect Report, and assume that the device manages its blocks internally, since this and the other reports are used only by such devices as per specification.
I just used the same kind of value range as some sample descriptors from the PID spec, or as the SideWinder HID descriptors that can be found on internet. This can definitely be modified to better match backend ranges and avoid unnecessary rounding (although SDL and evdev may have slightly different ranges).
The ranges in the frontend dinput/joystick_hid.c are OK, IMO. They are similar to my device's report descriptor.
I meant the "virtual" HID report descriptors in the UDEV lnxev and the SDL backends. Their logical ranges are too small, and are being passed without any rescaling to the ff_effect / SDL_HapticEffect structures. Unless I am overlooking something :)
Oh yeah you're right indeed, there is some scaling for some of the values like direction or magnitude, but I completely missed some others... Duh! -- 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=51873 --- Comment #25 from Rémi Bernon <rbernon(a)codeweavers.com> --- I sent some fixes to address the parameter scaling problems as well as patches for this specific bug (https://source.winehq.org/patches/data/220003 to https://source.winehq.org/patches/data/220010), although the discussion here was more general. I've also been able to make device managed effect creation reports work in the test driver, and added a couple of test to validate the implementation, and sent the patches as a separate batch. What remains then would be partial updates, which I have locally and intend to send later, and effect state input reports, which may need some more work and investigation wrt how HID_GET_INPUT_REPORT should be implemented. -- 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=51873 Rémi Bernon <rbernon(a)codeweavers.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.winehq.org/sho | |w_bug.cgi?id=52061 -- 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=51873 Rémi Bernon <rbernon(a)codeweavers.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.winehq.org/sho | |w_bug.cgi?id=52062 -- 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=51873 --- Comment #26 from Ivo Ivanov <logos128(a)gmail.com> --- (In reply to Rémi Bernon from comment #25)
I sent some fixes to address the parameter scaling problems as well as patches for this specific bug (https://source.winehq.org/patches/data/220003 to https://source.winehq.org/patches/data/220010), although the discussion here was more general.
Great! Seems very well implemented. I'll test it this weekend with my device and let you know how it performs.
I've also been able to make device managed effect creation reports work in the test driver, and added a couple of test to validate the implementation, and sent the patches as a separate batch. What remains then would be partial updates, which I have locally and intend to send later, and effect state input reports, which may need some more work and investigation wrt how HID_GET_INPUT_REPORT should be implemented.
I have installed the latest firmware on the Simucube 2, but it still doesn't send PID State input reports. I plan to implement some basic logic on the dinput side, to calculate the state of the effects. Just as a backup for such cases. Will ask the manufacturer, how this feature is supposed to be used. -- 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=51873 --- Comment #27 from Rémi Bernon <rbernon(a)codeweavers.com> --- I believe this specific issue has been fixed since c2235c44c5f8fedaad775c69b9a475725874bbc7, could you confirm? Regarding effect state report and the way IOCTL_HID_GET_INPUT_REPORT is supposed to work I suggest moving the discussion to https://bugs.winehq.org/show_bug.cgi?id=52062. -- 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=51873 Ivo Ivanov <logos128(a)gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|UNCONFIRMED |RESOLVED Resolution|--- |FIXED --- Comment #28 from Ivo Ivanov <logos128(a)gmail.com> --- (In reply to Rémi Bernon from comment #27)
I believe this specific issue has been fixed since c2235c44c5f8fedaad775c69b9a475725874bbc7, could you confirm?
Regarding effect state report and the way IOCTL_HID_GET_INPUT_REPORT is supposed to work I suggest moving the discussion to https://bugs.winehq.org/show_bug.cgi?id=52062.
I can confirm it is fixed. I was going to change the bug status, just wanted to write my report from the weekend :) I am happy to say that on the UDEV hidraw backend everything works as expected. With the games I tested the FFB is pretty much equal to Windows. I compared some captured usb traffic and the sent commands are similar in frequency and sequence. There are some small differences mostly around the RESET command, but I'll have to confirm that further with some conformance testing. The direction used by WheelCheck and the most games is 0x2d on both Windows and Wine. I tested also the other backends. The SDL backend works very well now. The FFB is functional, although seems somewhat smoother with less details in comparison to Windows / Wine (hidraw). The characteristic is the same, just with less 'punch'. My first thought was that it could be higher frequency of the FFB rate, which in theory could provide a smoother force curve. As these direct drive devices are capable of playing very small changes of the forces on almost ms level. The frequencies were similar, though, ~560Hz on Wine (hidraw / SDL) and ~400Hz on Windows (Automobilista 2). So I compared some usb traffic and the first noticeable thing is a difference in the direction value, 0x2d on Windows / Wine (hidraw) and 0x5a on SDL. IMO this seems like a candidate for such slight difference in the FFB behavior. I plan to compare it with proton's joystick_sdl behavior. The UDEV lnxev backend doesn't produce FFB, though. It manages to turn off the bump stops of the wheel, which can be resolved normally with a power-cycle of the device. There is a Reset FFB command in the TrueDrive software (Simucube 2's controlling app), which also resolves the normal behavior. That leads me to think it just turns off the actuators with an FFB command. Probably this will need another bug report. I have also a strange crash of the winebus.sys process when using the UDEV bus. It occurs only when using an administrator account (with superuser rights). I will post a bug report with a patch, since finally had some time and found the issue. -- 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=51873 Ivo Ivanov <logos128(a)gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Fixed by SHA1| |c2235c44c5f8fedaad775c69b9a | |475725874bbc7 -- 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=51873 Alexandre Julliard <julliard(a)winehq.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |CLOSED --- Comment #29 from Alexandre Julliard <julliard(a)winehq.org> --- Closing bugs fixed in 6.23. -- 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