Skip to content

Harden the USB adapter lifecycle - #116

Open
iflyhere wants to merge 1 commit into
OpenIPC:masterfrom
iflyhere:fix/usb-adapter-lifecycle
Open

Harden the USB adapter lifecycle#116
iflyhere wants to merge 1 commit into
OpenIPC:masterfrom
iflyhere:fix/usb-adapter-lifecycle

Conversation

@iflyhere

@iflyhere iflyhere commented Aug 27, 2026

Copy link
Copy Markdown

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.cpp defines

#define CRASH()                 \
    do {                        \
        int *i = 0;             \
        *i = 42;               \
    } while (0)

and runs it in WfbngLink::stop() when the fd is no longer in rtl_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()

UsbDeviceConnection usbDeviceConnection = usbManager.openDevice(usbDevice);
int fd = usbDeviceConnection.getFileDescriptor();

openDevice() returns null when the permission was revoked or the device
disappeared between hasPermission() and here. WfbNgLink.start() now returns
boolean, and WfbLinkManager.startAdapter() reports the failure instead of
crashing.

That also fixes a second-order bug: refreshAdapters() used to add the device to
activeWifiAdapters unconditionally, so an adapter that failed to start was
recorded as running and never retried on a later refresh. It is only tracked now
if it actually came up.

3. Leaked usbfs descriptors

UsbDeviceConnection was never close()d and linkConns was never cleared, so
every attach/detach cycle leaked one file descriptor plus the map entry. Both
stop() and stopAll() now close and remove.

4. USB permission dialog on Android 14

PendingIntent.getBroadcast(context, 0,
        new Intent(WfbLinkManager.ACTION_USB_PERMISSION), PendingIntent.FLAG_IMMUTABLE);

The intent is implicit. Since Android 14 a PendingIntent built from an implicit
intent 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() dereferenced getAttachedAdapters() without checking the
    null it returns when usb_device_filter.xml fails to parse
  • the wfb thread name indexed split("/dev/bus/usb/")[1] without checking the
    device name actually matched

Not in this PR

The wfb-ng RX thread is a plain new Thread(...) at default priority, even
though 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 small series of independent fixes found while profiling the receive path.
Each one is standalone and mergeable on its own, in any order — no dependencies
between them, and no shared files except VideoActivity.java / VideoPlayer.*,
which touch different methods:

All five compile clean for arm64-v8a + armeabi-v7a.

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/.
@qodo-free-for-open-source-projects

Copy link
Copy Markdown

PR Summary by Qodo

Harden USB adapter lifecycle and recovery

🐞 Bug fix 🕐 20-40 Minutes

Grey Divider

AI Description

• Gracefully handles missing, revoked, or re-enumerated USB adapters without process crashes.
• Retries failed adapter starts and fixes Android 14 permission-result delivery.
• Closes USB connections and clears lifecycle state during individual and bulk shutdown.
Diagram

sequenceDiagram
    participant B as USB Broadcast
    participant M as Link Manager
    participant U as USB Manager
    participant J as Java Link
    participant N as Native Link
    B->>M: Refresh adapters
    M->>U: Check permission
    alt Permission missing
        M->>U: Request explicit intent
    else Permission granted
        M->>J: Start adapter
        J->>U: Open device
        alt Open succeeds
            J->>N: Run with fd
            M-->>M: Track active
        else Open fails
            J-->>M: Return false
        end
    end
    B->>M: Detach refresh
    M->>J: Stop adapter
    J->>N: Stop fd
    J->>U: Close connection
Loading
High-Level Assessment

The scoped approach is appropriate: propagate start success through the existing manager boundary, retain connection ownership in the Java link wrapper, and make native stop idempotent for already-removed devices. A broader adapter-session state machine could centralize lifecycle state, but it would add behavioral scope without improving these targeted recovery fixes.

Files changed (3) +49 / -15

Bug fix (3) +49 / -15
WfbLinkManager.javaRecover adapter refresh and permission failures +19/-4

Recover adapter refresh and permission failures

• Skips refresh when USB filter parsing fails and uses a package-scoped permission intent compatible with Android 14. Records adapters as active only after a successful start and displays open failures so later refreshes can retry them.

app/src/main/java/com/openipc/pixelpilot/WfbLinkManager.java

WfbngLink.cppMake native adapter stop idempotent +3/-8

Make native adapter stop idempotent

• Removes the deliberate null dereference when an adapter file descriptor is absent. Already-removed devices now produce a warning and return safely.

app/wfbngrtl8812/src/main/cpp/WfbngLink.cpp

WfbNgLink.javaValidate USB opens and release connection resources +27/-3

Validate USB opens and release connection resources

• Returns start success, handles null connections and invalid descriptors, and safely derives thread names from unexpected device paths. Individual and bulk stops now close UsbDeviceConnection objects and remove retained connection state.

app/wfbngrtl8812/src/main/java/com/openipc/wfbngrtl8812/WfbNgLink.java

@qodo-free-for-open-source-projects

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📜 Skill insights (0)

Grey Divider


Action required

1. Early detach wedges worker 🐞 Bug ☼ Reliability
Description
If stop() runs after Java starts the worker but before native run() registers the fd, the new
early return sends no stop signal and Java then blocks indefinitely in Thread.join(). The worker
can subsequently finish initialization and enter the blocking RX loop, wedging detach handling or
activity shutdown.
Code

app/wfbngrtl8812/src/main/cpp/WfbngLink.cpp[R282-283]

+        __android_log_print(ANDROID_LOG_WARN, TAG, "stop: no rtl device for fd=%d, already gone", fd);
        return;
Evidence
Java records the thread and connection before starting asynchronous native initialization; native
code only registers rtl_devices[fd] after libusb setup, then later enters StartRxLoop, which
returns only after StopRxLoop. The detach path calls native stop and unconditionally joins, so
returning while registration is still pending loses the only shutdown request and leaves that join
blocked.

app/wfbngrtl8812/src/main/java/com/openipc/wfbngrtl8812/WfbNgLink.java[111-115]
app/wfbngrtl8812/src/main/cpp/WfbngLink.cpp[86-128]
app/wfbngrtl8812/src/main/cpp/WfbngLink.cpp[241-243]
app/wfbngrtl8812/src/main/java/com/openipc/wfbngrtl8812/WfbNgLink.java[144-157]
app/src/main/java/com/openipc/pixelpilot/WfbLinkManager.java[151-159]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A stop request arriving before native fd registration is discarded, after which Java can wait forever for the worker that proceeds into its blocking RX loop.

## Issue Context
`WfbNgLink.start()` publishes and starts the Java thread asynchronously. Native setup does not insert the fd into `rtl_devices` until later, while Java `stop()` immediately calls native stop and then joins without a timeout.

## Fix Focus Areas
- app/wfbngrtl8812/src/main/cpp/WfbngLink.cpp[86-128]
- app/wfbngrtl8812/src/main/cpp/WfbngLink.cpp[278-283]
- app/wfbngrtl8812/src/main/java/com/openipc/wfbngrtl8812/WfbNgLink.java[111-115]
- app/wfbngrtl8812/src/main/java/com/openipc/wfbngrtl8812/WfbNgLink.java[149-157]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Tip of the day
💡 Did you know, you can ask Qodo to dismiss a finding you disagree with, with your reason on record

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment on lines +282 to 283
__android_log_print(ANDROID_LOG_WARN, TAG, "stop: no rtl device for fd=%d, already gone", fd);
return;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

1. Early detach wedges worker 🐞 Bug ☼ Reliability

If stop() runs after Java starts the worker but before native run() registers the fd, the new
early return sends no stop signal and Java then blocks indefinitely in Thread.join(). The worker
can subsequently finish initialization and enter the blocking RX loop, wedging detach handling or
activity shutdown.
Agent Prompt
## Issue description
A stop request arriving before native fd registration is discarded, after which Java can wait forever for the worker that proceeds into its blocking RX loop.

## Issue Context
`WfbNgLink.start()` publishes and starts the Java thread asynchronously. Native setup does not insert the fd into `rtl_devices` until later, while Java `stop()` immediately calls native stop and then joins without a timeout.

## Fix Focus Areas
- app/wfbngrtl8812/src/main/cpp/WfbngLink.cpp[86-128]
- app/wfbngrtl8812/src/main/cpp/WfbngLink.cpp[278-283]
- app/wfbngrtl8812/src/main/java/com/openipc/wfbngrtl8812/WfbNgLink.java[111-115]
- app/wfbngrtl8812/src/main/java/com/openipc/wfbngrtl8812/WfbNgLink.java[149-157]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant