Harden the USB adapter lifecycle - #116
Conversation
PR Summary by QodoHarden USB adapter lifecycle and recovery
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
"No compatible wifi adapter found." was shown whenever no adapter ended up running, which covers three unrelated problems: nothing plugged in, a dongle whose id is not in usb_device_filter.xml, and a dongle that was found but failed to start. Only the middle one is what the message claims, and the last one became reachable when OpenIPC#116 stopped recording an adapter as active unless it actually came up. - getAttachedAdapters() logs every attached device with VID:PID, manufacturer and product name, and whether the filter matched. That id is exactly what a bug report needs in order to add an adapter, and there is no other way to read it - sysfs is not accessible to the shell on Horizon OS and dumpsys usb does not print host devices there. - the message now distinguishes "none attached", "found but could not be started - see the log" and "waiting for permission". Also: joinBounded() now releases the device claim even when the driver thread outstays the timeout. Holding it would leave the adapter unopenable until the process restarts, and the user would be told there is no compatible adapter - a worse failure than the duplicate RX loop it guards against, which the per-instance check and the single-owner handoff already cover.
|
@iflyhere thank you for the PR! I checked Qodo's comment, it was correct, and the bounded join fixes the ANR half of it. The other half is still there though, see below. could you please fix and I'll merge the PR
|
Four separate ways the adapter path can take the app down or wedge it. All of them are easy to hit on a powered hub that re-enumerates the dongle, which is how a lot of ground stations are wired. 1. Deliberate null deref. WfbngLink::stop() ran a CRASH() macro (`int *i = 0; *i = 42;`) when the fd was no longer in rtl_devices. That is a recoverable state - the adapter was already gone - and it killed the process. Removed, now a warning and return. 2. NPE on openDevice(). UsbManager.openDevice() returns null when the permission was revoked or the device disappeared between the permission check and the open; getFileDescriptor() was called on it unconditionally. start() now returns false instead, WfbLinkManager reports it and leaves the adapter out of activeWifiAdapters so the next refresh retries it. Before, a failed adapter was recorded as active and never retried. 3. Leaked usbfs descriptors. UsbDeviceConnection was never closed and linkConns was never cleared, so every attach/detach cycle leaked one fd plus the map entry. 4. USB permission dialog on Android 14. requestPermission() got a PendingIntent built from an implicit Intent. Android 14 refuses to deliver those to a runtime registered receiver, so the result never arrived and the app sat on "No permission for wifi adapter(s)". setPackage() added. Also: refreshAdapters() dereferenced getAttachedAdapters() without checking for the null it returns when the device filter fails to parse, and the wfb thread name indexed split()[1] without checking the device name matched /dev/bus/usb/.
Found on a Quest 3 while the app was unresponsive: the main thread was asleep inside stopAll()'s t.join() and Android killed the window with Input dispatching timed out ... Waited 5000ms for MotionEvent ANR in com.openipc.pixelpilot (com.openipc.pixelpilot/.VideoActivity) stopAdapters() is called from onPause(), onStop() and the channel/bandwidth menus, so this join runs on the main thread. StopRxLoop() only breaks the receive loop; the thread then still has to stop the TX frame and the adaptive link, power the chip down, release the USB interface and exit libusb. If any of that does not come back, the UI is frozen until the watchdog fires. The join is now bounded at 3000 ms - about what a healthy unwind needs - and logs when a thread outstays it instead of hanging the UI. Also: start() refuses a device that already has a live thread. linkThreads.put() overwrites the entry, so an older thread would be orphaned, never joined, and its interface never released.
Recording an adapter as active only when it actually came up means an empty activeWifiAdapters now covers two different problems: nothing compatible is attached, or something compatible is attached and could not be opened. Showing "No compatible wifi adapter found." for both sends people looking for a usb_device_filter.xml entry that is already there.
f0599eb to
901c0e8
Compare
The bounded join fixed the ANR but not the reason the join was timing out in the first place, as pointed out in review. StopRxLoop() only sets a flag, and RtlJaguarDevice::StartRxLoop() clears it on entry. So a stop is thrown away anywhere between the fd being handed to run() and the loop actually starting - which includes the whole chip bring-up in InitWrite(), the longest part of run(). Until CreateRtlDevice() there is not even an entry in rtl_devices for stop() to find, so it returns "already gone" and does nothing at all. run() then blocks in a loop nobody asked for. stop() now records the fd in stop_requested_fds before anything else, and run() checks it at the two points where the flag itself cannot be trusted: after CreateRtlDevice(), and again immediately before entering the loop. Skipping the loop falls through to the same teardown a StopRxLoop() would have taken. run() clears the entry on the way in, because fd numbers are reused and a stale request must not abort a new session. This narrows the window to a few instructions rather than closing it - closing it needs devourer to stop clearing the flag. The second half was the timeout path itself. libusb_wrap_sys_device() keeps the fd it is given rather than duplicating it - the comment claiming otherwise was wrong - so closing the UsbDeviceConnection after a timed-out join pulled the fd out from under a libusb that was still polling it. The kernel cancels the URBs on close, but libusb never reaps them, because op_handle_events() checks POLLERR and not POLLNVAL: poll() then returns immediately forever and the loop spins on one core waiting for a transfer count that never drops. Dropping the map entries at the same time hid it from the duplicate check in start(), so the next openDevice() would most likely be handed the same fd number back and overwrite rtl_devices[fd] underneath the spinning thread. So a timed-out join now leaves both the thread and its connection in place. start() refuses a second RX loop on that device, and releases the connection once the old thread has actually finished. Still worth a follow-up: 3s of join on the main thread from onPause is under the ANR limit but visible. Moving the stop off the main thread would remove it. OpenIPC#105 touches WfbngLink::stop too, so whichever lands second will need a rebase.
|
You're right on both counts, and the The lost-stop window. I confirmed the devourer side on the pinned submodule — /* Restartable: clear any stop request left by a prior StopRxLoop(). */
should_stop = false;so as you say this narrows the window to a few instructions rather than closing it. Closing it The fd close. Corrected — The reason to keep the fd open rather than close it is exactly the On the follow-up: agreed that 3 s of join on the main thread is still visible even though On #105: noted, and I'd suggest #105 goes first. It has the wider change to Compile tested for arm64-v8a + armeabi-v7a. |
2 participants
Note
Compile tested only (arm64-v8a + armeabi-v7a). The happy path is unchanged;
what changes is what happens when the adapter is not there. Testing with a
hub that re-enumerates the dongle, and with the permission dialog dismissed,
would be the useful check.
Four separate ways the adapter path can take the app down or wedge it. All of
them are easy to hit on a powered OTG hub that re-enumerates the dongle, which is
how a lot of ground stations are wired — and a crash here means going blind mid
flight.
1. Deliberate null dereference
WfbngLink.cppdefinesand runs it in
WfbngLink::stop()when the fd is no longer inrtl_devices.That is a recoverable state — the adapter was already gone — and it kills the
process. Removed; now a warning and a return.
2. NPE on
openDevice()openDevice()returnsnullwhen the permission was revoked or the devicedisappeared between
hasPermission()and here.WfbNgLink.start()now returnsboolean, andWfbLinkManager.startAdapter()reports the failure instead ofcrashing.
That also fixes a second-order bug:
refreshAdapters()used to add the device toactiveWifiAdaptersunconditionally, so an adapter that failed to start wasrecorded as running and never retried on a later refresh. It is only tracked now
if it actually came up.
3. Leaked usbfs descriptors
UsbDeviceConnectionwas neverclose()d andlinkConnswas never cleared, soevery attach/detach cycle leaked one file descriptor plus the map entry. Both
stop()andstopAll()now close and remove.4. USB permission dialog on Android 14
The intent is implicit. Since Android 14 a
PendingIntentbuilt from an implicitintent is not delivered to a runtime-registered receiver, so the permission
result never arrives and the app sits on "No permission for wifi adapter(s)"
even after the user granted it.
setPackage(context.getPackageName())added.Also
refreshAdapters()dereferencedgetAttachedAdapters()without checking thenullit returns whenusb_device_filter.xmlfails to parsesplit("/dev/bus/usb/")[1]without checking thedevice name actually matched
Not in this PR
The wfb-ng RX thread is a plain
new Thread(...)at default priority, eventhough it is the thread pumping libusb. Giving it a realtime-ish priority is
probably worth doing, but it is a behaviour change that deserves its own PR.
Part of a series of independent fixes found while building an immersive (OpenXR) mode on a
Quest 3, each standalone and mergeable in any order:
wirelessInfo()safeVideoPlayer/WfbNgLinktake aContext#113 and #116 are now confirmed on hardware (Quest 3, Horizon OS, Android 14).