From 41366f73b23a207ef2519e41ca02e9ad337506e3 Mon Sep 17 00:00:00 2001
From: Sam Lantinga <[EMAIL REDACTED]>
Date: Thu, 17 Oct 2024 11:27:15 -0700
Subject: [PATCH] Don't block in Android USB device open
We might be waiting a long time for a permissions dialog, so let that complete asynchronously. While that's happening, we'll remove the device from the device list so that when we get permission the application sees the device as newly available and can open it again.
Fixes https://github.com/libsdl-org/SDL/issues/6347
---
src/hidapi/android/hid.cpp | 113 +++++++++++++++--------
src/joystick/hidapi/SDL_hidapijoystick.c | 50 ----------
2 files changed, 76 insertions(+), 87 deletions(-)
diff --git a/src/hidapi/android/hid.cpp b/src/hidapi/android/hid.cpp
index 3a5f53c0bffe6..86040d655f54e 100644
--- a/src/hidapi/android/hid.cpp
+++ b/src/hidapi/android/hid.cpp
@@ -500,46 +500,38 @@ class CHIDDevice
return false;
}
- m_bIsWaitingForOpen = false;
- m_bOpenResult = env->CallBooleanMethod( g_HIDDeviceManagerCallbackHandler, g_midHIDDeviceManagerOpen, m_nId );
- ExceptionCheck( env, "BOpen" );
-
if ( m_bIsWaitingForOpen )
{
- hid_mutex_guard cvl( &m_cvLock );
-
- const int OPEN_TIMEOUT_SECONDS = 60;
- struct timespec ts, endtime;
- clock_gettime( CLOCK_REALTIME, &ts );
- endtime = ts;
- endtime.tv_sec += OPEN_TIMEOUT_SECONDS;
- do
- {
- if ( pthread_cond_timedwait( &m_cv, &m_cvLock, &endtime ) != 0 )
- {
- break;
- }
- }
- while ( m_bIsWaitingForOpen && get_timespec_ms( ts ) < get_timespec_ms( endtime ) );
+ SDL_SetError( "Waiting for permission" );
+ return false;
}
if ( !m_bOpenResult )
{
+ m_bOpenResult = env->CallBooleanMethod( g_HIDDeviceManagerCallbackHandler, g_midHIDDeviceManagerOpen, m_nId );
+ ExceptionCheck( env, "BOpen" );
+
if ( m_bIsWaitingForOpen )
{
- LOGV( "Device open failed - timed out waiting for device permission" );
+ LOGV( "Device open waiting for permission" );
+ SDL_SetError( "Waiting for permission" );
+ m_bWasOpenPending = true;
+ return false;
}
- else
+
+ if ( !m_bOpenResult )
{
LOGV( "Device open failed" );
+ SDL_SetError( "Device open failed" );
+ return false;
}
- return false;
}
m_pDevice = new hid_device;
m_pDevice->m_nId = m_nId;
m_pDevice->m_nDeviceRefCount = 1;
LOGD("Creating device %d (%p), refCount = 1\n", m_pDevice->m_nId, m_pDevice);
+
return true;
}
@@ -548,16 +540,44 @@ class CHIDDevice
m_bIsWaitingForOpen = true;
}
+ bool BOpenPending() const
+ {
+ return m_bIsWaitingForOpen;
+ }
+
+ void SetWasOpenPending( bool bState )
+ {
+ m_bWasOpenPending = bState;
+ }
+
+ bool BWasOpenPending() const
+ {
+ return m_bWasOpenPending;
+ }
+
void SetOpenResult( bool bResult )
{
if ( m_bIsWaitingForOpen )
{
m_bOpenResult = bResult;
m_bIsWaitingForOpen = false;
- pthread_cond_signal( &m_cv );
+
+ if ( m_bOpenResult )
+ {
+ LOGV( "Device open succeeded" );
+ }
+ else
+ {
+ LOGV( "Device open failed" );
+ }
}
}
+ bool BOpenResult() const
+ {
+ return m_bOpenResult;
+ }
+
void ProcessInput( const uint8_t *pBuf, size_t nBufSize )
{
hid_mutex_guard l( &m_dataLock );
@@ -606,18 +626,16 @@ class CHIDDevice
{
JNIEnv *env = SDL_GetAndroidJNIEnv();
- int nRet = -1;
- if ( g_HIDDeviceManagerCallbackHandler )
- {
- jbyteArray pBuf = NewByteArray( env, pData, nDataLen );
- nRet = env->CallIntMethod( g_HIDDeviceManagerCallbackHandler, g_midHIDDeviceManagerWriteReport, m_nId, pBuf, bFeature );
- ExceptionCheck( env, "WriteReport" );
- env->DeleteLocalRef( pBuf );
- }
- else
+ if ( !g_HIDDeviceManagerCallbackHandler )
{
LOGV( "WriteReport without callback handler" );
+ return -1;
}
+
+ jbyteArray pBuf = NewByteArray( env, pData, nDataLen );
+ int nRet = env->CallIntMethod( g_HIDDeviceManagerCallbackHandler, g_midHIDDeviceManagerWriteReport, m_nId, pBuf, bFeature );
+ ExceptionCheck( env, "WriteReport" );
+ env->DeleteLocalRef( pBuf );
return nRet;
}
@@ -713,8 +731,11 @@ class CHIDDevice
if ( g_HIDDeviceManagerCallbackHandler )
{
- env->CallVoidMethod( g_HIDDeviceManagerCallbackHandler, g_midHIDDeviceManagerClose, m_nId );
- ExceptionCheck( env, "Close" );
+ if ( !m_bIsWaitingForOpen && m_bOpenResult )
+ {
+ env->CallVoidMethod( g_HIDDeviceManagerCallbackHandler, g_midHIDDeviceManagerClose, m_nId );
+ ExceptionCheck( env, "Close" );
+ }
}
hid_mutex_guard dataLock( &m_dataLock );
@@ -749,6 +770,7 @@ class CHIDDevice
pthread_mutex_t m_cvLock = PTHREAD_MUTEX_INITIALIZER; // This lock has to be held to access any variables below
pthread_cond_t m_cv = PTHREAD_COND_INITIALIZER;
bool m_bIsWaitingForOpen = false;
+ bool m_bWasOpenPending = false;
bool m_bOpenResult = false;
bool m_bIsWaitingForReportResponse = false;
int m_nReportResponseError = 0;
@@ -1045,6 +1067,18 @@ struct hid_device_info HID_API_EXPORT * HID_API_CALL hid_enumerate(unsigned shor
hid_mutex_guard l( &g_DevicesMutex );
for ( hid_device_ref<CHIDDevice> pDevice = g_Devices; pDevice; pDevice = pDevice->next )
{
+ // Don't enumerate devices that are currently being opened, we'll re-enumerate them when we're done
+ // Make sure we skip them at least once, so they get removed and then re-added to the caller's device list
+ if ( pDevice->BWasOpenPending() )
+ {
+ // Don't enumerate devices that failed to open, otherwise the application might try to keep prompting for access
+ if ( !pDevice->BOpenPending() && pDevice->BOpenResult() )
+ {
+ pDevice->SetWasOpenPending( false );
+ }
+ continue;
+ }
+
const hid_device_info *info = pDevice->GetDeviceInfo();
/* See if there are any devices we should skip in enumeration */
@@ -1105,7 +1139,12 @@ HID_API_EXPORT hid_device * HID_API_CALL hid_open_path(const char *path)
}
}
}
- if ( pDevice && pDevice->BOpen() )
+ if ( !pDevice )
+ {
+ SDL_SetError( "Couldn't find device with path %s", path );
+ return NULL;
+ }
+ if ( pDevice->BOpen() )
{
return pDevice->GetDevice();
}
@@ -1116,7 +1155,7 @@ int HID_API_EXPORT HID_API_CALL hid_write(hid_device *device, const unsigned ch
{
if ( device )
{
- LOGV( "hid_write id=%d length=%zu", device->m_nId, length );
+// LOGV( "hid_write id=%d length=%zu", device->m_nId, length );
hid_device_ref<CHIDDevice> pDevice = FindDevice( device->m_nId );
if ( pDevice )
{
@@ -1180,7 +1219,7 @@ int HID_API_EXPORT HID_API_CALL hid_read_timeout(hid_device *device, unsigned ch
// TODO: Implement blocking
int HID_API_EXPORT HID_API_CALL hid_read(hid_device *device, unsigned char *data, size_t length)
{
- LOGV( "hid_read id=%d length=%zu", device->m_nId, length );
+// LOGV( "hid_read id=%d length=%zu", device->m_nId, length );
return hid_read_timeout( device, data, length, 0 );
}
diff --git a/src/joystick/hidapi/SDL_hidapijoystick.c b/src/joystick/hidapi/SDL_hidapijoystick.c
index c65d6b9a99e4e..3a0f8169c7d58 100644
--- a/src/joystick/hidapi/SDL_hidapijoystick.c
+++ b/src/joystick/hidapi/SDL_hidapijoystick.c
@@ -454,57 +454,7 @@ static void HIDAPI_SetupDeviceDriver(SDL_HIDAPI_Device *device, bool *removed) S
// Wait a little bit for the device to initialize
SDL_Delay(10);
-#ifdef SDL_PLATFORM_ANDROID
- /* On Android we need to leave joysticks unlocked because it calls
- * out to the main thread for permissions and the main thread can
- * be in the process of handling controller input.
- *
- * See https://github.com/libsdl-org/SDL/issues/6347 for details
- */
- {
- SDL_HIDAPI_Device *curr;
- int lock_count = 0;
- char *path = SDL_strdup(device->path);
-
- SDL_AssertJoysticksLocked();
- while (SDL_JoysticksLocked()) {
- ++lock_count;
- SDL_UnlockJoysticks();
- }
-
- dev = SDL_hid_open_path(path);
-
- while (lock_count > 0) {
- --lock_count;
- SDL_LockJoysticks();
- }
- SDL_free(path);
-
- // Make sure the device didn't get removed while opening the HID path
- for (curr = SDL_HIDAPI_devices; curr && curr != device; curr = curr->next) {
- continue;
- }
- if (curr == NULL) {
- *removed = true;
- if (dev) {
- SDL_hid_close(dev);
- }
- return;
- }
- }
-#else
- /* On other platforms we want to keep the lock so other threads wait for
- * us to finish opening the controller before checking to see whether the
- * HIDAPI driver is handling the device.
- *
- * On Windows, for example, the main thread can be enumerating DirectInput
- * devices while the Windows.Gaming.Input thread is calling back with a new
- * controller available.
- *
- * See https://github.com/libsdl-org/SDL/issues/7304 for details.
- */
dev = SDL_hid_open_path(device->path);
-#endif
if (dev == NULL) {
SDL_LogDebug(SDL_LOG_CATEGORY_INPUT,