Conversation
out_appsink (live RTP pipeline and DVR file playback) set drop=true with no max-buffers, which defaults to 0 (unlimited) in GStreamer, so drop=true was a no-op: a decode/pull stall let frames pile up with no bound, and the picture would catch up in fast-forward once the stall cleared instead of skipping ahead to live. Add max-buffers=1. Also trim the restream branch's queue from max-size-time=1s to 150ms so a slow/stalled restream peer bounds out sooner, and bump the socket-read packet-rate log from debug to an [FPS-TRACE] info line so it lines up with the decode/display bottleneck counters.
Drop the once/sec [FPS-TRACE] socket packet-rate log added while bottleneck-hunting; the decode/handoff/display counters it was meant to line up with (main.cpp) were never committed and have also been removed. Back to the original debug-level packet-rate log.
The dev checkout's parent directory has a literal space in its name
("openipc@100hz wifilinkvrx"), which sits above both this repo and
sbc-groundstations. pkg-config emits -I/-L flags as one plain,
unquoted, space-joined string; CMake's FindPkgConfig naively splits
that on whitespace, so any path containing a space loses its -I/-L
prefix. Symptom when configuring a target (aarch64) build against the
sbc-groundstations buildroot sysroot: "fatal error: drm.h: No such
file or directory" from xf86drm.h, plus gcc warnings about sysroot
include paths being passed as bare linker inputs.
tools/cross_configure_target.sh documents the cause and regenerates a
pkg-config wrapper that forces PKG_CONFIG_* to a space-free /tmp
mirror of the buildroot output tree, then configures a build directory
against it. The wrapper's env assignments must be unconditional, not
${VAR:-default}: buildroot's generated toolchainfile.cmake exports
PKG_CONFIG_SYSROOT_DIR (pointing at the real, space-having path) before
pkg-config ever runs, which silently defeats a fallback-style default.
The kernel-generated CVT-RB and standard CVT timings for 1280x720@100 either corrupt the picture (cropping/miscoloring/striping) or produce no signal at all on this RK3566 board's HDMI-in on a Skyzone O4X Pro (firmware 4.2.1). Even the "textbook" CEA-family timing captured from an EDID describing a real Walksnail VRX's expected output still crops on this unit. Add the modeline arrived at by hand-tuning porches via live modetest against this specific VRX+goggles pairing (mainly a larger horizontal back porch and adjusted vertical front porch), applied only when the requested mode is exactly 1280x720@100. Also force quant_range=1 (limited range) on the connector in the same atomic commit as the modeset -- a non-atomic property write was silently ignored and left colors oversaturated. This is flagged experimental/hardware-specific, not a general default: directly verified not to generalize to the textbook timing presumably correct for other Walksnail VRX + goggles pairings. See issue_draft.md for the full writeup and the open question about actual Walksnail VRX HDMI output timing.
Adds a master switch (dvr_on_signal_enabled) that starts DVR recording when the GS starts receiving data from the air unit and stops it when that signal drops out. Wired into menu.c's drone_detect_timer -- the same detected/lost transition that already greys out the drone pages -- so no separate polling is needed, plus a live GS-menu toggle (Auto-record on Signal, on the DVR settings page) and a --dvr-on-signal CLI flag for starting enabled from launch.
New IconSelectorWidget entries in config_osd.json driven by the added src/icons/*.png set: an armed/disarmed indicator, an 8-point compass heading icon, and a GPS fix-quality icon (2D/3D/no-fix/none).
Reverts the icon-based widgets and their PNG assets, no longer referenced anywhere in config_osd.json. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LWYaFJL5iNrmr73dY6Tvuz
The VTX's SoC temp isn't local hardware and isn't known to the flight controller, so it can't go through the existing os_sensors/MSP paths. A small script on the VTX opens a plain TCP connection over the wfb-ng tunnel and writes the temperature as ASCII; VtxTempSensor listens for it independently of MspDisplayPortWidget telemetry. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LWYaFJL5iNrmr73dY6Tvuz
SO_REUSEADDR: on some boards an unrelated process (wpa_supplicant/ udhcpc sharing a duplicated fd) intermittently holds this port at boot, failing bind() and silently aborting widget setup -- the startup preview then stays stuck with nothing left to clear it. Buffer: msposd aggregates multiple MSP DisplayPort commands per UDP datagram (wfb-ng's radio_mtu is 1445 bytes), so a full-screen OSD update can exceed the previous 1024-byte buffer. recvfrom() silently truncates oversized datagrams, dropping trailing commands (often the terminating DRAW_SCREEN) and leaving stale glyphs on screen. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LWYaFJL5iNrmr73dY6Tvuz
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Adds System -> WFB-NG -> Channel Scan: a colmenu dynamic page (build/ prefetch, same pattern as the existing WiFi-networks page) that reads per-channel RF activity from gsmenu.sh (channel:count lines), sorts noisiest-first, and renders each as a selectable row with a bullet-dot bar. Selecting a row applies that channel via the same "set gs wfbng gs_channel" command the existing Channel dropdown already uses, so it also pushes to the air unit when a drone link is detected. The actual scan (hopping the secondary RX NIC, sampling rx_packets, restoring the live channel) lives in the integrator's gsmenu.sh, not here -- see debian/manpage.md's sibling gsmenu.sh template for the new "chanscan_results" protocol entry this page expects. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
PR Summary by QodoAdd channel scanning, signal DVR, and receiver reliability fixes
AI Description
Diagram
High-Level Assessment
Files changed (13)
|
Open
2 tasks
Code Review by Qodo
1. The Auto Record control does nothing
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| static const colmenu_page_t sys_display_page = { "Display", "gs", "system", sys_display_items, 5 }; | ||
| static const colmenu_item_t sys_dvr_items[] = { | ||
| { .kind=COLMENU_SWITCH, .label="Enabled", .param="rec_enabled", .on_change=on_rec_enabled }, | ||
| { .kind=COLMENU_SWITCH, .label="Auto Record", .param="dvr_on_signal", .on_change=on_dvr_on_signal }, |
There was a problem hiding this comment.
1. The auto record control does nothing 🐞 Bug ≡ Correctness
colmenu_get() and do_set() do not special-case the runtime-only dvr_on_signal parameter, so the row falls through to gsmenu.sh, whose integrator template has no matching get or set command. Opening the page consequently reads the switch as off, while changing it reaches an unknown shell command whose failure occurs before the deferred on_dvr_on_signal() callback can update dvr_on_signal_enabled.
Agent Prompt
## Issue description
The `dvr_on_signal` menu parameter represents application runtime state, but it is routed through `gsmenu.sh`, which has no handler for it. As a result, the Auto Record switch cannot display or update `dvr_on_signal_enabled`.
## Fix Focus Areas
- src/gsmenu/colmenu_pages.c[500-512]
- src/gsmenu/colmenu_pages.c[110-113]
- src/gsmenu/colmenu.c[81-130]
- src/gsmenu/colmenu.c[399-421]
- src/main.cpp[848-852]
## Recommended Fix
Handle `dvr_on_signal` directly as application runtime state, following the existing restream controls. Expose `dvr_get_on_signal()` through the column-menu C interface and return its `0`/`1` value from `colmenu_get()` for this parameter; in `do_set()`, bypass `gsmenu.sh`, apply the change with `dvr_set_on_signal()`, and invoke the existing `on_change` callback after the direct update.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+848
to
+852
| // Live toggle for --dvr-on-signal, so the GS menu can flip it without a | ||
| // restart (dvr_on_signal_enabled itself is read by menu.c's | ||
| // drone_detect_timer on every signal-acquired/lost transition). | ||
| void dvr_set_on_signal(int enabled) { dvr_on_signal_enabled = (bool)enabled; } | ||
| int dvr_get_on_signal(void) { return (int)dvr_on_signal_enabled; } |
There was a problem hiding this comment.
2. Live auto record changes miss the signal 🐞 Bug ≡ Correctness
dvr_set_on_signal() only changes the boolean, while drone_detect_timer() starts or stops recording exclusively when its cached detection state transitions. Enabling the option after a link is already present does not start recording, and disabling it during an automatic recording prevents the later signal-loss transition from stopping that recording.
Agent Prompt
## Issue description
Changing Auto Record while the signal state is stable does not reconcile the active recording with that state.
## Fix Focus Areas
- src/main.cpp[848-852]
- src/menu.c[62-87]
- src/gsmenu/colmenu_pages.c[110-113]
## Recommended Fix
Expose the current detection state and reconcile it inside the live setter: start when enabling with a currently detected link, and stop an auto-started recording when disabling. Track whether this feature started the recording so disabling it does not inadvertently stop an independently requested recording.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+420
to
+423
| static void apply_channel(void * ctx) | ||
| { | ||
| colmenu_exec((const char *)ctx); | ||
| colmenu_rescan(); |
There was a problem hiding this comment.
5. Channel results can reflect the old setup 🐞 Bug ≡ Correctness
apply_channel() launches colmenu_exec() and immediately calls colmenu_rescan() without waiting for the worker running the channel-change command to complete. Because rebuilding the dynamic page independently starts the prefetched chanscan_results read, it can collect and display measurements from the previous channel and receiver configuration while the setter is still active, particularly when the change restarts WFB-NG.
Agent Prompt
## Issue description
Selecting a scanned channel queues a new scan immediately after starting the asynchronous channel-change command, so the scan can overlap the setter and display measurements collected before the new channel and receiver configuration is active.
## Fix Focus Areas
- src/gsmenu/colmenu_pages.c[420-424]
- src/gsmenu/colmenu.c[257-364]
- src/gsmenu/colmenu.c[635-688]
## Recommended Fix
Add or expose an asynchronous command execution API that accepts a completion callback, and invoke `colmenu_rescan()` from that callback only when the channel-set command succeeds. If the command fails, preserve the current scan results, surface the existing command error, and do not schedule a new scan.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+455
to
+456
| char buf[64] = {0}; | ||
| ssize_t n = read(fd, buf, sizeof(buf) - 1); |
There was a problem hiding this comment.
3. One idle client freezes temperature updates 🐞 Bug ☼ Reliability
listenLoop() performs an unbounded blocking read() on each accepted socket, while VtxTempSensor destruction closes only the listening socket before waiting for the worker. Because the service is enabled unconditionally and binds to every IPv4 interface, any reachable client that connects without sending data can block subsequent accepts and temperature updates, and leave worker.join() unable to complete during shutdown.
Agent Prompt
## Issue description
An accepted VTX temperature client that sends no bytes can indefinitely block the sole listener in `read()`, stopping subsequent temperature telemetry and preventing destruction from completing because only the listening socket is closed before `worker.join()`.
## Fix Focus Areas
- src/os_mon.cpp[406-418]
- src/os_mon.cpp[421-429]
- src/os_mon.cpp[447-465]
## Recommended Fix
Bound all client-read waits by making accepted sockets nonblocking, assigning a short receive timeout, or using `poll`/`select` with a bounded timeout that regularly checks `running`. Track the active accepted descriptor and shut it down during destruction so both `accept()` and client-read paths are guaranteed to unblock before `worker.join()`; return to `accept()` after a timeout or malformed input, and restrict the bind address or otherwise limit access to the intended tunnel where possible.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+456
to
+459
| ssize_t n = read(fd, buf, sizeof(buf) - 1); | ||
| if (n > 0) { | ||
| try { | ||
| latest.store(std::stod(std::string(buf, static_cast<std::size_t>(n)))); |
There was a problem hiding this comment.
6. Split readings show wrong temperatures 🐞 Bug ≡ Correctness
listenLoop() treats the first successful TCP read() as the complete message, parses it immediately, and closes the connection. TCP does not preserve write boundaries, so a fragmented value such as 55.2 arriving first as 5 can be stored and published as 5°C while the remaining bytes are discarded.
Agent Prompt
## Issue description
The TCP listener assumes one read returns the sender's complete temperature value, allowing fragmented input to be accepted as a different valid number.
## Fix Focus Areas
- src/os_mon.cpp[447-465]
## Recommended Fix
Define a message terminator or use connection close as framing, then accumulate bounded input until that terminator or EOF before parsing. Require the parser to consume the entire trimmed message and reject non-finite or out-of-range values.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| osd_tag tags[1]; | ||
| strcpy(tags[0].key, "name"); | ||
| strcpy(tags[0].val, "vtx"); | ||
| osd_publish_double_fact("vtx.temperature", tags, 1, latest.load()); |
There was a problem hiding this comment.
7. Missing telemetry appears as zero degrees 🐞 Bug ≡ Correctness
VtxTempSensor::run() publishes latest on every polling cycle even though that atomic starts at 0.0 and has no received or freshness state. Before the first packet, and indefinitely after the sender stops or sends invalid data, consumers receive an invented zero or stale temperature as current telemetry.
Agent Prompt
## Issue description
The VTX sensor publishes a default or indefinitely stale value when no current temperature has been received.
## Fix Focus Areas
- src/os_mon.cpp[432-440]
- src/os_mon.cpp[457-473]
## Recommended Fix
Track whether a valid reading has been received and its timestamp. Do not publish before the first valid message, and expire or clear the fact once the feed has not refreshed it within the expected reporting interval.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+587
to
+590
| if (mode_width == 1280 && mode_height == 720 && mode_vrefresh == 100) { | ||
| /* Custom timing tuned by hand for this Skyzone O4X Pro unit's HDMI-in lock: | ||
| swept horizontal/vertical front porch and back porch against the | ||
| connector's stock 720p60 shape until crop on all four edges cleared. */ |
There was a problem hiding this comment.
8. Other 720p100 displays can lose video 🐞 Bug ≡ Correctness
modeset_output_create() substitutes a timing tuned for one Skyzone unit whenever the requested dimensions and refresh are 1280x720@100, without identifying the display or checking whether the selected connector advertises or supports that timing. Because modeset_prepare() applies this to generic connected candidates before advertised-mode selection, another display can reject the atomic test commit or fail to lock video, reaching the application's fatal startup path rather than retrying with an advertised preferred mode.
Agent Prompt
## Issue description
A timing tuned for one Skyzone display replaces the normal mode selection for every connector requested at 1280x720@100. Displays other than the intended target may reject the mode or fail to lock video, causing startup modesetting to fail instead of using a connector-advertised mode.
## Fix Focus Areas
- src/drm.c[565-618]
- src/drm.c[620-660]
- src/drm.c[761-792]
## Recommended Fix
Use the connector-advertised 1280x720@100 mode by default. Gate the custom timing behind an explicit target-display or platform configuration option, or reliable connector identification; for all other connectors, preserve the existing advertised-mode selection path. If the custom timing's atomic test fails, retry with an advertised mode, preferring the matching 1280x720@100 mode when available and otherwise falling back to the connector's advertised preferred mode.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+406
to
+410
| sockaddr_in addr{}; | ||
| addr.sin_family = AF_INET; | ||
| addr.sin_addr.s_addr = INADDR_ANY; | ||
| addr.sin_port = htons(static_cast<uint16_t>(port)); | ||
| if (bind(server_fd, reinterpret_cast<sockaddr *>(&addr), sizeof(addr)) < 0) { |
There was a problem hiding this comment.
4. Peers can forge air-unit temperature 🐞 Bug ⛨ Security
VtxTempSensor binds TCP port 15552 to INADDR_ANY and accepts the first payload from every connecting peer without checking its interface, source address, or identity. Because addVtxTemp(15552) is unconditional, any host able to reach the ground station can overwrite the published vtx.temperature fact with arbitrary telemetry.
Agent Prompt
Issue description
The new VTX temperature listener accepts unauthenticated TCP connections on every network interface. Reachable peers can submit arbitrary data that becomes the published VTX temperature.
Fix Focus Areas
- src/os_mon.cpp[396-419]
- src/os_mon.cpp[447-465]
- src/main.cpp[1781-1783]
Recommended Fix
Bind the listener only to the configured tunnel endpoint, or enforce an allowlisted tunnel source address before accepting a payload. Validate the complete payload and reject connections from all other interfaces or peers; make listener activation explicitly configurable if the tunnel endpoint is not always present.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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
1 participant
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.
Summary
.build/.prefetch, same as the WiFi-networks page) so the scan runs behind a spinner without freezing the UI.set gs wfbng gs_channelcommand the existing "Channel" dropdown already uses, so it also pushes the change to the air unit when a drone link is detected.chanscan_resultsprotocol entry in thegsmenu.shintegrator template. The real scan implementation (hopping a secondary RX NIC, sampling activity, restoring the live channel) is integrator/platform-specific and lives in the deploying project's owngsmenu.sh, same as every otherair/gsparameter here.Test plan
🤖 Generated with Claude Code