Skip to content

fix: msposd OSD lost on Star6E boot (missing -d, waybeam respawn race) #2395

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
moro161 wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix: msposd OSD lost on Star6E boot (missing -d, waybeam respawn race) #2395

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 53 additions & 1 deletion general/package/wifibroadcast-ng/files/wifibroadcast
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,13 +129,65 @@ start_telemetry() {
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 &
Comment on lines 129 to +132

@openipc-ai openipc-ai Sep 11, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

-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" ;;
esac

Keep -d as the belt-and-braces so the overlay no longer depends on that curl succeeding.


[ "$router" = "msposd" ] && watch_waybeam_respawn "$osd_fps" "$serial" "$port_tx" "$size"

@qodo-free-for-open-source-projects qodo-free-for-open-source-projects Bot Sep 11, 2026

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

@openipc-ai openipc-ai Sep 11, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.


sleep 5
echo "&L70&F28&G8CPU:&C TEMP:&T\n&B" >/tmp/MSPOSD.msg
fi
}

# msposd needs -d/--osd to actually draw the OSD overlay (MI_RGN_Init) —
# without it, it runs and parses FC telemetry fine but never renders
# anything. See github.com/OpenIPC/msposd for the flag.
#
# 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),
Comment on lines +145 to +147

@openipc-ai openipc-ai Sep 11, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

# which destroys and recreates its venc channel. msposd binds its OSD
# region to whatever venc channel exists at its own startup, once, via
# MI_RGN_Init, and never reattaches — so if msposd starts before this
# happens (which it always does: this script starts msposd right after
# S95waybeam returns, which is before waybeam's internal respawn fires),
# the OSD region is silently destroyed along with the old channel and
# never comes back. Re-scan /tmp/waybeam.log (whole file each check,
# since the respawn can complete before we start watching) for the
# respawn, then wait for its "ISP channel ready" to appear a second time
# (the real signal the new channel is up, not a guessed delay — it also
# appears once during the normal pre-respawn boot), then restart msposd
# so it reattaches. The bounded loops below are a safety net, not the
# wait mechanism. No-op on SoCs/builds where waybeam doesn't do this
# 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"

@openipc-ai openipc-ai Sep 11, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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
Comment on lines +166 to +171

@openipc-ai openipc-ai Sep 11, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

grep -q "Respawning:" /tmp/waybeam.log 2>/dev/null || exit 0

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
Comment on lines +181 to +182

@qodo-free-for-open-source-projects qodo-free-for-open-source-projects Bot Sep 11, 2026

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

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

Comment on lines +174 to +182

@openipc-ai openipc-ai Sep 11, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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
Comment on lines +183 to +185

@openipc-ai openipc-ai Sep 11, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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 &
Comment on lines +186 to +187

@openipc-ai openipc-ai Sep 11, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

) &

@qodo-free-for-open-source-projects qodo-free-for-open-source-projects Bot Sep 11, 2026

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

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 openipc-ai Sep 11, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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 stop inside 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 start run twice leaves two watchers. They race each other's killall, and the older one's unbounded pidof wait (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.)

}

video_settings() {
for card in $(lsusb | awk '{print $6}' | uniq); do
case "$card" in
Expand Down