Re: [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.