Failed Assertion on Connected Joysticks Terminates EmulationStation on Specific Conditions

Lolonois

Describe the bug
On startup of EmulationStation during splash screen it is possible to force an assertion to fail. In consequence ES bails out.

How To Reproduce

Connect an (addtional) (USB-)joystick controller to the Rpi
Start EmulationStation
During splash screen, anytime before the progress shows Done. remove USB joystick
ES bails out with an failed assertion at this line
Expected: ES starts in to system carousel / usual UI

Reproducability: Always. While it might be a corner case for wired controllers it may be more relevant for wireless controllers losing connection during ES start.

Technical information

Tested with RPI4, 4GB, Meanwell power supply (set to 5.15V)
RetroPie-Setup commit 2431ec88 (master/HEAD on the time of writing)
Affected ES: 2.10.3 (and most likely earlier versions) and current development (86a0823)
libSDL-dev 2.0.10
Tested Joystick have been either already configured via ES or are new to ES config
The ThreadedLoading-switch in es_settings.cfg had no impact on result

Additional context

After some debugging I identified that a surplus SDL_JOYDEVICEADDED is put in the eventqueue on removing a joystick. Starting from main.cpp#431 this event will cause a call to the method InputManager::addJoystickByDeviceIndex() where the assertion will fail
Not tested on Windows build of ES

Proposed and verified mitigation

Add SDL_JOYDEVICEADDED to the events to be flushed before ES carousel is presented to user

mitu

Thanks for taking a look at this.

Add SDL_JOYDEVICEADDED to the events to be flushed before ES carousel is presented to user

Does this have any impact on detection when the controller is not disconnecting during startup ? Flushing this event may cause the device addition event to be missed and the device to not get initialized - I'll need to test this before we could add the suggested modification.

Lolonois

Thanks for challenging this. Yes, the proposed change created a regression: Any controller added when ES is ready for userinput where not recognized. :-/

However, I dug deeper and learned that not only the remove event was in the queue but also an stale add event with an -1 (for which in https://wiki.libsdl.org/SDL_JoyDeviceEvent), which causes the assertion to fail.

Thus, I propose a more granular change here.

You may wonder why I removed the device enumeration from the InputManager.cpp constructor: If one has only on configured controller and add it in the current implementation during the splash screen, you will get greeted with the message, that no controllers are found, albeit it is connected. This is why I postponed the joystick detection to the very end of the ES bootup, to mitigate also this effect.

With this change I ran this tests and all passed:

No.	Test	Pass	Remark
1.	USB device already present to ES on boot and not removed	y
2.	USB device added to ES while booting (before "Done.")	y
3.	USB device removed while booting ES (before "Done.")	y	This was the reported issue
4.	USB device removed/added multiple times while booting ES (before "Done.")	y	Simulate a flaky controller connection on ES boot
5.	USB device added during ES runs	y
6.	USB device removed during ES runs	y
7.	USB device added/removed multiple times during ES runs	y

Pass = y, means the controller is recognized in ES and can be used (or configured, if not configured yet).

Any Qs, let me know.

mitu

Why not move the initialization (init) at the very end - wouldn't this solve everything (plus we can get rid of the event flushing added in https://github.com/Gemba/EmulationStation/commit/fe650965a0544cc6aae97efb6a608e8b97ee7322) ? The other changes wouldn't be needed at all.

Lolonois

I thought so, too, as I always prefer a proper solution with less code. But the joystick information must be known to ES/InputManager before it is tested if any joysticks are configured.

However, I am open if there is a different approach or I misunderstood the ES startup sequence.

mitu

@Lolonois said in Failed Assertion on Connected Joysticks Terminates EmulationStation on Specific Conditions:

But the joystick information must be known to ES/InputManager before it is tested if any joysticks are configured.

The InputManager could be initialized outside of Window::init, right before the startup UI dialog is chosen. This modification would need to handle the InputManager init/deinit during launching a game/command (since it was handled by the Window class).

Another, simpler, solution would be to just replace the assert with just a log && return, so the crash wouldn't be triggered. I was used to it being used as a litmus test for broken/flailing connections.

Lolonois

Nice idea. Thanks, @mitu . I applied it and it causes no regression. Added a testcase to launch a game / return from game which was also ok. (See my updated branch in the repo for the suggested changes).

As the Window::init/deinit is only called on two places (main.cpp
and FileData.cpp) the trade-off between having to init/deinit the Inputmanager at two places and having less and cleaner code, the decision was on the latter.

I have left the assertion intact and also added a remove joystick message to the log (to have matching log-events on add/remove).

mitu

The changes looks fine, but I see some un-related modifications in InputManager::removeJoystickByJoystickID - what are they used for ?

Lolonois

Ah, yeah. Some leftovers from the initial suggested change. As there is with the new suggested change no more dangling SDL_JOYDEVICEREMOVED in the eventqueue I have reverted it.
However, if the joyId in this method is not valid as identifier for the different lists, something really odd must happened which I can not reproduce with the testcases above. I would even claim that the last if/else is surplus: If one of the iterator-find() results is NULL before this if/else EmulationStation will bail out anyway. It would be necessary and sufficient to only keep the if branch, which I did.

pjft

Just praising you two on working out a solution to this long-time flaky error. Well done both.

mitu

@pjft Thanks, but I was mostly cheerleading, @Lolonois did all the work :).

pjft

@mitu Never understate the importance of cheerleading :D

Lolonois

Whatever you point of view is on this. I like to have peers (you, the other maintainers and also forum members) which help to streamline a change or fix. It was fun to solve this with you. :)