Failed Assertion on Connected Joysticks Terminates EmulationStation on Specific Conditions

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. :)