Failed Assertion on Connected Joysticks Terminates EmulationStation on Specific Conditions

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