RetroPie forum home
    • Recent
    • Tags
    • Popular
    • Home
    • Docs
    • Register
    • Login

    Failed Assertion on Connected Joysticks Terminates EmulationStation on Specific Conditions

    Scheduled Pinned Locked Moved Ideas and Development
    emulationstatiojoystickbug
    13 Posts 4 Posters 960 Views
    Loading More Posts
    • Oldest to Newest
    • Newest to Oldest
    • Most Votes
    Reply
    • Reply as topic
    Log in to reply
    This topic has been deleted. Only users with topic management privileges can see it.
    • mituM
      mitu Global Moderator @Lolonois
      last edited by

      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.

      LolonoisL 1 Reply Last reply Reply Quote 0
      • LolonoisL
        Lolonois @mitu
        last edited by

        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.

        1 Reply Last reply Reply Quote 0
        • mituM
          mitu Global Moderator
          last edited by

          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.

          1 Reply Last reply Reply Quote 0
          • LolonoisL
            Lolonois
            last edited by

            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.

            mituM 1 Reply Last reply Reply Quote 0
            • mituM
              mitu Global Moderator @Lolonois
              last edited by

              @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.

              LolonoisL 1 Reply Last reply Reply Quote 0
              • LolonoisL
                Lolonois @mitu
                last edited by

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

                1 Reply Last reply Reply Quote 1
                • mituM
                  mitu Global Moderator
                  last edited by

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

                  LolonoisL 1 Reply Last reply Reply Quote 0
                  • LolonoisL
                    Lolonois @mitu
                    last edited by

                    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.

                    1 Reply Last reply Reply Quote 0
                    • pjftP
                      pjft
                      last edited by

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

                      mituM 1 Reply Last reply Reply Quote 1
                      • mituM
                        mitu Global Moderator @pjft
                        last edited by

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

                        pjftP 1 Reply Last reply Reply Quote 1
                        • pjftP
                          pjft @mitu
                          last edited by

                          @mitu Never understate the importance of cheerleading :D

                          1 Reply Last reply Reply Quote 1
                          • LolonoisL
                            Lolonois
                            last edited by

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

                            1 Reply Last reply Reply Quote 1
                            • First post
                              Last post

                            Contributions to the project are always appreciated, so if you would like to support us with a donation you can do so here.

                            Hosting provided by Mythic-Beasts. See the Hosting Information page for more information.