Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Amiberry segfaults whenever launched with Bluetooth WiiMote connected. #542

Closed
rickwookie opened this issue Nov 26, 2019 · 46 comments
Closed
Assignees
Labels

Comments

@rickwookie
Copy link

rickwookie commented Nov 26, 2019

After trying to create a custom control within Amiberry so that I could assign J1 Fire button to one of the buttons on my WiiMote, now whenever I try to launch Amiberry while a WiiMote is connected, it fails with the following in runcommand.log:
Parameters: Executing: bash "/home/pi/RetroPie/roms/amiga/+Start Amiberry.sh" INFO: --- New exception --- INFO: JIT not in use. INFO: Segmentation Fault INFO: info.si_signo = 11 INFO: info.si_errno = 0 INFO: info.si_code = -6 INFO: info.si_addr = 00001288 INFO: r0 = 0x00000000 INFO: r1 = 0xbef6650c INFO: r2 = 0x00000000 INFO: r3 = 0x00000008 INFO: r4 = 0xb6fc4968 INFO: r5 = 0x0000000b INFO: r6 = 0xbef6650c INFO: r7 = 0x000000af INFO: r8 = 0x040f0fa4 INFO: r9 = 0x40000045 INFO: r10 = 0x00000001 INFO: FP = 0x00000001 INFO: IP = 0x00000000 INFO: SP = 0xbef66508 INFO: LR = 0x00000000 INFO: PC = 0xb6dea8ec INFO: CPSR = 0x00000010 INFO: Fault Address = 0x00000000 INFO: Trap no = 0x0000000e INFO: Err Code = 0x00000207 INFO: Old Mask = 0x00000000 INFO: LR - 0x00000000: symbol not found INFO: Stack trace: INFO: 0x001a8350 <(null) + 0x001a8350> (./amiberry) INFO: 0xb67b7130 <__default_rt_sa_restorer + 0x00000000> (/lib/arm-linux-gnueabihf/libc.so.6) INFO: IP out of range INFO: Stack trace (non-dedicated): INFO: ./amiberry() [0x1a8468] INFO: /lib/arm-linux-gnueabihf/libc.so.6(__default_rt_sa_restorer+0) [0xb67b7130] INFO: End of stack trace. INFO: --- end exception --- ~

This is on an RPi 4 using RetroPie built from source on a fullt updated Raspbian Lite yesterday.

I've tried using the RetroPie setup script to remove Amiberry (I then delected the amiga folders in both config and roms) and then used Retropie setup script to re-install it (so it all got recompiled/re-built again), but something is obviously lingering as the segfault happens immediately when I try to launch Amiberry again with a WiiMote connected.

If I disconnect the WiiMote, Amiberry runs fine again.

@midwan
Copy link
Collaborator

midwan commented Nov 26, 2019

Thanks for the report.
Could you try one thing?
There's an option in amiberry.conf named gui_joystick_control, which defaults to yes.
Could you please change that to no instead, and test again?

You can find the amiberry.conf file in the conf/ directory of where amiberry is installed.
And the full documentation for the contents of that file are here: https://github.com/midwan/amiberry/wiki/Amiberry.conf-options

@midwan midwan self-assigned this Nov 26, 2019
@rickwookie
Copy link
Author

Ok, doing that doesn't cure the problem. Some more info though: I found an old (sticky) USB Thrustmaster Firestorm Dual Power Gamepad, so, with the WiiMote disconnected I tried using that and it works (and doesn't control the GUI as expected with that changed setting, but does when it's changed back). When I THEN re added the WiiMote, Amiberry would still launch, and I could see (several) Wii controllers listed in the Amiberry input device selector (Though THRUSTMASTER was still selected as expected). HOWEVER - when I then disconnected the USB Thrustmaster Gamepad and tried to launch (with just the WiiMote still connected) it segfaulted again, and if I reconnect the USB Thrustmaster Gamepad, it still segfaults. It only launches successfully again when I disconnect the WiiMote.
So:
First time I set up the WiiMote, Amiberry launched.
Doing "something" (not sure now the exact cause, but assigning the Joy1 Fire button to WiiMote East was the change I made) caused it to then break (fait to launch) forever (even after removing and re-adding Amiberry) if the WiiMote was connected.
Adding a new USB gamepad (with the WiiMote disconnected), Amiberry launched.
Reconnecting the paired WiiMote while new USB gamepad was connected, Amiberry launched.
Disconnecting the new USB gamepad, but with the WiiMote still connected, Amiberry fails to launch.
Now, reconnecting the USB gamepad and having the WiiMote disconnected, Amiberry launches.
Reconnecting the WiiMote (with or without the USB Gamepad connected), Amiberry fails to launch.

