[PATCH 0/3] MR8744: 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.
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;
Tyson Whitehead (@twhitehead) commented about dlls/dinput/joystick_hid.c:
*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 );
@rbernon I pushed the `dinput_device` structure initialization down a bit as I needed `instance.tszInstanceName` set in order to check if there were any overrides when setting the initial value for `autocenter`.
It looked to me like the only part that was used right away was the `crit` critical section, so I pulled that out and initialized it at the original call site. This didn't seem that bad as the original call site was already directly setting the `crit.DebugInfo` and some other items.
Compiled and worked okay for me, but thought I would mention it to get a second pair of eyes on it. Thanks!
@TomaszPakula please test and let us know if this address your autocenter is on issue.
The following in _user.reg_ is an example of overriding the default for the Sidewinder 2 for all applications ``` [Software\Wine\DirectInput\Autocenter] "Microsoft SideWinder Force Feedback 2 Joystick"="off" ``` or just for _foo.exe_ ``` [Software\Wine\AppDefaults\foo.exe\DirectInput\Autocenter] "Microsoft SideWinder Force Feedback 2 Joystick"="off" ```
This needlessly complicates a non issue. I don't know if purposefully or not but you're completely missing the point of https://gitlab.winehq.org/wine/wine/-/merge_requests/8605. If you woul actually understand what I was saying, my device doesn't even support autocenter natively and the PR was made on behalf on a wider community. Yes, I was testing this as well with my work to bring effect based autocenter support to `hid-pidff` driver but I can't really use this as a real argument if it's not upstream and fully usable.
1. This duplicates what's already in wine when it comes to registry properties 2. It's not just "my" issue, this autocenter behavior on SDL/udev is unwanted by a big part of community as it was just fine. Again, modifying autocenter outside of an ffb application is a valid use case on Linux. Of course, Wine is not Linux, but as Linus said "If it's a bug that people rely on it's not a bug, it's a feature" 3. Applying registry overrides to 30 separate proton prefixes is not easy nor a robust solution 4. Autocenter is a minor part of force feedback. Moreover, games are encouraged to use spring + friction/damper to properly manage active autocentering. It's not a core effect and doesn't affect "real" gameplay. IMO forcing end users to use overrides to "fix" misbehaving games is not great from usability standpoint where the previous behavior was working just fine and doesn't require any actions on their part. 5. Do we really need another "quirks" list/database that will grow endlessly, especially tracked outside of wine? 6. As I said before, this could be revisited when Linux force-feedback api is improved and more fleshed-out.
I said my piece and explained myself and the simracing community's point at length and as best as I can. Whatever may come, we'll deal with and I won't defy what Wine developer this is the proper solution. Please, kindly stop tagging me now.
TL DR: The point about good defaults is a good one. I believe there are more wheels than joysticks, so in the interest of limiting the need to override I will add a further path to change the Wine default to be off unless the device name contains the work _joystick_. If anyone has a better heuristic, I am all ears.
Not tagging Tomasz as he has requested. I can tell my persistence has been frustrating for Tomasz. His refusal to engage with me to find a better solution than just stripping `DIPROP_AUTOCENTER` support out has been frustrating for me too.
I am glad he clarified that the high level problem is that the simracing community has been suffering from autocentering on on their devices. This makes sense. The focus on [USB PID](https://www.usb.org/sites/default/files/documents/pid1_01.pdf) and how devices implement autocentering didn't make sense as that could only be pertinent to the udev hidraw backend. The SDL and udev input backends control devices entirely through the kernel API. Wine doesn't know or care how the device works for these backends.
Wine always explicitly turns autocenter on or off to match `DIPROP_AUTOCENTER`. If it is on, it is because `DIPROP_AUTOCENTER` is `DIPROPAUTOCENTER_ON`. If it is off, it is because `DIPROP_AUTOCENTER` is `DIPROPAUTOCENTER_OFF`. This is how Windows works. The only potential difference from Windows is the initial value. Programs that rely on a particular initial values instead of checking and setting won't work if the Wine initial value is different than the Windows initial value.
The initial value in Wine is `DIPROPAUTOCENTER_ON`. This was based on my Sidewinder 2. It is most likely correct for joysticks given the lack of complaints about them. It is almost certain incorrect for wheels given what Tomasz has said about the sim racing community suffering from autocentering on. It would be nice if Tomasz would run the [test program](https://github.com/twhitehead/issue-wine-ff-autocenter) I wrote to verify this on his own wheel like I have repeatedly requested, but I guess that isn't strictly necessary.
I read wheels are now the dominate force feedback devices (more sim racers than flyers?), so I will add a further commit to only make the default on if the devices name contains the word _joystick_ and off otherwise. This will miss joysticks without _joystick_ in their name, like the _Logitech WingMan_, so, if anyone has a better idea how to differentiate between wheels and joysticks, please speak up.
Thanks to everyone for continuing with this!
-
Tomasz Pakuła (＠TomaszPakula) -
Tyson Whitehead
-
Tyson Whitehead (＠twhitehead)