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

[Arkadiusz Hiler]

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

-- 
Cheers,
Arek