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.

Sign up for GitHub

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

Jump to bottom

ControllerInterface: Fix crashes when using SDL controllers #4343

Merged
merged 2 commits into from
Dec 1, 2016
Merged

ControllerInterface: Fix crashes when using SDL controllers #4343

merged 2 commits into from
Dec 1, 2016

Conversation

ligfx
Copy link
Contributor

@ligfx ligfx commented Oct 11, 2016 •
edited by delroth
Loading

Couple changes across the codebase, the end result should be that no handles to controllers exist after ControllerInterface::Shutdown is called.

This change is Reviewable

leoetlino
leoetlino reviewed Oct 12, 2016
View reviewed changes
Source/Core/DolphinWX/Frame.cpp
Wiimote::Initialize(reinterpret_cast<void*>(win),
Wiimote::InitializeMode::DO_NOT_WAIT_FOR_WIIMOTES);
HotkeyManagerEmu::Initialize(reinterpret_cast<void*>(win));
void *win = reinterpret_cast<void*>(X11Utils::XWindowFromHandle(GetHandle()));

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

Source/Core/DolphinWX/Frame.cpp
Wiimote::Initialize(reinterpret_cast<void*>(GetHandle()),
Wiimote::InitializeMode::DO_NOT_WAIT_FOR_WIIMOTES);
HotkeyManagerEmu::Initialize(reinterpret_cast<void*>(GetHandle()));
void *win = reinterpret_cast<void*>(GetHandle());

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

Source/Core/InputCommon/ControllerEmu.cpp
void ControllerEmu::RefreshDevices()
{
ControllerInterface face;
UpdateReferences(face);

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

@ligfx ligfx force-pushed the fixsdlcrash branch 2 times, most recently from 9086415 to 6d4e400 Compare October 12, 2016 17:28
@shuffle2
Copy link
Contributor

Aren't these changes duplicated in one of your more recent PRs?

@ligfx
Copy link
Contributor Author

ligfx commented Oct 16, 2016

@shuffle2 Yes, I pulled out #4357 because it felt more logically separate, and I also wanted to rework this PR a bit.

It's now been reworked and ready for review again.

@shuffle2
Copy link
Contributor

@dolphin-emu-bot rebuild

leoetlino
leoetlino requested changes Oct 17, 2016
View reviewed changes
Source/Core/InputCommon/ControllerInterface/ControllerInterface.cpp
{
if (!m_is_init)
return;

Shutdown();
Initialize(m_hwnd);
m_devices.clear();

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

Source/Core/InputCommon/ControllerInterface/ControllerInterface.cpp
m_devices.clear();

#ifdef CIFACE_USE_DINPUT
ciface::DInput::GetDevices((HWND)m_hwnd);

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

Source/Core/InputCommon/ControllerInterface/evdev/evdev.cpp
@@ -184,6 +184,12 @@ void Init()
StartHotplugThread();
}

void GetDevices()

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

Source/Core/InputCommon/ControllerInterface/evdev/evdev.h
@@ -15,6 +15,7 @@ namespace ciface
namespace evdev
{
void Init();
void GetDevices();

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

@ligfx ligfx force-pushed the fixsdlcrash branch 2 times, most recently from b2dbf84 to ee859c1 Compare October 17, 2016 23:37
@shuffle2
Copy link
Contributor

@dolphin-emu-bot rebuild

@leoetlino
Copy link
Member

Reviewed 5 of 24 files at r2, 1 of 1 files at r3, 2 of 2 files at r4.
Review status: 7 of 21 files reviewed at latest revision, 1 unresolved discussion.

Source/Core/InputCommon/ControllerInterface/evdev/evdev.cpp, line 187 at r3 (raw file):

Previously, ligfx wrote…

IMO, GetDevices is not the correct name for what this does.

I agree with you, but right now I'm blanking on a better name. Any ideas?

Also, you probably don't need to shut down completely, i.e. it is unnecessary to shut down the hotplug thread and start it again simply for re-adding devices. I suggest leaving the hotplug stuff in Init and Shutdown, and moving the device enumerating part to this function.

Done.

maybe populate?

Comments from Reviewable

@ligfx
Copy link
Contributor Author

ligfx commented Oct 21, 2016

Review status: 2 of 21 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

Source/Core/InputCommon/ControllerInterface/evdev/evdev.cpp, line 187 at r3 (raw file):

Previously, leoetlino (Léo Lam) wrote…

maybe populate?

Works for me. Done.

Comments from Reviewable

@leoetlino
Copy link
Member

@dolphin-emu-bot rebuild

Reviewed 7 of 19 files at r5.
Review status: 9 of 21 files reviewed at latest revision, all discussions resolved, some commit checks failed.

Comments from Reviewable

@ligfx
ControllerInterface: clear devices before shutting down backends
cebb4d8 
The SDL backend faults if it tries to close a joystick after SDL_Quit
has been called.
@ligfx
ControllerInterface: replace Reinitialize with RefreshDevices
3e69d06 
The SDL backend crashes when you close a joystick after SDL_Quit has
been called. Some backends don't need to be shutdown and
re-initialized everytime, we can just ask to enumerate devices again.
@ligfx
Copy link
Contributor Author

ligfx commented Dec 1, 2016

Rebased on master and fixed conflicts.

@Helios747
Copy link
Contributor

:lgtm:

If nobody has any more comments in a couple hours I'll merge this.

@dolphin-emu-bot rebuild

Reviewed 1 of 24 files at r2, 18 of 19 files at r5, 2 of 3 files at r6.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

Comments from Reviewable

@Helios747 Helios747 merged commit a95e3c8 into dolphin-emu:master Dec 1, 2016
@ligfx ligfx deleted the fixsdlcrash branch December 1, 2016 23:15
@Tilka
Copy link
Member

Tilka commented Dec 3, 2016 •
edited
Loading

@ligfx: Since 3e69d06 Dolphin hangs when I try to close it. (on Linux)

@orbea
Copy link
Contributor

orbea commented Dec 4, 2016 •
edited
Loading

OS: Slackware64-current
libevdev-1.4.1-x86_64-1

I can confirm it was commit 3e69d06. Here is a backtrace of the hang.

http://pastebin.com/sTp7RRyg

@leoetlino
Copy link
Member

It seems to wait for the hotplug thread to stop, but it's stuck in select() waiting for an event, even though we wrote to the eventfd to wake up the thread…

I can't manage to reproduce it, with libevdev 1.5.5. flacs can reproduce it consistently though with the same libevdev version. Not sure why/how 3e69d06 introduced this issue…

@ligfx
Copy link
Contributor Author

ligfx commented Dec 4, 2016

I can't manage to reproduce it, with libevdev 1.5.5. flacs can reproduce it consistently though with the same libevdev version.

I can't reproduce either, on Debian Jessie amd64 (libudev1 215-17+deb8u5 / libevdev2 1.3+dfsg-1). What is "flacs"?

@JosJuice
Copy link
Member

JosJuice commented Dec 4, 2016

flacs is @Tilka.

@Tilka
Copy link
Member

Tilka commented Dec 5, 2016

Pushed some stuff into my evdev branch, still need to make s_devnode_name_map threadsafe, feel free to cherrypick or whatever.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leoetlino leoetlino leoetlino requested changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@ligfx @shuffle2 @leoetlino @Helios747 @Tilka @orbea @JosJuice