[PATCH v2 1/5] dinput: Introduce new HID joystick backend.

[Arkadiusz Hiler]

On Tue, Jun 29, 2021 at 05:40:12PM +0200, Rémi Bernon wrote:
> > > +static HRESULT hid_joystick_device_open( DWORD index, WCHAR *device_path, HANDLE *device, DWORD version,
> > > +                                         PHIDP_PREPARSED_DATA *preparsed, HIDD_ATTRIBUTES *attrs,
> > > +                                         HIDP_CAPS *caps, DIDEVICEINSTANCEW *instance )
> > > +{
> > > +    RAWINPUTDEVICELIST *list;
> > > +    DWORD count, i, j;
> > > +
> > > +    GetRawInputDeviceList( NULL, &count, sizeof(*list) );
> > > +    if (!(list = HeapAlloc( GetProcessHeap(), 0, count * sizeof(*list) ))) return DIERR_OUTOFMEMORY;
> > > +    GetRawInputDeviceList( list, &count, sizeof(*list) );
> > > +
> > > +    for (i = 0; i < count; ++i) if (list[i].dwType == RIM_TYPEHID) break;
> > > +    for (j = 0; j < index && i < count; ++i) if (list[i].dwType == RIM_TYPEHID) ++j;
> > > +    if (i == count)
> > > +    {
> > > +        HeapFree( GetProcessHeap(), 0, list );
> > > +        return DIERR_DEVICENOTREG;
> > > +    }
> > 
> > I am not sure you can depend on RIM_TYPEMOUSE and RIM_TYPEKEYBOARD being
> > clumped at the beggining of the list, as it's not guaranteed by the API
> > an may break in Wine in the future.
> > 
> 
> I don't think I am? It's supposed to look for the first RIM_TYPEHID and then
> starts counting HID devices from there, ignoring non-HID devices.

Oh, indeed you are not. It's just a bit confusing to read through the
looping logic here.

> > > +    count = MAX_PATH;
> > > +    GetRawInputDeviceInfoW( list[i].hDevice, RIDI_DEVICENAME, device_path, &count );

You should check return value here, otherwise you may end up with
uninitalized device_path.

> > > +    HeapFree( GetProcessHeap(), 0, list );
> > > +
> > > +    TRACE( "Opening %s\n", debugstr_w(device_path) );
> > > +    *device = CreateFileW( device_path, GENERIC_READ | GENERIC_WRITE,
> > > +                           FILE_SHARE_READ | FILE_SHARE_WRITE, NULL, OPEN_EXISTING, 0, 0 );
> > > +    *preparsed = NULL;
> > > +
> > > +    if (*device == INVALID_HANDLE_VALUE) return DI_NOTATTACHED;
> > > +    if (!HidD_GetPreparsedData( *device, preparsed )) goto failed;
> > > +    if (!HidD_GetAttributes( *device, attrs )) goto failed;
> > > +    if (HidP_GetCaps( *preparsed, caps ) != HIDP_STATUS_SUCCESS) goto failed;
> > > +
> > > +    if (caps->UsagePage == HID_USAGE_PAGE_GAME) FIXME( "Unimplemented HID game usage page!\n" );
> > > +    if (caps->UsagePage == HID_USAGE_PAGE_SIMULATION) FIXME( "Unimplemented HID simulation usage page!\n" );
> > > +    if (caps->UsagePage != HID_USAGE_PAGE_GENERIC) goto failed;
> > > +    if (caps->Usage != HID_USAGE_GENERIC_GAMEPAD && caps->Usage != HID_USAGE_GENERIC_JOYSTICK) goto failed;
> > > +
> > > +    instance->guidInstance = hid_joystick_guid;
> > > +    instance->guidInstance.Data3 = index;
> > > +    instance->guidProduct = DInput_PIDVID_Product_GUID;
> > > +    instance->guidProduct.Data1 = MAKELONG( attrs->VendorID, attrs->ProductID );
> > > +    instance->dwDevType = get_device_type( version, caps->Usage != HID_USAGE_GENERIC_GAMEPAD ) | DIDEVTYPE_HID;
> > > +    instance->guidFFDriver = GUID_NULL;
> > > +    instance->wUsagePage = caps->UsagePage;
> > > +    instance->wUsage = caps->Usage;
> > > +
> > > +    if (!HidD_GetProductString( *device, instance->tszInstanceName, MAX_PATH )) goto failed;
> > > +    if (!HidD_GetProductString( *device, instance->tszProductName, MAX_PATH )) goto failed;
> > > +    return DI_OK;
> > > +
> > > +failed:
> > > +    CloseHandle( *device );
> > > +    *device = INVALID_HANDLE_VALUE;
> > > +    HidD_FreePreparsedData( *preparsed );
> > > +    *preparsed = NULL;
> > > +    return DI_NOTATTACHED;
> > > +}
> > > +
> > > +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, device_path, &device, version,
> > > +                                   &preparsed, &attrs, &caps, instance );
> > 
> > Does GetRawInputDeviceList() give us guarantees on device ordering? I
> > think the relative ordering should be preserved (plus/minus the
> > hotplugs, but IMO that's not an issue), but I haven't checked.
> > 
> 
> It's using setupapi, and keeps its order, I think? I could use setupapi
> directly, but rawinput API is slightly simpler to use IMHO.

Fair enough. I think the ordering is also stable on Windows so that
behavior should not change. Anyway it will be easy to notice if it
regresses.

> > > +    hr = hid_joystick_device_open( index, device_path, &device, dinput->dwVersion,
> > > +                                   &preparsed, &attrs, &caps, &instance );
> > 
> > I find it unlikely that hotplug is going to be an issue for enumerating,
> > but using only the idx for device creation may be problematic,
> > especially that Enum -> Crate can be more separated in time.
> > 
> > How about using VID + PID + ordinal? This should also help with making
> > the enumeration logic a bit more robust.
> > 
> 
> Yeah I tried to use the same method as the other drivers, I'm not completely
> sure to understand what these instance / product GUIDs really are.

>From my observations:

guidProduct - represents the product, i.e. all the Model 1914 Xbox
	      controllers I have have the same guidProduct, so it makes
	      sense to include vidpidrev in it.

guidInstance - uniquely identifies the given device, not sure how stable
               it across hotplus/reboots is though.

Just throwing some more ideas here, but since hDevice is an opaque value
that's stable across processes and uniquely identifies the device (until
the reboot), maybe we should try to fit it into the instance GUID? This
will save you the need to do any counting and you can pass it directly
to GetRawInputDeviceInfo().

> It was also handy to be able to re-use hid_joystick_device_open here, but
> that can probably be factored somehow.
> 
> Thanks for the comments!

With enough refactoring anything is possible ;-)

-- 
Cheers,
Arek