[PATCH v3] dinput: Introduce new HID joystick backend.

[Rémi Bernon]

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.
> 
> 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.

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.
-- 
Rémi Bernon <rbernon at codeweavers.com>