feat: per-game vibration mode (off/controller/device) + intensity#1564
Open
TideGear wants to merge 1 commit into
Open
feat: per-game vibration mode (off/controller/device) + intensity#1564TideGear wants to merge 1 commit into
TideGear wants to merge 1 commit into
Conversation
Adds user-selectable vibration routing and intensity on top of upstream's evshim, single-controller. "device" mode vibrates the phone from the rumble values in the existing futex shared memory; "controller" drives the pad's motors per-motor via VibratorManager; "off" suppresses rumble. WinHandler: setVibrationMode/setVibrationIntensity + reconcileActiveRumble; per-motor rumbleViaVibratorManager + vibrateDevice + blendMotors (floors to >=1 so a high-freq-only rumble isn't silenced); VibrationAttributes gated to API 33 with an AudioAttributes fallback (minSdk 26). A keepalive thread refreshes the Android one-shot while the poller is blocked in waitForRumble (controller 500ms frequent; 60s device one-shot only near expiry); apply/cancel serialized on rumbleLock across the poller, keepalive, and UI threads. evshim.c: SDL rumble keepalive. Wine drives the virtual joystick via SDL_JoystickRumble with a duration; SDL auto-expires it and calls OnRumble(0,0) itself (~1s), capping vibration. A thread re-arms SDL with the live shm rumble every 500ms so the expiry never fires, preserving XInput set-and-forget; the t_keepalive_active guard makes the synchronous OnRumble re-entry a no-op so it isn't echoed back into shm. libevshim.so rebuilt (build-evshim.ps1 + sdl2_stub; 16KB-aligned, stripped). Settings/UI: vibrationMode + vibrationIntensity in ContainerData / PrefManager / ContainerUtils, a mode dropdown + intensity slider in ControllerTab, applied on launch via XServerScreen.
3 tasks
Contributor
📝 WalkthroughWalkthroughThis PR adds a comprehensive vibration keepalive and configuration system spanning native SDL rumble re-arming in C, Kotlin configuration and persistence, Compose UI controls, and a refactored Java vibration execution engine with per-motor control and mode-based routing. ChangesVibration Keepalive and Configuration System
Sequence Diagram(s)sequenceDiagram
participant Game
participant evshim
participant rumble_keepalive
participant SDL_JoystickRumble
Game->>evshim: OnRumble (write to shm)
rumble_keepalive->>evshim: read shm rumble values (500ms)
evshim->>SDL_JoystickRumble: call with 2000ms duration
SDL_JoystickRumble->>rumble_keepalive: re-arm joystick
Note over evshim: t_keepalive_active blocks OnRumble echo
sequenceDiagram
participant Game
participant RumblePoller
participant rumbleLock
participant VibratorManager
participant Vibrator
Game->>RumblePoller: writes rumble to shm
RumblePoller->>rumbleLock: waitForRumble detects change
rumbleLock->>VibratorManager: startVibration routes by mode
alt mode == "controller"
VibratorManager->>VibratorManager: rumbleViaVibratorManager (API 31+)
else mode == "device"
VibratorManager->>Vibrator: vibrateDevice with curved amplitude
else mode == "off"
VibratorManager->>Vibrator: stopVibration
end
VibratorManager->>RumblePoller: return
RumblePoller->>RumblePoller: keepalive reschedules at cadence
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Infer (1.2.0)app/src/main/cpp/evshim/evshim.capp/src/main/cpp/evshim/evshim.c:16:10: fatal error: 'jni.h' file not found ... [truncated 689 characters] ... ux-x86_64-v1.2.0/lib/infer/facebook-clang-plugins/clang/install/lib/clang/18/include" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
5 issues found across 11 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt">
<violation number="1" location="app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt:1921">
P2: Per-game vibration settings are only initialized inside `if (PluviaApp.xEnvironment == null)`, so they are lost when a new WinHandler is created during activity recreation while a game is still running.</violation>
</file>
<file name="app/src/main/java/app/gamenative/ui/component/dialog/ControllerTab.kt">
<violation number="1" location="app/src/main/java/app/gamenative/ui/component/dialog/ControllerTab.kt:71">
P2: Vibration mode string literals are duplicated in the UI instead of referencing a single source of truth, creating schema-drift risk across PrefManager, WinHandler, ContainerData, and XServerScreen.</violation>
</file>
<file name="app/src/main/res/values/strings.xml">
<violation number="1" location="app/src/main/res/values/strings.xml:768">
P2: New vibration UI strings are missing from locale-specific `values-*/strings.xml` files, causing non-English users to see English fallback text for these settings.</violation>
</file>
<file name="build-evshim.ps1">
<violation number="1" location="build-evshim.ps1:16">
P2: Hardcoded native compile API level (29) may be incompatible with lower-minSdk variants that load this shared library.</violation>
<violation number="2" location="build-evshim.ps1:48">
P2: Strip step lacks exit-code checking, so script can silently report success on failure</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| } | ||
| handler.setPreferredInputApi(PreferredInputApi.values()[container.inputType]) | ||
| handler.setDInputMapperType(container.dinputMapperType) | ||
| handler.setVibrationMode( |
Contributor
There was a problem hiding this comment.
P2: Per-game vibration settings are only initialized inside if (PluviaApp.xEnvironment == null), so they are lost when a new WinHandler is created during activity recreation while a game is still running.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt, line 1921:
<comment>Per-game vibration settings are only initialized inside `if (PluviaApp.xEnvironment == null)`, so they are lost when a new WinHandler is created during activity recreation while a game is still running.</comment>
<file context>
@@ -1918,6 +1918,12 @@ fun XServerScreen(
}
handler.setPreferredInputApi(PreferredInputApi.values()[container.inputType])
handler.setDInputMapperType(container.dinputMapperType)
+ handler.setVibrationMode(
+ PrefManager.normalizeVibrationModeInput(
+ container.getExtra("vibrationMode", "controller"),
</file context>
| stringResource(R.string.vibration_mode_option_controller), | ||
| stringResource(R.string.vibration_mode_option_device), | ||
| ) | ||
| val vibrationModeValues = listOf("off", "controller", "device") |
Contributor
There was a problem hiding this comment.
P2: Vibration mode string literals are duplicated in the UI instead of referencing a single source of truth, creating schema-drift risk across PrefManager, WinHandler, ContainerData, and XServerScreen.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/src/main/java/app/gamenative/ui/component/dialog/ControllerTab.kt, line 71:
<comment>Vibration mode string literals are duplicated in the UI instead of referencing a single source of truth, creating schema-drift risk across PrefManager, WinHandler, ContainerData, and XServerScreen.</comment>
<file context>
@@ -51,6 +63,40 @@ fun ControllerTabContent(state: ContainerConfigState, default: Boolean) {
+ stringResource(R.string.vibration_mode_option_controller),
+ stringResource(R.string.vibration_mode_option_device),
+ )
+ val vibrationModeValues = listOf("off", "controller", "device")
+ val vibrationModeIndex = vibrationModeValues.indexOf(normalizedVibrationMode).coerceAtLeast(0)
+ SettingsListDropdown(
</file context>
Suggested change
| val vibrationModeValues = listOf("off", "controller", "device") | |
| val vibrationModeValues = PrefManager.VIBRATION_MODE_VALUES |
| @@ -764,6 +764,14 @@ | |||
| <string name="shooter_mode_toggle_description">Auto-replace stick elements with dynamic joysticks</string> | |||
Contributor
There was a problem hiding this comment.
P2: New vibration UI strings are missing from locale-specific values-*/strings.xml files, causing non-English users to see English fallback text for these settings.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/src/main/res/values/strings.xml, line 768:
<comment>New vibration UI strings are missing from locale-specific `values-*/strings.xml` files, causing non-English users to see English fallback text for these settings.</comment>
<file context>
@@ -764,6 +764,14 @@
<string name="shooter_mode_on">Enable Mouse</string>
<string name="shooter_mode_off">Enable Sticks</string>
+
+ <!-- Vibration Settings -->
+ <string name="vibration_mode">Vibration Mode</string>
+ <string name="vibration_intensity">Vibration Intensity</string>
</file context>
| # Any installed NDK with 16 KB page-size support works; this is the one used to | ||
| # produce the committed .so. | ||
| $NDK_VERSION = "26.1.10909125" | ||
| $API = 29 # matches the modern flavor's minSdk |
Contributor
There was a problem hiding this comment.
P2: Hardcoded native compile API level (29) may be incompatible with lower-minSdk variants that load this shared library.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At build-evshim.ps1, line 16:
<comment>Hardcoded native compile API level (29) may be incompatible with lower-minSdk variants that load this shared library.</comment>
<file context>
@@ -0,0 +1,50 @@
+# Any installed NDK with 16 KB page-size support works; this is the one used to
+# produce the committed .so.
+$NDK_VERSION = "26.1.10909125"
+$API = 29 # matches the modern flavor's minSdk
+
+if ($env:ANDROID_SDK_ROOT) { $SDK_ROOT = $env:ANDROID_SDK_ROOT }
</file context>
| & $CLANG @clangArgs | ||
| if ($LASTEXITCODE -ne 0) { Write-Error "Compilation failed (exit $LASTEXITCODE)" } | ||
|
|
||
| & $STRIP --strip-unneeded $OUT |
Contributor
There was a problem hiding this comment.
P2: Strip step lacks exit-code checking, so script can silently report success on failure
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At build-evshim.ps1, line 48:
<comment>Strip step lacks exit-code checking, so script can silently report success on failure</comment>
<file context>
@@ -0,0 +1,50 @@
+& $CLANG @clangArgs
+if ($LASTEXITCODE -ne 0) { Write-Error "Compilation failed (exit $LASTEXITCODE)" }
+
+& $STRIP --strip-unneeded $OUT
+
+Write-Host "OK -> $OUT"
</file context>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
app/src/main/cpp/evshim/evshim.c (1)
291-309: 💤 Low valueInfinite keepalive loop with no shutdown mechanism.
The
rumble_keepalivethread runs an unboundedfor(;;)loop (line 295) and is detached (line 373), so it cannot be joined or signaled to stop. While the OS will reclaim the thread when the Wine process exits, the lack of a clean shutdown creates a window where the thread may accessshm[]after it has been unmapped by other cleanup paths.♻️ Optional: Add shutdown signal
Introduce a global shutdown flag:
+static volatile int g_shutdown = 0; + static void *rumble_keepalive(void *arg) { (void)arg; for (;;) { + if (g_shutdown) break; usleep(RUMBLE_KEEPALIVE_US);And expose a JNI function to set it:
+JNIEXPORT void JNICALL +Java_com_winlator_winhandler_WinHandler_shutdownKeepalive(JNIEnv *env, jclass cls) +{ + g_shutdown = 1; +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/cpp/evshim/evshim.c` around lines 291 - 309, The rumble_keepalive thread uses an infinite loop and is detached, so add a global atomic/shutdown flag (e.g., volatile sig_atomic_t or atomic_bool named rumble_keepalive_shutdown) and check it inside rumble_keepalive's loop to break out cleanly; update the loop condition from for(;;) to while(!rumble_keepalive_shutdown) (or check and break after usleep) and ensure any access to shm[]/vjoy_handles is guarded by that flag to avoid use-after-unmap. Also add and export a small setter function (e.g., evshim_request_rumble_shutdown or JNI-exported method) that sets rumble_keepalive_shutdown = true during cleanup so the detached thread can exit before shared memory is released; leave t_keepalive_active semantics unchanged but ensure cleanup sequences set the shutdown flag before unmapping shm[].build-evshim.ps1 (2)
13-16: 💤 Low valueConsider documenting the NDK version requirement more prominently.
The script requires a specific NDK version (26.1.10909125) which may not be installed on all developer machines. The error message on line 31 mentions this, but developers discovering the script for the first time might miss the requirement.
📝 Optional: Add version check guidance to header comment
# build-evshim.ps1 # Rebuilds app/src/main/cpp/evshim/evshim.c into libevshim.so using the Android # NDK clang toolchain, then strips it and copies it into jniLibs. # +# Prerequisites: +# - Android NDK 26.1.10909125 (or edit $NDK_VERSION below for an installed version) +# - Windows OS (uses windows-x86_64 prebuilt toolchain) +# # SDL functions are resolved at runtime via dlsym(), so only the minimal SDL type🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@build-evshim.ps1` around lines 13 - 16, The script hardcodes NDK_VERSION = "26.1.10909125" and API = 29 but the requirement is only mentioned later; update the top-of-file header comment to prominently state the required NDK version (26.1.10909125), the reason (16 KB page-size support) and how to install or select it, and optionally add a small runtime validation that reads the installed NDK/version and compares it to NDK_VERSION (emitting a clear error advising installation or selection of the required NDK) so developers won’t miss the requirement when first opening the script.
23-23: ⚖️ Poor tradeoffWindows-only toolchain path limits cross-platform builds.
The script hardcodes
windows-x86_64in the toolchain path (line 23), preventing use on Linux or macOS. Developers on those platforms would need to manually modify the script or use a different build approach.♻️ Optional: Detect host OS for cross-platform support
+$OS_ARCH = if ($IsLinux) { "linux-x86_64" } + elseif ($IsMacOS) { "darwin-x86_64" } + else { "windows-x86_64" } + $NDK_ROOT = "$SDK_ROOT\ndk\$NDK_VERSION" -$CLANG = "$NDK_ROOT\toolchains\llvm\prebuilt\windows-x86_64\bin\aarch64-linux-android$API-clang.cmd" -$STRIP = "$NDK_ROOT\toolchains\llvm\prebuilt\windows-x86_64\bin\llvm-strip.exe" +$CLANG = "$NDK_ROOT\toolchains\llvm\prebuilt\$OS_ARCH\bin\aarch64-linux-android$API-clang$(if ($IsWindows) {'.cmd'} else {''})" +$STRIP = "$NDK_ROOT\toolchains\llvm\prebuilt\$OS_ARCH\bin\llvm-strip$(if ($IsWindows) {'.exe'} else {''})"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@build-evshim.ps1` at line 23, The build script hardcodes the toolchain host folder in the $CLANG path which breaks non-Windows builds; update build-evshim.ps1 to detect the host OS (use PowerShell's automatic variables like $IsWindows, $IsLinux, $IsMacOS or [System.Runtime.InteropServices.RuntimeInformation] if needed), compute a host tag variable (e.g. $HOST_TAG = 'windows-x86_64' / 'linux-x86_64' / 'darwin-x86_64') and substitute it into the $CLANG assignment (replace the hardcoded "windows-x86_64" with $HOST_TAG) so $CLANG is correct across platforms.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/main/cpp/evshim/evshim.c`:
- Around line 300-301: The rumble_keepalive function is performing non-atomic
reads of shm[i]->state.low_freq_rumble and high_freq_rumble while OnRumble
writes them and issues a seq_cst fence, which is a data race under C11; fix it
by adding an acquire ordering before reading the rumble fields in
rumble_keepalive (e.g., call atomic_thread_fence(memory_order_acquire) or use
atomic_load_explicit on the shared state) so the reads synchronize with the
seq_cst fence in OnRumble and guarantee visibility of the latest
low_freq_rumble/high_freq_rumble values from shm.
- Around line 291-309: The rumble_keepalive thread reads vjoy_handles[]
concurrently with vjoy_updater writing it, causing a race; fix by preventing the
keepalive loop from accessing vjoy_handles until initialization completes —
e.g., in rumble_keepalive add a short startup wait or loop that sleeps until an
initialization flag set by vjoy_updater is true (use a simple volatile or atomic
bool like vjoy_initialized) before entering the main for(;;) loop, or
alternatively make vjoy_handles stores/loads atomic (change vjoy_handles to
atomic pointers, store in vjoy_updater and load atomically in rumble_keepalive)
so p_SDL_JoystickRumble is only called with a valid js pointer.
In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt`:
- Line 1926: The call to handler.setVibrationIntensity uses
container.getExtra(...).toIntOrNull() ?: 100 without bounding the value,
allowing out-of-range inputs; update the call site to parse the int and clamp it
into 0..100 (e.g., use Kotlin's coerceIn) before passing to
setVibrationIntensity so values like -1 or 500 are constrained to the valid
range; locate this in XServerScreen where container.getExtra(...) and
handler.setVibrationIntensity(...) are used and apply the clamp to the parsed
integer.
In `@app/src/main/java/com/winlator/winhandler/WinHandler.java`:
- Around line 869-873: resolveInputDevice currently uses currentControllerId
which can be stale; change it to derive the InputDevice from the live controller
object (currentController) instead: if currentController is null return null,
otherwise use the controller's device id/method to fetch the InputDevice (e.g.,
InputDevice.getDevice(currentController.getId()) or equivalent) so rumble/cancel
targets the active controller; ensure setCurrentController remains the single
source of truth for currentController and remove reliance on currentControllerId
inside resolveInputDevice.
- Around line 413-421: stop() currently tears down threads but doesn't abort any
in-flight one-shot vibration; ensure you explicitly cancel active rumble before
joining threads by invoking a cancel/stop routine (e.g., extend or use
rumbleTeardown to send an immediate "stop vibration" command or add a
cancelRumble() method) and call that immediately at the start of stop() (before
interrupt/join of rumbleKeepaliveThread and rumblePollerThread) so the device
won't continue vibrating after handler shutdown; update rumbleTeardown or add
cancelRumble() to emit the device stop command and ensure stop() calls it.
- Around line 927-938: The code sets isRumbling true before actually starting
the rumble path, causing false positives if vibrateController/vibrateDevice
fails; change startVibration (synchronized on rumbleLock) to only set isRumbling
after a successful start: call vibrateDevice(lowFreq, highFreq) or
vibrateController(lowFreq, highFreq) based on vibrationMode, check their
return/status (or catch exceptions) to confirm the route started, and then set
isRumbling = true; on failure, call stopVibration() and leave isRumbling false
so the keepalive thread won’t repeatedly retry. Ensure you update logic around
startVibration, vibrateController, vibrateDevice, stopVibration and isRumbling
accordingly.
---
Nitpick comments:
In `@app/src/main/cpp/evshim/evshim.c`:
- Around line 291-309: The rumble_keepalive thread uses an infinite loop and is
detached, so add a global atomic/shutdown flag (e.g., volatile sig_atomic_t or
atomic_bool named rumble_keepalive_shutdown) and check it inside
rumble_keepalive's loop to break out cleanly; update the loop condition from
for(;;) to while(!rumble_keepalive_shutdown) (or check and break after usleep)
and ensure any access to shm[]/vjoy_handles is guarded by that flag to avoid
use-after-unmap. Also add and export a small setter function (e.g.,
evshim_request_rumble_shutdown or JNI-exported method) that sets
rumble_keepalive_shutdown = true during cleanup so the detached thread can exit
before shared memory is released; leave t_keepalive_active semantics unchanged
but ensure cleanup sequences set the shutdown flag before unmapping shm[].
In `@build-evshim.ps1`:
- Around line 13-16: The script hardcodes NDK_VERSION = "26.1.10909125" and API
= 29 but the requirement is only mentioned later; update the top-of-file header
comment to prominently state the required NDK version (26.1.10909125), the
reason (16 KB page-size support) and how to install or select it, and optionally
add a small runtime validation that reads the installed NDK/version and compares
it to NDK_VERSION (emitting a clear error advising installation or selection of
the required NDK) so developers won’t miss the requirement when first opening
the script.
- Line 23: The build script hardcodes the toolchain host folder in the $CLANG
path which breaks non-Windows builds; update build-evshim.ps1 to detect the host
OS (use PowerShell's automatic variables like $IsWindows, $IsLinux, $IsMacOS or
[System.Runtime.InteropServices.RuntimeInformation] if needed), compute a host
tag variable (e.g. $HOST_TAG = 'windows-x86_64' / 'linux-x86_64' /
'darwin-x86_64') and substitute it into the $CLANG assignment (replace the
hardcoded "windows-x86_64" with $HOST_TAG) so $CLANG is correct across
platforms.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 24bef65d-bc3e-49a1-8c39-0e100a90af6f
⛔ Files ignored due to path filters (1)
app/src/main/jniLibs/arm64-v8a/libevshim.sois excluded by!**/*.so
📒 Files selected for processing (10)
app/src/main/cpp/evshim/evshim.capp/src/main/cpp/evshim/sdl2_stub/SDL2/SDL.happ/src/main/java/app/gamenative/PrefManager.ktapp/src/main/java/app/gamenative/ui/component/dialog/ControllerTab.ktapp/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.ktapp/src/main/java/app/gamenative/utils/ContainerUtils.ktapp/src/main/java/com/winlator/container/ContainerData.ktapp/src/main/java/com/winlator/winhandler/WinHandler.javaapp/src/main/res/values/strings.xmlbuild-evshim.ps1
Comment on lines
+291
to
+309
| // Re-arms SDL's rumble before its internal expiry timer can fire a false OnRumble(0,0). | ||
| static void *rumble_keepalive(void *arg) | ||
| { | ||
| (void)arg; | ||
| for (;;) { | ||
| usleep(RUMBLE_KEEPALIVE_US); | ||
| for (int i = 0; i < MAX_GAMEPADS; i++) { | ||
| SDL_Joystick *js = vjoy_handles[i]; | ||
| if (!js || !shm[i]) continue; | ||
| uint16_t lo = shm[i]->state.low_freq_rumble; | ||
| uint16_t hi = shm[i]->state.high_freq_rumble; | ||
| if (lo == 0 && hi == 0) continue; | ||
| t_keepalive_active = 1; | ||
| p_SDL_JoystickRumble(js, lo, hi, RUMBLE_KEEPALIVE_DUR_MS); | ||
| t_keepalive_active = 0; | ||
| } | ||
| } | ||
| return NULL; | ||
| } |
Contributor
There was a problem hiding this comment.
Race on vjoy_handles[] between initialization and keepalive reads.
rumble_keepalive (line 298) reads vjoy_handles[i] concurrently with vjoy_updater writing it (line 250) with no synchronization. If the keepalive thread starts before vjoy_updater completes initialization, it may read NULL or a partially-written pointer, leading to a null-pointer dereference or invalid rumble calls.
🔒 Proposed fix: Add memory barrier or delay keepalive start
Option 1 (preferred): Add a small startup delay to rumble_keepalive
static void *rumble_keepalive(void *arg)
{
(void)arg;
+ // Wait for vjoy_updater threads to complete initialization
+ usleep(RUMBLE_KEEPALIVE_US);
for (;;) {
usleep(RUMBLE_KEEPALIVE_US);Option 2: Use atomic store/load for vjoy_handles[]
Change the declaration:
-static SDL_Joystick *vjoy_handles[MAX_GAMEPADS];
+static _Atomic(SDL_Joystick *) vjoy_handles[MAX_GAMEPADS];Update the store in vjoy_updater:
- vjoy_handles[idx] = js;
+ atomic_store_explicit(&vjoy_handles[idx], js, memory_order_release);Update the load in rumble_keepalive:
- SDL_Joystick *js = vjoy_handles[i];
+ SDL_Joystick *js = atomic_load_explicit(&vjoy_handles[i], memory_order_acquire);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Re-arms SDL's rumble before its internal expiry timer can fire a false OnRumble(0,0). | |
| static void *rumble_keepalive(void *arg) | |
| { | |
| (void)arg; | |
| for (;;) { | |
| usleep(RUMBLE_KEEPALIVE_US); | |
| for (int i = 0; i < MAX_GAMEPADS; i++) { | |
| SDL_Joystick *js = vjoy_handles[i]; | |
| if (!js || !shm[i]) continue; | |
| uint16_t lo = shm[i]->state.low_freq_rumble; | |
| uint16_t hi = shm[i]->state.high_freq_rumble; | |
| if (lo == 0 && hi == 0) continue; | |
| t_keepalive_active = 1; | |
| p_SDL_JoystickRumble(js, lo, hi, RUMBLE_KEEPALIVE_DUR_MS); | |
| t_keepalive_active = 0; | |
| } | |
| } | |
| return NULL; | |
| } | |
| // Re-arms SDL's rumble before its internal expiry timer can fire a false OnRumble(0,0). | |
| static void *rumble_keepalive(void *arg) | |
| { | |
| (void)arg; | |
| // Wait for vjoy_updater threads to complete initialization | |
| usleep(RUMBLE_KEEPALIVE_US); | |
| for (;;) { | |
| usleep(RUMBLE_KEEPALIVE_US); | |
| for (int i = 0; i < MAX_GAMEPADS; i++) { | |
| SDL_Joystick *js = vjoy_handles[i]; | |
| if (!js || !shm[i]) continue; | |
| uint16_t lo = shm[i]->state.low_freq_rumble; | |
| uint16_t hi = shm[i]->state.high_freq_rumble; | |
| if (lo == 0 && hi == 0) continue; | |
| t_keepalive_active = 1; | |
| p_SDL_JoystickRumble(js, lo, hi, RUMBLE_KEEPALIVE_DUR_MS); | |
| t_keepalive_active = 0; | |
| } | |
| } | |
| return NULL; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/main/cpp/evshim/evshim.c` around lines 291 - 309, The
rumble_keepalive thread reads vjoy_handles[] concurrently with vjoy_updater
writing it, causing a race; fix by preventing the keepalive loop from accessing
vjoy_handles until initialization completes — e.g., in rumble_keepalive add a
short startup wait or loop that sleeps until an initialization flag set by
vjoy_updater is true (use a simple volatile or atomic bool like
vjoy_initialized) before entering the main for(;;) loop, or alternatively make
vjoy_handles stores/loads atomic (change vjoy_handles to atomic pointers, store
in vjoy_updater and load atomically in rumble_keepalive) so p_SDL_JoystickRumble
is only called with a valid js pointer.
Comment on lines
+300
to
+301
| uint16_t lo = shm[i]->state.low_freq_rumble; | ||
| uint16_t hi = shm[i]->state.high_freq_rumble; |
Contributor
There was a problem hiding this comment.
Concurrent reads of shm rumble state without acquire semantics.
rumble_keepalive reads shm[i]->state.low_freq_rumble and high_freq_rumble (lines 300-301) concurrently with OnRumble writing them (lines 205-206). While OnRumble issues a seq_cst fence on line 209, rumble_keepalive does not use an acquire load, so it may see stale rumble values on weakly-ordered architectures.
In practice, the impact is low—stale values cause at most a 500ms delay before the next refresh—but the lack of explicit ordering is a data race under the C11 memory model.
🛡️ Proposed fix: Add acquire fence before reading rumble state
for (int i = 0; i < MAX_GAMEPADS; i++) {
SDL_Joystick *js = vjoy_handles[i];
if (!js || !shm[i]) continue;
+ atomic_thread_fence(memory_order_acquire);
uint16_t lo = shm[i]->state.low_freq_rumble;
uint16_t hi = shm[i]->state.high_freq_rumble;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| uint16_t lo = shm[i]->state.low_freq_rumble; | |
| uint16_t hi = shm[i]->state.high_freq_rumble; | |
| for (int i = 0; i < MAX_GAMEPADS; i++) { | |
| SDL_Joystick *js = vjoy_handles[i]; | |
| if (!js || !shm[i]) continue; | |
| atomic_thread_fence(memory_order_acquire); | |
| uint16_t lo = shm[i]->state.low_freq_rumble; | |
| uint16_t hi = shm[i]->state.high_freq_rumble; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/main/cpp/evshim/evshim.c` around lines 300 - 301, The
rumble_keepalive function is performing non-atomic reads of
shm[i]->state.low_freq_rumble and high_freq_rumble while OnRumble writes them
and issues a seq_cst fence, which is a data race under C11; fix it by adding an
acquire ordering before reading the rumble fields in rumble_keepalive (e.g.,
call atomic_thread_fence(memory_order_acquire) or use atomic_load_explicit on
the shared state) so the reads synchronize with the seq_cst fence in OnRumble
and guarantee visibility of the latest low_freq_rumble/high_freq_rumble values
from shm.
| container.getExtra("vibrationMode", "controller"), | ||
| ), | ||
| ) | ||
| handler.setVibrationIntensity(container.getExtra("vibrationIntensity", "100").toIntOrNull() ?: 100) |
Contributor
There was a problem hiding this comment.
Clamp vibration intensity before passing to WinHandler.
Line 1926 parses an integer but does not bound it to 0..100. Invalid legacy/imported extras (for example -1 or 500) can bypass UI limits and create out-of-range runtime behavior. Clamp at this call site to keep launch-time behavior consistent.
Proposed fix
- handler.setVibrationIntensity(container.getExtra("vibrationIntensity", "100").toIntOrNull() ?: 100)
+ val vibrationIntensity = (container.getExtra("vibrationIntensity", "100").toIntOrNull() ?: 100)
+ .coerceIn(0, 100)
+ handler.setVibrationIntensity(vibrationIntensity)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| handler.setVibrationIntensity(container.getExtra("vibrationIntensity", "100").toIntOrNull() ?: 100) | |
| val vibrationIntensity = (container.getExtra("vibrationIntensity", "100").toIntOrNull() ?: 100) | |
| .coerceIn(0, 100) | |
| handler.setVibrationIntensity(vibrationIntensity) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt` at line
1926, The call to handler.setVibrationIntensity uses
container.getExtra(...).toIntOrNull() ?: 100 without bounding the value,
allowing out-of-range inputs; update the call site to parse the int and clamp it
into 0..100 (e.g., use Kotlin's coerceIn) before passing to
setVibrationIntensity so values like -1 or 500 are constrained to the valid
range; locate this in XServerScreen where container.getExtra(...) and
handler.setVibrationIntensity(...) are used and apply the clamp to the parsed
integer.
Comment on lines
413
to
+421
| public void stop() { | ||
| this.running = false; | ||
| rumbleTeardown(0); | ||
| if (rumbleKeepaliveThread != null) rumbleKeepaliveThread.interrupt(); | ||
| try { | ||
| if (rumblePollerThread != null) | ||
| this.rumblePollerThread.join(); | ||
| if (rumbleKeepaliveThread != null) | ||
| this.rumbleKeepaliveThread.join(); |
Contributor
There was a problem hiding this comment.
Cancel any active rumble during shutdown.
stop() tears down the threads, but it never cancels the in-flight vibration effect itself. In device mode that leaves the phone vibrating for the remainder of the 60s one-shot after the handler stops.
💡 Suggested fix
public void stop() {
this.running = false;
+ stopVibration();
rumbleTeardown(0);
if (rumbleKeepaliveThread != null) rumbleKeepaliveThread.interrupt();
try {
if (rumblePollerThread != null)
this.rumblePollerThread.join();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public void stop() { | |
| this.running = false; | |
| rumbleTeardown(0); | |
| if (rumbleKeepaliveThread != null) rumbleKeepaliveThread.interrupt(); | |
| try { | |
| if (rumblePollerThread != null) | |
| this.rumblePollerThread.join(); | |
| if (rumbleKeepaliveThread != null) | |
| this.rumbleKeepaliveThread.join(); | |
| public void stop() { | |
| this.running = false; | |
| stopVibration(); | |
| rumbleTeardown(0); | |
| if (rumbleKeepaliveThread != null) rumbleKeepaliveThread.interrupt(); | |
| try { | |
| if (rumblePollerThread != null) | |
| this.rumblePollerThread.join(); | |
| if (rumbleKeepaliveThread != null) | |
| this.rumbleKeepaliveThread.join(); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/main/java/com/winlator/winhandler/WinHandler.java` around lines 413 -
421, stop() currently tears down threads but doesn't abort any in-flight
one-shot vibration; ensure you explicitly cancel active rumble before joining
threads by invoking a cancel/stop routine (e.g., extend or use rumbleTeardown to
send an immediate "stop vibration" command or add a cancelRumble() method) and
call that immediately at the start of stop() (before interrupt/join of
rumbleKeepaliveThread and rumblePollerThread) so the device won't continue
vibrating after handler shutdown; update rumbleTeardown or add cancelRumble() to
emit the device stop command and ensure stop() calls it.
Comment on lines
+869
to
+873
| /** Resolves the physical InputDevice currently driving gamepad input, or null if none. */ | ||
| private InputDevice resolveInputDevice() { | ||
| if (currentControllerId < 0) return null; | ||
| return InputDevice.getDevice(currentControllerId); | ||
| } |
Contributor
There was a problem hiding this comment.
Resolve the rumble target from currentController, not the side-channel ID.
This helper ignores the controller object the class actually maintains. currentController is reassigned in this file, but currentControllerId is only updated through setCurrentController(...), so controller-mode rumble/cancel can target null or a stale device even while input is flowing through the new controller.
💡 Suggested fix
private InputDevice resolveInputDevice() {
- if (currentControllerId < 0) return null;
- return InputDevice.getDevice(currentControllerId);
+ ExternalController controller = currentController;
+ if (controller != null) {
+ return InputDevice.getDevice(controller.getDeviceId());
+ }
+ if (currentControllerId < 0) return null;
+ return InputDevice.getDevice(currentControllerId);
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/main/java/com/winlator/winhandler/WinHandler.java` around lines 869 -
873, resolveInputDevice currently uses currentControllerId which can be stale;
change it to derive the InputDevice from the live controller object
(currentController) instead: if currentController is null return null, otherwise
use the controller's device id/method to fetch the InputDevice (e.g.,
InputDevice.getDevice(currentController.getId()) or equivalent) so rumble/cancel
targets the active controller; ensure setCurrentController remains the single
source of truth for currentController and remove reliance on currentControllerId
inside resolveInputDevice.
Comment on lines
+927
to
+938
| private void startVibration(short lowFreq, short highFreq) { | ||
| synchronized (rumbleLock) { | ||
| if ("off".equals(vibrationMode) || vibrationIntensity == 0) { | ||
| stopVibration(); | ||
| return; | ||
| } | ||
| isRumbling = true; | ||
| if ("device".equals(vibrationMode)) { | ||
| vibrateDevice(lowFreq, highFreq); | ||
| } else { // "controller" | ||
| vibrateController(lowFreq, highFreq); | ||
| } |
Contributor
There was a problem hiding this comment.
Only mark rumble active after the route succeeds.
isRumbling is set before the controller/device path is known to have started. If vibrateController(...) fails, the keepalive thread keeps retrying every cycle and will spam warnings until the game sends 0,0.
💡 Suggested fix
private void startVibration(short lowFreq, short highFreq) {
synchronized (rumbleLock) {
if ("off".equals(vibrationMode) || vibrationIntensity == 0) {
stopVibration();
return;
}
- isRumbling = true;
- if ("device".equals(vibrationMode)) {
- vibrateDevice(lowFreq, highFreq);
- } else { // "controller"
- vibrateController(lowFreq, highFreq);
- }
+ boolean started;
+ if ("device".equals(vibrationMode)) {
+ vibrateDevice(lowFreq, highFreq);
+ started = true;
+ } else { // "controller"
+ started = vibrateController(lowFreq, highFreq);
+ }
+ isRumbling = started;
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/main/java/com/winlator/winhandler/WinHandler.java` around lines 927 -
938, The code sets isRumbling true before actually starting the rumble path,
causing false positives if vibrateController/vibrateDevice fails; change
startVibration (synchronized on rumbleLock) to only set isRumbling after a
successful start: call vibrateDevice(lowFreq, highFreq) or
vibrateController(lowFreq, highFreq) based on vibrationMode, check their
return/status (or catch exceptions) to confirm the route started, and then set
isRumbling = true; on failure, call stopVibration() and leave isRumbling false
so the keepalive thread won’t repeatedly retry. Ensure you update logic around
startVibration, vibrateController, vibrateDevice, stopVibration and isRumbling
accordingly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Adds per-game, user-selectable controller vibration (mode + intensity) on top of upstream's evshim, single-controller — and fixes rumble being capped at ~1 second.
A new Vibration dropdown (off / controller / device) and an intensity slider in the controller settings let each game route rumble to the physical pad's motors, to the phone's vibrator, or off, scaled 0–100%.
The ~1s cap is fixed too: SDL auto-expires a rumble after its duration and calls the virtual joystick's
OnRumble(0,0)itself, which evshim relayed to the app as a stop. That can't be fixed app-side (the value is zeroed at the source), so the fix lives inevshim.c.Scope: single controller only — multi-controller is intentionally out of scope (upstream's evshim is single-controller). Phone vibration is pure Java reading the rumble values already present in the futex shared memory, so there's no new shared-memory contract.
Changes
WinHandler.java—setVibrationMode/setVibrationIntensity+reconcileActiveRumble(applies a setting change to in-flight rumble); per-motorrumbleViaVibratorManager,vibrateDevice, andblendMotors(floors to ≥1 so a high-freq-only rumble isn't silenced on single-motor targets);VibrationAttributesgated to API 33 with anAudioAttributesfallback (minSdk 26). A keepalive thread refreshes the Android one-shot while the poller is blocked inwaitForRumble(controller refreshed frequently at 500 ms; the 60 s device one-shot only near expiry). All apply/cancel is serialized onrumbleLockacross the poller, keepalive, and UI threads.evshim.c— SDL rumble keepalive. A thread re-arms SDL with the live shm rumble every 500 ms so its internal expiry timer never fires the falseOnRumble(0,0), preserving XInput "set and forget". A thread-localt_keepalive_activeguard makes the synchronousOnRumblere-entry from our own re-arm a no-op so it isn't echoed back into shm.libevshim.sois rebuilt from this source (build-evshim.ps1+sdl2_stubtype stubs; 16 KB-aligned, stripped; JNI exports verified, SDL still resolved viadlsym).vibrationMode+vibrationIntensitypersisted inContainerData/PrefManager/ContainerUtils; mode dropdown + intensity slider inControllerTab; applied on launch viaXServerScreen.Testing
Built
:app:installModernDebugand verified on a Galaxy S25 Ultra with a physical XInput controller: controller-mode rumble now sustains for its full in-game duration (a 3 s effect is no longer cut to 1 s) and stops promptly when the game ends it; device mode buzzes the phone; intensity scales; off suppresses.libevshim.sorebuilt withbuild-evshim.ps1(NDK 26.1, 16 KB-aligned).Recording
https://photos.app.goo.gl/uRuX5h6xcWU9iZHf9
Type of Change
Checklist
#code-changes, I have discussed this change there and it has been green-lighted. If I do not have access, I have still provided clear context in this PR. If I skip both, I accept that this change may face delays in review, may not be reviewed at all, or may be closed.CONTRIBUTING.md.Summary by cubic
Adds per‑game vibration mode (off/controller/device) with intensity control and sustains rumble for its full in‑game duration. Fixes the ~1s rumble cutoff caused by SDL auto‑expiry.
New Features
ContainerData/PrefManagerand apply on launch inXServerScreen.VibratorManager(per‑motor when available); Device mode vibrates the phone; Off disables. Scope: single controller.WinHandler.setVibrationMode/setVibrationIntensityandreconcileActiveRumble; a keepalive thread refreshes one‑shots so long effects continue.Bug Fixes
evshim.cusingSDL_JoystickRumbleon a timer and guardingOnRumblere‑entry, preserving XInput “set and forget” without changing the shared‑memory contract.Written for commit 36e58fd. Summary will update on new commits.
Summary by CodeRabbit
Release Notes