This is a preparation for a patch that changes setupapi to return lowercased device paths, which is the source of RawInput device names. On Windows RawInput returns mostly uppercase names (except the GUID part) and we want to keep Wine's old behavior in this regards as SDL 2.0.14+ looks for uppercase IG_ to match xinput devices to their RawInput counterparts.
Signed-off-by: Arkadiusz Hiler ahiler@codeweavers.com --- dlls/user32/rawinput.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/dlls/user32/rawinput.c b/dlls/user32/rawinput.c index ba11a121bc5..13aac0278d5 100644 --- a/dlls/user32/rawinput.c +++ b/dlls/user32/rawinput.c @@ -94,7 +94,7 @@ static struct device *add_device(HDEVINFO set, SP_DEVICE_INTERFACE_DATA *iface) SP_DEVICE_INTERFACE_DETAIL_DATA_W *detail; struct device *device; HANDLE file; - WCHAR *path; + WCHAR *path, *pos; DWORD size;
SetupDiGetDeviceInterfaceDetailW(set, iface, NULL, 0, &size, NULL); @@ -121,6 +121,9 @@ static struct device *add_device(HDEVINFO set, SP_DEVICE_INTERFACE_DATA *iface) } heap_free(detail);
+ /* upper case everything but the GUID */ + for (pos = path; *pos && *pos != '{'; pos++) *pos = towupper(*pos); + file = CreateFileW(path, GENERIC_READ | GENERIC_WRITE, FILE_SHARE_READ | FILE_SHARE_WRITE, NULL, OPEN_EXISTING, 0, 0); if (file == INVALID_HANDLE_VALUE)
This prepares xinput for the upcoming change in setupapi path casing.
Signed-off-by: Arkadiusz Hiler ahiler@codeweavers.com --- dlls/xinput1_3/hid.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/dlls/xinput1_3/hid.c b/dlls/xinput1_3/hid.c index 8aea638a8d7..3a7d3f4ead6 100644 --- a/dlls/xinput1_3/hid.c +++ b/dlls/xinput1_3/hid.c @@ -247,7 +247,7 @@ void HID_find_gamepads(xinput_controller *devices) &interface_data, data, sizeof(*data) + detail_size, NULL, NULL)) continue;
- if (!wcsstr(data->DevicePath, L"IG_")) + if (!wcsstr(data->DevicePath, L"IG_") && !wcsstr(data->DevicePath, L"ig_")) continue;
open_device_idx = -1;
Hi,
While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check?
Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=85964
Your paranoid android.
=== debiant2 (32 bit report) ===
user32: win.c:3079: Test succeeded inside todo block: Focus should be on child 000800FE, not 000800FE
=== debiant2 (32 bit Chinese:China report) ===
user32: win.c:3079: Test succeeded inside todo block: Focus should be on child 000800FE, not 000800FE
=== debiant2 (32 bit WoW report) ===
user32: win.c:3079: Test succeeded inside todo block: Focus should be on child 000800FE, not 000800FE
=== debiant2 (64 bit WoW report) ===
user32: win.c:3079: Test succeeded inside todo block: Focus should be on child 000800FE, not 000800FE
Some games are doing case sensitive matches on the device paths obtained from setupapi and expect them to be lowercase.
This fixes Virginia (and possibly other games using Rewired) not being able to discover DualShock 4 over bus_udev.
Signed-off-by: Arkadiusz Hiler ahiler@codeweavers.com --- dlls/setupapi/devinst.c | 3 +++ dlls/setupapi/tests/devinst.c | 13 +++++++++++++ 2 files changed, 16 insertions(+)
diff --git a/dlls/setupapi/devinst.c b/dlls/setupapi/devinst.c index de0413e74f5..d5ddd3a6b41 100644 --- a/dlls/setupapi/devinst.c +++ b/dlls/setupapi/devinst.c @@ -403,6 +403,9 @@ static LPWSTR SETUPDI_CreateSymbolicLinkPath(LPCWSTR instanceId, lstrcpyW(ret + printed + 1, ReferenceString); } } + + CharLowerW(ret); + return ret; }
diff --git a/dlls/setupapi/tests/devinst.c b/dlls/setupapi/tests/devinst.c index 48b84c21e84..ea8d9b5a682 100644 --- a/dlls/setupapi/tests/devinst.c +++ b/dlls/setupapi/tests/devinst.c @@ -1025,6 +1025,18 @@ static void test_register_device_info(void) SetupDiDestroyDeviceInfoList(set); }
+static void check_all_lower_case(int line, const char* str) +{ + const char *cur; + + for (cur = str; *cur; cur++) + { + BOOL is_lower = (tolower(*cur) == *cur); + ok_(__FILE__, line)(is_lower, "Expected device path to be all lowercase but got %s.\n", str); + if (!is_lower) break; + } +} + static void check_device_iface_(int line, HDEVINFO set, SP_DEVINFO_DATA *device, const GUID *class, int index, DWORD flags, const char *path) { @@ -1046,6 +1058,7 @@ static void check_device_iface_(int line, HDEVINFO set, SP_DEVINFO_DATA *device, ret = SetupDiGetDeviceInterfaceDetailA(set, &iface, detail, sizeof(buffer), NULL, NULL); ok_(__FILE__, line)(ret, "Failed to get interface detail, error %#x.\n", GetLastError()); ok_(__FILE__, line)(!strcasecmp(detail->DevicePath, path), "Got unexpected path %s.\n", detail->DevicePath); + check_all_lower_case(line, detail->DevicePath); } else {
Hi,
While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check?
Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=85965
Your paranoid android.
=== debiant2 (32 bit report) ===
user32: win.c:3079: Test succeeded inside todo block: Focus should be on child 000800FE, not 000800FE
=== debiant2 (32 bit Chinese:China report) ===
user32: menu.c:2337: Test failed: test 25 win.c:3079: Test succeeded inside todo block: Focus should be on child 000800FE, not 000800FE
=== debiant2 (32 bit WoW report) ===
user32: win.c:3079: Test succeeded inside todo block: Focus should be on child 000800FE, not 000800FE
=== debiant2 (64 bit WoW report) ===
user32: clipboard.c:1168: Test failed: OpenClipboard failed: 5 clipboard.c:1170: Test failed: EmptyClipboard failed: 1418 clipboard.c:1172: Test failed: CloseClipboard failed: 1418 clipboard.c:1174: Test failed: sequence diff 0 clipboard.c:1017: Test failed: wait failed clipboard.c:1174: Test failed: WM_DRAWCLIPBOARD not received clipboard.c:1174: Test failed: WM_CLIPBOARDUPDATE not received clipboard.c:1199: Test failed: WM_DESTROYCLIPBOARD received menu.c:2337: Test failed: test 25 win.c:3079: Test succeeded inside todo block: Focus should be on child 000800FE, not 000800FE
Hi,
While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check?
Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=85963
Your paranoid android.
=== debiant2 (32 bit report) ===
user32: win.c:3079: Test succeeded inside todo block: Focus should be on child 000800FE, not 000800FE
=== debiant2 (32 bit Chinese:China report) ===
user32: win.c:3079: Test succeeded inside todo block: Focus should be on child 000800FE, not 000800FE
=== debiant2 (32 bit WoW report) ===
user32: win.c:3079: Test succeeded inside todo block: Focus should be on child 000800FE, not 000800FE win.c:10097: Test failed: Expected foreground window 0, got 00E10102 win.c:10103: Test failed: Expected foreground window 000E013E, got 00E10102
=== debiant2 (64 bit WoW report) ===
user32: win.c:3079: Test succeeded inside todo block: Focus should be on child 000800FE, not 000800FE
On 2/22/21 12:50 PM, Arkadiusz Hiler wrote:
This is a preparation for a patch that changes setupapi to return lowercased device paths, which is the source of RawInput device names. On Windows RawInput returns mostly uppercase names (except the GUID part) and we want to keep Wine's old behavior in this regards as SDL 2.0.14+ looks for uppercase IG_ to match xinput devices to their RawInput counterparts.
Signed-off-by: Arkadiusz Hiler ahiler@codeweavers.com
dlls/user32/rawinput.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/dlls/user32/rawinput.c b/dlls/user32/rawinput.c index ba11a121bc5..13aac0278d5 100644 --- a/dlls/user32/rawinput.c +++ b/dlls/user32/rawinput.c @@ -94,7 +94,7 @@ static struct device *add_device(HDEVINFO set, SP_DEVICE_INTERFACE_DATA *iface) SP_DEVICE_INTERFACE_DETAIL_DATA_W *detail; struct device *device; HANDLE file;
- WCHAR *path;
WCHAR *path, *pos; DWORD size;
SetupDiGetDeviceInterfaceDetailW(set, iface, NULL, 0, &size, NULL);
@@ -121,6 +121,9 @@ static struct device *add_device(HDEVINFO set, SP_DEVICE_INTERFACE_DATA *iface) } heap_free(detail);
- /* upper case everything but the GUID */
- for (pos = path; *pos && *pos != '{'; pos++) *pos = towupper(*pos);
file = CreateFileW(path, GENERIC_READ | GENERIC_WRITE, FILE_SHARE_READ | FILE_SHARE_WRITE, NULL, OPEN_EXISTING, 0, 0); if (file == INVALID_HANDLE_VALUE)
Hi Arek!
I'd think a test to validate this could be nice. I know we don't have any gamepad device available on the test bot, but maybe it's still possible to validate at least with with mouse / keyboard devices? It would also let us check the test manually on a real Windows with a gamepad plugged in.
Also, the commit title should probably be "user32:", not "RawInput:".
Cheers,
On Wed, Feb 24, 2021 at 08:55:07AM +0100, Rémi Bernon wrote:
On 2/22/21 12:50 PM, Arkadiusz Hiler wrote:
This is a preparation for a patch that changes setupapi to return lowercased device paths, which is the source of RawInput device names. On Windows RawInput returns mostly uppercase names (except the GUID part) and we want to keep Wine's old behavior in this regards as SDL 2.0.14+ looks for uppercase IG_ to match xinput devices to their RawInput counterparts.
Signed-off-by: Arkadiusz Hiler ahiler@codeweavers.com
dlls/user32/rawinput.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/dlls/user32/rawinput.c b/dlls/user32/rawinput.c index ba11a121bc5..13aac0278d5 100644 --- a/dlls/user32/rawinput.c +++ b/dlls/user32/rawinput.c @@ -94,7 +94,7 @@ static struct device *add_device(HDEVINFO set, SP_DEVICE_INTERFACE_DATA *iface) SP_DEVICE_INTERFACE_DETAIL_DATA_W *detail; struct device *device; HANDLE file;
- WCHAR *path;
- WCHAR *path, *pos; DWORD size; SetupDiGetDeviceInterfaceDetailW(set, iface, NULL, 0, &size, NULL);
@@ -121,6 +121,9 @@ static struct device *add_device(HDEVINFO set, SP_DEVICE_INTERFACE_DATA *iface) } heap_free(detail);
- /* upper case everything but the GUID */
- for (pos = path; *pos && *pos != '{'; pos++) *pos = towupper(*pos);
file = CreateFileW(path, GENERIC_READ | GENERIC_WRITE, FILE_SHARE_READ | FILE_SHARE_WRITE, NULL, OPEN_EXISTING, 0, 0); if (file == INVALID_HANDLE_VALUE)Hi Arek!
I'd think a test to validate this could be nice. I know we don't have any gamepad device available on the test bot, but maybe it's still possible to validate at least with with mouse / keyboard devices? It would also let us check the test manually on a real Windows with a gamepad plugged in.
Hey,
Thanks for the feedback.
Looking at this a bit closer there seems to be more going on with the paths on Windows:
rawinput: \?\HID#VID_054C&PID_0CE6&MI_03#7&207751a1&0&0000#{4d1e55b2-f16f-11cf-88cb-001111000030} setupapi: \?\hid#vid_054c&pid_0ce6&mi_03#7&207751a1&0&0000#{4d1e55b2-f16f-11cf-88cb-001111000030} instance_id: HID\VID_054C&PID_0CE6&MI_03\7&207751A1&0&0000
or
rawinput: \?\HID#VID_046D&PID_C52B&MI_02&Col01#7&25fe7032&0&0000#{4d1e55b2-f16f-11cf-88cb-001111000030} setupapi: \?\hid#vid_046d&pid_c52b&mi_01&col01#7&392e8c70&0&0000#{4d1e55b2-f16f-11cf-88cb-001111000030} instance_id: HID\VID_046D&PID_C52B&MI_01&COL01\7&392E8C70&0&0000
It's not even uppercasing up to second '#'. It looks like there's another source of path/instance_id that is case sensitive (Col01 vs COL01). So having a test that checks if everything is uppercase would fail on Windows.
I would treat the patch in current form more as "keeping the old behavior that we know some software depends on[0]" instead of "fixing this once and for all".
[0]: https://github.com/libsdl-org/SDL/blob/fadfa5/src/joystick/windows/SDL_rawin...
I'll do a bit more digging in the registry.
Also, the commit title should probably be "user32:", not "RawInput:".
Not sure why I did that... I'll fix it in the next revision.
On 2/24/21 9:50 AM, Arkadiusz Hiler wrote:
On Wed, Feb 24, 2021 at 08:55:07AM +0100, Rémi Bernon wrote:
On 2/22/21 12:50 PM, Arkadiusz Hiler wrote:
This is a preparation for a patch that changes setupapi to return lowercased device paths, which is the source of RawInput device names. On Windows RawInput returns mostly uppercase names (except the GUID part) and we want to keep Wine's old behavior in this regards as SDL 2.0.14+ looks for uppercase IG_ to match xinput devices to their RawInput counterparts.
Signed-off-by: Arkadiusz Hiler ahiler@codeweavers.com
dlls/user32/rawinput.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/dlls/user32/rawinput.c b/dlls/user32/rawinput.c index ba11a121bc5..13aac0278d5 100644 --- a/dlls/user32/rawinput.c +++ b/dlls/user32/rawinput.c @@ -94,7 +94,7 @@ static struct device *add_device(HDEVINFO set, SP_DEVICE_INTERFACE_DATA *iface) SP_DEVICE_INTERFACE_DETAIL_DATA_W *detail; struct device *device; HANDLE file;
- WCHAR *path;
- WCHAR *path, *pos; DWORD size; SetupDiGetDeviceInterfaceDetailW(set, iface, NULL, 0, &size, NULL);
@@ -121,6 +121,9 @@ static struct device *add_device(HDEVINFO set, SP_DEVICE_INTERFACE_DATA *iface) } heap_free(detail);
- /* upper case everything but the GUID */
- for (pos = path; *pos && *pos != '{'; pos++) *pos = towupper(*pos);
file = CreateFileW(path, GENERIC_READ | GENERIC_WRITE, FILE_SHARE_READ | FILE_SHARE_WRITE, NULL, OPEN_EXISTING, 0, 0); if (file == INVALID_HANDLE_VALUE)Hi Arek!
I'd think a test to validate this could be nice. I know we don't have any gamepad device available on the test bot, but maybe it's still possible to validate at least with with mouse / keyboard devices? It would also let us check the test manually on a real Windows with a gamepad plugged in.
Hey,
Thanks for the feedback.
Looking at this a bit closer there seems to be more going on with the paths on Windows:
rawinput: \?\HID#VID_054C&PID_0CE6&MI_03#7&207751a1&0&0000#{4d1e55b2-f16f-11cf-88cb-001111000030} setupapi: \?\hid#vid_054c&pid_0ce6&mi_03#7&207751a1&0&0000#{4d1e55b2-f16f-11cf-88cb-001111000030} instance_id: HID\VID_054C&PID_0CE6&MI_03\7&207751A1&0&0000
or
rawinput: \?\HID#VID_046D&PID_C52B&MI_02&Col01#7&25fe7032&0&0000#{4d1e55b2-f16f-11cf-88cb-001111000030} setupapi: \?\hid#vid_046d&pid_c52b&mi_01&col01#7&392e8c70&0&0000#{4d1e55b2-f16f-11cf-88cb-001111000030} instance_id: HID\VID_046D&PID_C52B&MI_01&COL01\7&392E8C70&0&0000
It's not even uppercasing up to second '#'. It looks like there's another source of path/instance_id that is case sensitive (Col01 vs COL01). So having a test that checks if everything is uppercase would fail on Windows.
I would treat the patch in current form more as "keeping the old behavior that we know some software depends on[0]" instead of "fixing this once and for all".
Alright, then maybe not checking the whole string but at least a test reflecting the SDL assumption, with a comment pointing to it?
On Wed, Feb 24, 2021 at 12:54:42PM +0100, Rémi Bernon wrote:
On 2/24/21 9:50 AM, Arkadiusz Hiler wrote:
On Wed, Feb 24, 2021 at 08:55:07AM +0100, Rémi Bernon wrote:
On 2/22/21 12:50 PM, Arkadiusz Hiler wrote:
This is a preparation for a patch that changes setupapi to return lowercased device paths, which is the source of RawInput device names. On Windows RawInput returns mostly uppercase names (except the GUID part) and we want to keep Wine's old behavior in this regards as SDL 2.0.14+ looks for uppercase IG_ to match xinput devices to their RawInput counterparts.
Signed-off-by: Arkadiusz Hiler ahiler@codeweavers.com
dlls/user32/rawinput.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/dlls/user32/rawinput.c b/dlls/user32/rawinput.c index ba11a121bc5..13aac0278d5 100644 --- a/dlls/user32/rawinput.c +++ b/dlls/user32/rawinput.c @@ -94,7 +94,7 @@ static struct device *add_device(HDEVINFO set, SP_DEVICE_INTERFACE_DATA *iface) SP_DEVICE_INTERFACE_DETAIL_DATA_W *detail; struct device *device; HANDLE file;
- WCHAR *path;
- WCHAR *path, *pos; DWORD size; SetupDiGetDeviceInterfaceDetailW(set, iface, NULL, 0, &size, NULL);
@@ -121,6 +121,9 @@ static struct device *add_device(HDEVINFO set, SP_DEVICE_INTERFACE_DATA *iface) } heap_free(detail);
- /* upper case everything but the GUID */
- for (pos = path; *pos && *pos != '{'; pos++) *pos = towupper(*pos);
file = CreateFileW(path, GENERIC_READ | GENERIC_WRITE, FILE_SHARE_READ | FILE_SHARE_WRITE, NULL, OPEN_EXISTING, 0, 0); if (file == INVALID_HANDLE_VALUE)Hi Arek!
I'd think a test to validate this could be nice. I know we don't have any gamepad device available on the test bot, but maybe it's still possible to validate at least with with mouse / keyboard devices? It would also let us check the test manually on a real Windows with a gamepad plugged in.
Hey,
Thanks for the feedback.
Looking at this a bit closer there seems to be more going on with the paths on Windows:
rawinput: \?\HID#VID_054C&PID_0CE6&MI_03#7&207751a1&0&0000#{4d1e55b2-f16f-11cf-88cb-001111000030} setupapi: \?\hid#vid_054c&pid_0ce6&mi_03#7&207751a1&0&0000#{4d1e55b2-f16f-11cf-88cb-001111000030} instance_id: HID\VID_054C&PID_0CE6&MI_03\7&207751A1&0&0000
or
rawinput: \?\HID#VID_046D&PID_C52B&MI_02&Col01#7&25fe7032&0&0000#{4d1e55b2-f16f-11cf-88cb-001111000030} setupapi: \?\hid#vid_046d&pid_c52b&mi_01&col01#7&392e8c70&0&0000#{4d1e55b2-f16f-11cf-88cb-001111000030} instance_id: HID\VID_046D&PID_C52B&MI_01&COL01\7&392E8C70&0&0000
It's not even uppercasing up to second '#'. It looks like there's another source of path/instance_id that is case sensitive (Col01 vs COL01). So having a test that checks if everything is uppercase would fail on Windows.
I would treat the patch in current form more as "keeping the old behavior that we know some software depends on[0]" instead of "fixing this once and for all".
Alright, then maybe not checking the whole string but at least a test reflecting the SDL assumption, with a comment pointing to it?
CCed zf as she may have something to say.
I may take a stab at a proper fix first though.
1. Both CM_Get_DeviceID() and SetupDiGetDeviceInstanceId() return strupr() version. Looks like we handle this correctly.
2. RawInput seems to be using the values verbatim from the registry as they are in HLM/System/CurrentControlSet/Enum and HLM/System/CurrentControlSet/Control/DeviceClasses
Looks like we need to implement opening relevant keys and using pathing from there in rawinput.c (NtQueryKey()?)
We also need to add tests to SetupAPI to make sure that the casing is preserved in the registy entries.
3. Our casing in the registry is wrong depending on the source of HID devices, winebus.sys requires some fixes too.
On 2/24/21 7:13 AM, Arkadiusz Hiler wrote:
On Wed, Feb 24, 2021 at 12:54:42PM +0100, Rémi Bernon wrote:
On 2/24/21 9:50 AM, Arkadiusz Hiler wrote:
On Wed, Feb 24, 2021 at 08:55:07AM +0100, Rémi Bernon wrote:
On 2/22/21 12:50 PM, Arkadiusz Hiler wrote:
This is a preparation for a patch that changes setupapi to return lowercased device paths, which is the source of RawInput device names. On Windows RawInput returns mostly uppercase names (except the GUID part) and we want to keep Wine's old behavior in this regards as SDL 2.0.14+ looks for uppercase IG_ to match xinput devices to their RawInput counterparts.
Signed-off-by: Arkadiusz Hiler ahiler@codeweavers.com
dlls/user32/rawinput.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)diff --git a/dlls/user32/rawinput.c b/dlls/user32/rawinput.c index ba11a121bc5..13aac0278d5 100644 --- a/dlls/user32/rawinput.c +++ b/dlls/user32/rawinput.c @@ -94,7 +94,7 @@ static struct device *add_device(HDEVINFO set, SP_DEVICE_INTERFACE_DATA *iface) SP_DEVICE_INTERFACE_DETAIL_DATA_W *detail; struct device *device; HANDLE file;
- WCHAR *path;
- WCHAR *path, *pos; DWORD size; SetupDiGetDeviceInterfaceDetailW(set, iface, NULL, 0, &size, NULL);
@@ -121,6 +121,9 @@ static struct device *add_device(HDEVINFO set, SP_DEVICE_INTERFACE_DATA *iface) } heap_free(detail);
- /* upper case everything but the GUID */
- for (pos = path; *pos && *pos != '{'; pos++) *pos = towupper(*pos);
file = CreateFileW(path, GENERIC_READ | GENERIC_WRITE, FILE_SHARE_READ | FILE_SHARE_WRITE, NULL, OPEN_EXISTING, 0, 0); if (file == INVALID_HANDLE_VALUE)Hi Arek!
I'd think a test to validate this could be nice. I know we don't have any gamepad device available on the test bot, but maybe it's still possible to validate at least with with mouse / keyboard devices? It would also let us check the test manually on a real Windows with a gamepad plugged in.
Hey,
Thanks for the feedback.
Looking at this a bit closer there seems to be more going on with the paths on Windows:
rawinput: \?\HID#VID_054C&PID_0CE6&MI_03#7&207751a1&0&0000#{4d1e55b2-f16f-11cf-88cb-001111000030} setupapi: \?\hid#vid_054c&pid_0ce6&mi_03#7&207751a1&0&0000#{4d1e55b2-f16f-11cf-88cb-001111000030} instance_id: HID\VID_054C&PID_0CE6&MI_03\7&207751A1&0&0000
or
rawinput: \?\HID#VID_046D&PID_C52B&MI_02&Col01#7&25fe7032&0&0000#{4d1e55b2-f16f-11cf-88cb-001111000030} setupapi: \?\hid#vid_046d&pid_c52b&mi_01&col01#7&392e8c70&0&0000#{4d1e55b2-f16f-11cf-88cb-001111000030} instance_id: HID\VID_046D&PID_C52B&MI_01&COL01\7&392E8C70&0&0000
It's not even uppercasing up to second '#'. It looks like there's another source of path/instance_id that is case sensitive (Col01 vs COL01). So having a test that checks if everything is uppercase would fail on Windows.
I would treat the patch in current form more as "keeping the old behavior that we know some software depends on[0]" instead of "fixing this once and for all".
Alright, then maybe not checking the whole string but at least a test reflecting the SDL assumption, with a comment pointing to it?
CCed zf as she may have something to say.
I may take a stab at a proper fix first though.
- Both CM_Get_DeviceID() and SetupDiGetDeviceInstanceId() return strupr() version. Looks like we handle this correctly.
I assume you mean strlwr()?
RawInput seems to be using the values verbatim from the registry as they are in HLM/System/CurrentControlSet/Enum and HLM/System/CurrentControlSet/Control/DeviceClasses
Looks like we need to implement opening relevant keys and using pathing from there in rawinput.c (NtQueryKey()?)
We also need to add tests to SetupAPI to make sure that the casing is preserved in the registy entries.
Yeah, this is what I noticed when I tested. If you manually change the casing with regedit (and log out and back in), user32 will even reflect that.
That said, my take is it's probably not worth reimplementing (admittedly not that much of) setupapi just for this, at least, not if we can just uppercase the whole string and have done.
- Our casing in the registry is wrong depending on the source of HID devices, winebus.sys requires some fixes too.
On Wed, Feb 24, 2021 at 02:09:41PM -0600, Zebediah Figura (she/her) wrote:
On 2/24/21 7:13 AM, Arkadiusz Hiler wrote:
On Wed, Feb 24, 2021 at 12:54:42PM +0100, Rémi Bernon wrote:
On 2/24/21 9:50 AM, Arkadiusz Hiler wrote:
On Wed, Feb 24, 2021 at 08:55:07AM +0100, Rémi Bernon wrote:
On 2/22/21 12:50 PM, Arkadiusz Hiler wrote:
This is a preparation for a patch that changes setupapi to return lowercased device paths, which is the source of RawInput device names. On Windows RawInput returns mostly uppercase names (except the GUID part) and we want to keep Wine's old behavior in this regards as SDL 2.0.14+ looks for uppercase IG_ to match xinput devices to their RawInput counterparts.
Signed-off-by: Arkadiusz Hiler ahiler@codeweavers.com
dlls/user32/rawinput.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)diff --git a/dlls/user32/rawinput.c b/dlls/user32/rawinput.c index ba11a121bc5..13aac0278d5 100644 --- a/dlls/user32/rawinput.c +++ b/dlls/user32/rawinput.c @@ -94,7 +94,7 @@ static struct device *add_device(HDEVINFO set, SP_DEVICE_INTERFACE_DATA *iface) SP_DEVICE_INTERFACE_DETAIL_DATA_W *detail; struct device *device; HANDLE file;
- WCHAR *path;
- WCHAR *path, *pos; DWORD size; SetupDiGetDeviceInterfaceDetailW(set, iface, NULL, 0, &size, NULL);
@@ -121,6 +121,9 @@ static struct device *add_device(HDEVINFO set, SP_DEVICE_INTERFACE_DATA *iface) } heap_free(detail);
- /* upper case everything but the GUID */
- for (pos = path; *pos && *pos != '{'; pos++) *pos = towupper(*pos);
file = CreateFileW(path, GENERIC_READ | GENERIC_WRITE, FILE_SHARE_READ | FILE_SHARE_WRITE, NULL, OPEN_EXISTING, 0, 0); if (file == INVALID_HANDLE_VALUE)Hi Arek!
I'd think a test to validate this could be nice. I know we don't have any gamepad device available on the test bot, but maybe it's still possible to validate at least with with mouse / keyboard devices? It would also let us check the test manually on a real Windows with a gamepad plugged in.
Hey,
Thanks for the feedback.
Looking at this a bit closer there seems to be more going on with the paths on Windows:
rawinput: \?\HID#VID_054C&PID_0CE6&MI_03#7&207751a1&0&0000#{4d1e55b2-f16f-11cf-88cb-001111000030} setupapi: \?\hid#vid_054c&pid_0ce6&mi_03#7&207751a1&0&0000#{4d1e55b2-f16f-11cf-88cb-001111000030} instance_id: HID\VID_054C&PID_0CE6&MI_03\7&207751A1&0&0000
or
rawinput: \?\HID#VID_046D&PID_C52B&MI_02&Col01#7&25fe7032&0&0000#{4d1e55b2-f16f-11cf-88cb-001111000030} setupapi: \?\hid#vid_046d&pid_c52b&mi_01&col01#7&392e8c70&0&0000#{4d1e55b2-f16f-11cf-88cb-001111000030} instance_id: HID\VID_046D&PID_C52B&MI_01&COL01\7&392E8C70&0&0000
It's not even uppercasing up to second '#'. It looks like there's another source of path/instance_id that is case sensitive (Col01 vs COL01). So having a test that checks if everything is uppercase would fail on Windows.
I would treat the patch in current form more as "keeping the old behavior that we know some software depends on[0]" instead of "fixing this once and for all".
Alright, then maybe not checking the whole string but at least a test reflecting the SDL assumption, with a comment pointing to it?
CCed zf as she may have something to say.
I may take a stab at a proper fix first though.
- Both CM_Get_DeviceID() and SetupDiGetDeviceInstanceId() return strupr() version. Looks like we handle this correctly.
I assume you mean strlwr()?
SetupAPI's InstanceId is strupr() and DevicePath is strlwr().
RawInput seems to be using the values verbatim from the registry as they are in HLM/System/CurrentControlSet/Enum and HLM/System/CurrentControlSet/Control/DeviceClasses
Looks like we need to implement opening relevant keys and using pathing from there in rawinput.c (NtQueryKey()?)
We also need to add tests to SetupAPI to make sure that the casing is preserved in the registy entries.
Yeah, this is what I noticed when I tested. If you manually change the casing with regedit (and log out and back in), user32 will even reflect that.
That said, my take is it's probably not worth reimplementing (admittedly not that much of) setupapi just for this, at least, not if we can just uppercase the whole string and have done.
I was thinking about doing something along the lines of:
SetupDiGetDeviceInstanceId(instance_id); instance_key = RegOpenKey(enum_key, instance_id); NtQueryKey(instance_key, KeyNameInformation, cased_instance_id);
/* \Registry\Machine\System\CurrentControlSet\Enum\HID\VID_0000&PID_0000\0&WINEMOUSE&0&0 */
Then we can take everything starting with HID and overwrite a part of the device path, replacing '' with '#'.
If you think that uppercasing (either part of or the whole) device path in user32/rawinput is good enough for now I am more than fine with that.
On 2/25/21 3:46 AM, Arkadiusz Hiler wrote:
On Wed, Feb 24, 2021 at 02:09:41PM -0600, Zebediah Figura (she/her) wrote:
On 2/24/21 7:13 AM, Arkadiusz Hiler wrote:
On Wed, Feb 24, 2021 at 12:54:42PM +0100, Rémi Bernon wrote:
On 2/24/21 9:50 AM, Arkadiusz Hiler wrote:
On Wed, Feb 24, 2021 at 08:55:07AM +0100, Rémi Bernon wrote:
On 2/22/21 12:50 PM, Arkadiusz Hiler wrote: > This is a preparation for a patch that changes setupapi to return lowercased > device paths, which is the source of RawInput device names. On Windows > RawInput returns mostly uppercase names (except the GUID part) and we want to > keep Wine's old behavior in this regards as SDL 2.0.14+ looks for uppercase IG_ > to match xinput devices to their RawInput counterparts. > > Signed-off-by: Arkadiusz Hiler ahiler@codeweavers.com > --- > dlls/user32/rawinput.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/dlls/user32/rawinput.c b/dlls/user32/rawinput.c > index ba11a121bc5..13aac0278d5 100644 > --- a/dlls/user32/rawinput.c > +++ b/dlls/user32/rawinput.c > @@ -94,7 +94,7 @@ static struct device *add_device(HDEVINFO set, SP_DEVICE_INTERFACE_DATA *iface) > SP_DEVICE_INTERFACE_DETAIL_DATA_W *detail; > struct device *device; > HANDLE file; > - WCHAR *path; > + WCHAR *path, *pos; > DWORD size; > SetupDiGetDeviceInterfaceDetailW(set, iface, NULL, 0, &size, NULL); > @@ -121,6 +121,9 @@ static struct device *add_device(HDEVINFO set, SP_DEVICE_INTERFACE_DATA *iface) > } > heap_free(detail); > + /* upper case everything but the GUID */ > + for (pos = path; *pos && *pos != '{'; pos++) *pos = towupper(*pos); > + > file = CreateFileW(path, GENERIC_READ | GENERIC_WRITE, > FILE_SHARE_READ | FILE_SHARE_WRITE, NULL, OPEN_EXISTING, 0, 0); > if (file == INVALID_HANDLE_VALUE) >
Hi Arek!
I'd think a test to validate this could be nice. I know we don't have any gamepad device available on the test bot, but maybe it's still possible to validate at least with with mouse / keyboard devices? It would also let us check the test manually on a real Windows with a gamepad plugged in.
Hey,
Thanks for the feedback.
Looking at this a bit closer there seems to be more going on with the paths on Windows:
rawinput: \?\HID#VID_054C&PID_0CE6&MI_03#7&207751a1&0&0000#{4d1e55b2-f16f-11cf-88cb-001111000030} setupapi: \?\hid#vid_054c&pid_0ce6&mi_03#7&207751a1&0&0000#{4d1e55b2-f16f-11cf-88cb-001111000030} instance_id: HID\VID_054C&PID_0CE6&MI_03\7&207751A1&0&0000
or
rawinput: \?\HID#VID_046D&PID_C52B&MI_02&Col01#7&25fe7032&0&0000#{4d1e55b2-f16f-11cf-88cb-001111000030} setupapi: \?\hid#vid_046d&pid_c52b&mi_01&col01#7&392e8c70&0&0000#{4d1e55b2-f16f-11cf-88cb-001111000030} instance_id: HID\VID_046D&PID_C52B&MI_01&COL01\7&392E8C70&0&0000
It's not even uppercasing up to second '#'. It looks like there's another source of path/instance_id that is case sensitive (Col01 vs COL01). So having a test that checks if everything is uppercase would fail on Windows.
I would treat the patch in current form more as "keeping the old behavior that we know some software depends on[0]" instead of "fixing this once and for all".
Alright, then maybe not checking the whole string but at least a test reflecting the SDL assumption, with a comment pointing to it?
CCed zf as she may have something to say.
I may take a stab at a proper fix first though.
- Both CM_Get_DeviceID() and SetupDiGetDeviceInstanceId() return strupr() version. Looks like we handle this correctly.
I assume you mean strlwr()?
SetupAPI's InstanceId is strupr() and DevicePath is strlwr().
Eh, I wasn't paying attention to which function you were talking about.
RawInput seems to be using the values verbatim from the registry as they are in HLM/System/CurrentControlSet/Enum and HLM/System/CurrentControlSet/Control/DeviceClasses
Looks like we need to implement opening relevant keys and using pathing from there in rawinput.c (NtQueryKey()?)
We also need to add tests to SetupAPI to make sure that the casing is preserved in the registy entries.
Yeah, this is what I noticed when I tested. If you manually change the casing with regedit (and log out and back in), user32 will even reflect that.
That said, my take is it's probably not worth reimplementing (admittedly not that much of) setupapi just for this, at least, not if we can just uppercase the whole string and have done.
I was thinking about doing something along the lines of:
SetupDiGetDeviceInstanceId(instance_id); instance_key = RegOpenKey(enum_key, instance_id); NtQueryKey(instance_key, KeyNameInformation, cased_instance_id);
/* \Registry\Machine\System\CurrentControlSet\Enum\HID\VID_0000&PID_0000\0&WINEMOUSE&0&0 */
Then we can take everything starting with HID and overwrite a part of the device path, replacing '' with '#'.
If you think that uppercasing (either part of or the whole) device path in user32/rawinput is good enough for now I am more than fine with that.
I think it'd be fine to uppercase the whole path in user32, personally.
On Mon, Feb 22, 2021 at 01:50:36PM +0200, Arkadiusz Hiler wrote:
This is a preparation for a patch that changes setupapi to return lowercased device paths, which is the source of RawInput device names. On Windows RawInput returns mostly uppercase names (except the GUID part) and we want to keep Wine's old behavior in this regards as SDL 2.0.14+ looks for uppercase IG_ to match xinput devices to their RawInput counterparts.
Signed-off-by: Arkadiusz Hiler ahiler@codeweavers.com
dlls/user32/rawinput.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/dlls/user32/rawinput.c b/dlls/user32/rawinput.c index ba11a121bc5..13aac0278d5 100644 --- a/dlls/user32/rawinput.c +++ b/dlls/user32/rawinput.c @@ -94,7 +94,7 @@ static struct device *add_device(HDEVINFO set, SP_DEVICE_INTERFACE_DATA *iface) SP_DEVICE_INTERFACE_DETAIL_DATA_W *detail; struct device *device; HANDLE file;
- WCHAR *path;
WCHAR *path, *pos; DWORD size;
SetupDiGetDeviceInterfaceDetailW(set, iface, NULL, 0, &size, NULL);
@@ -121,6 +121,9 @@ static struct device *add_device(HDEVINFO set, SP_DEVICE_INTERFACE_DATA *iface) } heap_free(detail);
- /* upper case everything but the GUID */
- for (pos = path; *pos && *pos != '{'; pos++) *pos = towupper(*pos);
I assume that this patch is the reason why this series is still pending. I wonder if using instance id to overwrite part of the path would make the change more acceptable.
Oh, and all the failures look like false positives. I haven't seen any difference in the results when running the tests locally.
On 2/24/21 9:25 AM, Arkadiusz Hiler wrote:
On Mon, Feb 22, 2021 at 01:50:36PM +0200, Arkadiusz Hiler wrote:
This is a preparation for a patch that changes setupapi to return lowercased device paths, which is the source of RawInput device names. On Windows RawInput returns mostly uppercase names (except the GUID part) and we want to keep Wine's old behavior in this regards as SDL 2.0.14+ looks for uppercase IG_ to match xinput devices to their RawInput counterparts.
Signed-off-by: Arkadiusz Hiler ahiler@codeweavers.com
dlls/user32/rawinput.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/dlls/user32/rawinput.c b/dlls/user32/rawinput.c index ba11a121bc5..13aac0278d5 100644 --- a/dlls/user32/rawinput.c +++ b/dlls/user32/rawinput.c @@ -94,7 +94,7 @@ static struct device *add_device(HDEVINFO set, SP_DEVICE_INTERFACE_DATA *iface) SP_DEVICE_INTERFACE_DETAIL_DATA_W *detail; struct device *device; HANDLE file;
- WCHAR *path;
WCHAR *path, *pos; DWORD size;
SetupDiGetDeviceInterfaceDetailW(set, iface, NULL, 0, &size, NULL);
@@ -121,6 +121,9 @@ static struct device *add_device(HDEVINFO set, SP_DEVICE_INTERFACE_DATA *iface) } heap_free(detail);
- /* upper case everything but the GUID */
- for (pos = path; *pos && *pos != '{'; pos++) *pos = towupper(*pos);
I assume that this patch is the reason why this series is still pending. I wonder if using instance id to overwrite part of the path would make the change more acceptable.
Oh, and all the failures look like false positives. I haven't seen any difference in the results when running the tests locally.
Yes, user32 tests results are not very stable.
-
Arkadiusz Hiler -
Marvin -
Rémi Bernon -
Zebediah Figura (she/her)