[PATCH v2 0/4] MR8744: dinput/winebus: Restore autocenter support to SDL and udev input backends and provide default override to address on issues
This merge request restores `DIPROP_AUTOCENTER` support to the SDL and udev input backends. It also * adds an `Autocenter` registry app key that lets the default `DIPROP_AUTOCENTER` setting to be set on a per device and per application and device level, * logs a warning on acquisition when `DIPROP_AUTOCENTER` hasn't been checked or set that that is the case and the default can be changed via the 'Autocenter' app key if required.
Together these two should provide a robust solution for the issues some where having with autocenter being incorrectly on for some programs and devices.
-- v2: dinput: Autocenter on default unless device name contains joystick
From: Tyson Whitehead twhitehead@gmail.com
The removal of autocenter support from the SDL and udev input backends was based on the belief that the device should not turn auto centering on upon receiving a device reset command from the front end as only some devices do it this way and others have different ways of doing it.
This is a fundamental misunderstanding of what is going on. The messages between the front end and the SDL and udev backends are the front end telling the backends what to do. These are in no way sent to the device, and have nothing to do with how autocenter is turned on or off on the device. Rather the backends interpret them and then makes SDL and udev input API calls to drive the device, including turning on or off autocentering.
The reason the backends call their respective APIs to turn on autocentering on receiving a reset is because that is they way turn on autocenter is communicated to the backend. In the event that the autocenter should not be on, the front end will follow the reset with a stop all effects command which tells the backends to call their underlying API to turn it back off.
If autocenter is different than a users experts it to be, either * the program assumed a given initial state, didn't verify it or or set it, and the initial state was the other, or * the backend API calls are failing to set it (the kernel driver either does not support setting it or is buggy).
I have written a test program that lets people test their devices to ensure things are working correctly and discover what the initial state is under Windows
https://github.com/twhitehead/issue-wine-ff-autocenter
A log file is generated for users to submit so initial states under Windows can be discovered and broken drivers identified. This will allow the unwanted autocenter issue of the author of the commit being reverted to be properly figured out and addressed.
As a final note, the SDL and udev input backends are actually the preferred backends for force feedback as they ultimately dispatch to the kernel driver which can handle any required quirks as well as all the different way autocentering can be implemented.
By comparison, the udev hidraw backend is quite limited. It requires a PID compliant device without any quirks that uses reset and stop all effect autocentering. Such devices are perfectly supported by the kernel ff_pdiff driver, so they to function perfectly with the SDL and udev input backends.
This reverts commit b0db6d50530023a19846f08f1c62f14803448f59. --- dlls/winebus.sys/bus_sdl.c | 2 ++ dlls/winebus.sys/bus_udev.c | 20 ++++++++++++++++++++ 2 files changed, 22 insertions(+)
diff --git a/dlls/winebus.sys/bus_sdl.c b/dlls/winebus.sys/bus_sdl.c index 6e2c8bef3e4..7c622155fd8 100644 --- a/dlls/winebus.sys/bus_sdl.c +++ b/dlls/winebus.sys/bus_sdl.c @@ -518,6 +518,7 @@ static NTSTATUS sdl_device_physical_device_control(struct unix_device *iface, US return STATUS_SUCCESS; case PID_USAGE_DC_STOP_ALL_EFFECTS: pSDL_HapticStopAll(impl->sdl_haptic); + pSDL_HapticSetAutocenter(impl->sdl_haptic, 0); return STATUS_SUCCESS; case PID_USAGE_DC_DEVICE_RESET: pSDL_HapticStopAll(impl->sdl_haptic); @@ -527,6 +528,7 @@ static NTSTATUS sdl_device_physical_device_control(struct unix_device *iface, US pSDL_HapticDestroyEffect(impl->sdl_haptic, impl->effect_ids[i]); impl->effect_ids[i] = -1; } + pSDL_HapticSetAutocenter(impl->sdl_haptic, 100); return STATUS_SUCCESS; case PID_USAGE_DC_DEVICE_PAUSE: pSDL_HapticPause(impl->sdl_haptic); diff --git a/dlls/winebus.sys/bus_udev.c b/dlls/winebus.sys/bus_udev.c index 9951e3d0ec8..c6b71492f9e 100644 --- a/dlls/winebus.sys/bus_udev.c +++ b/dlls/winebus.sys/bus_udev.c @@ -827,6 +827,24 @@ static NTSTATUS lnxev_device_physical_effect_run(struct lnxev_device *impl, BYTE return STATUS_SUCCESS; }
+static NTSTATUS lnxev_device_physical_device_set_autocenter(struct unix_device *iface, BYTE percent) +{ + struct lnxev_device *impl = lnxev_impl_from_unix_device(iface); + struct input_event ie = + { + .type = EV_FF, + .code = FF_AUTOCENTER, + .value = 0xffff * percent / 100, + }; + + TRACE("iface %p, percent %#x.\n", iface, percent); + + if (write(impl->base.device_fd, &ie, sizeof(ie)) == -1) + WARN("write failed %d %s\n", errno, strerror(errno)); + + return STATUS_SUCCESS; +} + static NTSTATUS lnxev_device_physical_device_control(struct unix_device *iface, USAGE control) { struct lnxev_device *impl = lnxev_impl_from_unix_device(iface); @@ -870,6 +888,7 @@ static NTSTATUS lnxev_device_physical_device_control(struct unix_device *iface, if (impl->effect_ids[i] < 0) continue; lnxev_device_physical_effect_run(impl, i, 0); } + lnxev_device_physical_device_set_autocenter(iface, 0); return STATUS_SUCCESS; case PID_USAGE_DC_DEVICE_RESET: for (i = 0; i < ARRAY_SIZE(impl->effect_ids); ++i) @@ -879,6 +898,7 @@ static NTSTATUS lnxev_device_physical_device_control(struct unix_device *iface, WARN("couldn't free effect, EVIOCRMFF ioctl failed: %d %s\n", errno, strerror(errno)); impl->effect_ids[i] = -1; } + lnxev_device_physical_device_set_autocenter(iface, 100); return STATUS_SUCCESS; case PID_USAGE_DC_DEVICE_PAUSE: WARN("device pause not supported\n");
From: Tyson Whitehead twhitehead@gmail.com
Programs should check and/or set DIPROP_AUTOCENTER to ensure it is the value they want. Likely some do not though if the developers never tested on devices where the Windows' default wasn't correct for them.
These programs will be broken under Wine if the Wine default is different than the Windows. This commit adds an 'Autocenter' appkey (to the existing 'Joysticks' and 'MouseWarpOverride' keys) so users can override the DIPROP_AUTOCENTER default in this case.
The Wine default is currently always DIPROPAUTOCENTER_ON, which is the correct Windows default for the MS Sidewinder 2. I don't know any other correct defaults as this is the only FF device I have to test. I started a project to collect defaults (and verify operations) for other devices though
https://github.com/twhitehead/issue-wine-ff-autocenter
so hopefully a good heuristic and a more comprehensive set of defaults can be implemented at some point. --- dlls/dinput/device.c | 56 ++++++++++++++++++++++++++++++++++++-- dlls/dinput/joystick_hid.c | 3 +- 2 files changed, 56 insertions(+), 3 deletions(-)
diff --git a/dlls/dinput/device.c b/dlls/dinput/device.c index 31240d03f65..2aa50a844f1 100644 --- a/dlls/dinput/device.c +++ b/dlls/dinput/device.c @@ -145,6 +145,59 @@ DWORD get_config_key( HKEY defkey, HKEY appkey, const WCHAR *name, WCHAR *buffer return ERROR_FILE_NOT_FOUND; }
+static BOOL device_instance_autocenter_initial( DIDEVICEINSTANCEW *instance ) +{ + static const WCHAR on_str[] = {'o', 'n', 0}; + static const WCHAR off_str[] = {'o', 'f', 'f', 0}; + static const WCHAR joystick_key[] = {'A', 'u', 't', 'o', 'c', 'e', 'n', 't', 'e', 'r', 0}; + WCHAR buffer[MAX_PATH]; + HKEY hkey, appkey, temp; + BOOL autocenter = DIPROPAUTOCENTER_ON; /* MS Sidewinder 2 initial value. No others known yet. There is + a project to collect other device defaults to improved this + https://github.com/twhitehead/issue-wine-ff-autocenter */ + + get_app_key( &hkey, &appkey ); + + /* Autocenter settings are in the 'Autocenter' subkey */ + if (appkey) + { + if (RegOpenKeyW( appkey, joystick_key, &temp )) temp = 0; + RegCloseKey( appkey ); + appkey = temp; + } + + if (hkey) + { + if (RegOpenKeyW( hkey, joystick_key, &temp )) temp = 0; + RegCloseKey( hkey ); + hkey = temp; + } + + /* Look for the "controllername"="on"/"off" key */ + if (!get_config_key( hkey, appkey, instance->tszInstanceName, buffer, sizeof(buffer) )) + { + if (!wcscmp( on_str, buffer )) + { + TRACE( "Joystick '%s' autocenter on based on registry key.\n", debugstr_w(instance->tszInstanceName) ); + autocenter = DIPROPAUTOCENTER_ON; + } + else if (!wcscmp( off_str, buffer )) + { + TRACE( "Joystick '%s' autocenter off based on registry key.\n", debugstr_w(instance->tszInstanceName) ); + autocenter = DIPROPAUTOCENTER_OFF; + } + else + { + WARN( "Joystick '%s' autocenter registry key invalid (can only be 'on' or 'off').\n", debugstr_w(instance->tszInstanceName) ); + } + } + + if (appkey) RegCloseKey( appkey ); + if (hkey) RegCloseKey( hkey ); + + return autocenter; +} + BOOL device_instance_is_disabled( DIDEVICEINSTANCEW *instance, BOOL *override ) { static const WCHAR disabled_str[] = {'d', 'i', 's', 'a', 'b', 'l', 'e', 'd', 0}; @@ -2153,9 +2206,8 @@ void dinput_device_init( struct dinput_device *device, const struct dinput_devic device->caps.dwSize = sizeof(DIDEVCAPS); device->caps.dwFlags = DIDC_ATTACHED | DIDC_EMULATED; device->device_gain = 10000; - device->autocenter = DIPROPAUTOCENTER_ON; + device->autocenter = device_instance_autocenter_initial( &device->instance ); device->force_feedback_state = DIGFFS_STOPPED | DIGFFS_EMPTY; - InitializeCriticalSectionEx( &device->crit, 0, RTL_CRITICAL_SECTION_FLAG_FORCE_DEBUG_INFO ); dinput_internal_addref( (device->dinput = dinput) ); device->vtbl = vtbl;
diff --git a/dlls/dinput/joystick_hid.c b/dlls/dinput/joystick_hid.c index 27d08fd2af2..dc7f8b25f70 100644 --- a/dlls/dinput/joystick_hid.c +++ b/dlls/dinput/joystick_hid.c @@ -2036,7 +2036,7 @@ HRESULT hid_joystick_create_device( struct dinput *dinput, const GUID *guid, IDi *out = NULL;
if (!(impl = calloc( 1, sizeof(*impl) ))) return E_OUTOFMEMORY; - dinput_device_init( &impl->base, &hid_joystick_vtbl, guid, dinput ); + InitializeCriticalSectionEx( &impl->base.crit, 0, RTL_CRITICAL_SECTION_FLAG_FORCE_DEBUG_INFO ); impl->base.crit.DebugInfo->Spare[0] = (DWORD_PTR)(__FILE__ ": hid_joystick.base.crit"); impl->base.dwCoopLevel = DISCL_NONEXCLUSIVE | DISCL_BACKGROUND; impl->base.read_event = CreateEventW( NULL, TRUE, FALSE, NULL ); @@ -2050,6 +2050,7 @@ HRESULT hid_joystick_create_device( struct dinput *dinput, const GUID *guid, IDi hr = hid_joystick_device_try_open( impl->device_path, &impl->device, &impl->preparsed, &attrs, &impl->caps, &impl->base.instance, dinput->dwVersion ); } + dinput_device_init( &impl->base, &hid_joystick_vtbl, guid, dinput ); if (hr != DI_OK) goto failed;
impl->base.caps.dwDevType = impl->base.instance.dwDevType;
From: Tyson Whitehead twhitehead@gmail.com
Programs should check and/or set DIPROP_AUTOCENTER to ensure it is the value they want. Likely some do not though if the developers never tested on devices where the Windows' default wasn't correct for them. These programs will be broken under Wine if the Wine default is different than the Windows.
This commit logs a warning upon acquisition, if the program does it without first checking or setting DIPROP_AUTOCENTER. The warning tells the user what the default is, that it may be incorrect, and that they can override it with the new 'Autocenter' app key. --- dlls/dinput/device.c | 7 +++++++ dlls/dinput/device_private.h | 1 + 2 files changed, 8 insertions(+)
diff --git a/dlls/dinput/device.c b/dlls/dinput/device.c index 2aa50a844f1..6985a6b279a 100644 --- a/dlls/dinput/device.c +++ b/dlls/dinput/device.c @@ -548,6 +548,10 @@ static HRESULT WINAPI dinput_device_Acquire( IDirectInputDevice8W *iface ) impl->status = STATUS_ACQUIRED; if (FAILED(hr = impl->vtbl->acquire( iface ))) impl->status = STATUS_UNACQUIRED; } + if (impl->autocenter_warning) + WARN( "Joystick %s was acquired without checking or setting autocenter, defaulting %s (override with 'Autocenter' registry key)\n", + debugstr_w(impl->instance.tszInstanceName), + impl->autocenter == DIPROPAUTOCENTER_ON ? "on" : "off" ); LeaveCriticalSection( &impl->crit ); if (hr != DI_OK) return hr;
@@ -1178,6 +1182,7 @@ static HRESULT dinput_device_get_property( IDirectInputDevice8W *iface, const GU DIPROPDWORD *value = (DIPROPDWORD *)header; if (!(impl->caps.dwFlags & DIDC_FORCEFEEDBACK)) return DIERR_UNSUPPORTED; value->dwData = impl->autocenter; + impl->autocenter_warning = FALSE; return DI_OK; } case (DWORD_PTR)DIPROP_BUFFERSIZE: @@ -1362,6 +1367,7 @@ static HRESULT dinput_device_set_property( IDirectInputDevice8W *iface, const GU const DIPROPDWORD *value = (const DIPROPDWORD *)header; if (!(impl->caps.dwFlags & DIDC_FORCEFEEDBACK)) return DIERR_UNSUPPORTED; impl->autocenter = value->dwData; + impl->autocenter_warning = FALSE; return DI_OK; } case (DWORD_PTR)DIPROP_FFGAIN: @@ -2206,6 +2212,7 @@ void dinput_device_init( struct dinput_device *device, const struct dinput_devic device->caps.dwSize = sizeof(DIDEVCAPS); device->caps.dwFlags = DIDC_ATTACHED | DIDC_EMULATED; device->device_gain = 10000; + device->autocenter_warning = TRUE; device->autocenter = device_instance_autocenter_initial( &device->instance ); device->force_feedback_state = DIGFFS_STOPPED | DIGFFS_EMPTY; dinput_internal_addref( (device->dinput = dinput) ); diff --git a/dlls/dinput/device_private.h b/dlls/dinput/device_private.h index fa791f08a9d..a0da6c7d9f7 100644 --- a/dlls/dinput/device_private.h +++ b/dlls/dinput/device_private.h @@ -116,6 +116,7 @@ struct dinput_device BYTE device_state_report_id; BYTE device_state[DEVICE_STATE_MAX_SIZE];
+ BOOL autocenter_warning; BOOL autocenter; LONG device_gain; DWORD force_feedback_state;
From: Tyson Whitehead twhitehead@gmail.com
While ideally programs should check and turn autocenter off, it is likely that many do not. The default autocenter under Wine has been on. Tomasz Pakuta reported in !8744 that this is the wrong default for wheels and it has been causing a problem for many in the sim racing community.
This commit makes `DIPROPAUTOCENTER_OFF` the default value for `DIPROP_AUTOCENTER` unless the device name contains "joystick". Although this will miss some joysticks (those without joystick in their name, like the "Logitech WingMan"), it should hopefully be an overall win to have joysticks be the exception and not the default as I read there are more wheels than joysticks.
Wrongly classified devices can still be overriden by the new user app key "Autocenter". As in
[Software\Wine\DirectInput\Autocenter] "Device instance name"="on"
or
[Software\Wine\AppDefaults\app.exe\DirectInput\Autocenter] "Device instance name"="on" --- dlls/dinput/device.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-)
diff --git a/dlls/dinput/device.c b/dlls/dinput/device.c index 6985a6b279a..543495d7a09 100644 --- a/dlls/dinput/device.c +++ b/dlls/dinput/device.c @@ -152,13 +152,21 @@ static BOOL device_instance_autocenter_initial( DIDEVICEINSTANCEW *instance ) static const WCHAR joystick_key[] = {'A', 'u', 't', 'o', 'c', 'e', 'n', 't', 'e', 'r', 0}; WCHAR buffer[MAX_PATH]; HKEY hkey, appkey, temp; - BOOL autocenter = DIPROPAUTOCENTER_ON; /* MS Sidewinder 2 initial value. No others known yet. There is - a project to collect other device defaults to improved this - https://github.com/twhitehead/issue-wine-ff-autocenter */ + BOOL autocenter = DIPROPAUTOCENTER_OFF;
- get_app_key( &hkey, &appkey ); + /* Default devices with joysticks in their name to on */ + wcscpy_s( buffer, sizeof(buffer)/sizeof(buffer[0]), instance->tszProductName ); + _wcslwr_s( buffer, sizeof(buffer)/sizeof(buffer[0]) ); + if ( wcsstr(buffer, L"joystick") != NULL ) + { + autocenter = DIPROPAUTOCENTER_ON; + } + TRACE( "Joystick '%s' autocenter default is %s.\n", debugstr_w(instance->tszInstanceName), + autocenter == DIPROPAUTOCENTER_ON ? "on" : "off" );
/* Autocenter settings are in the 'Autocenter' subkey */ + get_app_key( &hkey, &appkey ); + if (appkey) { if (RegOpenKeyW( appkey, joystick_key, &temp )) temp = 0; @@ -178,12 +186,12 @@ static BOOL device_instance_autocenter_initial( DIDEVICEINSTANCEW *instance ) { if (!wcscmp( on_str, buffer )) { - TRACE( "Joystick '%s' autocenter on based on registry key.\n", debugstr_w(instance->tszInstanceName) ); + TRACE( "Joystick '%s' autocenter default overridden to on based on registry key.\n", debugstr_w(instance->tszInstanceName) ); autocenter = DIPROPAUTOCENTER_ON; } else if (!wcscmp( off_str, buffer )) { - TRACE( "Joystick '%s' autocenter off based on registry key.\n", debugstr_w(instance->tszInstanceName) ); + TRACE( "Joystick '%s' autocenter default overridden to off based on registry key.\n", debugstr_w(instance->tszInstanceName) ); autocenter = DIPROPAUTOCENTER_OFF; } else
I had a look through the backend to see about doing a better job of determining the controller type than just looking at its name.
For SDL the [usage page and usage are chosen nicely](https://gitlab.winehq.org/wine/wine/-/blob/91d3874b6528cb71b346c1d114914bcfd...) to encode the [`SDL_JoystickGetDeviceType`](https://wiki.libsdl.org/SDL2/SDL_JoystickGetDeviceType) result. So distinguishing is as easy as checking `instance.wUsagePage` and `instance.wUsage` in the front end.
Unfortunately udev [doesn't give any such nice breakdown](https://gitlab.winehq.org/wine/wine/-/blob/91d3874b6528cb71b346c1d114914bcfd...). Everything just gets assigned to gamepad. Fixing this would require getting the udev people to add subcategories to `ID_JOYSTICK` (e.g., `ID_JOYSTICK_GAMECONTROLLER`, `ID_JOYSTICK_WHEEL`, etc.).
-
Tyson Whitehead -
Tyson Whitehead (＠twhitehead)