diff --git a/app/src/main/java/com/openipc/pixelpilot/WfbLinkManager.java b/app/src/main/java/com/openipc/pixelpilot/WfbLinkManager.java index a0f552b5..5f1a82cb 100644 --- a/app/src/main/java/com/openipc/pixelpilot/WfbLinkManager.java +++ b/app/src/main/java/com/openipc/pixelpilot/WfbLinkManager.java @@ -133,6 +133,10 @@ public Map getAttachedAdapters() { public synchronized void refreshAdapters() { Map attachedAdapters = getAttachedAdapters(); + if (attachedAdapters == null) { + Log.e(TAG, "Could not read the usb device filter, skipping adapter refresh."); + return; + } boolean missingPermissions = false; android.hardware.usb.UsbManager usbManager = @@ -141,8 +145,13 @@ public synchronized void refreshAdapters() { if (!usbManager.hasPermission(entry.getValue())) { binding.tvMessage.setVisibility(View.VISIBLE); binding.tvMessage.setText("No permission for wifi adapter(s) " + entry.getValue().getDeviceName()); + // Android 14 refuses to deliver a PendingIntent built from an implicit + // intent to a runtime registered receiver, so the permission result never + // arrives unless the package is set explicitly. + Intent permissionIntent = new Intent(WfbLinkManager.ACTION_USB_PERMISSION); + permissionIntent.setPackage(context.getPackageName()); PendingIntent pendingIntent = PendingIntent.getBroadcast(context, 0, - new Intent(WfbLinkManager.ACTION_USB_PERMISSION), PendingIntent.FLAG_IMMUTABLE); + permissionIntent, PendingIntent.FLAG_IMMUTABLE); usbManager.requestPermission(entry.getValue(), pendingIntent); missingPermissions = true; } @@ -164,16 +173,27 @@ public synchronized void refreshAdapters() { } // Starts newly attached adapters. + boolean startFailed = false; for (Map.Entry entry : attachedAdapters.entrySet()) { if (activeWifiAdapters.containsKey(entry.getKey())) { continue; } - startAdapter(entry.getValue()); - activeWifiAdapters.put(entry.getKey(), entry.getValue()); + // Only track it as active if it actually came up, otherwise a failed adapter + // is never retried on the next refresh. + if (startAdapter(entry.getValue())) { + activeWifiAdapters.put(entry.getKey(), entry.getValue()); + } else { + startFailed = true; + } } if (activeWifiAdapters.isEmpty()) { - String text = "No compatible wifi adapter found."; + // Now that a failed start no longer counts as active, an empty map covers two + // different problems, and blaming the filter for both sends people looking in + // the wrong place. + String text = startFailed + ? "Wifi adapter found but could not be started - see the log." + : "No compatible wifi adapter found."; binding.tvMessage.setText(text); binding.tvMessage.setVisibility(View.VISIBLE); @@ -217,7 +237,10 @@ public synchronized boolean startAdapter(UsbDevice dev) { String text = "Starting wfb-ng channel " + wifiChannel + " with " + String.format( "[%04X", dev.getVendorId()) + ":" + String.format("%04X]", dev.getProductId()); binding.tvMessage.setText(text); - wfbLink.start(wifiChannel, bandWidth.getValue(), dev); + if (!wfbLink.start(wifiChannel, bandWidth.getValue(), dev)) { + binding.tvMessage.setText("Could not open wifi adapter " + dev.getDeviceName()); + return false; + } return true; } } diff --git a/app/wfbngrtl8812/src/main/cpp/WfbngLink.cpp b/app/wfbngrtl8812/src/main/cpp/WfbngLink.cpp index e8d9c14c..2e374a7b 100644 --- a/app/wfbngrtl8812/src/main/cpp/WfbngLink.cpp +++ b/app/wfbngrtl8812/src/main/cpp/WfbngLink.cpp @@ -34,12 +34,6 @@ #undef TAG #define TAG "pixelpilot" -#define CRASH() \ - do { \ - int *i = 0; \ - *i = 42; \ - } while (0) - std::string generate_random_string(size_t length) { const std::string characters = "abcdefghijklmnopqrstuvwxyz"; std::random_device rd; @@ -92,6 +86,7 @@ void WfbngLink::initAgg() { int WfbngLink::run(JNIEnv *env, jobject context, jint wifiChannel, jint bw, jint fd) { int r; libusb_context *ctx = NULL; + clear_stop_request(fd); txFrame = std::make_shared(); r = libusb_set_option(NULL, LIBUSB_OPTION_NO_DEVICE_DISCOVERY); @@ -138,6 +133,14 @@ int WfbngLink::run(JNIEnv *env, jobject context, jint wifiChannel, jint bw, jint return -1; } + if (stop_requested(fd)) { + __android_log_print(ANDROID_LOG_WARN, TAG, "stop requested for fd=%d before bring-up, aborting", fd); + rtl_devices.erase(fd); + libusb_release_interface(dev_handle, 0); + libusb_exit(ctx); + return -1; + } + uint8_t *video_channel_id_be8 = reinterpret_cast(&video_channel_id_be); uint8_t *udp_channel_id_be8 = reinterpret_cast(&udp_channel_id_be); uint8_t *mavlink_channel_id_be8 = reinterpret_cast(&mavlink_channel_id_be); @@ -246,7 +249,12 @@ int WfbngLink::run(JNIEnv *env, jobject context, jint wifiChannel, jint bw, jint // Blocking RX loop on this thread; devourer pumps the libusb events // itself. Returns once StopRxLoop() is called. - current_device->StartRxLoop(packetProcessor); + if (stop_requested(fd)) { + __android_log_print( + ANDROID_LOG_WARN, TAG, "stop requested for fd=%d during bring-up, not entering the rx loop", fd); + } else { + current_device->StartRxLoop(packetProcessor); + } } catch (const std::runtime_error &error) { __android_log_print(ANDROID_LOG_ERROR, TAG, "runtime_error: %s", error.what()); txFrame->stop(); @@ -282,9 +290,13 @@ int WfbngLink::run(JNIEnv *env, jobject context, jint wifiChannel, jint bw, jint } void WfbngLink::stop(JNIEnv *env, jobject context, jint fd) { + // Recorded first, and whether or not the device exists yet: run() may not have got as far + // as creating it, and StartRxLoop() would clear the flag set below anyway. + note_stop_requested(fd); if (rtl_devices.find(fd) == rtl_devices.end()) { - __android_log_print(ANDROID_LOG_ERROR, TAG, "rtl_devices.find(%d) == rtl_devices.end()", fd); - CRASH(); + // Happens when the adapter was already gone by the time the stop arrived, e.g. it + // was unplugged or the hub re-enumerated it. Nothing left to stop. + __android_log_print(ANDROID_LOG_WARN, TAG, "stop: no rtl device for fd=%d, already gone", fd); return; } auto dev = rtl_devices.at(fd).get(); diff --git a/app/wfbngrtl8812/src/main/cpp/WfbngLink.hpp b/app/wfbngrtl8812/src/main/cpp/WfbngLink.hpp index 24f74c5b..e4b8b8c2 100644 --- a/app/wfbngrtl8812/src/main/cpp/WfbngLink.hpp +++ b/app/wfbngrtl8812/src/main/cpp/WfbngLink.hpp @@ -18,6 +18,7 @@ extern "C" { #include #include #include +#include #include #include // Added for std::vector @@ -59,6 +60,32 @@ class WfbngLink { bool stbc_enabled{true}; std::map> rtl_devices; + + // Set by stop() and read by run(). A StopRxLoop() only takes effect once the RX loop is + // running: RtlJaguarDevice::StartRxLoop() clears should_stop on entry, so a stop that + // lands anywhere before that - including the whole chip bring-up in InitWrite(), which + // is the longest part of run() - is thrown away, and run() then blocks in a loop nobody + // asked for. Recorded here instead, so run() can see it at the points where the flag + // itself cannot be trusted. + std::mutex stop_requested_mutex; + std::set stop_requested_fds; + + void note_stop_requested(int fd) { + std::lock_guard lock(stop_requested_mutex); + stop_requested_fds.insert(fd); + } + + // Cleared at the start of run(): fd numbers are reused, so a request left over from a + // previous session on the same number must not abort the new one. + void clear_stop_request(int fd) { + std::lock_guard lock(stop_requested_mutex); + stop_requested_fds.erase(fd); + } + + bool stop_requested(int fd) { + std::lock_guard lock(stop_requested_mutex); + return stop_requested_fds.count(fd) > 0; + } std::unique_ptr link_quality_thread{nullptr}; bool should_clear_stats{false}; FecChangeController fec; diff --git a/app/wfbngrtl8812/src/main/java/com/openipc/wfbngrtl8812/WfbNgLink.java b/app/wfbngrtl8812/src/main/java/com/openipc/wfbngrtl8812/WfbNgLink.java index 9717c4b2..cf0fd101 100644 --- a/app/wfbngrtl8812/src/main/java/com/openipc/wfbngrtl8812/WfbNgLink.java +++ b/app/wfbngrtl8812/src/main/java/com/openipc/wfbngrtl8812/WfbNgLink.java @@ -9,6 +9,7 @@ import androidx.annotation.Keep; import java.util.HashMap; +import java.util.Iterator; import java.util.Map; import java.util.Timer; import java.util.TimerTask; @@ -91,31 +92,108 @@ public void nativeSetUseStbc(int use) { nativeSetUseStbc(nativeWfbngLink, use); } - public synchronized void start(int wifiChannel, int bandWidth, UsbDevice usbDevice) { + public synchronized boolean start(int wifiChannel, int bandWidth, UsbDevice usbDevice) { + // linkThreads.put() below overwrites the entry for a device, which would orphan an + // older thread so that stopAll() never joins it and the interface is never released. + Thread existing = linkThreads.get(usbDevice); + if (existing != null && existing.isAlive()) { + Log.w(TAG, "wfb-ng already running on " + usbDevice.getDeviceName() + + ", not starting a second"); + return true; + } + if (existing != null) { + // A thread that outlived its join has since finished, so the connection stop() + // deliberately left open can go now. Otherwise the put() below would drop the + // last reference to it and leak the fd. + UsbDeviceConnection stale = linkConns.remove(usbDevice); + if (stale != null) { + Log.d(TAG, "releasing the usb connection left behind on " + usbDevice.getDeviceName()); + stale.close(); + } + linkThreads.remove(usbDevice); + } Log.d(TAG, "wfb-ng monitoring on " + usbDevice.getDeviceName() + " using wifi channel " + wifiChannel); UsbManager usbManager = (UsbManager) context.getSystemService(Context.USB_SERVICE); + // Returns null when the permission was revoked or the device disappeared between + // the permission check and here, which is easy to hit on a re-enumerating hub. UsbDeviceConnection usbDeviceConnection = usbManager.openDevice(usbDevice); + if (usbDeviceConnection == null) { + Log.e(TAG, "Could not open " + usbDevice.getDeviceName() + " (no permission or already gone)"); + return false; + } int fd = usbDeviceConnection.getFileDescriptor(); + if (fd < 0) { + Log.e(TAG, "Invalid file descriptor for " + usbDevice.getDeviceName()); + usbDeviceConnection.close(); + return false; + } Thread t = new Thread(() -> nativeRun(nativeWfbngLink, context, wifiChannel, bandWidth, fd)); - t.setName("wfb-" + usbDevice.getDeviceName().split("/dev/bus/usb/")[1]); + t.setName(threadNameFor(usbDevice)); linkThreads.put(usbDevice, t); linkConns.put(usbDevice, usbDeviceConnection); - linkThreads.get(usbDevice).start(); + t.start(); Log.d(TAG, "wfb-ng thread on " + usbDevice.getDeviceName() + " started."); + return true; + } + + private static String threadNameFor(UsbDevice usbDevice) { + String name = usbDevice.getDeviceName(); + String[] parts = name.split("/dev/bus/usb/"); + return "wfb-" + (parts.length > 1 ? parts[1] : name); + } + + /** + * The RX loop is joined so the USB interface is released before anything reopens it, but + * these calls come from Activity lifecycle callbacks on the main thread, where an + * unbounded join is a five second ANR waiting to happen. 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 interface and exit libusb - so the wait has to be + * generous, but bounded. + */ + private static final long JOIN_TIMEOUT_MS = 3000; + + /** + * @return true if the thread is gone and its usb connection can be released. + */ + private static boolean joinBounded(Thread t, String what) throws InterruptedException { + if (t == null) { + return true; + } + t.join(JOIN_TIMEOUT_MS); + if (t.isAlive()) { + // Nothing else may be torn down while the thread is still inside devourer. + // libusb_wrap_sys_device() keeps the fd it is given rather than duplicating it, + // so closing the UsbDeviceConnection now pulls the fd out from under a libusb + // that is still polling it. The kernel cancels the URBs on close but libusb + // never reaps them - op_handle_events() looks at POLLERR and not POLLNVAL - so + // poll() returns immediately, forever, and the loop spins on one core waiting + // for a transfer count that will never drop. Both map entries stay too, so + // start() can still see the thread and refuse a second RX loop on the device. + Log.e(TAG, "wfb-ng thread on " + what + " did not stop within " + JOIN_TIMEOUT_MS + + "ms, leaving it and its usb connection in place"); + return false; + } + Log.d(TAG, "wfb-ng thread on " + what + " done."); + return true; } public synchronized void stopAll() throws InterruptedException { for (Map.Entry entry : linkConns.entrySet()) { nativeStop(nativeWfbngLink, context, entry.getValue().getFileDescriptor()); } - for (Map.Entry entry : linkConns.entrySet()) { - Thread t = linkThreads.get(entry.getKey()); - if (t != null) { - t.join(); + Iterator> it = linkConns.entrySet().iterator(); + while (it.hasNext()) { + Map.Entry entry = it.next(); + UsbDevice dev = entry.getKey(); + if (!joinBounded(linkThreads.get(dev), dev.getDeviceName())) { + continue; } - Log.d(TAG, "wfb-ng thread on " + entry.getKey().getDeviceName() + " done."); + // Without close() every attach/detach cycle leaks the fd openDevice() handed + // out, until the process runs out - but only once nothing is using it. + linkThreads.remove(dev); + it.remove(); + entry.getValue().close(); } - linkThreads.clear(); } public synchronized void stop(UsbDevice dev) throws InterruptedException { @@ -125,11 +203,12 @@ public synchronized void stop(UsbDevice dev) throws InterruptedException { } int fd = conn.getFileDescriptor(); nativeStop(nativeWfbngLink, context, fd); - Thread t = linkThreads.get(dev); - if (t != null) { - t.join(); + if (!joinBounded(linkThreads.get(dev), dev.getDeviceName())) { + return; } linkThreads.remove(dev); + linkConns.remove(dev); + conn.close(); } public void SetWfbNGStatsChanged(final WfbNGStatsChanged callback) {