fix: msposd OSD lost on Star6E boot (missing -d, waybeam respawn race) - #2395
fix: msposd OSD lost on Star6E boot (missing -d, waybeam respawn race)#2395moro161 wants to merge 1 commit into
Conversation
msposd's start_telemetry() invocation was missing -d/--osd, so it never called MI_RGN_Init and drew nothing even though it ran fine and parsed FC telemetry correctly. Separately, on Star6E SoCs (e.g. RunCam WiFiLink/ssc338q) waybeam does an internal cold-restart shortly after its own startup to lock in a cold-boot FPS re-kick, which destroys and recreates its venc channel. msposd binds its OSD region to whatever channel exists at its own startup, once, and this script always starts msposd before that respawn fires -- so the OSD region was silently destroyed with the old channel and never came back, even with -d. Symptom: OSD shows for a few seconds after boot, then vanishes for good. Added a background watcher that detects the waybeam respawn (by content, not a fixed delay, since the respawn can complete before we start watching) and restarts msposd once the respawned channel is actually up. No-op on SoCs/builds where waybeam doesn't do this respawn, or majestic is in use instead.
PR Summary by QodoRestore msposd OSD after waybeam boot respawn
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
Code Review by Qodo
1. Some camera targets remain untested
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| -o 127.0.0.1:"$port_tx" -z "$size" > /dev/null & | ||
| -o 127.0.0.1:"$port_tx" -z "$size" -d > /dev/null & | ||
|
|
||
| [ "$router" = "msposd" ] && watch_waybeam_respawn "$osd_fps" "$serial" "$port_tx" "$size" |
There was a problem hiding this comment.
1. Some camera targets remain untested 📘 Rule violation ☼ Reliability
watch_waybeam_respawn is added to the shipped telemetry startup path, but the PR test plan leaves the non-Star6E and majestic hardware item unchecked and explicitly says it was not tested. The added watcher is intended to execute or no-op across those builds, so those camera paths remain within the claimed blast radius without board output confirming either behavior.
Agent Prompt
## Issue description
The shipped telemetry startup behavior changes across camera builds, while the PR explicitly leaves non-Star6E and majestic hardware verification incomplete.
## Fix Focus Areas
- general/package/wifibroadcast-ng/files/wifibroadcast[134-188]
## Recommended Fix
Run the applicable non-Star6E and majestic no-op checks on real cameras, record observable startup, streaming, process, and log results in the PR description, and check the remaining test-plan item.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| echo_log "waybeam respawned post-boot, restarting msposd to reattach OSD" | ||
| killall -q msposd |
There was a problem hiding this comment.
2. Slow encoder restarts lose the overlay 🐞 Bug ≡ Correctness
watch_waybeam_respawn falls through to the restart path when its readiness loop times out, without confirming that a second ISP channel ready marker exists. If replacement-channel initialization takes longer than six seconds or fails, msposd starts before a usable channel exists and cannot perform the intended reattachment.
Agent Prompt
## Issue description
The readiness loop can time out without finding the second `ISP channel ready` marker, but the code still restarts `msposd` as though the replacement encoder channel were available.
## Fix Focus Areas
- general/package/wifibroadcast-ng/files/wifibroadcast[174-187]
## Recommended Fix
After the bounded loop, explicitly verify that the log contains at least two readiness markers. Restart `msposd` only when that condition is true; otherwise log the timeout and exit the watcher without claiming a successful respawn.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| done | ||
| msposd -b 115200 -c 8 -r "$osd_fps" -m /dev/"$serial" \ | ||
| -o 127.0.0.1:"$port_tx" -z "$size" -d > /dev/null & | ||
| ) & |
There was a problem hiding this comment.
3. Stopped telemetry can start itself again 🐞 Bug ☼ Reliability
watch_waybeam_respawn backgrounds an untracked subshell, while stop() kills only named telemetry binaries and never cancels that watcher. A stop or rapid restart during either wait leaves the prior watcher alive, so it can later kill the new instance or launch msposd after the service was stopped.
Agent Prompt
## Issue description
The detached waybeam watcher is not owned by the service lifecycle, allowing it to survive a stop or overlap a later service generation.
## Fix Focus Areas
- general/package/wifibroadcast-ng/files/wifibroadcast[163-188]
- general/package/wifibroadcast-ng/files/wifibroadcast[255-279]
## Recommended Fix
Record the background watcher's PID when it is launched, cancel and wait for that PID before stopping or starting telemetry, and remove the PID record when the watcher exits. Ensure only the current service generation is allowed to restart `msposd`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
openipc-ai
left a comment
openipc-ai
left a comment
There was a problem hiding this comment.
Thanks for the detailed write-up — the -d half is a real bug with a real fix, and the msposd -v evidence for it is exactly the right kind. The watcher half has a blocking problem, though: it keys on a package that no board in either repository builds.
Blocking — waybeam is selected by no defconfig, here or in builder
watch_waybeam_respawn depends entirely on /tmp/waybeam.log existing, which requires waybeam to be installed. It never is:
general/package/waybeam/Config.incarriesdepends on !BR2_PACKAGE_MAJESTIC— the two drive the same sensor and encoder, andsigmastar-osdrv-infinity6einstalls the MI libraries only when Majestic is off.- Every defconfig in OpenIPC/builder that sets
BR2_PACKAGE_WIFIBROADCAST_NG=y— all nine of them,ssc338q_fpv,ssc338q_fpv_runcam-wifilink,ssc338q_fpv_openipc-urllc-aio,ssc338q_fpv_openipc-mario-aio,ssc338q_fpv_openipc-thinker-aio,ssc338q_fpv_caddx-fly,ssc338q_fpv_emax-wyvern-link,ssc30kq_fpv,ssc377qe_fpv_ccdcam-im50q01-tipoman— also setsBR2_PACKAGE_MAJESTIC=y. So waybeam is excluded by construction on precisely the boards that ship this script. grep -rl BR2_PACKAGE_WAYBEAM=yacross both trees returns nothing.
On every image built from these repositories the streamer is majestic, the marker never appears, and the watcher spends ~15 s after each boot grepping a file that does not exist before giving up. That is best_practices.md §6.1 — code no defconfig can reach.
If you were misled by the NOT_BUILT comment in .github/scripts/ci-matrix.py ("It is selected by the FPV variants in OpenIPC/builder"), that sentence is stale and I will fix it separately.
This also means the test-plan evidence cannot have come from a stock nightly of ssc338q_fpv_openipc-urllc-aio-nor — grep -c "ISP channel ready" /tmp/waybeam.log reading 2 requires a waybeam image, and that board builds majestic. Could you say how the image under test was built? If you are carrying a local waybeam-enabled variant, the watcher belongs wherever that variant lives, not in the shared script.
The good news
The -d bug is real and it is on the configuration everyone actually runs. Split that out with the $size guard described inline and it is a small, shippable PR. See the inline comments for why -z alone was supposed to cover this and what actually goes wrong.
If the reattach problem is real beyond waybeam — and it plausibly is, since msposd binds its region once and majestic restarts on config changes too — the durable fix is in msposd: reattach on channel loss, rather than have a shell script scrape a log and kill a process. A watcher in general/package/ reaches every FPV camera and can only ever cover the one restart path it was written for.
Process notes
- Commit subject is
fix: msposd OSD lost on Star6E boot (...). House style isarea: lowercase imperative summary, e.g.wifibroadcast: pass -d so msposd actually draws the OSD. - CI here builds all 99 boards for this path but covers none of the FPV consumers —
ci-matrix.py --stdinreports "reaches no board of its own" — so hardware evidence is the only gate this change has. - Local checks do pass on the branch:
test_shell_parse.sh,test_strip_shell_comments.shandci-matrix.py --self-testare all clean.
Two of the inline findings below were also raised by the automated review and I agree with both.
| size=$(curl -s localhost/api/v1/config.json | jsonfilter -qe "@.video0.size") | ||
| fi | ||
| msposd -b 115200 -c 8 -r "$osd_fps" -m /dev/"$serial" \ | ||
| -o 127.0.0.1:"$port_tx" -z "$size" > /dev/null & | ||
| -o 127.0.0.1:"$port_tx" -z "$size" -d > /dev/null & |
There was a problem hiding this comment.
-d is correct and I want to keep it — but it is not the whole story, and the PR description has the cause slightly wrong.
DrawOSD starts false in msposd and is set from two places in the option loop: case 'd', and case 'z' — but case 'z' sets it only inside if (limit), where limit = strchr(buffer, 'x'). So -z 1920x1080 already implies -d. The only way the existing line draws nothing is $size arriving empty or without an x, which happens when majestic's HTTP server is not yet listening at S98 time, or .video0.size is unset in majestic.yaml.
That matters because -d alone masks it rather than fixing it: with an empty -z, set_resolution() is never called and msposd falls back to its built-in default, clamped to 1280x720. The overlay then comes up at the wrong geometry instead of not at all — which is harder to notice than the current failure.
Please guard the value as well, something like:
size=$(curl -s localhost/api/v1/config.json | jsonfilter -qe "@.video0.size")
case "$size" in
*x*) ;;
*) size=1920x1080; echo_log "video0.size unavailable, OSD falling back to $size" ;;
esacKeep -d as the belt-and-braces so the overlay no longer depends on that curl succeeding.
| -o 127.0.0.1:"$port_tx" -z "$size" > /dev/null & | ||
| -o 127.0.0.1:"$port_tx" -z "$size" -d > /dev/null & | ||
|
|
||
| [ "$router" = "msposd" ] && watch_waybeam_respawn "$osd_fps" "$serial" "$port_tx" "$size" |
There was a problem hiding this comment.
This is the blocking line. waybeam is built by no defconfig in this repository or in OpenIPC/builder — its Config.in has depends on !BR2_PACKAGE_MAJESTIC, and every builder defconfig that selects BR2_PACKAGE_WIFIBROADCAST_NG also sets BR2_PACKAGE_MAJESTIC=y.
So on every image that ships this script, /tmp/waybeam.log does not exist and the watcher is unreachable code that costs ~15 s of background polling after each boot. Details and the full defconfig list are in the review body.
| # waybeam (the venc/encoder daemon on Star6E SoCs) does an internal | ||
| # cold-restart shortly after its own startup (fork+exec, to lock in a | ||
| # cold-boot FPS re-kick — see src/star6e_pipeline.c in OpenIPC/waybeam), |
There was a problem hiding this comment.
This mechanism is not what waybeam does, and the comment will outlive the misunderstanding if it lands.
star6e_pipeline_cold_boot_fps_rekick() in src/star6e_pipeline.c is a plain MI_SNR_SetFps re-issued from the run loop about 1.5 s after start. It does not restart the process, and it does not touch the venc channel.
The fork+exec cold restart lives in src/venc_respawn.c and is reached only from venc_api_request_reinit(), whose callers are SIGHUP, /api/v1/restart, /api/v1/defaults, and a RESTART-class /api/v1/set. There is no automatic self-restart shortly after boot for this script to race with.
So whatever produced the teardown you captured in logread had some other trigger, and it is worth identifying before building a workaround around it — the timing assumption this watcher encodes ("msposd always starts before the respawn fires") may not hold for the real trigger.
| # respawn (or majestic is in use instead) — /tmp/waybeam.log never gets | ||
| # the marker and this just exits after the timeout. | ||
| watch_waybeam_respawn() { | ||
| osd_fps="$1"; serial="$2"; port_tx="$3"; size="$4" |
There was a problem hiding this comment.
These assignments are not local, so they write the caller's variables. It happens to be harmless here only because start_telemetry() uses the same four names with the same values — a rename on either side turns it into a silent bug.
local is available in the busybox ash built here (general/overlay/usr/sbin/sysupgrade uses it throughout), so:
watch_waybeam_respawn() {
local osd_fps="$1" serial="$2" port_tx="$3" size="$4"| elapsed=0 | ||
| while [ "$elapsed" -lt 30 ]; do | ||
| grep -q "Respawning:" /tmp/waybeam.log 2>/dev/null && break | ||
| sleep 0.5 | ||
| elapsed=$((elapsed + 1)) | ||
| done |
There was a problem hiding this comment.
elapsed counts iterations, not seconds. With sleep 0.5 the bound is 15 s, not 30; the second loop's -lt 20 with sleep 0.3 is 6 s, not 20. The surrounding prose ("just exits after the timeout") reads as though those numbers were seconds.
Either name it tries, or keep seconds and derive the bound — whichever, make the comment state the real wall-clock budget, since that is the number a reviewer needs to weigh against how long the encoder actually takes to come back.
| elapsed=0 | ||
| while [ "$elapsed" -lt 20 ]; do | ||
| [ "$(grep -c "ISP channel ready" /tmp/waybeam.log 2>/dev/null)" -ge 2 ] && break | ||
| sleep 0.3 | ||
| elapsed=$((elapsed + 1)) | ||
| done | ||
|
|
||
| echo_log "waybeam respawned post-boot, restarting msposd to reattach OSD" | ||
| killall -q msposd |
There was a problem hiding this comment.
If the readiness loop times out, execution falls straight through to the restart anyway — there is no success flag and no post-loop guard. So when the replacement channel is slow or never arrives, msposd is killed and restarted against a channel that is not up, re-orphaning the region with no further retry. That is strictly worse than leaving the running instance alone: you lose whatever OSD was there.
Gate the restart on the condition actually being met:
ready=0
tries=0
while [ "$tries" -lt 20 ]; do
[ "$(grep -c "ISP channel ready" /tmp/waybeam.log 2>/dev/null)" -ge 2 ] && { ready=1; break; }
sleep 0.3
tries=$((tries + 1))
done
[ "$ready" = 1 ] || { echo_log "waybeam respawned but the channel never came up, leaving msposd alone"; exit 0; }(Also raised by the automated review — agreed.)
| while pidof msposd >/dev/null 2>&1; do | ||
| sleep 0.2 | ||
| done |
There was a problem hiding this comment.
This wait is unbounded and never escalates. killall -q sends SIGTERM; if msposd is wedged, or if a second watcher starts a fresh msposd while this one is waiting, pidof never clears and this subshell spins on a 0.2 s sleep forever — on a flight camera, with no way to notice.
general/package/waybeam/files/S95waybeam in this same tree already solves exactly this, and its stop() is the pattern to copy: a bounded wait_exit(), then killall -9, then report the failure rather than pretend the stop happened. Please bound this the same way and give up loudly if msposd survives SIGKILL.
| msposd -b 115200 -c 8 -r "$osd_fps" -m /dev/"$serial" \ | ||
| -o 127.0.0.1:"$port_tx" -z "$size" -d > /dev/null & |
There was a problem hiding this comment.
This is a byte-for-byte copy of the invocation at lines 131-132. Two copies of a command line with seven flags will drift — the -d this PR adds is itself an illustration of how easy it is for one of them to be wrong for a long time.
Factor it out:
start_msposd() {
msposd -b 115200 -c 8 -r "$osd_fps" -m /dev/"$serial" \
-o 127.0.0.1:"$port_tx" -z "$size" -d > /dev/null &
}and call it from both places.
| done | ||
| msposd -b 115200 -c 8 -r "$osd_fps" -m /dev/"$serial" \ | ||
| -o 127.0.0.1:"$port_tx" -z "$size" -d > /dev/null & | ||
| ) & |
There was a problem hiding this comment.
The subshell is backgrounded and untracked, and stop() kills only wfb_rx wfb_tx wfb_tun msposd mavfwd. So a watcher armed by a previous start outlives a stop, and there is no handle to cancel it.
Two reachable consequences:
wifibroadcast stopinside the watcher's ~21 s window kills msposd, then the watcher brings it back up afterwards — the service is nominally stopped with msposd running.wifibroadcast startrun twice leaves two watchers. They race each other'skillall, and the older one's unboundedpidofwait (line 183) can latch onto the msposd the newer one just started and never return.
If the watcher survives review at all it needs to be tracked — write its PID to /tmp and have stop() kill it, or have it re-check on each iteration that the msposd it was armed for is still the current one.
(Also raised by the automated review — agreed.)
Two entries in NOT_BUILT carry a prose reason naming who selects the package downstream, and both had gone stale. A reason nobody can check is worse than none: it is read as evidence. waybeam's said "It is selected by the FPV variants in OpenIPC/builder". It never was. waybeam depends on !BR2_PACKAGE_MAJESTIC, and every builder defconfig that selects wifibroadcast-ng sets BR2_PACKAGE_MAJESTIC=y, so the same exclusion that keeps it off this matrix keeps it off every downstream image too -- no defconfig in either tree sets BR2_PACKAGE_WAYBEAM=y. That line sent the author of #2395 chasing an FPV OSD fault through /tmp/waybeam.log, a file no shipped image writes. jsonfilter's cited "~97 builder devices". builder#128 removed it from the 95 that did not need it; what actually holds it in the tree now is wifibroadcast-ng's select and builder's apfpv devices. Comment only -- no selector, board or package list changes, and --self-test still reports 99 boards / 135 packages / 56 cases.
2 participants
Summary
Two related bugs found tracking down VTX-side OSD loss on a
ssc338q_fpv_openipc-urllc-aio-nor(RunCam WiFiLink) nightly build,both in
start_telemetry():OSD never appears at all: the
msposdinvocation is missing-d/--osd. Without it,msposdruns and correctly parses FCtelemetry (confirmed with
-v: detects the FC fine) but never callsMI_RGN_Init/ draws anything.OSD appears at boot, then vanishes for good after a few seconds
(Star6E SoCs, e.g. RunCam WiFiLink):
waybeamdoes an internalcold-restart shortly after its own startup (fork+exec, to lock in a
cold-boot FPS re-kick — see
src/star6e_pipeline.cinOpenIPC/waybeam), which
destroys and recreates its venc channel.
msposdbinds its OSDregion to whatever venc channel exists at its own startup, once, via
MI_RGN_Init, and never reattaches. This script always startsmsposdright afterS95waybeamreturns, which is beforewaybeam's internal respawn fires — so the OSD region is always
orphaned when the channel gets torn down, even with
-dfixed.Fix
-dto themsposdinvocation.watch_waybeam_respawn) that detects thewaybeam respawn via
/tmp/waybeam.logcontent (re-scanning the wholefile each check, since the respawn can complete before the watcher
starts — not a fixed delay) and, once the respawned channel is
actually up (a second
"ISP channel ready"line, not a guessedsleep), restarts
msposdso it reattaches. Bounded loops are asafety net, not the wait mechanism.
waybeamdoesn't do this respawn, ormajesticis in use instead — the log marker never appears and thewatcher just exits after its timeout.
Test plan
msposd -vin foreground: noMI_RGN_Init/region activity without-d; with-d, regioncreated successfully and live telemetry flows.
logread:client [X] disconnected, module:venc→MI_VENC_IMPL_DestroyChn→
client [Y] connected, module:venc, correlated withwaybeam's own"Reinit requested: cold restart via fork+exec"/"Respawning:"log lines.with no manual intervention: watcher log line
(
"waybeam respawned post-boot, restarting msposd to reattach OSD") appears,grep -c "ISP channel ready" /tmp/waybeam.logreads 2 at that point (real signal, not a timing guess),
msposdcomes back up with
-d, stable PID over 30+s of pollingafterward on each reboot.
hardware on hand) — the watcher is designed to be a no-op there,
but that's unverified.