From 906de748bd00a09486d2a16b337751c55ebac8f9 Mon Sep 17 00:00:00 2001 From: comex Date: Wed, 4 Sep 2013 03:10:09 -0400 Subject: [PATCH] Refactor thread handling to fix OS X bug. On OS X, openL2CAPChannelSync registers events on the current thread's run loop, so Connect needs to be called on a thread that's going to do CFRunLoopRun; this was causing all Wiimote input to be ignored. Easiest way to do that is to use the Wiimote thread, and have Read call CFRunLoopRun to block on events, bringing OS X's Wiimote event loop in line with every other platform's. This also means that the thread can't be stopped and recreated by Prepare, so make Prepare notify it instead, which has the side effect of not making the GUI block on Prepare. (It would be nice if the GUI also did not block on searching for devices, because blocking the GUI is gross, but for now...) --- .../Core/Core/Src/HW/WiimoteReal/IODummy.cpp | 4 +- Source/Core/Core/Src/HW/WiimoteReal/IONix.cpp | 7 +- Source/Core/Core/Src/HW/WiimoteReal/IOWin.cpp | 4 +- .../Core/Core/Src/HW/WiimoteReal/IOdarwin.mm | 27 ++- .../Core/Src/HW/WiimoteReal/WiimoteReal.cpp | 163 ++++++++++-------- .../Core/Src/HW/WiimoteReal/WiimoteReal.h | 22 ++- 6 files changed, 128 insertions(+), 99 deletions(-) diff --git a/Source/Core/Core/Src/HW/WiimoteReal/IODummy.cpp b/Source/Core/Core/Src/HW/WiimoteReal/IODummy.cpp index 59025f9040..870d281326 100644 --- a/Source/Core/Core/Src/HW/WiimoteReal/IODummy.cpp +++ b/Source/Core/Core/Src/HW/WiimoteReal/IODummy.cpp @@ -43,12 +43,12 @@ bool WiimoteScanner::IsReady() const return false; } -bool Wiimote::Connect() +bool Wiimote::ConnectInternal() { return 0; } -void Wiimote::Disconnect() +void Wiimote::DisconnectInternal() { return; } diff --git a/Source/Core/Core/Src/HW/WiimoteReal/IONix.cpp b/Source/Core/Core/Src/HW/WiimoteReal/IONix.cpp index edd79f2bf6..014eadc9a8 100644 --- a/Source/Core/Core/Src/HW/WiimoteReal/IONix.cpp +++ b/Source/Core/Core/Src/HW/WiimoteReal/IONix.cpp @@ -27,8 +27,7 @@ namespace WiimoteReal { WiimoteScanner::WiimoteScanner() - : m_run_thread() - , m_want_wiimotes() + : m_want_wiimotes() , device_id(-1) , device_sock(-1) { @@ -135,7 +134,7 @@ void WiimoteScanner::FindWiimotes(std::vector & found_wiimotes, Wiimot } // Connect to a wiimote with a known address. -bool Wiimote::Connect() +bool Wiimote::ConnectInternal() { sockaddr_l2 addr; addr.l2_family = AF_BLUETOOTH; @@ -168,7 +167,7 @@ bool Wiimote::Connect() return true; } -void Wiimote::Disconnect() +void Wiimote::DisconnectInternal() { close(cmd_sock); close(int_sock); diff --git a/Source/Core/Core/Src/HW/WiimoteReal/IOWin.cpp b/Source/Core/Core/Src/HW/WiimoteReal/IOWin.cpp index 5d4d26108f..866284b323 100644 --- a/Source/Core/Core/Src/HW/WiimoteReal/IOWin.cpp +++ b/Source/Core/Core/Src/HW/WiimoteReal/IOWin.cpp @@ -459,7 +459,7 @@ bool WiimoteScanner::IsReady() const } // Connect to a wiimote with a known device path. -bool Wiimote::Connect() +bool Wiimote::ConnectInternal() { if (IsConnected()) return false; @@ -535,7 +535,7 @@ bool Wiimote::Connect() return true; } -void Wiimote::Disconnect() +void Wiimote::DisconnectInternal() { if (!IsConnected()) return; diff --git a/Source/Core/Core/Src/HW/WiimoteReal/IOdarwin.mm b/Source/Core/Core/Src/HW/WiimoteReal/IOdarwin.mm index 8596b12080..17e867bc7a 100644 --- a/Source/Core/Core/Src/HW/WiimoteReal/IOdarwin.mm +++ b/Source/Core/Core/Src/HW/WiimoteReal/IOdarwin.mm @@ -64,7 +64,7 @@ return; } - if (wm->inputlen != 0) { + if (wm->inputlen != -1) { WARN_LOG(WIIMOTE, "Dropping packet for wiimote %i, queue full", wm->index + 1); return; @@ -73,9 +73,8 @@ memcpy(wm->input, data, length); wm->inputlen = length; - (void)wm->Read(); - (void)UpdateSystemActivity(UsrActivity); + CFRunLoopStop(CFRunLoopGetCurrent()); } - (void) l2capChannelClosed: (IOBluetoothL2CAPChannel *) l2capChannel @@ -100,7 +99,7 @@ WARN_LOG(WIIMOTE, "Lost channel to wiimote %i", wm->index + 1); - wm->Disconnect(); + wm->DisconnectInternal(); } @end @@ -189,7 +188,7 @@ bool WiimoteScanner::IsReady() const } // Connect to a wiimote with a known address. -bool Wiimote::Connect() +bool Wiimote::ConnectInternal() { if (IsConnected()) return false; @@ -204,12 +203,12 @@ bool Wiimote::Connect() { ERROR_LOG(WIIMOTE, "Unable to open L2CAP channels " "for wiimote %i", index + 1); - Disconnect(); + DisconnectInternal(); [cbt release]; return false; } - + NOTICE_LOG(WIIMOTE, "Connected to wiimote %i at %s", index + 1, [[btd addressString] UTF8String]); @@ -220,7 +219,7 @@ bool Wiimote::Connect() } // Disconnect a wiimote. -void Wiimote::Disconnect() +void Wiimote::DisconnectInternal() { if (ichan != NULL) [ichan release]; @@ -250,16 +249,12 @@ bool Wiimote::IsConnected() const int Wiimote::IORead(unsigned char *buf) { - int bytes; + input = buf; + inputlen = -1; - if (!IsConnected()) - return 0; + CFRunLoopRun(); - bytes = inputlen; - memcpy(buf, input, bytes); - inputlen = 0; - - return bytes; + return inputlen; } int Wiimote::IOWrite(const unsigned char *buf, int len) diff --git a/Source/Core/Core/Src/HW/WiimoteReal/WiimoteReal.cpp b/Source/Core/Core/Src/HW/WiimoteReal/WiimoteReal.cpp index d2e7e2f57a..aead9d60fa 100644 --- a/Source/Core/Core/Src/HW/WiimoteReal/WiimoteReal.cpp +++ b/Source/Core/Core/Src/HW/WiimoteReal/WiimoteReal.cpp @@ -40,7 +40,7 @@ WiimoteScanner g_wiimote_scanner; Wiimote::Wiimote() : index() #ifdef __APPLE__ - , btd(), ichan(), cchan(), inputlen(), m_connected() + , btd(), ichan(), cchan(), input(), inputlen(), m_connected() #elif defined(__linux__) && HAVE_BLUEZ , cmd_sock(-1), int_sock(-1) #elif defined(_WIN32) @@ -49,7 +49,7 @@ Wiimote::Wiimote() , m_last_input_report() , m_channel(0) , m_rumble_state() - , m_run_thread(false) + , m_need_prepare() { #if defined(__linux__) && HAVE_BLUEZ bdaddr = (bdaddr_t){{0, 0, 0, 0, 0, 0}}; @@ -60,9 +60,6 @@ Wiimote::~Wiimote() { StopThread(); - if (IsConnected()) - Disconnect(); - ClearReadQueue(); m_write_reports.Clear(); } @@ -225,8 +222,8 @@ bool Wiimote::Read() } else if (0 == result) { - WARN_LOG(WIIMOTE, "Wiimote::IORead failed. Disconnecting Wiimote %d.", index + 1); - Disconnect(); + ERROR_LOG(WIIMOTE, "Wiimote::IORead failed. Disconnecting Wiimote %d.", index + 1); + DisconnectInternal(); } return false; @@ -308,27 +305,31 @@ void Wiimote::Update() rpt.data(), rpt.size()); } -bool Wiimote::Prepare(int _index) +void Wiimote::Prepare(int _index) { index = _index; + m_need_prepare = true; +} +bool Wiimote::PrepareOnThread() +{ // core buttons, no continuous reporting - u8 const mode_report[] = {WM_SET_REPORT | WM_BT_OUTPUT, WM_REPORT_MODE, 0, WM_REPORT_CORE}; + u8 static const mode_report[] = {WM_SET_REPORT | WM_BT_OUTPUT, WM_REPORT_MODE, 0, WM_REPORT_CORE}; // Set the active LEDs and turn on rumble. - u8 const led_report[] = {WM_SET_REPORT | WM_BT_OUTPUT, WM_LEDS, u8(WIIMOTE_LED_1 << (index%WIIMOTE_BALANCE_BOARD) | 0x1)}; + u8 static const led_report[] = {WM_SET_REPORT | WM_BT_OUTPUT, WM_LEDS, u8(WIIMOTE_LED_1 << (index%WIIMOTE_BALANCE_BOARD) | 0x1)}; // Turn off rumble - u8 rumble_report[] = {WM_SET_REPORT | WM_BT_OUTPUT, WM_RUMBLE, 0}; + u8 static const rumble_report[] = {WM_SET_REPORT | WM_BT_OUTPUT, WM_RUMBLE, 0}; // Request status report - u8 const req_status_report[] = {WM_SET_REPORT | WM_BT_OUTPUT, WM_REQUEST_STATUS, 0}; + u8 static const req_status_report[] = {WM_SET_REPORT | WM_BT_OUTPUT, WM_REQUEST_STATUS, 0}; // TODO: check for sane response? return (IOWrite(mode_report, sizeof(mode_report)) - && IOWrite(led_report, sizeof(led_report)) - && (SLEEP(200), IOWrite(rumble_report, sizeof(rumble_report))) - && IOWrite(req_status_report, sizeof(req_status_report))); + && IOWrite(led_report, sizeof(led_report)) + && (SLEEP(200), IOWrite(rumble_report, sizeof(rumble_report))) + && IOWrite(req_status_report, sizeof(req_status_report))); } void Wiimote::EmuStart() @@ -480,35 +481,75 @@ void WiimoteScanner::ThreadFunc() NOTICE_LOG(WIIMOTE, "Wiimote scanning has stopped."); } +bool Wiimote::Connect() +{ + m_thread_ready = false; + StartThread(); + WaitReady(); + return IsConnected(); +} + void Wiimote::StartThread() { - m_run_thread = true; m_wiimote_thread = std::thread(std::mem_fun(&Wiimote::ThreadFunc), this); } void Wiimote::StopThread() { - m_run_thread = false; if (m_wiimote_thread.joinable()) m_wiimote_thread.join(); } +void Wiimote::SetReady() +{ + if (!m_thread_ready) + { + { + std::lock_guard Guard(m_thread_ready_mutex); + m_thread_ready = true; + } + m_thread_ready_cond.notify_all(); + } +} + +void Wiimote::WaitReady() +{ + std::unique_lock lock(m_thread_ready_mutex); + while (!m_thread_ready) + { + m_thread_ready_cond.wait(lock); + } +} + void Wiimote::ThreadFunc() { Common::SetCurrentThreadName("Wiimote Device Thread"); + bool ok = ConnectInternal(); + + SetReady(); + + if (!ok) + { + return; + } // main loop - while (m_run_thread && IsConnected()) + while (IsConnected()) { -#ifdef __APPLE__ - // Reading happens elsewhere on OSX - bool const did_something = Write(); -#else - bool const did_something = Write() || Read(); -#endif - if (!did_something) - Common::SleepCurrentThread(1); + if (m_need_prepare) + { + m_need_prepare = false; + if (!PrepareOnThread()) + { + ERROR_LOG(WIIMOTE, "Wiimote::PrepareOnThread failed. Disconnecting Wiimote %d.", index + 1); + DisconnectInternal(); + } + } + Write(); + Read(); } + + DisconnectInternal(); } void LoadSettings() @@ -629,24 +670,32 @@ void ChangeWiimoteSource(unsigned int index, int source) Host_ConnectWiimote(index, true); } +static bool TryToConnectWiimoteN(Wiimote* wm, unsigned int i) +{ + if (WIIMOTE_SRC_REAL & g_wiimote_sources[i] + && !g_wiimotes[i]) + { + if (wm->Connect()) + { + wm->Prepare(i); + NOTICE_LOG(WIIMOTE, "Connected to Wiimote %i.", i + 1); + g_wiimotes[i] = wm; + Host_ConnectWiimote(i, true); + } + return true; + } + return false; +} + void TryToConnectWiimote(Wiimote* wm) { std::unique_lock lk(g_refresh_lock); for (unsigned int i = 0; i < MAX_WIIMOTES; ++i) { - if (WIIMOTE_SRC_REAL & g_wiimote_sources[i] - && !g_wiimotes[i]) + if (TryToConnectWiimoteN(wm, i)) { - if (wm->Connect() && wm->Prepare(i)) - { - NOTICE_LOG(WIIMOTE, "Connected to Wiimote %i.", i + 1); - - std::swap(g_wiimotes[i], wm); - g_wiimotes[i]->StartThread(); - - Host_ConnectWiimote(i, true); - } + wm = NULL; break; } } @@ -661,19 +710,10 @@ void TryToConnectWiimote(Wiimote* wm) void TryToConnectBalanceBoard(Wiimote* wm) { std::unique_lock lk(g_refresh_lock); - - if (WIIMOTE_SRC_REAL & g_wiimote_sources[WIIMOTE_BALANCE_BOARD] - && !g_wiimotes[WIIMOTE_BALANCE_BOARD]) + + if (TryToConnectWiimoteN(wm, WIIMOTE_BALANCE_BOARD)) { - if (wm->Connect() && wm->Prepare(WIIMOTE_BALANCE_BOARD)) - { - NOTICE_LOG(WIIMOTE, "Connected to Balance Board %i.", WIIMOTE_BALANCE_BOARD + 1); - - std::swap(g_wiimotes[WIIMOTE_BALANCE_BOARD], wm); - g_wiimotes[WIIMOTE_BALANCE_BOARD]->StartThread(); - - Host_ConnectWiimote(WIIMOTE_BALANCE_BOARD, true); - } + wm = NULL; } g_wiimote_scanner.WantBB(0 != CalculateWantedBB()); @@ -687,26 +727,13 @@ void DoneWithWiimote(int index) { std::lock_guard lk(g_refresh_lock); - if (g_wiimotes[index]) + Wiimote* wm = g_wiimotes[index]; + + if (wm) { - g_wiimotes[index]->StopThread(); - + g_wiimotes[index] = NULL; // First see if we can use this real Wiimote in another slot. - for (unsigned int i = 0; i < MAX_WIIMOTES; ++i) - { - if (WIIMOTE_SRC_REAL & g_wiimote_sources[i] - && !g_wiimotes[i]) - { - if (g_wiimotes[index]->Prepare(i)) - { - std::swap(g_wiimotes[i], g_wiimotes[index]); - g_wiimotes[i]->StartThread(); - - Host_ConnectWiimote(i, true); - } - break; - } - } + TryToConnectWiimote(wm); } // else, just disconnect the Wiimote @@ -763,9 +790,7 @@ void Refresh() { if (g_wiimotes[i]) { - g_wiimotes[i]->StopThread(); g_wiimotes[i]->Prepare(i); - g_wiimotes[i]->StartThread(); } } diff --git a/Source/Core/Core/Src/HW/WiimoteReal/WiimoteReal.h b/Source/Core/Core/Src/HW/WiimoteReal/WiimoteReal.h index 2ab4e435e2..e48567acbb 100644 --- a/Source/Core/Core/Src/HW/WiimoteReal/WiimoteReal.h +++ b/Source/Core/Core/Src/HW/WiimoteReal/WiimoteReal.h @@ -52,13 +52,17 @@ public: // connecting and disconnecting from physical devices // (using address inserted by FindWiimotes) + // these are called from the wiimote's thread. + bool ConnectInternal(); + void DisconnectInternal(); + bool Connect(); - void Disconnect(); // TODO: change to something like IsRelevant bool IsConnected() const; - bool Prepare(int index); + void Prepare(int index); + bool PrepareOnThread(); void DisableDataReporting(); void EnableDataReporting(u8 mode); @@ -72,9 +76,10 @@ public: IOBluetoothDevice *btd; IOBluetoothL2CAPChannel *ichan; IOBluetoothL2CAPChannel *cchan; - char input[MAX_PAYLOAD]; + unsigned char* input; int inputlen; bool m_connected; + CFRunLoopRef m_wiimote_thread_run_loop; #elif defined(__linux__) && HAVE_BLUEZ bdaddr_t bdaddr; // Bluetooth address int cmd_sock; // Command socket @@ -100,12 +105,17 @@ private: int IOWrite(u8 const* buf, int len); void ThreadFunc(); + void SetReady(); + void WaitReady(); bool m_rumble_state; - bool m_run_thread; - std::thread m_wiimote_thread; - + std::thread m_wiimote_thread; + volatile bool m_thread_ready; + volatile bool m_need_prepare; + std::mutex m_thread_ready_mutex; + std::condition_variable m_thread_ready_cond; + Common::FifoQueue m_read_reports; Common::FifoQueue m_write_reports;