https://bugs.winehq.org/show_bug.cgi?id=51822
--- Comment #6 from Rémi Bernon rbernon@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...