fix(input): support controller discovery and hotplug - #3
Conversation
WalkthroughA dedicated SDL2 controller backend now handles dynamic loading, controller discovery, polling, input queries, and shutdown. ChangesSDL controller backend integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Input as s3e_input
participant Backend as sdl_controller
participant SDL2
participant Controllers
Input->>Backend: sdl_controller_update(now_ms)
Backend->>SDL2: Pump and update events
Backend->>Controllers: Scan and refresh mapped controllers
Input->>Backend: Query buttons, axes, and hat mask
Backend-->>Input: Return controller state
Input->>Backend: sdl_controller_shutdown()
Backend->>Controllers: Close opened controllers
Backend->>SDL2: Quit subsystem and unload library
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/s3e_input.c`:
- Around line 539-553: Update the controller handling around
sdl_controller_update so the no-controller case only performs release and
state-reset actions when had_controller indicates a disconnect, without jumping
to out. Allow the generic input update block, including input_update_cursor,
input_update_game_touchpads, and input_update_game_keys, to run even when no SDL
controller is connected; retain controller-specific hat processing only for
connected controllers.
In `@src/sdl_controller.c`:
- Around line 299-316: Add a concise comment to sdl_controller_hat_mask
documenting that it ORs hat bits across all opened controllers and hats, so
opposing directions may produce combined states; do not change the existing
aggregation behavior.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: cb34347b-a20e-4528-97cb-db8ea1343c92
📒 Files selected for processing (4)
Makefileinclude/sdl_controller.hsrc/s3e_input.csrc/sdl_controller.c
| int had_controller = sdl_controller_connected(); | ||
| uint64_t now = monotonic_ms(); | ||
| sdl_controller_update(now); | ||
| if (!sdl_controller_connected()) { | ||
| if (had_controller) { | ||
| input_release_pointer(); | ||
| touchpad_release_all(); | ||
| keyboard_release_all(); | ||
| g_prev_select = 0; | ||
| g_prev_a = 0; | ||
| } | ||
| goto out; | ||
| } | ||
| g_sdl.GameControllerUpdate(); | ||
| g_hat_mask = input_current_hat_mask(); | ||
| g_hat_mask = sdl_controller_hat_mask(); | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Touchpad/keyboard pump is skipped entirely whenever no controller is connected.
goto out fires whenever sdl_controller_connected() is false — not just on a disconnect transition — bypassing the whole cursor/touchpad/keyboard update block (lines 554-591) any time no SDL game controller is present. Since that block (input_update_cursor, input_update_game_touchpads, input_update_game_keys) has no controller dependency, this means touch and keyboard input stop working entirely whenever there's no controller attached (the common case for controller-less sessions), not only on disconnect.
🐛 Proposed fix: only gate the release-on-disconnect logic, let the generic pump always run
int had_controller = sdl_controller_connected();
uint64_t now = monotonic_ms();
sdl_controller_update(now);
- if (!sdl_controller_connected()) {
- if (had_controller) {
- input_release_pointer();
- touchpad_release_all();
- keyboard_release_all();
- g_prev_select = 0;
- g_prev_a = 0;
- }
- goto out;
- }
- g_hat_mask = sdl_controller_hat_mask();
+ int has_controller = sdl_controller_connected();
+ if (had_controller && !has_controller) {
+ input_release_pointer();
+ touchpad_release_all();
+ keyboard_release_all();
+ g_prev_select = 0;
+ g_prev_a = 0;
+ }
+ g_hat_mask = has_controller ? sdl_controller_hat_mask() : 0;📝 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.
| int had_controller = sdl_controller_connected(); | |
| uint64_t now = monotonic_ms(); | |
| sdl_controller_update(now); | |
| if (!sdl_controller_connected()) { | |
| if (had_controller) { | |
| input_release_pointer(); | |
| touchpad_release_all(); | |
| keyboard_release_all(); | |
| g_prev_select = 0; | |
| g_prev_a = 0; | |
| } | |
| goto out; | |
| } | |
| g_sdl.GameControllerUpdate(); | |
| g_hat_mask = input_current_hat_mask(); | |
| g_hat_mask = sdl_controller_hat_mask(); | |
| int had_controller = sdl_controller_connected(); | |
| uint64_t now = monotonic_ms(); | |
| sdl_controller_update(now); | |
| int has_controller = sdl_controller_connected(); | |
| if (had_controller && !has_controller) { | |
| input_release_pointer(); | |
| touchpad_release_all(); | |
| keyboard_release_all(); | |
| g_prev_select = 0; | |
| g_prev_a = 0; | |
| } | |
| g_hat_mask = has_controller ? sdl_controller_hat_mask() : 0; |
🤖 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 `@src/s3e_input.c` around lines 539 - 553, Update the controller handling
around sdl_controller_update so the no-controller case only performs release and
state-reset actions when had_controller indicates a disconnect, without jumping
to out. Allow the generic input update block, including input_update_cursor,
input_update_game_touchpads, and input_update_game_keys, to run even when no SDL
controller is connected; retain controller-specific hat processing only for
connected controllers.
| uint8_t sdl_controller_hat_mask(void) { | ||
| uint8_t mask = 0; | ||
| if (!g_sdl.JoystickNumHats || !g_sdl.JoystickGetHat) { | ||
| return 0; | ||
| } | ||
|
|
||
| for (size_t controller = 0; controller < g_controller_count; ++controller) { | ||
| void *joystick = g_controllers[controller].joystick; | ||
| if (!joystick) { | ||
| continue; | ||
| } | ||
| int count = g_sdl.JoystickNumHats(joystick); | ||
| for (int hat = 0; hat < count; ++hat) { | ||
| mask |= g_sdl.JoystickGetHat(joystick, hat); | ||
| } | ||
| } | ||
| return mask; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
Hat mask ORs bits across all controllers/hats into one value.
If more than one controller is opened (or a single controller exposes multiple hats), opposing directions from different sources get OR'd together into a single mask, producing combined states (e.g. up+down) that don't correspond to any single physical input. Likely a non-issue for the common single-controller case, but worth a comment noting the aggregation semantics for future multi-controller support.
🧰 Tools
🪛 Cppcheck (2.21.0)
[style] 299-299: The function 'sdl_controller_hat_mask' is never used.
(unusedFunction)
🤖 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 `@src/sdl_controller.c` around lines 299 - 316, Add a concise comment to
sdl_controller_hat_mask documenting that it ORs hat bits across all opened
controllers and hats, so opposing directions may produce combined states; do not
change the existing aggregation behavior.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes