[PATCH v3 0/3] MR1448: hid: parser and HidP_SetUsageValue fix
This fixes two things: * HID report descriptor parser enum for local items. Close inspection of page 40 of [Device Class Definition for HID 1.11](https://www.usb.org/document-library/device-class-definition-hid-111) reveals a jump between Designator Maximum (0101) and String Index (0111). This caused a controller of mine to not get recognized. * HidP_SetUsageValue for items that define multiple controls (with Report Count > 1). Attempting to set one value would result in HIDP_STATUS_BUFFER_TOO_SMALL. -- v3: hid: Add tests for fixed HidP_SetUsageValue hid: Fix HidP_SetUsageValue for usage ranges hidparse.sys: Fix incorrect enum https://gitlab.winehq.org/wine/wine/-/merge_requests/1448
From: Matthew Tran <0e4ef622(a)gmail.com> --- dlls/dinput/tests/psh_hid_macros.h | 8 ++++---- dlls/hidparse.sys/main.c | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/dlls/dinput/tests/psh_hid_macros.h b/dlls/dinput/tests/psh_hid_macros.h index 4623af20598..4b93ee09e18 100644 --- a/dlls/dinput/tests/psh_hid_macros.h +++ b/dlls/dinput/tests/psh_hid_macros.h @@ -79,7 +79,7 @@ #define DESIGNATOR_INDEX(n,data) SHORT_ITEM_##n(0x3,2,data) #define DESIGNATOR_MINIMUM(n,data) SHORT_ITEM_##n(0x4,2,data) #define DESIGNATOR_MAXIMUM(n,data) SHORT_ITEM_##n(0x5,2,data) -#define STRING_INDEX(n,data) SHORT_ITEM_##n(0x6,2,data) -#define STRING_MINIMUM(n,data) SHORT_ITEM_##n(0x7,2,data) -#define STRING_MAXIMUM(n,data) SHORT_ITEM_##n(0x8,2,data) -#define DELIMITER(n,data) SHORT_ITEM_##n(0x9,2,data) +#define STRING_INDEX(n,data) SHORT_ITEM_##n(0x7,2,data) +#define STRING_MINIMUM(n,data) SHORT_ITEM_##n(0x8,2,data) +#define STRING_MAXIMUM(n,data) SHORT_ITEM_##n(0x9,2,data) +#define DELIMITER(n,data) SHORT_ITEM_##n(0xA,2,data) diff --git a/dlls/hidparse.sys/main.c b/dlls/hidparse.sys/main.c index 9c753c0f365..7058093e03c 100644 --- a/dlls/hidparse.sys/main.c +++ b/dlls/hidparse.sys/main.c @@ -93,7 +93,7 @@ enum TAG_LOCAL_DESIGNATOR_INDEX, TAG_LOCAL_DESIGNATOR_MINIMUM, TAG_LOCAL_DESIGNATOR_MAXIMUM, - TAG_LOCAL_STRING_INDEX, + TAG_LOCAL_STRING_INDEX = 0x7, TAG_LOCAL_STRING_MINIMUM, TAG_LOCAL_STRING_MAXIMUM, TAG_LOCAL_DELIMITER -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1448
From: Matthew Tran <0e4ef622(a)gmail.com> The original set_usage_value was renamed to set_usage_value_array. --- dlls/hid/hidp.c | 46 ++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 38 insertions(+), 8 deletions(-) diff --git a/dlls/hid/hidp.c b/dlls/hid/hidp.c index a1c5805c19f..2fedb3f9586 100644 --- a/dlls/hid/hidp.c +++ b/dlls/hid/hidp.c @@ -495,16 +495,31 @@ NTSTATUS WINAPI HidP_SetScaledUsageValue( HIDP_REPORT_TYPE report_type, USAGE us return enum_value_caps( preparsed, report_type, report_len, &filter, set_scaled_usage_value, ¶ms, &count ); } +struct set_usage_value_params +{ + ULONG value; + void *report_buf; + USAGE usage; +}; + static NTSTATUS set_usage_value( const struct hid_value_caps *caps, void *user ) { - struct usage_value_params *params = user; - ULONG bit_count = caps->bit_size * caps->report_count; - unsigned char *report_buf; + struct set_usage_value_params *params = user; + unsigned char *report_buf, start_bit, usage_index; - if ((bit_count + 7) / 8 > params->value_len) return HIDP_STATUS_BUFFER_TOO_SMALL; + if (caps->flags & HID_VALUE_CAPS_IS_RANGE) + { + usage_index = params->usage - caps->usage_min; + report_buf = (unsigned char *)params->report_buf + caps->start_byte + usage_index * caps->bit_size / 8; + start_bit = caps->start_bit + (usage_index * caps->bit_size) % 8; + } + else + { + report_buf = (unsigned char *)params->report_buf + caps->start_byte; + start_bit = caps->start_bit; + } - report_buf = (unsigned char *)params->report_buf + caps->start_byte; - copy_bits( report_buf, params->value_buf, bit_count, caps->start_bit ); + copy_bits( report_buf, (unsigned char*) ¶ms->value, caps->bit_size, start_bit ); return HIDP_STATUS_NULL; } @@ -512,7 +527,7 @@ static NTSTATUS set_usage_value( const struct hid_value_caps *caps, void *user ) NTSTATUS WINAPI HidP_SetUsageValue( HIDP_REPORT_TYPE report_type, USAGE usage_page, USHORT collection, USAGE usage, ULONG value, PHIDP_PREPARSED_DATA preparsed_data, char *report_buf, ULONG report_len ) { - struct usage_value_params params = {.value_buf = &value, .value_len = sizeof(value), .report_buf = report_buf}; + struct set_usage_value_params params = {.value = value, .report_buf = report_buf, .usage = usage}; struct hid_preparsed_data *preparsed = (struct hid_preparsed_data *)preparsed_data; struct caps_filter filter = {.values = TRUE, .usage_page = usage_page, .collection = collection, .usage = usage}; USHORT count = 1; @@ -526,6 +541,21 @@ NTSTATUS WINAPI HidP_SetUsageValue( HIDP_REPORT_TYPE report_type, USAGE usage_pa return enum_value_caps( preparsed, report_type, report_len, &filter, set_usage_value, ¶ms, &count ); } +static NTSTATUS set_usage_value_array( const struct hid_value_caps *caps, void *user ) +{ + struct usage_value_params *params = user; + ULONG bit_count = caps->bit_size * caps->report_count; + unsigned char *report_buf; + + if (caps->flags & HID_VALUE_CAPS_IS_RANGE) return HIDP_STATUS_NOT_VALUE_ARRAY; + if ((bit_count + 7) / 8 > params->value_len) return HIDP_STATUS_BUFFER_TOO_SMALL; + + report_buf = (unsigned char *)params->report_buf + caps->start_byte; + copy_bits( report_buf, params->value_buf, bit_count, caps->start_bit ); + + return HIDP_STATUS_NULL; +} + NTSTATUS WINAPI HidP_SetUsageValueArray( HIDP_REPORT_TYPE report_type, USAGE usage_page, USHORT collection, USAGE usage, char *value_buf, USHORT value_len, PHIDP_PREPARSED_DATA preparsed_data, char *report_buf, ULONG report_len ) @@ -542,7 +572,7 @@ NTSTATUS WINAPI HidP_SetUsageValueArray( HIDP_REPORT_TYPE report_type, USAGE usa if (!report_len) return HIDP_STATUS_INVALID_REPORT_LENGTH; filter.report_id = report_buf[0]; - return enum_value_caps( preparsed, report_type, report_len, &filter, set_usage_value, ¶ms, &count ); + return enum_value_caps( preparsed, report_type, report_len, &filter, set_usage_value_array, ¶ms, &count ); } struct set_usage_params -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1448
From: Matthew Tran <0e4ef622(a)gmail.com> --- dlls/dinput/tests/hid.c | 84 ++++++++++++++++++++++++++++++++--------- include/hidusage.h | 1 + 2 files changed, 68 insertions(+), 17 deletions(-) diff --git a/dlls/dinput/tests/hid.c b/dlls/dinput/tests/hid.c index 88565283735..345aede2c73 100644 --- a/dlls/dinput/tests/hid.c +++ b/dlls/dinput/tests/hid.c @@ -1286,7 +1286,10 @@ static void test_hidp_set_output( HANDLE file, int report_id, ULONG report_len, .code = IOCTL_HID_SET_OUTPUT_REPORT, .report_id = report_id, .report_len = report_len - (report_id ? 0 : 1), - .report_buf = {report_id,0,0xcd,0xcd,0xcd}, + .report_buf = { + report_id,0,0,0,0,0,0,0,0,0,0,0,0xcd,0xcd,0xcd,0xcd, + 0xcd,0xcd,0xcd,0xcd,0xcd,0xcd,0xcd,0xcd, + }, .ret_length = 3, .ret_status = STATUS_SUCCESS, }, @@ -1299,9 +1302,7 @@ static void test_hidp_set_output( HANDLE file, int report_id, ULONG report_len, memset( report, 0xcd, sizeof(report) ); status = HidP_InitializeReportForID( HidP_Output, report_id, preparsed, report, report_len ); - ok( status == HIDP_STATUS_REPORT_DOES_NOT_EXIST, "HidP_InitializeReportForID returned %#lx\n", status ); - memset( report, 0, report_len ); - report[0] = report_id; + ok( status == HIDP_STATUS_SUCCESS, "HidP_InitializeReportForID returned %#lx\n", status ); SetLastError( 0xdeadbeef ); ret = HidD_SetOutputReport( file, report, 0 ); @@ -1322,7 +1323,7 @@ static void test_hidp_set_output( HANDLE file, int report_id, ULONG report_len, .code = IOCTL_HID_SET_OUTPUT_REPORT, .broken = TRUE, .report_len = report_len - 1, - .report_buf = {0x5a,0x5a}, + .report_buf = {0x5a,0x5a,0x5a,0x5a,0x5a,0x5a,0x5a,0x5a,0x5a,0x5a,0x5a,0x5a}, .ret_length = 3, .ret_status = STATUS_SUCCESS, }; @@ -1372,7 +1373,9 @@ static void test_write_file( HANDLE file, int report_id, ULONG report_len ) .code = IOCTL_HID_WRITE_REPORT, .report_id = report_id, .report_len = report_len - (report_id ? 0 : 1), - .report_buf = {report_id ? report_id : 0xcd,0xcd,0xcd,0xcd,0xcd}, + .report_buf = { + report_id ? report_id : 0xcd,0xcd,0xcd,0xcd,0xcd,0xcd,0xcd,0xcd,0xcd,0xcd,0xcd,0xcd,0xcd,0xcd,0xcd,0xcd, + 0xcd,0xcd,0xcd,0xcd,0xcd,0xcd,0xcd,0xcd,0xcd}, .ret_length = 3, .ret_status = STATUS_SUCCESS, }; @@ -1558,8 +1561,8 @@ static void test_hidp( HANDLE file, HANDLE async_file, int report_id, BOOL polle .LinkUsage = HID_USAGE_GENERIC_JOYSTICK, .LinkUsagePage = HID_USAGE_PAGE_GENERIC, .CollectionType = 1, - .NumberOfChildren = 7, - .FirstChild = 9, + .NumberOfChildren = 8, + .FirstChild = 10, }, { .LinkUsage = HID_USAGE_GENERIC_JOYSTICK, @@ -1591,6 +1594,7 @@ static void test_hidp( HANDLE file, HANDLE async_file, int report_id, BOOL polle char buffer[200], report[200]; DWORD collection_count; DWORD waveform_list; + DWORD generic_output_list; HIDP_DATA data[64]; USAGE usages[16]; ULONG off, value; @@ -1701,8 +1705,6 @@ static void test_hidp( HANDLE file, HANDLE async_file, int report_id, BOOL polle ok( count == 0, "HidP_GetSpecificButtonCaps returned count %d, expected %d\n", count, 0 ); count = ARRAY_SIZE(value_caps); - status = HidP_GetValueCaps( HidP_Output, value_caps, &count, preparsed_data ); - ok( status == HIDP_STATUS_USAGE_NOT_FOUND, "HidP_GetValueCaps returned %#lx\n", status ); status = HidP_GetValueCaps( HidP_Feature + 1, value_caps, &count, preparsed_data ); ok( status == HIDP_STATUS_INVALID_REPORT_TYPE, "HidP_GetValueCaps returned %#lx\n", status ); count = 0; @@ -1726,8 +1728,6 @@ static void test_hidp( HANDLE file, HANDLE async_file, int report_id, BOOL polle } count = ARRAY_SIZE(value_caps) - 4; - status = HidP_GetSpecificValueCaps( HidP_Output, 0, 0, 0, value_caps, &count, preparsed_data ); - ok( status == HIDP_STATUS_USAGE_NOT_FOUND, "HidP_GetSpecificValueCaps returned %#lx\n", status ); status = HidP_GetSpecificValueCaps( HidP_Feature + 1, 0, 0, 0, value_caps, &count, preparsed_data ); ok( status == HIDP_STATUS_INVALID_REPORT_TYPE, "HidP_GetSpecificValueCaps returned %#lx\n", status ); count = 0; @@ -2036,7 +2036,7 @@ static void test_hidp( HANDLE file, HANDLE async_file, int report_id, BOOL polle value = HidP_MaxDataListLength( HidP_Input, preparsed_data ); ok( value == 58, "HidP_MaxDataListLength(HidP_Input) returned %ld, expected %d\n", value, 58 ); value = HidP_MaxDataListLength( HidP_Output, preparsed_data ); - ok( value == 0, "HidP_MaxDataListLength(HidP_Output) returned %ld, expected %d\n", value, 0 ); + ok( value == 10, "HidP_MaxDataListLength(HidP_Output) returned %ld, expected %d\n", value, 10 ); value = HidP_MaxDataListLength( HidP_Feature, preparsed_data ); ok( value == 14, "HidP_MaxDataListLength(HidP_Feature) returned %ld, expected %d\n", value, 14 ); @@ -2271,6 +2271,38 @@ static void test_hidp( HANDLE file, HANDLE async_file, int report_id, BOOL polle ok( status == HIDP_STATUS_SUCCESS, "HidP_GetUsageValue returned %#lx\n", status ); ok( value == 0xfffffd0b, "got value %#lx, expected %#x\n", value, 0xfffffd0b ); + memset( report, 0xcd, sizeof(report) ); + status = HidP_InitializeReportForID( HidP_Output, report_id, preparsed_data, report, + caps.OutputReportByteLength ); + ok( status == HIDP_STATUS_SUCCESS, "HidP_InitializeReportForID returned %#lx\n", status ); + + memset( buffer, 0xcd, sizeof(buffer) ); + memset( buffer, 0, caps.OutputReportByteLength ); + buffer[0] = report_id; + ok( !memcmp( buffer, report, sizeof(buffer) ), "unexpected report data\n" ); + + for (i = 0; i < caps.NumberLinkCollectionNodes; ++i) + { + if (collections[i].LinkUsagePage != HID_USAGE_PAGE_GENERIC) continue; + if (collections[i].LinkUsage == HID_USAGE_GENERIC_UNDEFINED) break; + } + ok( i < caps.NumberLinkCollectionNodes, + "HID_USAGE_GENERIC_UNDEFINED collection not found\n" ); + generic_output_list = i; + + for (i = 0; i < 10; ++i) + { + status = HidP_SetUsageValue( HidP_Output, HID_USAGE_PAGE_ORDINAL, generic_output_list, i + 1, + i, preparsed_data, report, caps.OutputReportByteLength ); + ok( status == HIDP_STATUS_SUCCESS, "HidP_SetUsageValue returned %#lx\n", status ); + + if (report_id) + buffer[i + 2] = i; + else + buffer[i + 3] = i; + } + ok( !memcmp( buffer, report, sizeof(buffer) ), "unexpected report data\n" ); + test_hidp_get_input( file, report_id, caps.InputReportByteLength, preparsed_data ); test_hidp_get_feature( file, report_id, caps.FeatureReportByteLength, preparsed_data ); test_hidp_set_feature( file, report_id, caps.FeatureReportByteLength, preparsed_data ); @@ -2883,7 +2915,7 @@ static void test_hid_driver( DWORD report_id, DWORD polled ) USAGE_PAGE(1, HID_USAGE_PAGE_LED), USAGE(1, HID_USAGE_LED_GREEN), COLLECTION(1, Report), - REPORT_ID_OR_USAGE_PAGE(1, report_id, 0), + REPORT_ID_OR_USAGE_PAGE(1, report_id, 1), USAGE_PAGE(1, HID_USAGE_PAGE_LED), REPORT_COUNT(1, 8), REPORT_SIZE(1, 1), @@ -2893,12 +2925,28 @@ static void test_hid_driver( DWORD report_id, DWORD polled ) USAGE_PAGE(1, HID_USAGE_PAGE_LED), USAGE(1, HID_USAGE_LED_RED), COLLECTION(1, Report), - REPORT_ID_OR_USAGE_PAGE(1, report_id, 1), + REPORT_ID_OR_USAGE_PAGE(1, report_id, 0), USAGE_PAGE(1, HID_USAGE_PAGE_LED), REPORT_COUNT(1, 8), REPORT_SIZE(1, 1), OUTPUT(1, Cnst|Var|Abs), END_COLLECTION, + + USAGE_PAGE(1, HID_USAGE_PAGE_GENERIC), + USAGE(1, HID_USAGE_GENERIC_UNDEFINED), + COLLECTION(1, Report), + REPORT_ID_OR_USAGE_PAGE(1, report_id, 0), + REPORT_COUNT(1, 10), + REPORT_SIZE(1, 8), + LOGICAL_MINIMUM(2, 0), + LOGICAL_MAXIMUM(2, 255), + USAGE_PAGE(1, HID_USAGE_PAGE_ORDINAL), + STRING_MINIMUM(1, 6), + STRING_MAXIMUM(1, 16), + USAGE_MINIMUM(1, 1), /* Instance 1 */ + USAGE_MAXIMUM(1, 10), /* Instance 31 */ + OUTPUT(1, Data|Var|Abs), + END_COLLECTION, END_COLLECTION, }; #undef REPORT_ID_OR_USAGE_PAGE @@ -2910,12 +2958,14 @@ static void test_hid_driver( DWORD report_id, DWORD polled ) .Usage = HID_USAGE_GENERIC_JOYSTICK, .UsagePage = HID_USAGE_PAGE_GENERIC, .InputReportByteLength = report_id ? 32 : 33, - .OutputReportByteLength = report_id ? 2 : 3, + .OutputReportByteLength = report_id ? 12 : 13, .FeatureReportByteLength = report_id ? 21 : 22, - .NumberLinkCollectionNodes = 10, + .NumberLinkCollectionNodes = 11, .NumberInputButtonCaps = 17, .NumberInputValueCaps = 7, .NumberInputDataIndices = 47, + .NumberOutputValueCaps = 1, + .NumberOutputDataIndices = 10, .NumberFeatureButtonCaps = 1, .NumberFeatureValueCaps = 6, .NumberFeatureDataIndices = 8, diff --git a/include/hidusage.h b/include/hidusage.h index 2ea3784d626..955ad348fb2 100644 --- a/include/hidusage.h +++ b/include/hidusage.h @@ -39,6 +39,7 @@ typedef USHORT USAGE, *PUSAGE; #define HID_USAGE_DIGITIZER_TIP_SWITCH ((USAGE) 0x42) #define HID_USAGE_DIGITIZER_BARREL_SWITCH ((USAGE) 0x44) +#define HID_USAGE_GENERIC_UNDEFINED ((USAGE) 0x00) #define HID_USAGE_GENERIC_POINTER ((USAGE) 0x01) #define HID_USAGE_GENERIC_MOUSE ((USAGE) 0x02) #define HID_USAGE_GENERIC_JOYSTICK ((USAGE) 0x04) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1448
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=126550 Your paranoid android. === w7u_2qxl (32 bit report) === dinput: hid.c:1423: Test failed: id 0: WriteFile wrote 13 hid.c:1418: Test failed: id 1: WriteFile wrote 12 hid.c:1423: Test failed: id 0 poll: WriteFile wrote 13 hid.c:1418: Test failed: id 1 poll: WriteFile wrote 12 === w7u_el (32 bit report) === dinput: hid.c:1423: Test failed: id 0: WriteFile wrote 13 hid.c:1418: Test failed: id 1: WriteFile wrote 12 hid.c:1423: Test failed: id 0 poll: WriteFile wrote 13 hid.c:1418: Test failed: id 1 poll: WriteFile wrote 12 === w8 (32 bit report) === dinput: hid.c:1423: Test failed: id 0: WriteFile wrote 13 hid.c:1418: Test failed: id 1: WriteFile wrote 12 hid.c:1423: Test failed: id 0 poll: WriteFile wrote 13 hid.c:1418: Test failed: id 1 poll: WriteFile wrote 12 === w1064_tsign (64 bit report) === dinput: hid.c:1423: Test failed: id 0: WriteFile wrote 13 hid.c:1418: Test failed: id 1: WriteFile wrote 12 hid.c:1423: Test failed: id 0 poll: WriteFile wrote 13 hid.c:1418: Test failed: id 1 poll: WriteFile wrote 12 === debian11 (32 bit report) === d3d8: stateblock: Timeout visual: Timeout d3d9: d3d9ex: Timeout device: Timeout stateblock: Timeout visual: Timeout d3dcompiler_43: asm: Timeout blob: Timeout hlsl_d3d11: Timeout hlsl_d3d9: Timeout reflection: Timeout d3dcompiler_46: asm: Timeout blob: Timeout hlsl_d3d11: Timeout hlsl_d3d9: Timeout reflection: Timeout d3dcompiler_47: asm: Timeout blob: Timeout hlsl_d3d11: Timeout hlsl_d3d9: Timeout reflection: Timeout d3drm: d3drm: Timeout vector: Timeout d3dx10_34: d3dx10: Timeout d3dx10_35: d3dx10: Timeout d3dx10_36: d3dx10: Timeout d3dx10_37: d3dx10: Timeout d3dx10_38: d3dx10: Timeout d3dx10_39: d3dx10: Timeout d3dx10_40: d3dx10: Timeout d3dx10_41: d3dx10: Timeout d3dx10_42: d3dx10: Timeout d3dx10_43: d3dx10: Timeout d3dx11_42: d3dx11: Timeout d3dx11_43: d3dx11: Timeout d3dx9_36: asm: Timeout core: Timeout effect: Timeout line: Timeout math: Timeout mesh: Timeout shader: Timeout surface: Timeout texture: Timeout volume: Timeout xfile: Timeout ddrawex: ddrawex: Timeout Report validation errors: surface: Timeout === debian11 (build log) === WineRunWineTest.pl:error: The task timed out
On Mon Nov 21 11:07:33 2022 +0000, Rémi Bernon wrote:
Thanks for spotting this! Do you think you could add a few tests to check the HidP_SetUsageValue change? Like in `test_hidp` (the descriptor being in `test_hid_driver`) in `dlls/dinput/tests/hid.c`? I've added some tests, although I've just realized `HidP_SetScaledUsageValue` and `HidP_Get[Scaled]UsageValue` likely have the same bug.
I also wanted to mention a [couple hacks](https://gitlab.winehq.org/0e4ef622/wine/-/commit/5c28d1f7f5436c5428b51962570...) I had to add to get my controller to fully work. 1. The USB HID interface providing controls defines 3 top-level collections: mouse, keyboard and joystick. Currently, the driver looks at the first one and calls the entire thing a mouse, and joystick inputs are not delivered. I think the proper solution would be to "generate a unique physical device object (PDO) for each Top Level Collection" ([ref](https://learn.microsoft.com/en-us/windows-hardware/drivers/hid/top-level-col...)). 2. The thread which receives HID input reports and makes `send_hardware_message` requests to wineserver does not have an associated desktop. This causes wineserver to return STATUS_INVALID_HANDLE and thus no rawinput messages get queued. How hard would it be to fix these in a more "proper" way? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1448#note_16656
Thanks for the tests! I actually made a few tweaks and took that opportunity to fix the Get and Scaled flavors too. Would you mind having a look at https://gitlab.winehq.org/rbernon/wine/-/commits/mr-1448-tweaks and updating this MR with it if that looks fine to you? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1448#note_16681
1. The USB HID interface providing controls defines 3 top-level collections: mouse, keyboard and joystick. Currently, the driver looks at the first one and calls the entire thing a mouse, and joystick inputs are not delivered. I think the proper solution would be to "generate a unique physical device object (PDO) for each Top Level Collection" ([ref](https://learn.microsoft.com/en-us/windows-hardware/drivers/hid/top-level-col...)).
Is this exposed as a multiple top-level collection HID descriptor, ie: A single device with a HID descriptor with multiple root collections, like there's one in `dinput/tests/hid.c` / `test_hid_multiple_tlc`? If that's the case we currently don't support that very well and indeed, we either discard the device entirely or just take the first collection, as you can see from the test. It's on my plate to do at some point but you're welcome if you're interested to work on implementing it. Otherwise I think we should take Unix level devices separately and I'm not sure to see how this happens. You are using the udev / hidraw backend I presume? Also, HID mouse / keyboard are currently completely unsupported, they may be exposed as devices but I don't think they would work well with any application.
2. The thread which receives HID input reports and makes `send_hardware_message` requests to wineserver does not have an associated desktop. This causes wineserver to return STATUS_INVALID_HANDLE and thus no rawinput messages get queued.
I think that we're supposed to have a desktop there... or otherwise no controller would work at all. Maybe there's a regression? It should normally be created automatically as hidclass.sys links with user32 and uses __wine_send_input to send the HID messages. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1448#note_16683
On Mon Nov 21 17:51:20 2022 +0000, Rémi Bernon wrote:
Thanks for the tests! I actually made a few tweaks and took that opportunity to fix the Get and Scaled flavors too. Would you mind having a look at https://gitlab.winehq.org/rbernon/wine/-/commits/mr-1448-tweaks and updating this MR with it if that looks fine to you?
if (!(caps->flags & HID_VALUE_CAPS_IS_RANGE)) bit_count *= caps->report_count;
We only want to do this if `HidP_*UsageValueArray` is called, if it's `HidP_*UsageValue` we only [handle the first item](https://learn.microsoft.com/en-us/windows-hardware/drivers/hid/value-capabil...). Or is the documentation wrong and Windows actually does this? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1448#note_16756
You are using the udev / hidraw backend I presume?
Yes, and to that end, I've disabled SDL mapping. If I leave SDL mapping enabled, the controller inputs work, but the HID lights don't. Perhaps that's related to why there's no desktop? Also I was applying these on wine-staging, will update when I try vanilla wine. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1448#note_16757
On Mon Nov 21 17:51:19 2022 +0000, Matthew Tran wrote:
```c if (!(caps->flags & HID_VALUE_CAPS_IS_RANGE)) bit_count *= caps->report_count; ``` We only want to do this if `HidP_*UsageValueArray` is called, if it's `HidP_*UsageValue` we only [handle the first item](https://learn.microsoft.com/en-us/windows-hardware/drivers/hid/value-capabil...). Or is the documentation wrong and Windows actually does this?
You're right, I've pushed some update to my branch, with new tests to validate that. Fun fact, I remember being confused by the array flavor that returned `HIDP_STATUS_NOT_IMPLEMENTED` when I tried using it. The new tests uncovered that it only does when the elements are smaller than a byte. I don't really see how it is any more difficult to cover that case than to cover it with more bits but well, so be it. Wine just does it better ;). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1448#note_16773
participants (4)
-
Marvin -
Matthew Tran -
Matthew Tran (＠0e4ef622)
-
Rémi Bernon