On Wed, Jun 30, 2021 at 03:16:13PM +0200, Rémi Bernon wrote:
On 6/30/21 2:30 PM, Rémi Bernon wrote:
On 6/30/21 1:08 PM, Arkadiusz Hiler wrote:
On Wed, Jun 30, 2021 at 10:54:19AM +0200, Rémi Bernon wrote:
+static HRESULT hid_joystick_enum_device( DWORD type, DWORD flags, DIDEVICEINSTANCEW *instance, + DWORD version, int index ) +{ + HIDD_ATTRIBUTES attrs = {sizeof(attrs)}; + PHIDP_PREPARSED_DATA preparsed; + WCHAR device_path[MAX_PATH]; + HIDP_CAPS caps; + HANDLE device; + HRESULT hr;
+ TRACE( "type %x, flags %#x, instance %p, version %04x, index %d\n", type, flags, instance, version, index );
+ hr = hid_joystick_device_open( index, 0, 0, device_path, &device, version, + &preparsed, &attrs, &caps, instance );
hid_joystick_device_open() is getting a bit unwieldy with all the possible parameter combinations. I don't think it needs the enumeration / vidpid lookup logic.
IMO it would read better if hid_joystick_device_open() would take the path or RI handle and enum logic would be moved either here or to a helper.
I don't think we need yet another device list cache, and I would rather avoid it if possible. It's really just two call sites.
Oh sorry, this is not what I was suggesting.
I was thinking about factoring out the setupapi enumeration / device finding logic to a separate function than the one that does all the Hid?_* handling.
Something like:
HANDLE hid_joystick_find_device( int index, HANDLE ri_handle, int vid_pid, HANDLE *ri_handle_out );
and then you can either:
device = hid_joystick_find_device( index, INVALID_HANDLE, -1, &ri_handle ); device = hid_joystick_find_device( -1, ri_handle, -1, NULL ); device = hid_joystick_find_device( -1, INVALID_HANDLE, vid_pid, NULL );
open would then look more like:
hr = hid_joystick_device_read( device, version, &preparsed, &attrs, &caps );
Those two should give you all the thing you need to fill DIDEVICEINSTANCEW in _enum_ and to fill in hid_joystick in _create_.
Also I like to use obviously wrong values for parameters that are not used.
Instead of the two additional params we could use the DIDEVICEINSTANCEW parameter as an in/out filter, providing the desired GUIDs to match (or zeroed when enumerating all devices).
+ if (hr != DI_OK) return hr;
+ HidD_FreePreparsedData( preparsed ); + CloseHandle( device );
+ if (instance->dwSize != sizeof(DIDEVICEINSTANCEW)) + return S_FALSE; + if (version < 0x0800 && type != DIDEVTYPE_JOYSTICK) + return S_FALSE; + if (version >= 0x0800 && type != DI8DEVCLASS_ALL && type != DI8DEVCLASS_GAMECTRL) + return S_FALSE;
+ if (device_disabled_registry( "HID", TRUE )) + return DIERR_DEVICENOTREG;
+ TRACE( "Found device %s, usage %04x:%04x, product %s, instance %s, name %s\n", debugstr_w(device_path), + instance->wUsagePage, instance->wUsage, debugstr_guid( &instance->guidProduct ), + debugstr_guid( &instance->guidInstance ), debugstr_w(instance->tszInstanceName) );
+ return DI_OK; +}
+static HRESULT hid_joystick_create_device( IDirectInputImpl *dinput, REFGUID guid, IDirectInputDevice8W **out ) +{ + DIDEVICEINSTANCEW instance = {sizeof(instance)}; + GUID inst_guid = *guid, prod_guid = *guid; + DWORD size = sizeof(struct hid_joystick); + HIDD_ATTRIBUTES attrs = {sizeof(attrs)}; + struct hid_joystick *impl = NULL; + PHIDP_PREPARSED_DATA preparsed; + UINT32 handle = 0, vid_pid = 0; + DIDATAFORMAT *format = NULL; + WCHAR device_path[MAX_PATH]; + HIDP_CAPS caps; + HANDLE device; + HRESULT hr;
+ TRACE( "dinput %p, guid %s, out %p\n", dinput, debugstr_guid( guid ), out );
+ *out = NULL;
+ inst_guid.Data3 = hid_joystick_guid.Data3; + prod_guid.Data1 = DInput_PIDVID_Product_GUID.Data1; + if (IsEqualGUID( &hid_joystick_guid, &inst_guid )) handle = guid->Data3;
Data3 is only a short. I think we should use 64 bits for our magic and another 64 bits for the RawInput handle.
Or 32 if we go with our internal knowledge about how those handles are implemented in Wine.
Should we really worry about overflowing the short capacity for the HID handle? If we do we could combine Data2 and Data3, or XOR it in Data1 (just to keep a bit of randomness), but as the property is also checked to be 32bit I don't think we need to use more than that.
as long as we are using the DEVPROPKEY_HID_HANDLE 32 bits should be enough
For instance I think something along those lines would actually be nice, with the "instance" filled in try_open, and then returned into "filter" as output when device matches:
/* enumerate device by GUID */ if (IsEqualGUID( &filter->guidProduct, &instance.guidProduct )) break; if (IsEqualGUID( &filter->guidInstance, &instance.guidInstance )) break; /* enumerate all devices */ if (IsEqualGUID( &filter->guidProduct, &DInput_PIDVID_Product_GUID ) && IsEqualGUID( &filter->guidInstance, &hid_joystick_guid ) && !index--) break;And the two call sites would look like:
instance->guidProduct = DInput_PIDVID_Product_GUID; instance->guidInstance = hid_joystick_guid; hr = hid_joystick_device_open( index, instance, device_path, &device, &preparsed, &attrs, &caps, version ); if (hr != DI_OK) return hr;DIDEVICEINSTANCEW instance = {.dwSize = sizeof(instance), .guidProduct = *guid, .guidInstance = *guid}; instance.guidProduct.Data1 = DInput_PIDVID_Product_GUID.Data1; instance.guidInstance.Data1 = hid_joystick_guid.Data1; if (IsEqualGUID( &DInput_PIDVID_Product_GUID, &instance.guidProduct )) { instance.guidProduct = *guid; instance.guidInstance = hid_joystick_guid; } else if (IsEqualGUID( &hid_joystick_guid, &instance.guidInstance )) { instance.guidProduct = DInput_PIDVID_Product_GUID; instance.guidInstance = *guid; } else { return DIERR_DEVICENOTREG; } hr = hid_joystick_device_open( 0, &instance, device_path, &device, &preparsed, &attrs, &caps, dinput->dwVersion ); if (hr != DI_OK) return hr;It also fulfill the vid_pid matching nicely.
That's even more logic added to _device_open... I am still not a fan of how hard it is to parse through hid_joystick_device_open() and I would prefer it to be split.
This seems to be a theme in Wine and Win32 APIs though, so I won't NAK or nag you on this anymore :-P