Repository navigation
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
tools/apfpv-open-static-cam.sh patches the apfpv firmware's /usr/bin/adapter for an open access point without DHCP and with disassoc_low_ack=0, optionally with a MAC filter; --revert restores the backup. Referenced from the README. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
PR Summary by QodoAdd APFPV station link mode for OpenIPC cameras
AI Description
Diagram
High-Level Assessment
Files changed (11)
|
Code Review by Qodo
1. A rejected adapter can crash later controls
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
|
|
||
| int WfbngLink::runStation(libusb_device_handle *dev_handle, libusb_context *ctx, int fd, int wifiChannel) { | ||
| IRtlDevice *dev = rtl_devices.at(fd).get(); | ||
| current_fd = fd; |
There was a problem hiding this comment.
1. A rejected adapter can crash later controls 🐞 Bug ☼ Reliability
runStation() assigns current_fd before checking whether another adapter already owns the AP association. If a second adapter is rejected, its device is erased but its descriptor remains current after the first adapter exits, so a later adaptive-link or transmit-power operation reaches rtl_devices.at() with a missing descriptor.
Agent Prompt
## Issue description
A rejected second AP-mode adapter overwrites the active descriptor, leaving later device controls able to access an erased device.
## Fix Focus Areas
- app/wfbngrtl8812/src/main/cpp/WfbngLink.cpp[304-331]
- app/wfbngrtl8812/src/main/cpp/WfbngLink.hpp[146-149]
## Recommended Fix
Set `current_fd` only after accepting the adapter as the station, and ensure rejection cannot change the active adapter's descriptor.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| } | ||
| if (it->payload.size() < off + plen) it->payload.resize(off + plen); | ||
| std::memcpy(it->payload.data() + off, ip + ihl, plen); | ||
| it->received += plen; |
There was a problem hiding this comment.
2. Video fragments can assemble with gaps 🐞 Bug ≡ Correctness
reassemble() counts every fragment's length toward completion without checking which byte ranges have arrived. An overlapping fragment or a delayed duplicate can make the count reach the expected size while bytes are missing, after which the zero-filled datagram can be forwarded to the video listener.
Agent Prompt
## Issue description
Summing fragment lengths treats overlapping or repeated bytes as newly received bytes and can complete an IP datagram with a gap.
## Fix Focus Areas
- app/wfbngrtl8812/src/main/cpp/StaLink.cpp[1009-1047]
- app/wfbngrtl8812/src/main/cpp/StaLink.h[224-236]
## Recommended Fix
Track received byte ranges or blocks per datagram, and return a reassembled packet only when every byte through the final fragment has been received.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+45
to
+48
| --allow-mac) shift; MAC=$(echo "$1" | tr 'A-F' 'a-f') ;; | ||
| *) die "unknown option: $1" ;; | ||
| esac | ||
| shift |
There was a problem hiding this comment.
3. A missing mac argument drops the filter 🐞 Bug ⛨ Security
The --allow-mac parser shifts to an argument without first checking that one exists. When the option is last, MAC stays empty and the script continues past the failed shift, skips both validation and ACL installation, and installs the open-network configuration.
Agent Prompt
## Issue description
Invoking `--allow-mac` without its value silently installs without the requested MAC restriction.
## Fix Focus Areas
- tools/apfpv-open-static-cam.sh[41-49]
- tools/apfpv-open-static-cam.sh[68-70]
## Recommended Fix
Check that an argument remains immediately after `--allow-mac` and exit with an error before modifying the adapter if it does not.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+118
to
+119
| mv "$TMP" "$ADAPTER" && chmod 755 "$ADAPTER" || die "writing $ADAPTER failed" | ||
| [ -n "$MAC" ] && { echo "$MAC" > "$ACCEPT" || die "writing $ACCEPT failed"; echo "Only $MAC may join ($ACCEPT)."; } |
There was a problem hiding this comment.
4. An acl write failure leaves a partial install 🐞 Bug ☼ Reliability
The script replaces /usr/bin/adapter before writing the requested MAC accept file. If that write fails, it exits without restoring the adapter, leaving the open-network configuration installed with a reference to a missing or unwritten ACL file.
Agent Prompt
## Issue description
Failure to create the MAC accept file leaves the adapter replaced despite the installation command failing.
## Fix Focus Areas
- tools/apfpv-open-static-cam.sh[96-120]
## Recommended Fix
Create and verify the accept file before replacing the adapter, or restore the backed-up adapter and clean up on any later failure.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+688
to
+692
| if (in_gap_ && state_.load() == State::Connected) { | ||
| // Dropout over: video flows again right now, nothing to rebuild. | ||
| in_gap_ = false; | ||
| recoveries_++; | ||
| __android_log_print(ANDROID_LOG_INFO, |
There was a problem hiding this comment.
5. Brief fades miss the key-frame request 🐞 Bug ≡ Correctness
tick() is the only place that sets in_gap_, while onRx() increments the recovery counter only if that flag is already set. If reception resumes just after the 250 ms dropout threshold but before the next worker tick, the app receives no recovery change and does not request a new key frame.
Agent Prompt
## Issue description
A dropout that ends between worker ticks never sets the recovery flag, so the video refresh request is skipped.
## Fix Focus Areas
- app/wfbngrtl8812/src/main/cpp/StaLink.cpp[688-708]
- app/wfbngrtl8812/src/main/cpp/StaLink.cpp[560-565]
## Recommended Fix
When an AP packet arrives, compare its arrival time with `last_heard_` and apply the dropout threshold there as well, before deciding whether to increment recoveries.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+1275
to
+1277
| if (tcp_.st == St::Established && !tcp_.rx.empty() && now - tcp_.last_rx >= kTcpReplyIdle) { | ||
| tcpFinish(true); | ||
| return; |
There was a problem hiding this comment.
7. Delayed replies can truncate camera settings 🐞 Bug ☼ Reliability
tcpTick() finishes a request after 1.5 seconds without new bytes even when the peer has not closed and an HTTP response is incomplete. If a later response segment is delayed beyond that interval, cgi() receives a non-null partial reply and returns on JSON parse failure rather than trying the request again.
Agent Prompt
## Issue description
An inter-segment delay makes native TCP return a partial but non-null HTTP reply, which the camera-settings parser does not retry.
## Fix Focus Areas
- app/wfbngrtl8812/src/main/cpp/StaLink.cpp[1270-1280]
- app/src/main/java/com/openipc/pixelpilot/CameraControlDialog.java[518-541]
## Recommended Fix
For HTTP requests, determine completion from the response framing or peer FIN rather than the short idle interval; also retry incomplete or unparseable replies within the existing request retry loop.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| std::string &reply) { | ||
| using St = TcpSession::St; | ||
| std::unique_lock<std::mutex> lock(mu_); | ||
| if (!running_ || state_.load() != State::Connected || tcp_.st != St::Idle) return false; |
There was a problem hiding this comment.
8. Camera commands fail when another is in flight 🐞 Bug ☼ Reliability
StaLink::tcpRequest returns false straight away when tcp_.st != St::Idle. Callers read that as "no answer": requestKeyFrame and the AP Channel menu (onApChannelSelected) don't retry, so they give up, and only the camera dialog's airman/cgi retry. A key-frame request started by a recovery (for example during a channel switch or right after a rejoin) can therefore make a user's set_ap_channel report "Camera did not answer, channel unchanged". In the other direction, a running dialog or channel command silently drops the IDR request that recovery needed.
Agent Prompt
## Issue description
`StaLink::tcpRequest` returns false right away when another TCP request is in flight. Callers can't tell this apart from "camera did not answer", so key-frame requests and AP channel switches fail at random.
## Fix Focus Areas
- app/wfbngrtl8812/src/main/cpp/StaLink.cpp[1200-1210]
- app/src/main/java/com/openipc/pixelpilot/VideoActivity.java[2133-2154]
- app/src/main/java/com/openipc/pixelpilot/VideoActivity.java[487-498]
## Recommended Fix
In `tcpRequest`, if `tcp_.st != Idle`, wait on `tcp_cv_` (with the caller's timeout) until the slot goes back to Idle before starting, and return false only when not connected or the wait times out. Have the callers notify waiters when they reset `tcp_.st = Idle`. Another option is to serialize requests with a dedicated mutex held for the whole request.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+374
to
+375
| dev->Stop(); | ||
| releaseDevice(fd); |
There was a problem hiding this comment.
9. Adapter object can be freed while stop runs 🐞 Bug ☼ Reliability
The new releaseDevice(fd) call at the end of run()/runStation() erases the shared_ptr<IRtlDevice> from rtl_devices on the run thread. Meanwhile WfbngLink::stop() on the Java thread does find/at on the same unsynchronized map and calls StopRxLoop() and stop_adaptive_link() through a raw pointer. When stop() ends the RX loop, the run thread wakes up and can destroy the device (and mutate the map) while stop() is still inside StopRxLoop(). That is a use-after-free and a data race on std::map. Before this PR, entries were never erased on the normal exit path.
Agent Prompt
## Issue description
`releaseDevice()` now erases devices from `rtl_devices` at the end of `run()`. `stop()` and `nativeSetTxPower` read the same map and call methods through raw pointers on other threads, with no synchronization.
## Fix Focus Areas
- app/wfbngrtl8812/src/main/cpp/WfbngLink.hpp[142-149]
- app/wfbngrtl8812/src/main/cpp/WfbngLink.cpp[382-398]
- app/wfbngrtl8812/src/main/cpp/WfbngLink.cpp[810-819]
## Recommended Fix
Add a mutex around every `rtl_devices` access (insert, erase, find/at, and `current_fd`). In `stop()` and the TX power setter, copy the `shared_ptr<IRtlDevice>` while holding the lock and call through that copy, so the object stays alive until the call returns.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+297
to
+304
| } catch (const std::exception &e) { | ||
| __android_log_print(ANDROID_LOG_ERROR, TAG, "retune to ch %d failed: %s", retune, e.what()); | ||
| } | ||
| std::lock_guard<std::mutex> lock(mu_); | ||
| channel_ = static_cast<uint8_t>(retune); | ||
| width_mhz_ = retune_width; | ||
| chan_offset_ = retune_offset; | ||
| dwell_start_ = Clock::now(); |
There was a problem hiding this comment.
10. Failed channel switch is taken as success 🐞 Bug ≡ Correctness
StaLink::worker() catches exceptions from FastRetune() or SetMonitorChannel() but still sets channel_, width_mhz_, chan_offset_, and dwell_start_ to the requested values and clears the pending switch. If the driver rejects a CSA width change or scan hop, the radio remains on its old settings while search and probe decisions, the status line, and connectedChannel() use the false state, with no retune retry scheduled.
Agent Prompt
## Issue description
In `StaLink::worker()`, a failed retune is recorded as successful, leaving station channel state, search behavior, and reported channel inconsistent with the radio.
## Fix Focus Areas
- app/wfbngrtl8812/src/main/cpp/StaLink.cpp[273-306]
- app/wfbngrtl8812/src/main/cpp/StaLink.cpp[466-485]
## Recommended Fix
Track a success flag set only when `FastRetune()` or `SetMonitorChannel()` returns normally, and update the tuned channel, width, offset, and dwell bookkeeping only on success. On failure, retain the actual settings and arrange a retry or recovery search—for example, restore `retune_to_` under `mu_` or fall back to a 20 MHz `SetMonitorChannel` on the requested primary channel.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+2139
to
+2144
| String auth = Base64.encodeToString((getDroneUsername() + ":" + getDronePassword()) | ||
| .getBytes(java.nio.charset.StandardCharsets.UTF_8), Base64.NO_WRAP); | ||
| String request = "GET /request/idr HTTP/1.0\r\n" | ||
| + "Host: " + cameraIp + "\r\n" | ||
| + "Authorization: Basic " + auth + "\r\n" | ||
| + "Connection: close\r\n\r\n"; |
There was a problem hiding this comment.
11. Camera login is sent in clear over open wi-fi 🐞 Bug ⛨ Security
requestKeyFrame and CameraControlDialog.cgi() send the drone username and password as HTTP Basic auth over the camera's access point. With this PR (and tools/apfpv-open-static-cam.sh) that access point has WPA2 removed. Anyone in radio range can capture those credentials, which are usually the camera's root login, and replay them against the camera's web API. Key-frame requests go out automatically on every association and every end of a dropout, so the credentials are broadcast often, not only when the user opens the controls.
Agent Prompt
## Issue description
APFPV mode runs over an unencrypted access point, yet the app sends the drone login as HTTP Basic auth, automatically for every key-frame request and for every camera-control request.
## Fix Focus Areas
- app/src/main/java/com/openipc/pixelpilot/VideoActivity.java[2133-2154]
- app/src/main/java/com/openipc/pixelpilot/CameraControlDialog.java[523-530]
- README.md
## Recommended Fix
Request key frames through an unauthenticated channel (for example an air_man_ap command, or majestic's IDR request on a port with no credentials), or make the automatic key-frame request opt-in. Warn the user in the APFPV settings dialog and in the README that the camera password is sent in clear over the open network, and recommend a dedicated password for this mode.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
josephnef
requested changes
Oct 18, 2026
josephnef
left a comment
josephnef
left a comment
Collaborator
There was a problem hiding this comment.
Thanks for this, it is a substantial and well-structured piece of work. The station state machine, the ARP/ICMP/TCP mini-stack and the devourer integration are more careful than most contributed network code: every header parse I traced is length-checked, lock ordering is consistent (mu_ → q_mu_, sta_mutex → mu_ only from JNI), the six new JNI signatures match, and the existing R8 keep rules already cover the new natives. No merge conflicts with master.
I am requesting changes for two blocking items and a handful of robustness fixes. Qodo's review above is largely accurate; where I agree I reference its numbering rather than repeat it.
CI preview
CI (pr.yml, merged after this PR was opened) has not run here yet. It will on your next push. Expect:
- clang-format job: fail. With the pinned clang-format 18.1.8 the diff is 195 lines in
StaLink.cpp, 22 inStaLink.h, 57 inWfbngLink.cpp, 4 inWfbngLink.hpp(mostly bin-packed arguments and one-line loop bodies, plus one 124-column line atStaLink.cpp:695). Fix withpip install clang-format==18.1.8 && scripts/format_sources.sh. - Host GoogleTest and debug APK build: expected to pass. The emulator smoke job is non-blocking.
Blocking
- Device teardown race, and it now affects WFB-NG mode too (Qodo #9, #1).
releaseDevice(fd)is called at the end ofrun()/runStation()(WfbngLink.cpp:278, 296, 375) and erases fromrtl_devices(WfbngLink.hpp:146-149). Master only erased on the early-abort path.rtl_devicesis an unsynchronisedstd::mapthatstop()(findthenat(fd).get()thenStopRxLoop()),nativeSetTxPower(:817-818) andstart_link_quality_thread(:679, 786) read from other threads. When the RX loop exits on its own (USB detach, libusb error) andWfbLinkManagerthen callsstop, the run thread can destroy the device whilestop()is insideStopRxLoop()through a raw pointer, and.at()on an erased fd throws through JNI. Keep the erase if devourer's per-adapter lock needs it, but guardrtl_devices/current_fdwith a mutex and havestop()and the setters take ashared_ptrcopy under it. Also setcurrent_fdonly after the adapter is accepted as the station (Qodo #1). - Formatting, see above.
Should fix
- ICMP echo of arbitrary size is injected as one MPDU (
StaLink.cpp:1161-1175). The reply copies the whole datagram, and reassembly allows up to 64 KB, so anyone on the open AP can make the worker hand a ~60 KB frame tosend_packet. Drop echo requests above ~1500 bytes. - Fragment reassembly completes with gaps (Qodo #2,
StaLink.cpp:1036).received += plencounts overlapping or duplicate fragments, so a zero-filled datagram can be forwarded to :5600. Track coverage or drop overlaps. - Unbounded buffers:
tcp_.rxgrows until the deadline with no cap (StaLink.cpp:1348, cap at e.g. 64 KB);seen_grows per distinct BSSID (:836, cap it). SetTxPowerfrom the JNI thread races the worker'sSetMonitorChannel/FastRetune(StaLink.cpp:280-289). devourer'sIRtlDevice.h:111-116requires a single control thread. Queue it through the worker likerequestTune.- Qodo #10 (failed channel switch recorded as success,
StaLink.cpp:304), #8 (TCP single-flight: a recovery key-frame request makes a user'sset_ap_channelreport "camera did not answer",:1208), #7 (partial HTTP reply accepted after 1.5 s idle,:1277), #5 and #6 (gap-recovery edge cases). All hold on my read. tools/apfpv-open-static-cam.sh:--allow-macas the last argument silently installs without the ACL (Qodo #3,:48); the adapter is replaced before the ACL file is written, so a write failure leaves a half-installed config (Qodo #4,:119); and$masteris assumed defined at theip neigh delsite (:107-108), add it to the pre-flight checks.
Document
- Open network without PMF: a spoofed deauth or a spoofed beacon carrying the SSID can steer the station (
StaLink.cpp:754, 836). Inherent to the design, but the README should say so and recommend--allow-mac. - Camera HTTP credentials go as Basic auth in cleartext over the open AP (
VideoActivity.java:2140,CameraControlDialog.java:519). Worth one sentence.
What would make this mergeable
- Run
scripts/format_sources.shwith clang-format 18.1.8. - Mutex around
rtl_devices/current_fd,shared_ptrcopies instop()and the setters. - Bound network input (ICMP echo size,
tcp_.rx,seen_) and fix fragment coverage. - Serialise
SetTxPowerwith the worker's retunes. - README notes on the security model.
If you want something merged sooner, the camera-side script plus its README section would stand on their own as a separate PR.
This was referenced Oct 18, 2026
Draft
Collaborator
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a second link mode next to WFB-NG: the USB adapter (devourer userspace driver) joins the open Wi-Fi access point of an OpenIPC apfpv camera as a regular station, and the camera pushes its RTP stream to the phone's static IP.
Camera side (apfpv firmware)
The stock firmware runs a WPA2 access point with a DHCP server. For this mode, /usr/bin/adapter (which writes hostapd.conf on every start) was patched:
The camera changes are scripted in tools/apfpv-open-static-cam.sh (keeps a backup, --revert restores it).
Limitations
Open networks only (no WEP/WPA), static IP (no DHCP), no 802.11 fragment reassembly, UDP only plus one short TCP request at a time.
Tested on a RTL8812EU adapter with OpenIPC apfpv firmware.
🤖 Generated with Claude Code