I've tried all sorts of combinations of reboots and deleting and repairing the WiiMote in bluetooth settings, but it seems as though the only time it worked is the first time ever I used it, and then the first time after adding a new gamepad. After that, it fails every time.

I've also just noticed that this failure (segfault) is now only for the Amiberry GUI itself. Since I have set up configurations pointing to the proper Kickstart ROMs, they will actually launch with the WiiMote connected, but it segfaults as soon as I hit F12 for the Amiberry GUI, and segfaults everytime on attempted launch of "+ START AMIBERRY".

@midwan
Copy link
Collaborator

midwan commented Nov 26, 2019

@rickwookie
Thanks for the feedback.
Amiberry tries to initialize the 1st found controller for usage within the GUI, so I suspect that's where things are going wrong in this case. Your tests above seem to indicate that if the WiiMote was the 1st connected controller in the system, it would crash - but if you had another one connected first, it would work.

I'll have to do some tests here to see if I can recreate the problem, which is going to be a little tricky as I don't have the same controller (and it works with those I have here). But we've had reports of a similar issue with a PS4 controller when connected via Bluetooth (only, cable works!), perhaps these two cases are related.

@rickwookie
Copy link
Author

Ok, so another quirk I just noticed considering that joystick for GUI setting (gui_joystick_control=no), if I start Amiberry GUI with gui_joystick_control=no, just have a keyboard and the USB gamepad connected, it all works. Then, if I turn on the paired WiiMote (wake it with a button press so it re-connects) WHILE AMIBERRY GUI IS RUNNING, the GUI freezes for about a second while the WiiMote seems to get detected and then I can move around the Amiberry GUI using the D-Pad on the WiiMote, even though gui_joystick_control=no, and there was no GUI control from the USB Gamepad (as expected). Further, the movement around the menu is as expected (press up on the D-Pad and it goes up one line) vs what happens with the WiiMote in the actual RetroPie Emulation Station UI where it jumps two lines every press of the D-Pad. So I wonder is the system picking up the WiiMote simultaneously as both a USB keyboard AND a controller? this would explain the double input behaviour and why Amiberry sees it as a keyboard too? Possibly not related issue, but thought it worth reporting anyway.

@midwan
Copy link
Collaborator

midwan commented Nov 26, 2019

It depends on how the system detects it, if it also sends keyboard commands that would explain the double input and also the fact that you can still use it in the GUI, even with that option disabled.
It might even explain the crash somehow...

@rickwookie
Copy link
Author

You must have a friend with an old Wii collecting dust somewhere, that you can use to test right?

@joolswills
Copy link
Contributor

It segfaults also for dualshock4 controllers. You can workaround it by disabling the accelerometer/motion control on ds4. I assume the same would work for the Wii controller.

See https://retropie.org.uk/forum/topic/17650/dualshock-controllers-on-4-4-with-3b/34

I did some debugging of the input code in amiberry. It crashes during an SDL call SDL_JoystickGetAxis but I'm not sure it's the fault of SDL yet.

The init_joystick code in amiberry gets triggered 4 times which seems wrong, but the input code isn't too easy to follow and I'm not that familiar with the codebase.

@joolswills
Copy link
Contributor

I made a quick fix to resolve this but I won't do a PR as I think it may be fixed better in a different way. I think the input code is in need of an overhaul (sorry).

The function read_joystick in src/osdep/amiberry_input.cpp does SDL_JoyStickGetAxis on joystick devices that have no axes. I wrapped the block of code in a check for SDL_JoystickNumAxes to fix it but this should be stored and checked via joysticktable for example I guess.

@midwan
Copy link
Collaborator

midwan commented Dec 7, 2019

@joolswills
Thanks for looking into this. It's true that that piece needs an overhaul (don't be sorry) :)

@midwan
Copy link
Collaborator

midwan commented Dec 7, 2019

@joolswills
SDL_JoystickGetAxis should just return 0 in case of failure, so I find it strange that it might be causing this (https://wiki.libsdl.org/SDL_JoystickGetAxis).
I tested this scenario with a (wired) PS4 controller, and I can't recreate it on a standard Raspbian Buster Desktop (didn't try RetroPie yet).

I also tested 2 more scenarios:

  • standard SDL2 version served through the Raspbian repo
  • latest SDL2 from Mercurial

It seems to work in both cases here. Does this only happen if the controller is paired through Bluetooth?

@midwan
Copy link
Collaborator

midwan commented Dec 7, 2019

Tested on RetroPie via Bluetooth also now, it didn't crash on me with the PS4 controller.

I tried the following:

  • Paired the controller
  • Launched Amiberry through RetroPie
  • Tested the controller in the GUI (worked normally)

@joolswills
Copy link
Contributor

Did you launch directly to a game/demo? The GUI is ok afaik. I've not tested with a ds4. That was a user on our forum. I tested with a Wii controller.

@midwan
Copy link
Collaborator

midwan commented Dec 7, 2019

@joolswills
I can recreate this if I test it on RetroPie using the ds4 controller (wired or Bluetooth), after the GUI as you said.
But I cannot recreate it on my other device which is running Raspbian Buster with the latest SDL2 compiled from source.

So the differences here are:

  • No Retroarch configuration on the other device
  • Newer SDL2 version
  • Newer OS version (doubt it matters here though)

SDL2 has had a number of controller-related fixes and improvements the last couple of weeks, so it could be related to that. I'll test on another system which has the same as above, but without the latest SDL2 to be sure.

Otherwise it might be that only controllers that are mapped with a Retroarch config have this problem.

I'll post more once I have more test results.

@joolswills
Copy link
Contributor

joolswills commented Dec 7, 2019

It doesn't matter if they are mapped or not. Btw I debugged this with gdb on a source level. It may well be fixed in a newer SDL but it's logical to not try and read non existing axis. So the code fix makes sense in any case.

@midwan
Copy link
Collaborator

midwan commented Dec 7, 2019

That's true, I'm just trying to understand why I'm not getting a crash on my other environments :)

@joolswills
Copy link
Contributor

joolswills commented Dec 7, 2019

Probably an SDL change. OS version doesn't make a difference btw. It happens on both stretch and buster (I was testing on buster with the RetroPie SDL 2.0.9).

@midwan midwan closed this as completed in 4768f8e Dec 7, 2019
@midwan midwan reopened this Dec 7, 2019
@midwan
Copy link
Collaborator

midwan commented Dec 7, 2019

@rickwookie @joolswills
Could you please test with the latest commit above? In my environment it seems to be fixed, but I don't have the same controller (tested here with a ds4).

@midwan midwan added the bug label Dec 7, 2019
@midwan midwan closed this as completed Dec 7, 2019
@joolswills
Copy link
Contributor

Sorry for the delay. Actually it seems we need more than this. My apologies as I missed this before - maybe I wrapped more code in the getaxis call. The code after the axis check now segfaults on the button reads. Need to check NumButtons also for the same reason. I did this just now and it solved it, but there are other getButton calls that might need wrapping.
Alternatively it might just be easier to exclude any joysticks with no axes or buttons when doing init_joystick.

@joolswills
Copy link
Contributor

This is for the Wii controller at least. I did a quick patch to ditch all joysticks with no axes or buttons. Need to check another segfault I got when entering GUI. Will update later though as I need sleep.

@joolswills
Copy link
Contributor

joolswills commented Dec 9, 2019

Ah looks like it's due to gui_joystick in main_window.cpp. we need to disable this if joystick 0 has no axes or buttons too (or skip to the next one).

The GUI is hard coded to the first controller it seems and in my test case this was the Wii device with no axes/buttons.

@joolswills
Copy link
Contributor

