Libusb#620
Conversation
andreknieriem
commented
Jun 23, 2026
- Added Libusb as an alternative usb driver
There was a problem hiding this comment.
Code Review
This pull request introduces a native USB driver using libusb via JNI to handle USB accessory mode connections, adds a toggle setting to enable it, and implements support for projection on secondary displays. It also improves WiFi Direct auto-start behavior, fixes loading screen replacement issues, and enhances dialog keyboard focus. The code review feedback highlights several critical and high-severity improvement opportunities, including fixing an infinite loop bug in recvBlocking on read errors, optimizing JNI read/write operations to achieve zero-allocation transfers, avoiding hardcoded USB interface IDs, ensuring native resources are released via finalize(), making activeActivityRef volatile for thread safety, and cleaning up old loading screen files to prevent storage leaks.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| override fun recvBlocking(buf: ByteArray, length: Int, timeout: Int, readFully: Boolean): Int { | ||
| val native = usbNative ?: return -1 | ||
| var totalReturned = 0 | ||
|
|
||
| while (totalReturned < length) { | ||
| val leftover = leftoverBuffer | ||
| if (leftover != null) { | ||
| val available = leftover.size - leftoverPos | ||
| val toCopy = minOf(length - totalReturned, available) | ||
| System.arraycopy(leftover, leftoverPos, buf, totalReturned, toCopy) | ||
| leftoverPos += toCopy | ||
| totalReturned += toCopy | ||
|
|
||
| if (leftoverPos >= leftover.size) { | ||
| leftoverBuffer = null | ||
| leftoverPos = 0 | ||
| } | ||
|
|
||
| if (totalReturned >= length || !readFully) break | ||
| continue | ||
| } | ||
|
|
||
| val readBytes = native.read(timeout) | ||
| if (readBytes == null) { | ||
| return if (totalReturned > 0) totalReturned else -1 | ||
| } | ||
| if (readBytes.isEmpty()) { | ||
| return totalReturned | ||
| } | ||
|
|
||
| leftoverBuffer = readBytes | ||
| leftoverPos = 0 | ||
| } | ||
|
|
||
| return totalReturned | ||
| } |
There was a problem hiding this comment.
Refactor recvBlocking to use the pre-allocated buffer and fix a critical infinite loop bug.
Critical Bug Fix: When a fatal read error occurs (native.read returns < 0), we must set isConnectedVal = false so that the connection state is updated. Otherwise, AapReadMultipleMessages.doRead will continuously spin at 100% CPU on a dead connection.
override fun recvBlocking(buf: ByteArray, length: Int, timeout: Int, readFully: Boolean): Int {
val native = usbNative ?: return -1
var totalReturned = 0
while (totalReturned < length) {
if (leftoverSize > 0) {
val available = leftoverSize - leftoverPos
val toCopy = minOf(length - totalReturned, available)
System.arraycopy(readBuffer, leftoverPos, buf, totalReturned, toCopy)
leftoverPos += toCopy
totalReturned += toCopy
if (leftoverPos >= leftoverSize) {
leftoverSize = 0
leftoverPos = 0
}
if (totalReturned >= length || !readFully) break
continue
}
val transferred = native.read(readBuffer, timeout)
if (transferred < 0) {
isConnectedVal = false
return if (totalReturned > 0) totalReturned else -1
}
if (transferred == 0) {
return totalReturned
}
leftoverSize = transferred
leftoverPos = 0
}
return totalReturned
}| JNIEXPORT jint JNICALL | ||
| Java_com_andrerinas_headunitrevived_connection_UsbNative_nativeWrite(JNIEnv *env, jobject thiz, jlong handle_ptr, jbyteArray data, jint endpoint, jint timeout) { | ||
| libusb_device_handle *handle = (libusb_device_handle *)handle_ptr; | ||
| jsize len = (*env)->GetArrayLength(env, data); | ||
| jbyte *buf = (*env)->GetByteArrayElements(env, data, NULL); | ||
| if (!buf) { | ||
| LOGE("nativeWrite: GetByteArrayElements returned NULL"); | ||
| return -2; | ||
| } | ||
|
|
||
| int transferred = 0; | ||
| int r = libusb_bulk_transfer(handle, (unsigned char)endpoint, (unsigned char *)buf, len, &transferred, timeout); | ||
|
|
||
| (*env)->ReleaseByteArrayElements(env, data, buf, JNI_ABORT); | ||
| if (r < 0) { | ||
| LOGE("libusb_bulk_transfer write failed: %d", r); | ||
| return r; | ||
| } | ||
| return transferred; | ||
| } | ||
|
|
||
| JNIEXPORT jbyteArray JNICALL | ||
| Java_com_andrerinas_headunitrevived_connection_UsbNative_nativeRead(JNIEnv *env, jobject thiz, jlong handle_ptr, jint endpoint, jint timeout) { | ||
| libusb_device_handle *handle = (libusb_device_handle *)handle_ptr; | ||
|
|
||
| int buf_size = 163840; | ||
| unsigned char *buffer = malloc(buf_size); | ||
| if (!buffer) { | ||
| LOGE("nativeRead: failed to allocate memory"); | ||
| return NULL; | ||
| } | ||
|
|
||
| int transferred = 0; | ||
| int r = libusb_bulk_transfer(handle, (unsigned char)endpoint, buffer, buf_size, &transferred, timeout); | ||
|
|
||
| if (r < 0 && r != LIBUSB_ERROR_TIMEOUT) { | ||
| LOGE("libusb_bulk_transfer read failed: %d", r); | ||
| free(buffer); | ||
| return NULL; | ||
| } | ||
|
|
||
| if (transferred < 0) transferred = 0; | ||
| else if (transferred > buf_size) transferred = buf_size; | ||
|
|
||
| jbyteArray arr = (*env)->NewByteArray(env, transferred); | ||
| if (arr != NULL && transferred > 0) { | ||
| (*env)->SetByteArrayRegion(env, arr, 0, transferred, (jbyte *)buffer); | ||
| } | ||
| free(buffer); | ||
| return arr; | ||
| } |
There was a problem hiding this comment.
Optimizing JNI read/write operations to avoid memory allocations and array copying in the steady state.
nativeWrite: Instead of copying the array in Kotlin usingcopyOfRangewhenbuf.size != length, we pass thelengthparameter directly tonativeWriteand use it inlibusb_bulk_transfer.nativeRead: Instead of allocating a new native buffer withmallocand a new JavajbyteArrayon every single read, we pass a pre-allocatedjbyteArrayfrom Kotlin and read directly into it. This completely eliminates GC pressure and native heap fragmentation during high-bandwidth video streaming.
JNIEXPORT jint JNICALL
Java_com_andrerinas_headunitrevived_connection_UsbNative_nativeWrite(JNIEnv *env, jobject thiz, jlong handle_ptr, jbyteArray data, jint length, jint endpoint, jint timeout) {
libusb_device_handle *handle = (libusb_device_handle *)handle_ptr;
jsize array_len = (*env)->GetArrayLength(env, data);
if (length > array_len) {
length = array_len;
}
jbyte *buf = (*env)->GetByteArrayElements(env, data, NULL);
if (!buf) {
LOGE("nativeWrite: GetByteArrayElements returned NULL");
return -2;
}
int transferred = 0;
int r = libusb_bulk_transfer(handle, (unsigned char)endpoint, (unsigned char *)buf, length, &transferred, timeout);
(*env)->ReleaseByteArrayElements(env, data, buf, JNI_ABORT);
if (r < 0) {
LOGE("libusb_bulk_transfer write failed: %d", r);
return r;
}
return transferred;
}
JNIEXPORT jint JNICALL
Java_com_andrerinas_headunitrevived_connection_UsbNative_nativeRead(JNIEnv *env, jobject thiz, jlong handle_ptr, jbyteArray jbuf, jint endpoint, jint timeout) {
libusb_device_handle *handle = (libusb_device_handle *)handle_ptr;
jsize buf_size = (*env)->GetArrayLength(env, jbuf);
jbyte *buffer = (*env)->GetByteArrayElements(env, jbuf, NULL);
if (!buffer) {
LOGE("nativeRead: GetByteArrayElements returned NULL");
return -2;
}
int transferred = 0;
int r = libusb_bulk_transfer(handle, (unsigned char)endpoint, (unsigned char *)buffer, buf_size, &transferred, timeout);
(*env)->ReleaseByteArrayElements(env, jbuf, buffer, 0);
if (r < 0) {
if (r != LIBUSB_ERROR_TIMEOUT) {
LOGE("libusb_bulk_transfer read failed: %d", r);
return r;
}
return 0;
}
return transferred;
}| fun write(data: ByteArray, timeout: Int): Int { | ||
| if (handlePtr == 0L) return -1 | ||
| return try { | ||
| nativeWrite(handlePtr, data, epOut, timeout) | ||
| } catch (e: Throwable) { | ||
| AppLog.e("UsbNative: Exception during write: ${e.message}") | ||
| -1 | ||
| } | ||
| } | ||
|
|
||
| fun read(timeout: Int): ByteArray? { | ||
| if (handlePtr == 0L) return null | ||
| return try { | ||
| nativeRead(handlePtr, epIn, timeout) | ||
| } catch (e: Throwable) { | ||
| AppLog.e("UsbNative: Exception during read: ${e.message}") | ||
| null | ||
| } | ||
| } |
There was a problem hiding this comment.
Update the write and read helper methods to match the optimized JNI signatures that accept a pre-allocated buffer and a length parameter.
| fun write(data: ByteArray, timeout: Int): Int { | |
| if (handlePtr == 0L) return -1 | |
| return try { | |
| nativeWrite(handlePtr, data, epOut, timeout) | |
| } catch (e: Throwable) { | |
| AppLog.e("UsbNative: Exception during write: ${e.message}") | |
| -1 | |
| } | |
| } | |
| fun read(timeout: Int): ByteArray? { | |
| if (handlePtr == 0L) return null | |
| return try { | |
| nativeRead(handlePtr, epIn, timeout) | |
| } catch (e: Throwable) { | |
| AppLog.e("UsbNative: Exception during read: ${e.message}") | |
| null | |
| } | |
| } | |
| fun write(data: ByteArray, length: Int, timeout: Int): Int { | |
| if (handlePtr == 0L) return -1 | |
| return try { | |
| nativeWrite(handlePtr, data, length, epOut, timeout) | |
| } catch (e: Throwable) { | |
| AppLog.e("UsbNative: Exception during write: ${e.message}") | |
| -1 | |
| } | |
| } | |
| fun read(buffer: ByteArray, timeout: Int): Int { | |
| if (handlePtr == 0L) return -1 | |
| return try { | |
| nativeRead(handlePtr, buffer, epIn, timeout) | |
| } catch (e: Throwable) { | |
| AppLog.e("UsbNative: Exception during read: ${e.message}") | |
| -1 | |
| } | |
| } |
| private external fun nativeWrite(handle: Long, data: ByteArray, endpoint: Int, timeout: Int): Int | ||
| private external fun nativeRead(handle: Long, endpoint: Int, timeout: Int): ByteArray? |
There was a problem hiding this comment.
Update the external JNI declarations to support the optimized zero-allocation read and write operations.
| private external fun nativeWrite(handle: Long, data: ByteArray, endpoint: Int, timeout: Int): Int | |
| private external fun nativeRead(handle: Long, endpoint: Int, timeout: Int): ByteArray? | |
| private external fun nativeWrite(handle: Long, data: ByteArray, length: Int, endpoint: Int, timeout: Int): Int | |
| private external fun nativeRead(handle: Long, buffer: ByteArray, endpoint: Int, timeout: Int): Int |
| // Leftover buffer — serves read data without truncation | ||
| private var leftoverBuffer: ByteArray? = null | ||
| private var leftoverPos = 0 |
There was a problem hiding this comment.
Pre-allocate a single reusable buffer of 163840 bytes to avoid allocating 160KB on every single read operation.
| // Leftover buffer — serves read data without truncation | |
| private var leftoverBuffer: ByteArray? = null | |
| private var leftoverPos = 0 | |
| // Leftover buffer — serves read data without truncation | |
| private val readBuffer = ByteArray(163840) | |
| private var leftoverSize = 0 | |
| private var leftoverPos = 0 |
| fun wrap(connection: UsbDeviceConnection, epInAddr: Int, epOutAddr: Int): Boolean { | ||
| epIn = epInAddr | ||
| epOut = epOutAddr | ||
| if (contextPtr == 0L) { | ||
| AppLog.e("UsbNative: Cannot wrap device, context is invalid") | ||
| return false | ||
| } | ||
| try { | ||
| handlePtr = wrapDevice(contextPtr, connection.fileDescriptor) | ||
| if (handlePtr == 0L) { | ||
| AppLog.e("UsbNative: wrapDevice returned NULL handle") | ||
| return false | ||
| } | ||
| detachKernel(handlePtr, 0) | ||
| claimInterface(handlePtr, 0) | ||
| AppLog.i("UsbNative: Wrapped device fd=${connection.fileDescriptor} successfully") | ||
| return true | ||
| } catch (e: Throwable) { | ||
| AppLog.e("UsbNative: Exception wrapping device: ${e.message}") | ||
| return false | ||
| } | ||
| } |
There was a problem hiding this comment.
Instead of hardcoding interface ID 0 for detachKernel and claimInterface, pass the actual interface ID from LibusbAccessoryConnection to make the wrapping logic more robust and compatible with devices that might use non-zero interface indices.
fun wrap(connection: UsbDeviceConnection, interfaceId: Int, epInAddr: Int, epOutAddr: Int): Boolean {
epIn = epInAddr
epOut = epOutAddr
if (contextPtr == 0L) {
AppLog.e("UsbNative: Cannot wrap device, context is invalid")
return false
}
try {
handlePtr = wrapDevice(contextPtr, connection.fileDescriptor)
if (handlePtr == 0L) {
AppLog.e("UsbNative: wrapDevice returned NULL handle")
return false
}
detachKernel(handlePtr, interfaceId)
claimInterface(handlePtr, interfaceId)
AppLog.i("UsbNative: Wrapped device fd=${connection.fileDescriptor} successfully")
return true
} catch (e: Throwable) {
AppLog.e("UsbNative: Exception wrapping device: ${e.message}")
return false
}
}| fun close() { | ||
| if (handlePtr != 0L) { | ||
| try { | ||
| closeDevice(handlePtr) | ||
| } catch (e: Throwable) { | ||
| AppLog.e("UsbNative: Exception during closeDevice: ${e.message}") | ||
| } | ||
| handlePtr = 0 | ||
| } | ||
| if (contextPtr != 0L) { | ||
| try { | ||
| exitContext(contextPtr) | ||
| } catch (e: Throwable) { | ||
| AppLog.e("UsbNative: Exception during exitContext: ${e.message}") | ||
| } | ||
| contextPtr = 0 | ||
| } | ||
| } |
There was a problem hiding this comment.
Implement a finalize() method to ensure native resources (libusb context and device handle) are safely closed and released if the UsbNative instance is garbage collected without close() being called explicitly.
fun close() {
if (handlePtr != 0L) {
try {
closeDevice(handlePtr)
} catch (e: Throwable) {
AppLog.e("UsbNative: Exception during closeDevice: ${e.message}")
}
handlePtr = 0
}
if (contextPtr != 0L) {
try {
exitContext(contextPtr)
} catch (e: Throwable) {
AppLog.e("UsbNative: Exception during exitContext: ${e.message}")
}
contextPtr = 0
}
}
protected fun finalize() {
if (contextPtr != 0L || handlePtr != 0L) {
AppLog.w("UsbNative: Native resources were not closed explicitly!")
close()
}
}| val native = UsbNative() | ||
| if (!native.wrap(conn, endpointIn!!.address, endpointOut!!.address)) { | ||
| AppLog.e("LibusbAccessoryConnection: Failed to wrap USB device via JNI") | ||
| native.close() | ||
| conn.releaseInterface(usbInterface) | ||
| conn.close() | ||
| usbDeviceConnection = null | ||
| return@withContext false | ||
| } |
There was a problem hiding this comment.
Pass the actual usbInterface.id to native.wrap instead of hardcoding 0 to support devices with non-zero interface indices.
| val native = UsbNative() | |
| if (!native.wrap(conn, endpointIn!!.address, endpointOut!!.address)) { | |
| AppLog.e("LibusbAccessoryConnection: Failed to wrap USB device via JNI") | |
| native.close() | |
| conn.releaseInterface(usbInterface) | |
| conn.close() | |
| usbDeviceConnection = null | |
| return@withContext false | |
| } | |
| val native = UsbNative() | |
| if (!native.wrap(conn, usbInterface!!.id, endpointIn!!.address, endpointOut!!.address)) { | |
| AppLog.e("LibusbAccessoryConnection: Failed to wrap USB device via JNI") | |
| native.close() | |
| conn.releaseInterface(usbInterface) | |
| conn.close() | |
| usbDeviceConnection = null | |
| return@withContext false | |
| } |
| var instance: App? = null | ||
| private set | ||
|
|
||
| private var activeActivityRef = java.lang.ref.WeakReference<android.app.Activity>(null) |
There was a problem hiding this comment.
Mark activeActivityRef as @Volatile to ensure thread safety and visibility across threads, as getActiveActivityDisplayId() can be called from background threads (e.g., in AapService or CommManager).
| private var activeActivityRef = java.lang.ref.WeakReference<android.app.Activity>(null) | |
| @Volatile | |
| private var activeActivityRef = java.lang.ref.WeakReference<android.app.Activity>(null) |
…creenFragment.kt Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a native USB driver alternative using libusb to improve device compatibility, adding JNI wrappers, CMake configurations, and a toggle setting. While the integration is well-structured, several critical issues must be addressed: a coarse-grained lock in LibusbAccessoryConnection.connect() over blocking I/O and sleep retries poses an ANR risk; closing the native device while a read is blocked in recvBlocking can trigger a native use-after-free crash (SIGSEGV); and using DirectByteBuffer instead of ByteArray is recommended to optimize high-throughput JNI transfers. Additionally, a duplicate key in SettingsBackupManager should be removed, and the useLibusb setting should be conditionally displayed only on Android Lollipop (API 21) and above to prevent compatibility issues.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| override suspend fun connect() = withContext(Dispatchers.IO) { | ||
| synchronized(sStateLock) { | ||
| if (isConnectedVal) { | ||
| disconnect() | ||
| } |
There was a problem hiding this comment.
Critical Synchronization Issue
Holding synchronized(sStateLock) over the entire connect() method is extremely dangerous because it performs blocking I/O (usbMgr.openDevice) and contains a retry loop with Thread.sleep(1000).
If disconnect() is called from another thread (e.g., the main thread to abort a connection attempt), it will block waiting for sStateLock to be released. This can easily lead to Application Not Responding (ANR) crashes and prevents clean cancellation of pending connection attempts.
Recommendation
Avoid holding a coarse-grained lock over long-running or blocking operations. Instead:
- Use a Kotlin coroutine-based
Mutexfor non-blocking synchronization of the connection state. - Or, use thread-safe state variables (like
AtomicBooleanor@Volatile) and only synchronize brief, critical sections of code. - Ensure that
disconnect()can run immediately to close any active or pending resources without being blocked byconnect().
| override fun disconnect() { | ||
| synchronized(sStateLock) { | ||
| isConnectedVal = false | ||
| try { | ||
| usbNative?.close() | ||
| } catch (e: Exception) { | ||
| AppLog.e("LibusbAccessoryConnection: Error closing native: ${e.message}") | ||
| } | ||
| usbNative = null |
There was a problem hiding this comment.
Native Use-After-Free / Race Condition
Calling usbNative?.close() immediately inside disconnect() while the reader thread is actively blocked inside recvBlocking -> native.read(...) will cause a native crash (SIGSEGV).
When usbNative?.close() is called, it executes closeDevice(handlePtr) and exitContext(contextPtr), which frees the underlying libusb structures. If the reader thread is currently executing libusb_bulk_transfer in JNI, it will access freed memory, leading to undefined behavior or an immediate crash.
Recommendation
- Set
isConnectedVal = falsefirst to signal the reader thread to exit. - Wait for the reader thread to completely exit (e.g., by joining its thread or coroutine job) before calling
usbNative?.close(). - Ensure that JNI methods are robust against null/zero pointers by adding defensive checks in the C code (e.g., checking if
handleis NULL before calling libusb functions).
| fun read(buffer: ByteArray, timeout: Int): Int { | ||
| if (handlePtr == 0L) return -1 | ||
| return try { | ||
| nativeRead(handlePtr, buffer, epIn, timeout) | ||
| } catch (e: Throwable) { | ||
| AppLog.e("UsbNative: Exception during read: ${e.message}") | ||
| -1 | ||
| } | ||
| } |
There was a problem hiding this comment.
Performance Optimization: Use DirectByteBuffer
Passing a large ByteArray (160KB) to JNI and calling GetByteArrayElements on every read operation introduces significant overhead due to potential array copying and pinning/unpinning by the JVM.
Since this is a high-throughput USB read loop for streaming video/audio, you can drastically improve efficiency and avoid garbage collection pressure by using a Direct ByteBuffer.
Suggested Refactoring
- Allocate a direct byte buffer in Kotlin:
private val readBuffer = ByteBuffer.allocateDirect(163840)
- Update the JNI signature to accept
ByteBuffer:private external fun nativeRead(handle: Long, buffer: ByteBuffer, endpoint: Int, timeout: Int): Int
- In C, retrieve the direct buffer address in O(1) time without copying:
void *buffer = (*env)->GetDirectBufferAddress(env, jbuf); jlong capacity = (*env)->GetDirectBufferCapacity(env, jbuf); // Use buffer and capacity directly in libusb_bulk_transfer
| "bluetooth-manager-service-name" to ValueType.STRING, | ||
| "use-libusb" to ValueType.BOOLEAN, | ||
| "bluetooth-manager-service-name" to ValueType.STRING |
| items.add(SettingItem.ToggleSettingEntry( | ||
| stableId = "useLibusb", | ||
| nameResId = R.string.use_libusb, | ||
| descriptionResId = R.string.use_libusb_description, | ||
| isChecked = pendingUseLibusb!!, | ||
| onCheckedChanged = { isChecked -> | ||
| pendingUseLibusb = isChecked | ||
| checkChanges() | ||
| updateSettingsList() | ||
| } | ||
| )) |
There was a problem hiding this comment.
Since libusb requires Android Lollipop (API 21) or higher (as indicated by the suppressed NDK min SDK error), this setting should only be displayed on supported devices to prevent users on older platforms from enabling a broken feature.
| items.add(SettingItem.ToggleSettingEntry( | |
| stableId = "useLibusb", | |
| nameResId = R.string.use_libusb, | |
| descriptionResId = R.string.use_libusb_description, | |
| isChecked = pendingUseLibusb!!, | |
| onCheckedChanged = { isChecked -> | |
| pendingUseLibusb = isChecked | |
| checkChanges() | |
| updateSettingsList() | |
| } | |
| )) | |
| if (android.os.Build.VERSION.SDK_INT >= android.os.Build.VERSION_CODES.LOLLIPOP) { | |
| items.add(SettingItem.ToggleSettingEntry( | |
| stableId = "useLibusb", | |
| nameResId = R.string.use_libusb, | |
| descriptionResId = R.string.use_libusb_description, | |
| isChecked = pendingUseLibusb!!, | |
| onCheckedChanged = { isChecked -> | |
| pendingUseLibusb = isChecked | |
| checkChanges() | |
| updateSettingsList() | |
| } | |
| )) | |
| } |