joolswills commented Dec 9, 2019

This diff is only intended as an illustration to stop the segfaults - not as a final fix but it's easier to illustrate. Applies against the commit before your change btw. Had to hack the SDL_JOYAXISMOTION: case also as this seems to segfault if gui_joystick is null.

diff --git a/external/libmpeg2 b/external/libmpeg2
index 185d891..6891249 160000
--- a/external/libmpeg2
+++ b/external/libmpeg2
@@ -1 +1 @@
-Subproject commit 185d891f1c282732584f5dbfed224ddefe864cc6
+Subproject commit 6891249c9ca2237b08665380b169d78f96878a51
diff --git a/src/osdep/amiberry_input.cpp b/src/osdep/amiberry_input.cpp
index 5bc1e9d..de70281 100644
--- a/src/osdep/amiberry_input.cpp
+++ b/src/osdep/amiberry_input.cpp
@@ -736,6 +736,11 @@ static int init_joystick()
 				strncpy(joystick_name[cpt], SDL_JoystickNameForIndex(cpt), sizeof joystick_name[cpt] - 1);
 			write_log("Controller Detection for Device: %s \n", joystick_name[cpt]);
 
+			if (SDL_JoystickNumAxes(joysticktable[cpt]) == 0 || SDL_JoystickNumButtons(joysticktable[cpt]) == 0) {
+				write_log("Controller has no Axes or Buttons - Skipping \n");
+				joysticktable[cpt] = nullptr;
+			}
+
 			//this now uses controllers path (in tmp)
 			char control_config[255];
 			strcpy(control_config, tmp);
diff --git a/src/osdep/gui/main_window.cpp b/src/osdep/gui/main_window.cpp
index 3ee846c..41bb5bd 100644
--- a/src/osdep/gui/main_window.cpp
+++ b/src/osdep/gui/main_window.cpp
@@ -615,6 +615,8 @@ void checkInput()
 			break;
 
 		case SDL_JOYAXISMOTION:
+			if (gui_joystick)
+			{
 			// Deadzone
 			if (std::abs(gui_event.jaxis.value) >= 10000 || std::abs(gui_event.jaxis.value) <= 5000)
 			{
@@ -663,7 +665,7 @@ void checkInput()
 				}
 			}
 			break;
-
+			}
 		case SDL_KEYDOWN:
 			if (gui_event.key.keysym.sym == key_for_gui)
 			{
@@ -778,7 +780,11 @@ void amiberry_gui_run()
 	if (gui_joystick_control && SDL_NumJoysticks() > 0)
 	{
 		gui_joystick = SDL_JoystickOpen(0);
-		joypad_axis_state.assign(SDL_JoystickNumAxes(gui_joystick), 0);
+		if (SDL_JoystickNumAxes(gui_joystick) > 0 && SDL_JoystickNumButtons(gui_joystick) > 0) {
+			joypad_axis_state.assign(SDL_JoystickNumAxes(gui_joystick), 0);
+		} else {
+			gui_joystick = nullptr;
+		}
 	}
 
 	// Prepare the screen once

@joolswills
Copy link
Contributor

joolswills commented Dec 9, 2019

Btw in case you were wondering. The first joystick device is "Nintendo Wii Remote IR". Hence the no axes/buttons. Of course SDL should handle this better and the new version may do. But still good to work around this imho for now. Also the missing check in SDL_JOYAXISMOTION seems needed (I think this code being hard coded to joystick 0 should be improved - maybe putting all input functions into its own class and reusing them with GUI/in-game). Anyway - a quick fix for now would be good :-) whatever you think is best

@morfeus77
Copy link

I can confirm this issue exists with PS4 dualshock. Works fine when launching games with keyboard only, segfaults while launching if the PS4 dualshock is paired. I tried setting gui_joystick_control to no which didn't resolve the issue.

@midwan
Copy link
Collaborator

midwan commented Dec 9, 2019

@joolswills
Thanks for the testing and follow-up! :)
I'll go for the quick fix for now, and improve upon this later with a better approach.
Let me know if the commit above fixes the problem please?

@joolswills
Copy link
Contributor

I think it resolves all cases but it's not ideal as it means no joystick control in GUI. Also the indent needs fixing for the Case statement - I excluded indent to make the diff simpler. But it stops the crash yep.

@midwan
Copy link
Collaborator

midwan commented Dec 9, 2019

@joolswills
Yeap, this is just a "quick fix" for now, I'll improve it over the next few days (lack of time currently means either this or we wait with the crash until I get more time)

@midwan
Copy link
Collaborator

midwan commented Dec 9, 2019

@joolswills
I've also added 269afdd which will use the first available joystick with Axes/Buttons, instead of Joystick 0 always, for the GUI. ;-)

PS: The indentation seems correct here, btw.

@midwan midwan closed this as completed Dec 9, 2019
@morfeus77
Copy link

Unfortunately didn't solve the issue for me. PS4 dual shocked paired. (SDL2 + DispmanX)

Parameters: Executing: /opt/retropie/emulators/amiberry/amiberry.sh auto "/home/pi/RetroPie/roms/amiga/4DSportsBoxing_$ INFO: --- New exception --- INFO: JIT not in use. INFO: Segmentation Fault INFO: info.si_signo = 11 INFO: info.si_errno = 0 INFO: info.si_code = 1 INFO: info.si_addr = ffffffff INFO: r0 = 0x74483a90 INFO: r1 = 0xffffffff INFO: r2 = 0x00000008 INFO: r3 = 0x00000000 INFO: r4 = 0x74483a90 INFO: r5 = 0x00000002 INFO: r6 = 0x00000004 INFO: r7 = 0x0401589c INFO: r8 = 0x00000000 INFO: r9 = 0x0000000a INFO: r10 = 0x00000000 INFO: FP = 0x00000000 INFO: IP = 0x0044e048 INFO: SP = 0x7ebe7888 INFO: LR = 0x00134730 INFO: PC = 0x76e3d8ac INFO: CPSR = 0x00000010 INFO: Fault Address = 0xffffffff INFO: Trap no = 0x0000000e INFO: Err Code = 0x00000037 INFO: Old Mask = 0x00000000 INFO: LR - 0x00134730: <(null)> (./amiberry) INFO: Stack trace: INFO: 0x00143d14 <(null) + 0x00143d14> (./amiberry) INFO: 0x7677f6c0 <__default_rt_sa_restorer + 0x00000000> (/lib/arm-linux-gnueabihf/libc.so.6) INFO: Stack trace (non-dedicated): INFO: ./amiberry() [0x143e44] INFO: /lib/arm-linux-gnueabihf/libc.so.6(__default_rt_sa_restorer+0) [0x7677f6c0] INFO: End of stack trace.

@midwan
Copy link
Collaborator

midwan commented Dec 9, 2019

@morfeus77
Strange, I cannot recreate this (actually even from my previous commit above).
Are you certain it's related to the same problem?

@morfeus77
Copy link

Seems unrelated. I solved the issue as mentioned in one of the above posts the issue was caused by motion controls. fixed with https://retropie.org.uk/forum/topic/17650/dualshock-controllers-on-4-4-with-3b/35

@joolswills
Copy link
Contributor

This should have fixed that issue but I'm looking into it as I was able to reproduce a crash when rebuilding master via RetroPie-Setup even though my debug build from master worked. Doing some more testing - maybe I missed something due to differing parameters etc.

@joolswills
Copy link
Contributor

The logic in my fix was changed. The joystick should be skipped if it has no axis or buttons. This resolves the crash with my Wii controller.

diff --git a/external/libmpeg2 b/external/libmpeg2
index 185d891..6891249 160000
--- a/external/libmpeg2
+++ b/external/libmpeg2
@@ -1 +1 @@
-Subproject commit 185d891f1c282732584f5dbfed224ddefe864cc6
+Subproject commit 6891249c9ca2237b08665380b169d78f96878a51
diff --git a/src/osdep/amiberry_input.cpp b/src/osdep/amiberry_input.cpp
index 8373d8a..079a261 100644
--- a/src/osdep/amiberry_input.cpp
+++ b/src/osdep/amiberry_input.cpp
@@ -729,8 +729,8 @@ static int init_joystick()
 	for (auto cpt = 0; cpt < nr_joysticks; cpt++)
 	{
 		joysticktable[cpt] = SDL_JoystickOpen(cpt);
-		if (SDL_JoystickNumAxes(joysticktable[cpt]) == 0 
-			&& SDL_JoystickNumButtons(joysticktable[cpt]) == 0) 
+		if (SDL_JoystickNumAxes(joysticktable[cpt]) == 0
+			|| SDL_JoystickNumButtons(joysticktable[cpt]) == 0)
 		{
 			if (SDL_JoystickNameForIndex(cpt) != nullptr)
 				strncpy(joystick_name[cpt], SDL_JoystickNameForIndex(cpt), sizeof joystick_name[cpt] - 1);

@midwan
Copy link
Collaborator

midwan commented Dec 10, 2019

@joolswills
But wouldn't this be a problem for joysticks that have no axes but do have buttons?
I'm thinking retro-style D-pads with no analog controls.

@midwan midwan reopened this Dec 10, 2019
@joolswills
Copy link
Contributor

joolswills commented Dec 10, 2019

Then you will need to wrap all the button code in a check so it works for controllers that miss buttons and the same for axes (which was done in the amiberry_input earlier in this ticket).

@joolswills
Copy link
Contributor

joolswills commented Dec 10, 2019

This was a quick fix. The input code should only try and read buttons if there are buttons and read axes if there are axes. And the same with hats etc.

@joolswills
Copy link
Contributor

joolswills commented Dec 10, 2019

My dpad controllers use axes for dpad and buttons for everything else. However of course a controller could use just buttons also. Your code would work for that for input but fail for an axes only device. The best fix is to check in read_joystick and not try and fetch buttons/axes/hats if they are not present).

@joolswills
Copy link
Contributor

Wrapping the button reads is simple enough btw but you have the hat code that handles hats or buttons. Perhaps reading button states into an array at the start which is wrapped then using that would simplify logic. But I'm not as familiar with this code as you. If this is done you can of course remove the checks on init_joystick. /Issue spam :-)

@joolswills
Copy link
Contributor

joolswills commented Dec 10, 2019

A quick example. Without re-indenting (the file has a mix of spaces and tabs that should be sorted imho)

diff --git a/external/libmpeg2 b/external/libmpeg2
index 185d891..6891249 160000
--- a/external/libmpeg2
+++ b/external/libmpeg2
@@ -1 +1 @@
-Subproject commit 185d891f1c282732584f5dbfed224ddefe864cc6
+Subproject commit 6891249c9ca2237b08665380b169d78f96878a51
diff --git a/src/osdep/amiberry_input.cpp b/src/osdep/amiberry_input.cpp
index 8373d8a..288f713 100644
--- a/src/osdep/amiberry_input.cpp
+++ b/src/osdep/amiberry_input.cpp
@@ -729,16 +729,7 @@ static int init_joystick()
 	for (auto cpt = 0; cpt < nr_joysticks; cpt++)
 	{
 		joysticktable[cpt] = SDL_JoystickOpen(cpt);
-		if (SDL_JoystickNumAxes(joysticktable[cpt]) == 0 
-			&& SDL_JoystickNumButtons(joysticktable[cpt]) == 0) 
-		{
-			if (SDL_JoystickNameForIndex(cpt) != nullptr)
-				strncpy(joystick_name[cpt], SDL_JoystickNameForIndex(cpt), sizeof joystick_name[cpt] - 1);
-			write_log("Controller %s has no Axes or Buttons - Skipping... \n", joystick_name[cpt]);
-			SDL_JoystickClose(joysticktable[cpt]);
-			joysticktable[cpt] = nullptr;
-		}
-		
+
 		if (joysticktable[cpt] != nullptr)
 		{
 			if (SDL_JoystickNameForIndex(cpt) != nullptr)
@@ -876,8 +867,7 @@ static void close_joystick()
 {
 	for (auto cpt = 0; cpt < nr_joysticks; cpt++)
 	{
-		if (joysticktable[cpt] != nullptr)
-			SDL_JoystickClose(joysticktable[cpt]);
+		SDL_JoystickClose(joysticktable[cpt]);
 	}
 	nr_joysticks = 0;
 }
@@ -1093,6 +1083,8 @@ static void read_joystick()
 
 			auto held_offset = 0;
 
+			if (SDL_JoystickNumButtons(joysticktable[hostjoyid]) > 0)
+			{
 			// detect standalone retroarch hotkeys
 			if (current_controller_map.hotkey_button == -1)
 			{
@@ -1145,6 +1137,8 @@ static void read_joystick()
 				held_offset = 0;
 			}
 
+			}
+
 			if (SDL_JoystickNumAxes(joysticktable[hostjoyid]) > 0)
 			{
 				int val;
@@ -1211,6 +1205,9 @@ static void read_joystick()
 
 				setjoystickstate(hostjoyid + 1, 3, val, upper_bound);
 			}
+
+			if (SDL_JoystickNumButtons(joysticktable[hostjoyid]) > 0)
+			{
 			// cd32 red, blue, green, yellow
 			// south 
 			setjoybuttonstate(hostjoyid + 1, 0 + held_offset,
@@ -1238,6 +1235,10 @@ static void read_joystick()
 			                  SDL_JoystickGetButton(joysticktable[hostjoyid], current_controller_map.start_button) & 1);
 			// start
 
+			}
+
+			if (SDL_JoystickNumHats(joysticktable[hostjoyid]) > 0 && SDL_JoystickNumButtons(joysticktable[hostjoyid]) > 0)
+			{
 			// up down left right
 			// HAT Handling *or* D-PAD buttons     
 			const int hat = SDL_JoystickGetHat(joysticktable[hostjoyid], 0);
@@ -1277,6 +1278,7 @@ static void read_joystick()
 			                  SDL_JoystickGetButton(joysticktable[hostjoyid],
 			                                        current_controller_map.select_button) & 1);
 			// select button
+			}
 		}
 	}
 }

@joolswills
Copy link
Contributor

Fixed a c&p error in last diff. Sorry again for all the ticket spam. The above solution (if indentated properly) is a better fix if only temporary until things are reworked a bit.

@joolswills
Copy link
Contributor

joolswills commented Dec 10, 2019

Btw you are right of course that SDL should handle this - SDL shouldn't segfault like this.

I decided to build SDL with debugging and even the latest code seems to be broken - but I'm somewhat surprised as it seems like a really obvious issue that would have been noticed before so either I have missed something or its just somehow never been fixed.

I opened an issue - https://bugzilla.libsdl.org/show_bug.cgi?id=4894

@joolswills
Copy link
Contributor

had a brain fart - the problem is elsewhere as this check isn't an issue at all. I think I need sleep :)

@joolswills
Copy link
Contributor

joolswills commented Dec 10, 2019

Last post - promise. After getting myself confused and rechecking via gdb from 3
0.6 before our changes I have realised the issue is not SDL and not even getting buttons or axis that don't exist.

The problem is the code initialises things like current_controller_map members to -1.

These are getting passed through to SDL and it's segfaulting. The SDL code correctly fails for non existent buttons but not if you ask for a button or axis of -1.

Which could be checked in SDL but the correct fix for this is to revert the workarounds and checking NumAxis etc and just make sure you don't send any -1 values to GetAxis or GetButton.

Of course you still shouldn't call these functions imho if there are no corresponding buttons/axis but I think it's best to rework this code rather than hiding the issue with the checks in my previous post.

Sorry I missed this before.

@midwan
Copy link
Collaborator

midwan commented Dec 10, 2019

Ah of course, that makes sense :)
I'll get that fixed!

midwan added a commit that referenced this issue Dec 11, 2019
Don't attempt to get Joystick Axis or Button if it's not mapped (set to -1)
@midwan
Copy link
Collaborator

midwan commented Dec 11, 2019

Could someone test with the latest commit above please?
It seems to work for me, but then again it was working before even... :)

This is meant as a temporary fix until that area is improved in a future update.

@morfeus77
Copy link

Works fine on my side! Good job.

@midwan
Copy link
Collaborator

midwan commented Dec 12, 2019

@morfeus77 Thanks for confirming!

@midwan midwan closed this as completed Dec 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants