AP_Mount, AP_Camera: apply aux switch positions once backends exist

[Hwurzburg]

Summary

Have AP_Mount and AP_Camera establish their own RC auxiliary switch positions on their first update(), once their backends exist. Replaces #34166, which fixed the same defect by deferring all aux initialisation until after backend creation.

Classification & Testing (check all that apply and add your own)

• Checked by a human programmer
• Non-functional change
• No-binary change
• Infrastructure change (e.g. unit tests, helper scripts)
• Automated test(s) verify changes (e.g. unit test, autotest)
• Tested manually, description below (e.g. SITL)
• Tested on hardware
• Logs attached
• Logs available on request

Testing performed:

• ./waf copter plane rover replay and ./waf AP_Periph (sitl_periph_universal)
• Tools/autotest/autotest.py test.Copter.MountAuxFunctionAtBoot test.Copter.CameraAuxFunctionAtBoot test.Copter.AuxFunctionAtBootWithoutRC test.Copter.MountAuxFunctionAtBootRCFailsafe test.Plane.MountAuxFunctionAtBoot — all pass here; the mount/camera ones fail on the merge base (Mode=3 in the first MNT sample; first logged application source=RC)
• Copter.CameraAuxFunctionAtBoot run 25 times consecutively, no failures (it was measured flaky at ~3% before the assertion was reworked); 10 more at this head with the announcement assertion added
• Mutation-tested: putting MOUNT_YAW_LOCK back in AP_Mount's list makes MountAuxFunctionAtBootRCFailsafe fail with "MOUNT_YAW_LOCK first applied by source=0, want RC"
• Mutation-tested: removing the GCS announcement makes CameraAuxFunctionAtBoot fail with "Failed to receive text: rc6: camera record video high"
• Mutation-tested: removing the pending gate makes MountAuxFunctionAtBootRCFailsafe fail with "applied (source=0 pos=2) from a receiver in failsafe"; removing the recorded position makes the RC-sourced-AUXF assertions fail
• The same plus test.Copter.Mount test.Copter.MountPOIFromAuxFunction test.Copter.AuxSwitchOptions test.Copter.AuxFunctionsInMission — 8/8, repeated to check the new tests are not flaky
• test.Rover.CameraMission — Rover creates the mount and camera before rc().init(), so it exercises the opposite ordering
• test.Copter.ModeLoiter — the one red CI job; passes locally, no aux involvement
• Flash, both trees built the same way: +200 B on MatekF405 (mount off, camera on; that board compiles the aux-function strings out, so the GCS announcement below is free there) and +348 B on CubeOrange, where both libraries and the strings are enabled. No RAM figure quoted: the .bss delta size reports is a remainder rather than a measurement of RAM
• Mutation-tested: removing the one-shot latch makes both boot tests fail on the at-most-once assertion (684 and 184 applications); recording the position without dispatching makes CameraAuxFunctionAtBoot fail with no AUXF logged
• Tools/scripts/check_branch_conventions.py --base-branch upstream/master, git diff --check upstream/master...HEAD

Description

RC_Channels::init() runs the auxiliary functions once, and on most vehicles it does so before the vehicle has created the backends those functions act on. On Copter rc().init() is in init_ardupilot() well ahead of camera_mount.init() and camera.init(), and both of those allocate their backends (NEW_NOTHROW) inside themselves.

So RETRACT_MOUNT1/2, MOUNT_YAW_LOCK, MOUNT_RP_LOCK and MOUNT_POI_LOCK reach AP_Mount's null-instance early return, and CAMERA_REC_VIDEO, CAMERA_ZOOM, CAMERA_MANUAL_FOCUS, CAMERA_AUTO_FOCUS, CAMERA_LENS, RUNCAM_CONTROL and RUNCAM_OSD_CONTROL reach the primary-instance overloads and return false on a null primary. A guaranteed silent no-op.

What this is worth, stated honestly

debounce_position and current_position both start at -1, so where RC becomes valid the first debounced read_aux() applies the switch about 200 ms later. What this fixes is that window, while disarmed. That is the whole of it. It deliberately does not cover the case where RC never becomes valid: with frames still arriving that is the failsafe case, which is exactly what must not be acted on.

It is not an explanation for this 4.7.1-beta report, which #34166 cited as motivation and which I should have checked more carefully before repeating. That report is MOUNT_RP_LOCK HIGH at boot, and HIGH sets _roll_lock/_pitch_lock true — already their initialiser values in AP_Mount_Backend.h, re-derived every update in RC targeting. A lost boot position would not change anything there, with or without this PR. Whatever that user hit, this does not fix it.

I still think the defect is worth closing — a switch position that is silently discarded is a bug whatever its blast radius, and it is one of the few places in the codebase where initialisation is known to run against a null backend — but reviewers should weigh it at its real size.

Each library establishes its own positions

RC_Channels::apply_aux_switch_positions() takes a set of functions, finds the channel carrying each, reads the switch, and re-dispatches through RC_Channel::run_aux_function() — not through a second copy of the handler, so there remains one implementation of what each switch position means. AP_Mount and AP_Camera each call it once, from the top of their first update(), with their own list.

Why the first update() and not the end of init(). The switch cannot be read at the end of init(): nothing has polled RC by then, and on Rover camera_mount.init() runs before rc().init() at all. By the first update() the vehicle's setup() has finished and on most vehicles something has read the receiver during it — Copter's esc_calibration_startup_check() waits up to two seconds for first RC input, Plane's failsafe_check() reads it once the main loop has stalled. Where nothing has (TradHeli and brushed-PWM Copter skip that poll; Rover, Sub, Blimp and Tracker have no equivalent) nothing is applied until RC does come up, at which point the position applied is the one the switch is in then - the same one the first debounced read_aux() would have applied.

Why not get_aux_cached()

This was the obvious cheap fix and it does not work, which is worth recording because it was suggested more than once.

RC_Channels already caches the position each function was last run with, and the run this is recovering from is exactly the one that had nothing real to record. Nothing calls rc().read_input() before rc().init(), so radio_in is zero for every channel, read_3pos_switch() fails, and RC_Channel::init_aux() substitutes LOW. I instrumented master and confirmed live=0 radio_in=0 on every Copter boot.

Replaying that cached LOW would not restore the pilot's switch position — it would act on a position the pilot never selected. CAMERA_ZOOM LOW is a continuous zoom-out rate command, CAMERA_MANUAL_FOCUS LOW a continuous focus-in, MOUNT_POI_LOCK LOW calls clear_poi_lock() and announces "POI: Cleared", RETRACT_MOUNT1 LOW calls set_mode_to_default(), overriding the RC_TARGETING Sub deliberately sets right after creating the mount.

Reading the switch at first update gives the position it is really in, and where it cannot be read nothing is applied at all. Copter.AuxFunctionAtBootWithoutRC pins that: it boots with no RC protocol and asserts no position is invented.

Only channel-assigned functions

A function invoked solely over MAVLink (MAV_CMD_DO_AUX_FUNCTION), by a mission item, or by a script needs no RCx_OPTION at all — GCS_MAVLINK::handle_command_do_aux_function() calls rc().run_aux_function() directly. Those are deliberately not recovered here: each of those callers is told when a dispatch fails (MAV_RESULT_FAILED). The RC path has no way to report anything, which is precisely what makes it the one that loses silently.

The duplicate application

Neither master's init_aux() nor this touches switch_state, and current_position starts at -1, so without care the first debounced read_aux() would run every function a second time. That is not harmless: set_poi_lock() re-saves saved_mount_mode, a Siyi gimbal sends a second autofocus packet, and AP_RunCam::osd_option() latches a menu action the update loop consumes.

The helper therefore records an applied position as the channel's current switch position, under three conditions, each of which keeps an application the first debounced read is still owed:

• Only a position that was really read. There is no other kind here — nothing is invented.
• Only when the dispatch succeeded. run_aux_function() returning false (a camera backend that rejects commands until discovery completes) is not recorded, so the read retries it.
• Only when a backend exists. The mount handlers return true whether or not an instance is there, so AP_Mount skips the whole thing when _num_instances == 0; otherwise a switch configured against MNT1_TYPE=0 would be recorded as applied and the read suppressed for a function that did nothing.

A switch that genuinely moves between the two is still picked up, since debounce_completed() compares against the recorded position. The tests assert at most one initialisation-sourced AUXF and no RC-sourced one, and go red if the recording or the one-shot latch is removed.

Functions in RC_Channel::init_position_on_first_radio_read() — ARMDISARM, ARM_EMERGENCY_STOP, PARACHUTE_RELEASE — are skipped outright by the helper. None are used by these two libraries, but reading a switch and acting on it is exactly what that list exists to prevent, and a generic helper must not become a way around it.

The GCS announcement is preserved

read_aux() announces each position it applies — RC7: RetractMount1 HIGH — but only once debounce_completed() returns true, so recording the applied position, which is what stops the function running twice, also suppresses that announcement. Left alone the switch would be applied and nothing said, and that message is exactly what a user checks when a switch appears to do nothing.

The helper therefore makes the same announcement itself, on a dispatch which took. One which did not is not recorded, so the first debounced read still announces it: one announcement either way, same format, same strings, same AP_RC_CHANNEL_AUX_FUNCTION_STRINGS_ENABLED guard. Copter.CameraAuxFunctionAtBoot waits for RC6: Camera Record Video HIGH and goes red without it.

MOUNT_YAW_LOCK is deliberately not recovered

AP_Mount_Backend::set_yaw_lock() does not just set a flag: while the axis is unlocked it captures _yaw_lock_heading_rad from the gimbal's attitude quaternion plus AP::ahrs().get_yaw_rad(), and it recaptures only from the unlocked state — so a heading taken at boot latches and stays until the pilot toggles the switch. Neither input is ready at the mount's first update().

Measured, servo mount, MOUNT_YAW_LOCK HIGH at boot, instrumented set_yaw_lock() on both trees (three runs each, identical every time):

	capture time	AHRS yaw read	heading captured
in the list	2459 ms (first AP_Mount::update())	0.000°	0.000°
merge base 35356c656c	2739 ms (first debounced read_aux())	−102.781°	−102.781°

A ~103° error, from the AHRS half alone, on a backend that reports no attitude at all. It is consumed at AP_Mount_Backend.cpp:159 under the default MNT1_DEFLT_MODE=RC_TARGETING and MNT1_RC_RATE=0, so it reaches the default configuration.

The other four functions do not do this — RETRACT_MOUNT1/2 call set_mode(), MOUNT_RP_LOCK sets two bools, and MOUNT_POI_LOCK starts a POI search that times out identically on both trees (AP::ahrs().get_location() does not succeed until ~10 s). So MOUNT_YAW_LOCK is left out of AP_Mount's list and keeps master's behaviour: the first debounced read_aux() applies it 200 ms later. Copter.MountAuxFunctionAtBootRCFailsafe asserts the split — RETRACT_MOUNT1 first applied by INIT, MOUNT_YAW_LOCK first applied by RC — and goes red if it is put back.

A healthy() gate would not have been enough: AP_Mount_Backend::healthy() defaults to true, and it says nothing about AHRS yaw.

Known limitation

This covers backends usable as soon as they are allocated. AP_Camera_MAVLinkCamV2 does not begin looking for its camera until ten seconds after boot and rejects commands until it has found it, so a switch position for one of those is still lost — as it is on master, since the first debounced read_aux() records the position whether or not its handler succeeded. Closing that needs a backend readiness protocol rather than a one-shot at first update, and is out of scope here.

Applying the position is held until RC input is valid

RC_Channels::read_aux_all() refuses to act when !has_valid_input(), and a receiver sitting in failsafe still streams frames with in-range PWM — so reading the switch at boot and acting on it immediately would apply the receiver's failsafe positions as though the pilot had selected them. That was the open trade-off in the first version of this PR, and it is now closed.

The one-shot is held pending until has_valid_input() is true, rather than consumed at the first update(). Copter.MountAuxFunctionAtBootRCFailsafe holds the vehicle in throttle failsafe from boot and asserts nothing is applied; it fails if the one-shot is taken early.

Returning early on !has_valid_input() instead of holding would not work, which is worth recording because it is the obvious version. Measured on the first MNT sample with RETRACT_MOUNT1 in the retract position at boot:

	Copter	Plane
held pending (this PR)	Mode=0 (retracted)	Mode=0 (retracted)
early return on invalid input	Mode=0 (retracted)	Mode=3 — not applied until the RC read

Plane's has_valid_input() is still false at the mount's first update(): read_radio() runs earlier in the same tick but has no fresh frame on it, so failsafe.last_valid_rc_ms is unset even though the startup failsafe path has already populated radio_in. Plane.MountAuxFunctionAtBoot exists so that difference cannot be lost by accident.

This does mean an aux function can no longer be recovered where RC never becomes valid — which is the point: with frames arriving, that case is the failsafe case.

Diffstat

501 insertions, 0 deletions. No parameter changes, no new feature defines, and no change to the aux position cache, which stays scripting-only.

This contribution was developed with Codex and Claude AI assistance. The AI helped analyse the initialisation ordering, implement the change and its regression tests, and prepare this description.

[tridge]

Deprecated — see below for the updated review.

Previous review (2026-09-15)

Automated review note — AI-generated (Claude), validated against the live diff (Claude + Codex cross-checked). Please sanity-check before acting.

Reviewed at head 23826e4177. Full report: https://uav.tridgell.net/DevCallReviews/2026_09_16_AIReview/devcall_pr_reviews.html#pr34413

Verdict: REQUEST CHANGES. The mechanism works, and the tests catch it: with AP_Mount/AP_Camera reverted, MountAuxFunctionAtBoot and CameraAuxFunctionAtBoot both fail. There are two one-line fixes, plus a question for maintainers about how much this gains.

Bug

• Both new test helpers crash when they actually have to wait (Tools/autotest/arducopter.py:8383, :8397).
  • delay_sim_time() takes a required reason argument (vehicle_test_suite.py:8051), but the helpers call self.delay_sim_time(1).
  • They pass today only because the first check usually succeeds; a slow log turns the wait into a TypeError.

Issues

• The boot read ignores RC validity (libraries/RC_Channel/RC_Channels.cpp:245).
  • read_aux_switch_position() only range-checks radio_in, while read_aux_all() returns early without valid input.
  • With SIM_RC_FAIL=2, FS_THR_ENABLE=1, RC7_OPTION=185, this PR logs AUXF fn=185 pos=MIDDLE source=INIT at 2.46 s (roll lock cleared) while the receiver is in throttle failsafe; master logs nothing. A second pass saw the mount retract and recording start in the same state.
  • You mention this trade-off in the description, but closing it is free: if (!rc_channels->has_valid_input()) { return; } after the null check in both init_aux_functions() stops it, and all three new tests still pass.
• The gain over master is small, and it probably doesn't explain the linked report.
  • Master already applies these switches at the first debounced read_aux(). In SITL the log shows INIT at 2.46 s against master's RC application at 2.74 s.
  • Where RC isn't readable at the first update, the new loop skips it just as master does, so the "RC comes up after the mount's first update" case is unchanged.
  • The Caddx report is MOUNT_RP_LOCK HIGH at boot. HIGH sets roll and pitch lock true (RC_Channel.cpp:1829-1831), which are already the backend defaults (AP_Mount_Backend.h:433-434), so a lost boot position wouldn't change anything there.

Notes

• Duplicate suppression isn't tested.
  • Removing both record_aux_switch_position() calls (AP_Mount.cpp:315, AP_Camera.cpp:860) leaves the tests green, while the log shows CAMERA_REC_VIDEO applied twice.
  • Please assert that there's no second AUXF.
• The same loop appears twice (AP_Mount.cpp:282-316, AP_Camera.cpp:831-861), and read_aux_switch_position() repeats the caller's find_channel_for_option() lookup.
  • One RC_Channels helper taking a table would do.
  • Cost: +184 bytes on MatekF405 (camera only), +1008 on SITL.
• The comments are much longer than usual for ArduPilot.
  • "45 lines ahead" (AP_Mount.cpp:223) and "50 lines ahead" (AP_Camera.cpp:800) will rot.
  • The = false in both constructors isn't needed.
• AuxFunctionAtBootWithoutRC's second half reboots with RC enabled (arducopter.py:8528-8530), so it doesn't cover RC arriving later in the same boot, as the docstring says.

Checked:

• Builds: SITL; SITL with RunCam/POI lock off; MatekF405 (mount off, camera on). AP_CAMERA_ENABLED=0 was checked by reading only, because a mount+camera-off SITL build fails identically on the base.
• Tests: all three new tests passed on every run in both passes.
• Code read: thread context, the _num_instances == 0 guard, and not-ready backends retrying as on master.

CI: 101 passing, 1 failing (copter-tests1d ModeLoiter), which looks unrelated.

[Hwurzburg]

Thanks — that was a useful review. Pushed ad770aebf8. Everything is addressed except one item, where the measurement disagrees with the suggestion.

The bug: fixed

delay_sim_time() does require reason, and both helpers were calling it without one. Correct that it only ever bit on the retry path. Fixed.

has_valid_input(): not free — it disables this on Plane

This is the one I'd push back on. I implemented exactly the suggested if (!rc_channels->has_valid_input()) { return; } and measured the first MNT sample on both vehicles, with RETRACT_MOUNT1 already in the retract position at boot:

	Copter	Plane
as pushed	Mode=0 (retracted)	Mode=0 (retracted)
with the gate	Mode=0 (retracted)	Mode=3 — not applied until the RC read

RC_Channels_Plane::has_valid_input() goes through in_rc_failsafe() → Plane::rc_failsafe_active(), which keys off failsafe.last_valid_rc_ms, written only inside the scheduled Plane::read_radio() — which has not run at the mount's first update(). So the gate closes the failsafe case on Copter and turns the feature off entirely on Plane. The three tests staying green is consistent with that: they are all Copter.

I've added Plane.MountAuxFunctionAtBoot so the trade can't be made by accident, and recorded the reason in its docstring and in the description. If the dev call decides the failsafe exposure is not acceptable, the answer is not this gate — it would be dropping the read, which leaves the mount case unfixed.

Your reproduction itself I'm not disputing: SIM_RC_FAIL=2 with a throttle failsafe does get the boot position applied here where master logs nothing. That is the open question in the description, and it is why ICE_START_STOP being outside this PR's function list matters.

Gain over master: agreed, and the description now says so

You're right on both counts, and I've rewritten that part rather than leave it overstated. The 4.7.1-beta report is MOUNT_RP_LOCK HIGH at boot, and HIGH sets _roll_lock/_pitch_lock true — already their initialiser values in AP_Mount_Backend.h and re-derived every update in RC targeting, so a lost boot position changes nothing there. I carried that citation over from #34166 without re-checking it and should not have. The description now states plainly that this is worth the ~200 ms window while disarmed plus the never-valid-RC case, and explicitly that it does not explain the linked report.

Duplicate suppression: now tested

Correct that removing both recordings left the tests green. Both tests now assert the AUXF count is exactly 1; with the recording removed they fail with "RETRACT_MOUNT1 applied 2 times, want 1" and "CAMERA_REC_VIDEO applied 2 times, want 1".

Duplicated loop: collapsed

Replaced by a single RC_Channels::apply_aux_switch_positions(functions, count). Since RC_Channels is already a friend of RC_Channel, that also removes both public helpers the previous push added (read_aux_switch_position(), record_aux_switch_position()) and the repeated find_channel_for_option() lookup you flagged. AP_Mount.cpp 118 → 59 lines, AP_Camera.cpp 87 → 42. MatekF405 is now +172 B (you measured +184 B).

Comments and style

Trimmed throughout; the "45 lines ahead" / "50 lines ahead" references are gone, as are the redundant constructor assignments.

AuxFunctionAtBootWithoutRC

Fixed — it now enables RC_PROTOCOLS in the same boot rather than rebooting, so it covers what the docstring claims.

CI

Copter.ModeLoiter passes locally on this branch; no aux involvement in it. Agreed it's unrelated.

[tridge]

Deprecated — see below for the updated review.

Previous review (2026-09-15)

Automated review note — AI-generated (Claude), validated against the live diff (Claude + Codex cross-checked). Please sanity-check before acting.

Re-reviewed at head ad770aebf8; my earlier comment above is superseded. Full report: https://uav.tridgell.net/DevCallReviews/followups/2026_09_16_0152/devcall_pr_reviews.html#pr34413

Verdict: still REQUEST CHANGES, but for a smaller set of problems.

• Of the 7 previous findings, 5 are resolved and 1 is partly resolved.
• You were right that my has_valid_input() gate breaks Plane, so I've withdrawn it.
• The failsafe case still has a cheap fix that keeps Plane working, and one of the new tests is flaky.

Resolved

• delay_sim_time() now gets reason (Tools/autotest/arducopter.py:8383, :8397).
• The description now matches the measured gain and no longer claims to explain the Caddx report.
• Duplicate suppression is tested. With the two switch_state assignments removed (libraries/RC_Channel/RC_Channels.cpp:280-281), both Copter tests fail with "applied 2 times, want 1". Both passes checked this by mutation.
• The loop is now a single RC_Channels::apply_aux_switch_positions() (RC_Channels.cpp:251). MatekF405 is +172 B, as you measured.
• AuxFunctionAtBootWithoutRC now brings RC up in the same boot (arducopter.py:8553).
• The tests catch a revert. With AP_Mount/AP_Camera reverted, Plane.MountAuxFunctionAtBoot and both Copter boot tests fail.

You were right

• The one-shot gate I suggested disables Plane. With it, Plane.MountAuxFunctionAtBoot fails (Mode=3), and both passes reproduced that. I've withdrawn it.
• The cause is slightly different from the one you gave.
  • read_radio runs before AP_Mount::update in the same tick (ArduPlane/Plane.cpp:68 vs :106).
  • On that tick it gets no fresh frame, while the startup failsafe path (ArduPlane/failsafe.cpp) has already called rc().read_input(). So last_valid_rc_ms is still unset.
  • The effect is the same.

Still open

• Failsafe positions are still applied at boot.
  • With SIM_RC_FAIL=2, FS_THR_ENABLE=1, RC7_OPTION=185, head logs AUXF fn=185 pos=MIDDLE source=INIT result=1 while the receiver is in throttle failsafe. Master logs nothing.
  • Dropping the read isn't the only alternative: keep the one-shot pending until RC is valid, instead of consuming it at the first update(). In AP_Mount::update() (AP_Mount.cpp:266) and AP_Camera::update() (AP_Camera.cpp:826):

    if (!_aux_functions_initialised) {
        RC_Channels *rcs = RC_Channels::get_singleton();
        if (rcs == nullptr || rcs->has_valid_input()) {
            _aux_functions_initialised = true;
            init_aux_functions();
        }
    }

  • Both passes found and tested this independently:
    • Plane.MountAuxFunctionAtBoot passes: Plane applies INIT at 1.765 s, before its first MNT sample at 1.805 s.
    • The failsafe reproduction applies nothing.
    • The Copter boot tests pass.
    • It costs +24 B on MatekF405.
  • It can't double-apply. read_aux_all() uses the same gate, and debounce needs another 200 ms after that.
  • AuxFunctionAtBootWithoutRC's second half then logs source=0, but it still passes.
  • If you take it, drop the "RC never becomes valid at all" benefit from the description: with frames arriving, that is the failsafe case.
• The comments are still long.
  • The block at AP_Mount.cpp:212-231, plus :235-239, RC_Channels.cpp:242-250 and :256-259.
  • AP_Mount.h:367-373 and AP_Camera.h:257-263 add two more comments below the block, on a bool.
  • The existing update() comments (AP_Mount.cpp:211, AP_Camera.cpp:786-788) now sit above init_aux_functions() instead of update().

New

• Copter.CameraAuxFunctionAtBoot is flaky, about 3%: 4 failures in 118 runs across both passes, with "No AUXF logged for CAMERA_REC_VIDEO".
  • Cause:
    • The INIT dispatch runs at ~2.460 s, before the new onboard log opens (~2.481 s).
    • AP_Logger_File::WritesOK() is false until the file is open, so that AUXF is dropped.
    • Because the position has been recorded, the RC-sourced AUXF that master would log never comes.
  • Why it matters:
    • The count==1 and source==INIT checks (arducopter.py:8500-8520, and the AUXF checks in MountAuxFunctionAtBoot) depend on that record, so expect intermittent tests1e failures.
    • On a vehicle, the boot application can be missing from the onboard log entirely.
    • The gate change above doesn't fix this: one run with it applied still failed.
• Plane.MountAuxFunctionAtBoot has two leftovers. It reboots without wait_new_onboard_log() (Tools/autotest/arduplane.py:9354), which the Copter tests use, and it raises a "PROBE: no MNT message logged" string (:9360).

Checked:

• Builds: SITL copter and plane, AP_CAMERA_ENABLED=0, and MatekF405 at head, at base, and with the gate. HAL_MOUNT_ENABLED=0 fails in AP_Mount_SoloGimbal.h:17, identically on master. AP_RC_CHANNEL_ENABLED=0 was checked by reading only.
• Commits: one per subsystem, which is fine.

CI at ad770aebf8: 102 passing, none failing.

[Hwurzburg]

Pushed dfa339521c. All four remaining items are taken, including the pending-until-valid suggestion — that one is a real improvement and it closes the objection I'd otherwise have carried into the dev call. Thanks.

Failsafe positions: taken, with a regression test

Adopted as suggested: the one-shot is now held pending rather than consumed at the first update(), in both AP_Mount::update() and AP_Camera::update(). Verified all three of your claims:

• Plane.MountAuxFunctionAtBoot passes — Plane still applies at boot.
• The failsafe reproduction applies nothing.
• MatekF405 is +24 B (+196 B total against master, up from +172 B).

I've added Copter.MountAuxFunctionAtBootRCFailsafe, which holds the vehicle in throttle failsafe from boot with the switch in the retract position and asserts nothing is applied. Mutation-tested: with the gate removed it fails with RETRACT_MOUNT1 applied (source=0 pos=2) from a receiver in failsafe, which is your reproduction.

I've also dropped the "RC never becomes valid at all" benefit from the description, as you said to — with frames arriving that is the failsafe case, so losing it is the point rather than a cost.

Your correction on the Plane cause is right and I've fixed it in the commit message, the test docstring and the description: read_radio does run first in the tick (priority 6 vs 105), it just has no fresh frame, so failsafe.last_valid_rc_ms stays unset while radio_in is already populated by the startup failsafe path.

CameraAuxFunctionAtBoot flakiness: fixed at the assertion

Good catch, and the diagnosis is right — the INIT dispatch can precede the log file opening and WritesOK() drops it, and because the position is recorded the RC-sourced entry never comes either.

Rather than chase the timing I've changed what is asserted. "Initialisation applied it" is now expressed as the absence of an RC-sourced AUXF, not the presence of an INIT-sourced one: the initialisation entry can be dropped, but if initialisation did not apply the switch then the first debounced read_aux() announces it, and that entry is always late enough to be logged. The INIT entry's pos/index/result are still checked, but only if it reached the log. Same for MountAuxFunctionAtBoot, whose primary assertion (first MNT sample is Mode=0) never depended on AUXF anyway.

That removes the failure mode by construction. 25 consecutive runs of Copter.CameraAuxFunctionAtBoot, no failures. The mutation checks still work: removing the recorded position makes both tests fail on the RC-sourced assertion.

The underlying observation — that on a vehicle the boot application can be missing from the onboard log — is real and worth stating, so I've left it in the test's comment rather than pretending it away. The mount case is still visible in MNT.

Comments

Trimmed again: the block above AP_Mount::init_aux_functions() is 11 lines, AP_Camera's is 5, and the header declarations are down to two lines each with the bool's separate comment removed.

Plane.MountAuxFunctionAtBoot leftovers

Both fixed — it uses wait_new_onboard_log() now, and the "PROBE" string is gone. The helpers (wait_new_onboard_log, wait_auxf_for_function, first_auxf_for_function, assert_no_rc_sourced_auxf) have moved to vehicle_test_suite.py so both vehicles share them rather than Plane reaching into arducopter.py.

State

424 insertions, 0 deletions, 4 commits, one per subsystem. ./waf copter plane rover replay; 8/8 on the new tests plus Copter.Mount, Copter.MountPOIFromAuxFunction and Copter.AuxSwitchOptions.

[tridge]

Deprecated — see below for the updated review.

Previous review (2026-09-15)

Automated review note — AI-generated (Claude), validated against the live diff (Claude + Codex cross-checked). Please sanity-check before acting.

Re-reviewed at head dfa339521c; my earlier comment above is superseded. Full report: https://uav.tridgell.net/DevCallReviews/2026_09_16_AIReview/devcall_pr_reviews.html#pr34413

Verdict: COMMENT (was REQUEST CHANGES): no blockers left in the code.

• Of the 5 findings still open from my previous comment, 4 are resolved and 1 is partly resolved.
• What remains is test coverage and wording.

Previous round

• Failsafe positions: resolved.
  • The one-shot now waits for has_valid_input() (libraries/AP_Mount/AP_Mount.cpp:262-268, libraries/AP_Camera/AP_Camera.cpp:825-832).
  • With that gate forced true, Copter.MountAuxFunctionAtBootRCFailsafe fails.
  • Plane.MountAuxFunctionAtBoot passed 3/3. Your +24 B on MatekF405 checks out.
• Camera test flake: resolved. Checking for no RC-sourced AUXF removes the log-open race: 20/20 Camera runs passed, and 40/40 in a mixed loop.
• Description overstatement and the Plane test leftovers: resolved.
• Comments: partly resolved.
  • The existing // update - give mount opportunity... comment still sits above init_aux_functions() rather than update() (AP_Mount.cpp:211, AP_Camera.cpp:786-788).
  • The RC_Channels.cpp comments are unchanged.

Test coverage (worth fixing, not blocking)

• Exactly-once is no longer tested (Tools/autotest/arducopter.py:8410, :8445).
  • Codex deleted _aux_functions_initialised = true, so the positions are re-applied on every update(). Both Copter boot tests still pass, with 178 INIT AUXF records in the first 6 s.
  • The assertions only look at the first MNT sample, the first AUXF, and the absence of an RC-sourced AUXF.
  • Asserting at most one INIT AUXF per function would catch it.
• The Camera test can pass without the camera doing anything (arducopter.py:8445-8460).
  • We both tried recording the position but skipping run_aux_function(): CameraAuxFunctionAtBoot still passes whenever the INIT AUXF misses the log.
  • Mount catches the same change through its MNT mode check. A camera-side state check would close this.
• The AP_Camera failsafe gate has no test (AP_Camera.cpp:828); only the mount one is covered.

Note

• RC becoming valid late: the pending one-shot then applies the current switch position once, which can override a GCS-set mount mode after takeoff.
  • Master does the same about 200 ms later through its first debounced read_aux(). Codex flew both in SITL, so this is not a new hazard.
  • Some wording is now inaccurate:
    • "the switch positions the vehicle booted with" (AP_Mount.cpp:214, :257)
    • "leaving master's behaviour untouched" in the description

Checked:

• The head-to-head delta and the full diff.
• Copter and Plane SITL at head.
• Four mutations by me and three by Codex, all in a private network namespace (not --uds).

CI: 97 passing, 3 pending, 2 failing. Both failures look unrelated: LoweheiserManual in copter-tests1e ("Did not fully drain MAV") and CANGPSCopterMission in sitltest-can. All four new Copter tests passed in copter-tests1e.

[Hwurzburg]

Pushed 8e6d2c2f1d. All four items taken, and both coverage holes are now mutation-tested against the exact changes you and Codex made.

Exactly-once is tested again

You're right that I lost this when I reworked the assertion to fix the flake — the absence-of-RC-sourced-AUXF check says nothing about how many times initialisation ran. Added assert_at_most_one_init_auxf(), asserted in both Copter boot tests. At-most rather than exactly, because the entry can still be dropped when it predates the log file opening.

Reproduced your mutation (deleting _aux_functions_initialised = true in both libraries):

function 27 applied by initialisation 684 times, want at most 1
function 166 applied by initialisation 184 times, want at most 1

The camera test now requires the camera to have done something

Your "record the position but skip run_aux_function()" mutation was passing because the INIT AUXF misses the log and the RC-sourced one is suppressed — a vacuous pass. The fix is to stop letting the entry go missing: CameraAuxFunctionAtBoot now boots in throttle failsafe, so the pending one-shot is held until RC is recovered, which is well after the log opens. The dispatch is then deterministic and the test asserts AUXF.result == 1 — RunCam reporting the recording actually started — plus source, pos, index and at-most-once.

That also gives the AP_Camera side of the failsafe gate its own coverage, which was your third point: the first half of the test asserts nothing is applied while the receiver is in failsafe.

With your skip-dispatch mutation it now fails: No AUXF logged for function 166.

Comments

Both stale ones moved back: // update - give mount opportunity to update servos... and /* update; triggers by distance moved and camera trigger */ had ended up above init_aux_functions() rather than update(). Fixed in both files.

Wording

Corrected — "the switch positions the vehicle booted with" isn't accurate once the one-shot can fire later. The comments now say it applies the current switch position, and note explicitly that where RC becomes valid late that is the position the switch is in then, which is what the first debounced read_aux() would have applied anyway. "leaving master's behaviour untouched" is out of the description.

I left the RC_Channels.cpp comments as they are — they're explaining why each of the three skip conditions exists, which is the part a future reader is most likely to undo by accident.

State

479 insertions, 0 deletions, 4 commits. ./waf copter plane rover replay; 7/7 including Copter.Mount and Copter.MountPOIFromAuxFunction, and the five boot tests run twice more clean.

Agreed on the two CI failures — LoweheiserManual and CANGPSCopterMission are unrelated.

[tridge]

Deprecated — see below for the updated review.

Previous review (2026-09-15)

Automated review note — AI-generated (Claude), validated against the live diff (Claude + Codex cross-checked). Please sanity-check before acting.

Re-reviewed at head 8e6d2c2f1d (you were last told dfa339521c); my earlier comment above is superseded. Full report: https://uav.tridgell.net/DevCallReviews/followups/2026_09_16_0747/devcall_pr_reviews.html#pr34413

Verdict: APPROVE (was COMMENT): no blockers.

• Of the 5 findings from my previous comment, 3 are resolved and 2 are partly resolved; what is left is wording.
• The firmware change in this push is comments only.
• There is one cheap test-hardening suggestion below.

Previous round

• Exactly-once: resolved. Deleting _aux_functions_initialised = true in both libraries now fails both boot tests: "applied by initialisation 345 times" (Mount) and "41 times" (Camera). See Tools/autotest/vehicle_test_suite.py:14002.
• Camera test doing nothing: resolved. Recording the position without run_aux_function() fails 5/5 with "No AUXF logged for function 166" (Tools/autotest/arducopter.py:8468).
• AP_Camera failsafe gate: resolved. With only the camera gate forced true, CameraAuxFunctionAtBoot fails 6/6 ("applied ... from a receiver in failsafe"), while MountAuxFunctionAtBootRCFailsafe still passes.
• Comments: partly resolved.
  • Both update() comments are back in place (libraries/AP_Mount/AP_Mount.cpp:253, libraries/AP_Camera/AP_Camera.cpp:817).
  • Agreed on the inline RC_Channels.cpp comments: each guards a line someone might delete.
• Wording: partly resolved. The code comments and the "untouched" line are fixed. Still stale:
  • "the position the vehicle booted with" at libraries/RC_Channel/RC_Channels.cpp:246.
  • The description says "The tests assert the AUXF count is exactly 1"; they now assert at most one INIT entry and no RC-sourced one.
  • The description says "424 insertions"; it is 479 at head.

Worth hardening (not blocking)

• Both failsafe boot tests depend on the first RC datagram (Tools/autotest/arducopter.py:8449, :8502).
  • Until it arrives, AP_RCProtocol_UDP fabricates 1000 µs throttle (libraries/AP_RCProtocol/AP_RCProtocol_UDP.cpp:24).
  • Failsafe needs throttle < FS_THR_VALUE (ArduCopter/radio.cpp:145), so with FS_THR_VALUE at 1000 that fabricated value counts as valid RC.
  • Codex withheld RC delivery across reboot_sitl() with the firmware unchanged, and CameraAuxFunctionAtBoot failed 10/10. With FS_THR_VALUE at 1100 the same probe passed. The Mount failsafe test has the identical setup (checked by reading only).
  • It never happened on its own: those two tests passed 238/238 below. This is the same fabricated-default race your channel 6/7 comments already guard against, and a threshold above 1000 closes it.

Minor

• AP_RunCam::record_video() always returns true (libraries/AP_Camera/AP_RunCam.h:110-117). AUXF.result == 1 therefore proves the dispatch reached a real backend, not that recording started. That is the defect being fixed, so the assertion is fine.
• The reflow at libraries/AP_Camera/AP_Camera.cpp:788-789 leaves a 3-word line.

Checked:

• Delta: the head-to-head change to the libraries is comments only.
• Recorded position: removing it still fails both boot tests on the RC-sourced check (Camera 8/8), so debounce timing does not beat that check.
• Repeat runs: the four Copter boot tests at head, in private network namespaces (not --uds):
  • CameraAuxFunctionAtBoot 121/121
  • MountAuxFunctionAtBoot 120/120
  • MountAuxFunctionAtBootRCFailsafe 117/117
  • AuxFunctionAtBootWithoutRC 117/117
• Lint: flake8 is clean.

CI: 102 passing.

[peterbarker]

We also have to be careful around
RC_Channel::init_position_on_first_radio_read

This PR doesn't introduce a problem, but adds structures which could allow for bypassing things in that list. It's worth a comment, probably.

[peterbarker]

use rc()

[peterbarker]

irrelevant

[peterbarker]

we don't need the history lesson

[peterbarker]

use rc()

[peterbarker]

technically has_valid_input is not sufficient to init_aux_functions generally.

Just because you has_valid_input does not mean you has_valid_input for every single channel!

e.g. RC_CHANNELS_OVERRIDE
e.g. a multi-RC-receiver aircraft where one of the backends doesn't supply all channel values

I think we need to let this go for now, but assuming has_valid_input is sufficient is actually the cause of some fairly significant bugs in ArduPilot ATM.

[peterbarker]

Use rc()

[peterbarker]

Hot garbage comment

[peterbarker]

rc()

[peterbarker]

cut this comment down

[peterbarker]

Restrict the docstring for methods to one line; if you want more text then use another """ after it.

Really long docstrings screw up the test output

[Hwurzburg]

Thanks Peter — all taken, pushed 05a650f711.

init_position_on_first_radio_read

Agreed, and I've gone further than a comment: apply_aux_switch_positions() now skips anything in that list outright.

// never act on whatever position these happen to be in when first
// read - that is the whole point of the list, and reading the switch
// here would otherwise be a way around it
if (c->init_position_on_first_radio_read(aux_function)) {
    continue;
}

RC_Channels is already a friend of RC_Channel, so it costs three lines. You're right that there's no problem today — none of ARMDISARM/ARMDISARM_AIRMODE/ARM_EMERGENCY_STOP/PARACHUTE_RELEASE are in the mount or camera lists — but the helper is generic and the next library to pick it up would have had nothing stopping it. Better to make it impossible than to document it.

rc()

Done, all four sites, and the null checks and their comments went with it. I did check rather than just swapping, because dropping a null guard on "the singleton must exist by now" is what crashed Replay in #34166: AP_Mount::update()/AP_Camera::update() are only reachable from the four vehicle scheduler tables, Replay instantiates neither library, and AP_Mount_Backend already calls rc() unguarded on the same code path.

has_valid_input() is not per-channel

Understood, and noted in the code rather than silently relied on:

// Note has_valid_input() is an aggregate; it does not promise
// every individual channel is fresh.

Leaving the behaviour as-is per your call. If you want the pointer to the real fix later — RC_CHANNELS_OVERRIDE and mixed-backend receivers — I'm happy to take that on as its own PR rather than smuggle it into this one.

Comments, #endif labels, docstrings

All cut down — the AP_Mount and AP_Camera blocks are three lines each now, the history lesson and the Replay aside are gone, and the #endifs in what I added carry their condition. Docstrings are one line with the prose in a following """ block, matching TakeoffAuto1 and friends.

I've left the three inline comments inside apply_aux_switch_positions() as they are — each one guards a specific continue that looks removable and isn't. Say the word if you'd rather they went too.

Also in this push

@tridge's last pass found a latent race in my own tests: I had FS_THR_VALUE 1000, and AP_RCProtocol_UDP fabricates throttle at exactly 1000us, so throttle < FS_THR_VALUE is false and the fabricated frame counted as valid RC until the harness's real datagram landed. Never fired on its own in 238 runs, but withholding RC across the reboot failed it 10/10. Both failsafe tests now use 1100.

Re-verified after all of the above: the gate mutation still fails both failsafe tests, the latch mutation still fails both boot tests on the at-most-once assertion, and the skip-dispatch mutation still fails the camera test. copter/plane/rover/replay build; 7/7 tests, twice.

[AP-Review]

Deprecated — see below for the updated review.

Previous review (2026-09-16)

Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting.

Re-reviewed at head 05a650f711 (previously 8e6d2c2f1d); my earlier comment above is superseded.

Full report: https://uav.tridgell.net/DevCallReviews/followups/2026_09_17_0037/devcall_pr_reviews.html#pr34413

---

Verdict: COMMENT (was APPROVE) — everything from my last round and all twelve of peterbarker's inline comments are addressed, and the tests are solid at 500/500. One behaviour change nobody has flagged, which is why this is not an APPROVE.

Previous round — all resolved

All 12 inline comments applied (rc() at AP_Camera.cpp:804/:816 and AP_Mount.cpp:234/:246; the "history lesson" and "hot garbage" blocks deleted; #endif labels on all 16). Plus: the stale "the position the vehicle booted with" phrase is gone (RC_Channels.cpp:242-249); the description's AUXF-count and insertion-count claims are both corrected and now match GitHub; and the failsafe-boot race is closed — FS_THR_VALUE is 1100 at arducopter.py:8451/:8508, and since AP_RCProtocol_UDP.cpp:22 fabricates pwm_input[2]=1000, failsafe now trips from the very first frame. 1100 is the top of the documented @Range: 910 1100.

ISSUE — libraries/RC_Channel/RC_Channels.cpp:286 — the boot-time GCS announcement is silently lost

read_aux() prints "RC%i: %s %s" only after debounce_completed() returns true (RC_Channel.cpp:1030-1039), and switch_state.current_position/debounce_position start at -1 (RC_Channel.h:573-574), so on master the first debounced read always announces. apply_aux_switch_positions() now sets both to the applied position, so debounce_completed() returns false and the message never comes — and the INIT dispatch path has no announcement of its own.

Copter, RC7_OPTION=27 (RETRACT_MOUNT1), switch HIGH at boot: master prints RC7: RetractMount1 HIGH a few hundred ms after boot; at this head nothing is printed for that switch at all. Same for all 5 mount and 7 camera functions, on every vehicle.

Verified by mutation: with return; added at the top of AP_Mount::init_aux_functions() (master behaviour), Copter.MountAuxFunctionAtBoot logs AP: RC7: RetractMount1 HIGH between "Rebooting SITL" and "Reboot complete"; at the unmodified head that window has no RC7: message.

Not a safety problem, but it is a user-visible diagnostic that disappears silently — it is exactly the message someone checks when "my retract switch does nothing" — and it is not in the description, the comments or the tests. Cheapest fix is the same GCS_SEND_TEXT from apply_aux_switch_positions() on a successful dispatch; if it is deliberate, it belongs in the description.

Notes

• AP_Camera.cpp:800 — AP_Camera has no equivalent of AP_Mount's _num_instances == 0 guard, and RUNCAM_CONTROL/RUNCAM_OSD_CONTROL have the same shape as the mount handlers (RC_Channel.cpp:1244, :1264 return void after a null check, so do_aux_function() returns true regardless). It does not bite today because AP_RunCam is allocated inside AP_Camera::init() (:255), so existence is already decided by the first update(). Worth a comment — the asymmetry currently reads as an oversight.
• The description's flash figures are stale. Measured, same board, same ./waf configure --board MatekF405 && ./waf copter on both trees: head text 871316 / bss 80700 (flash 874396), merge base 35356c656c text 871116 / bss 80700 (flash 874196). That is +200 B flash and 0 B RAM, not +196 B / −4 B — the extra 4 bytes match the init_position_on_first_radio_read() guard added in this push.

Measured: 500/500 repeat runs at this head across five isolated workers (separate checkout, separate BUILDLOGS, separate network namespace, no --uds) — the five boot tests, 100 each. The ~3% CameraAuxFunctionAtBoot flake from two rounds ago did not reappear. I also confirmed the rc() switch is safe: every call site of AP_Mount::update()/AP_Camera::update() is a main-loop scheduler task in Copter/Plane/Rover/Sub, and Replay/AP_Periph link neither library. CI at this head is 83 passing, 19 queued, nothing failing.

[Hwurzburg]

Pushed b8b183995f. The issue is real, and is fixed; both notes are taken.

The lost boot announcement — fixed

Confirmed exactly as described. read_aux() announces only after debounce_completed() returns true, and recording the applied position makes that return false for a switch which has not moved, so the message never came.

RC_Channels::apply_aux_switch_positions() now makes the same announcement itself, on a dispatch which took — same format string, same string_for_aux_function()/string_for_aux_pos(), same AP_RC_CHANNEL_AUX_FUNCTION_STRINGS_ENABLED guard. A dispatch which did not take is still not recorded, so the first debounced read announces that one: exactly one announcement either way. The only visible difference from master is ordering — read_aux() announces before dispatching, this announces after, so a handler's own text now precedes it.

Copter.CameraAuxFunctionAtBoot waits for RC6: Camera Record Video HIGH. That is the right test for it: it holds the vehicle in failsafe, so the application happens after the GCS is connected rather than in the boot window where the statustext would be a race. Mutation-tested — with the announcement removed it fails with Failed to receive text: rc6: camera record video high. 10/10 consecutive passes with it in, and the strings-disabled path still compiles (-DAP_RC_CHANNEL_AUX_FUNCTION_STRINGS_ENABLED=0 on RC_Channels.cpp).

AP_Camera asymmetry — comment added

Agreed it reads as an oversight. Comment on AP_Camera::init_aux_functions() now says why there is no _num_instances equivalent: the camera handlers return false with no backend so the helper does not record them, and AP_RunCam — whose two handlers, like the mount's, return void — is allocated in init(), so its existence is settled before the first update().

Flash figures — corrected

Your +200 B / 0 B was right and the description is now fixed. Worth adding why that board was cheap: MatekF405 pulls in minimize_fpv_osd.inc → minimize_common.inc, which sets AP_RC_CHANNEL_AUX_FUNCTION_STRINGS_ENABLED 0, so the announcement is free there and the figure is unchanged from the one you measured. Both boards rebuilt on both trees:

board	merge base 35356c656c	head b8b183995f	delta
MatekF405 (mount off, strings off)	flash 874196, bss 80700	flash 874396, bss 80700	+200 B flash, 0 B RAM
CubeOrange (both libraries, strings on)	flash 1650396, bss 137388	flash 1650748, bss 137396	+352 B flash, +8 B RAM

Re-run

test.Copter.MountAuxFunctionAtBoot CameraAuxFunctionAtBoot AuxFunctionAtBootWithoutRC MountAuxFunctionAtBootRCFailsafe, test.Plane.MountAuxFunctionAtBoot, plus test.Copter.Mount MountPOIFromAuxFunction AuxSwitchOptions AuxFunctionsInMission — all pass. check_branch_conventions.py clean, flake8 clean on the three autotest files (the one E741 is pre-existing).

[AP-Review]

Deprecated — see below for the updated review.

Previous review (2026-09-20)

Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting.

Re-reviewed at head b8b183995f (previously 05a650f711); my earlier comment above is superseded.

Full report: https://uav.tridgell.net/DevCallReviews/followups/2026_09_20_1145/devcall_pr_reviews.html#pr34413

---

Verdict: REQUEST CHANGES (was COMMENT). Everything from my last round is resolved, and I verified the fix rather than just its presence. But the independent second pass looked somewhere neither of us had looked, and chasing it down found a real behavioural regression in the default gimbal configuration. Details below, including two further claims from that pass that I investigated and refuted — please don't spend time on those two.

Previous round — all resolved

The lost GCS announcement is properly fixed (libraries/RC_Channel/RC_Channels.cpp:284-291). I checked more than its presence:

• Coverage is exact by construction. The announcement sits after the failed-dispatch continue and before the position recording, and recording is the only thing that suppresses read_aux()'s announcement — so every path that records announces, and every path that doesn't leaves read_aux() to do it. The one function that prints nothing, MOUNT_POI_LOCK, has no lookuptable entry, so master prints nothing for it either. Nothing is lost.
• Text and channel numbering are identical. c->ch() is ch_in + 1 (RC_Channel.h:49), exactly what read_aux() passes at RC_Channel.cpp:1038; same format string, same helpers, and string_for_aux_pos() can't return nullptr. Empirically both producers printed AP: RC7: RetractMount1 HIGH character-for-character.
• The guard is right and provably free. On MatekF405, which compiles the strings out, the flash delta is still exactly +200 B — unchanged from the previous head, which had no announcement at all. So it costs zero bytes there.
• Mutation-proved both ways: with the #if forced off, Copter.CameraAuxFunctionAtBoot fails on the missing statustext and the RC7: line vanishes from the post-reboot window.

Flash figures — your updated table is right on both boards. Re-measured from clean trees against 35356c656c: MatekF405 .text 870628 → 870828 = +200 B, 0 B RAM; CubeOrange .text 1645996 → 1646348 = +352 B, .bss +8 B.

The AP_Camera comment is accurate — I checked each claim, including both AP_RunCam allocation sites (AP_Camera.cpp:255, and :1093 via convert_params() at :203). There is no lazy allocation anywhere else.

---

BUG — libraries/AP_Mount/AP_Mount.cpp:228 — MOUNT_YAW_LOCK at boot captures its lock heading from state nothing has populated yet, and it sticks

AP_Mount_Backend::set_yaw_lock() (AP_Mount_Backend.cpp:351-366) doesn't just set a flag — when _yaw_lock is currently false it captures _yaw_lock_heading_rad from get_attitude_quaternion() plus AP::ahrs().get_yaw_rad(). _yaw_lock is zero-initialised, so the boot application always takes that branch.

The gate at AP_Mount.cpp:246-249 runs before the backend loop at :253, and measured, rc().has_valid_input() is already true on AP_Mount::update() call #1 — so the capture happens with zero backend update() calls behind it.

Measured in SITL on two instrumented builds (this head vs merge base 35356c656c, same patch on both, under a private netns, no --uds; identical across three repeats each). Siyi, MOUNT_YAW_LOCK HIGH at boot, stock MNT1_DEFLT_MODE and MNT1_RC_RATE:

	this head	base 35356c656c
capture time	2459 ms (first AP_Mount::update())	2739 ms (first debounced read_aux())
first Siyi attitude packet	2499 ms — 40 ms after the capture	2499 ms — 240 ms before it
gimbal body-frame yaw read	0.000° (unpopulated)	180.000° (real)
AHRS yaw read	−0.000° (uninitialised)	−19.820° (settled)
commanded EF gimbal yaw target at 6019 ms	−0.000°	+160.180°

Master sits outside the unready window; this head is inside it by 40 ms. The 280 ms head start is the whole difference.

Two independent causes, and the second is the worse one:

1. AP_Mount_Siyi::get_attitude_quaternion() (AP_Mount_Siyi.cpp:131-136) returns true unconditionally from _current_angle_rad, zero until packets arrive. That isn't a Siyi quirk — 11 of the 13 mount backends always return true. (Only Alexmos and SoloGimbal can return false, and that's no better: set_yaw_lock() then leaves _yaw_lock_heading_rad at its zero-init value, the same wrong answer.)
2. AP::ahrs().get_yaw_rad() is also still zero at 2459 ms, which is backend-independent. A Servo mount with no telemetry at all captured −0.000° at this head against −19.820° at base — so a ~20° error reaches every backend, with the gimbal-attitude error on top for those that report it.

It matters in the default configuration and it does not self-heal. _yaw_lock_heading_rad is consumed at exactly one place, AP_Mount_Backend.cpp:159, in update_mnt_target_from_rc_target()'s angle branch — reached with MNT1_DEFLT_MODE at its default RC_TARGETING (AP_Mount_Params.cpp:23) and MNT1_RC_RATE at its default 0 (:32). And set_yaw_lock() recaptures only when _yaw_lock is currently false, so once HIGH latches at boot the wrong heading persists until the pilot toggles the switch LOW→HIGH or a gimbal-manager command clears it.

Suggested direction (untested — a direction, not a patch): MOUNT_YAW_LOCK is the only one of your five mount functions whose handler captures state from the backend — RETRACT_MOUNT1/2 call set_mode(), MOUNT_RP_LOCK sets two bools, MOUNT_POI_LOCK is a no-op this early. Dropping it from the list at AP_Mount.cpp:228 restores master's behaviour for that one function while keeping the rest of the PR. Note a backend->healthy() gate would not be enough — AP_Mount_Backend::healthy() defaults to true (AP_Mount_Backend.h:57-58), so Servo/CADDX/SToRM32 slip through, and it does nothing about the AHRS-yaw half.

This is the same class of thing you already document for AP_Camera_MAVLinkCamV2 under "Known limitation" — a backend existing isn't a backend being ready. The difference, and why I think this one blocks: there the failure is a silent no-op, whereas here the PR latches a wrong value that master gets right.

Limit of this finding: SITL-verified and deterministic, but hardware timing is unverified. A real Siyi boots more slowly than the simulated one — but so does a real autopilot, and the AHRS-yaw half depends on EKF yaw-alignment timing, which I also could not measure here.

---

Two related claims I investigated and refuted — no action needed

The same second pass raised these; I'm recording them so you aren't sent after non-problems.

• Early SIYI record dispatch accepting an ineffective stop. The mechanism is real (RecordingStatus::OFF == 0, _config_info only populated by the memcpy at AP_Mount_Siyi.cpp:399), but the timing refutes it: both trees reach the dispatch with identical unpopulated state (record_status=0, cfg_seen=0). Nothing requests the gimbal config before the first record_video() call — that call makes the first request itself, and the update() path's first is at 4459 ms, ~1.7 s after even master's dispatch. Pre-existing and identical on master; if anything master is worse, since read_aux() latches the position regardless of the dispatch result (RC_Channel.cpp:1043) whereas your helper latches only on success.
• POI init readiness. Both trees fail identically at their dispatch times. AP::ahrs().get_location() first succeeds at 10019 ms — 7.56 s after this head's dispatch — so the 280 ms shift is 3.7 % of the margin.

---

Also checked and clean: the mount/camera AUX_FUNC lists match what those libraries initialise; failed-dispatch fallback, unreadable and unassigned channels, and the init_position_on_first_radio_read() exclusions (only ARMDISARM*, ARM_EMERGENCY_STOP, PARACHUTE_RELEASE — a no-op for these lists, which is the guard rail asked for); debounce bookkeeping on a later real switch change; no new or renumbered parameters; four commits, one per subsystem; git diff --check clean; 479 insertions / 0 deletions across 9 files, matching the description.

Tests: 4/4 on the boot tests, then 25 consecutive iterations of the two main ones — 54/54, no flakes, under a private netns with private buildlogs and no --uds. The ~3 % camera flake from earlier rounds did not reappear. CI: 102/102 passing.

One thing needing a human rather than another AI round: peterbarker's CHANGES_REQUESTED of 2026-09-16 is still the formal reviewDecision. All 12 inline comments have matching code changes, but he hasn't re-reviewed.

[Hwurzburg]

Pushed eca99fad85. The MOUNT_YAW_LOCK finding is right, and I reproduced it before acting on it rather than taking it on trust.

Reproduced

Instrumented AP_Mount_Backend::set_yaw_lock() on both trees with the same patch, servo mount (MNT1_TYPE=1), MOUNT_YAW_LOCK HIGH at boot, stock MNT1_DEFLT_MODE/MNT1_RC_RATE:

	capture time	gimbal bf yaw	AHRS yaw	heading captured
head b8b183995f	2459 ms	0.000°	0.000°	0.000°
base 35356c656c	2739 ms	0.000°	−102.781°	−102.781°

Your 2459/2739 ms are exact. My captured value differs from your −19.820° only because my sim starts on a different heading — 3/3 identical runs on each tree. A ~103° error here, and from the AHRS half alone, on a backend that reports no attitude at all. That is a regression this PR introduced, in the default configuration, and it does not self-heal because set_yaw_lock() recaptures only from the unlocked state.

Fixed by dropping it from the list

MOUNT_YAW_LOCK is out of AP_Mount's list, with the reason in a comment at AP_Mount.cpp:225. It keeps master's behaviour for that one function — the first debounced read_aux() applies it 200 ms later — and the other four are unaffected: set_mode(), two plain bool setters, and a POI search that times out identically on both trees.

I took your point about healthy() and did not use it. I also confirmed your POI refutation independently: update_poi_lock_target() gives up after 5000 ms (AP_Mount_Backend.cpp:338) and get_location() does not succeed until ~10 s, so both trees abandon it regardless of the 280 ms.

Copter.MountAuxFunctionAtBootRCFailsafe now asserts the split after RC recovers — RETRACT_MOUNT1 first applied by INIT, MOUNT_YAW_LOCK first applied by RC. That is the deterministic place for it: everything after the failsafe clears happens well past the log opening. Mutation-tested — putting MOUNT_YAW_LOCK back fails it with MOUNT_YAW_LOCK first applied by source=0, want RC. 10/10 consecutive passes with it in.

Flash, re-measured

Dropping the entry saves a little, so the CubeOrange figure moved. Both boards rebuilt against 35356c656c:

board	delta
MatekF405 (mount off, strings off)	+200 B flash, 0 B RAM (unchanged)
CubeOrange (both libraries, strings on)	+348 B flash, +4 B RAM (was +352 / +8)

Re-run

Nine tests: the four boot tests, Plane.MountAuxFunctionAtBoot, Copter.Mount, MountPOIFromAuxFunction, AuxSwitchOptions, AuxFunctionsInMission — all pass. check_branch_conventions.py clean, git diff --check clean, flake8 clean bar the pre-existing E741. 501 insertions, 0 deletions, still four commits.

@peterbarker — all twelve of your inline comments have been addressed since your review, and two AI-found issues on top. A re-look when you have time would be welcome.

[AP-Review]

Deprecated — see below for the updated review.

Previous review (2026-09-20)

Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting.

Re-reviewed at head eca99fad85 (previously b8b183995f); my earlier comment above is superseded.

Full report: https://uav.tridgell.net/DevCallReviews/followups/2026_09_20_2330/devcall_pr_reviews.html#pr34413

---

Re-reviewed at head eca99fad85: the one finding from my previous comment is resolved, and resolved the right way. Nothing new blocks. Verdict moves REQUEST CHANGES → COMMENT.

You took the suggested direction exactly: MOUNT_YAW_LOCK dropped from the list, the reason recorded in the code, and — which I did not ask for — a regression test that pins it. Thank you for reproducing the finding before acting on it; your flash figures agree with mine.

Previous round — resolved

BUG AP_Mount.cpp:228 — MOUNT_YAW_LOCK captured its lock heading from unpopulated state: RESOLVED. I verified the fix rather than its presence, five ways:

1. The list is now provably the right set, not just one entry shorter. I checked both lists against the only thing that defines what master applies at init — the second group of RC_Channel::init_aux_function(), whose cases fall through to run_aux_function(..., Source::INIT, ...). That group is exactly RETRACT_MOUNT1/2, MOUNT_YAW_LOCK, MOUNT_RP_LOCK, MOUNT_POI_LOCK on the mount side and exactly the seven camera functions you list. CAMERA_IMAGE_TRACKING and MOUNT_LRF_ENABLE sit in the first group ("do not need to be initialised"), so master does not apply them at init either and their absence is correct rather than an omission.
2. Master's behaviour is fully restored for the excluded function. switch_state.debounce_position/current_position start at -1 and the helper writes them only for functions it applied, so RC8's state is untouched and read_aux() fires on its first debounced read for any position, LOW included.
3. From the dataflash: fn=27 pos=2 source=0 index=6 at 16.379 s, then fn=163 pos=2 source=1 index=7 at 16.639 s — 260 ms later, on the RC path.
4. With an instrumented build: exactly one set_yaw_lock() call, ahrs=-86.851 cap=-86.851 — a real settled heading, against ahrs=0.000 cap=0.000 at the previous head.
5. Mutation-proved, so the new test is not vacuous: re-adding the entry makes it fail with MOUNT_YAW_LOCK first applied by source=0, want RC, and the mutated log shows fn=27 source=0 and fn=163 source=0 in the same millisecond.

I also checked the enum values your test hardcodes against the enum rather than the comment beside them: Source::INIT is 0, Source::RC is 1, AuxSwitchPos::HIGH is 2. All correct.

One correction to my own previous comment

MOUNT_POI_LOCK does capture state at handler time, so my "a no-op this early" was too strong. set_poi_lock() does saved_mount_mode = get_mode() (AP_Mount_Backend.cpp:267) plus poi_start_ms = millis(). It is benign and I am not asking for a change: AP_Mount::init() calls set_mode_to_default() for every instance before this runs, so get_mode() returns MNT1_DEFLT_MODE in both trees, and the saved value is only consumed by suspend_poi_lock() once roi_is_set(), which does not happen at boot.

The independent pass argued MOUNT_POI_LOCK should also be excluded, as a BUG. I investigated and refuted that — please don't spend time on it. The mechanism is real (update_poi_lock_target() marks the home-alt attempt done whether or not it succeeded, then zeroes poi_start_ms, so a boot-held POI lock burns one attempt and does not retry) but it is not a regression: calculate_poi_at_home_alt() needs ahrs.get_location(), which first succeeds around 10 s, and master's own dispatch is only ~280 ms later than yours — still ~7.5 s early — so master burns an equally doomed attempt and equally does not retry. Excluding it would change nothing.

Two small notes

• AP_Mount.cpp:225-229 — the new comment is 5 lines where 2 would do, and peterbarker has asked twice on this PR for shorter comments. Measured, 43 of the 141 non-blank added lines under libraries/ are comment. The content is right; something like "MOUNT_YAW_LOCK is left out: set_yaw_lock() captures a heading from the gimbal attitude and AHRS yaw, neither ready this early, and only recaptures while unlocked" keeps it.
• The +4 B CubeOrange RAM figure I could not confirm or refute. In these links text+data+bss is bit-identical between trees, so .bss as size reports it is a remainder rather than a RAM measure; a symbol-level diff would settle it. Flash I did re-measure: MatekF405 +200 B, CubeOrange +348 B, both matching your table — and the CubeOrange figure is 4 B smaller than last round, exactly as dropping one AUX_FUNC array entry predicts.

Also checked and clean

I audited every handler in both lists for handler-time state capture, which my previous round never did for the camera side: all seven camera handlers are pure switch-position dispatch, the two RunCam ones are flag setters, and every AP::ahrs() read in libraries/AP_Camera is in take_picture, the trigger-distance path, FOV status or camera feedback — none reachable from the list. AP_Camera_MAVLinkCamV2 refuses until the camera is discovered, so the helper does not record it and the RC read retries (your documented "Known limitation", and master's behaviour too). No second instance of the boot-capture bug exists in either list. Debounce bookkeeping re-derived: not setting switch_state.initialised is harmless, because every function in these lists returns false from init_position_on_first_radio_read(); and last_edge_time_ms is always written before it is read on a later genuine switch move.

Tests, under a private network namespace with private buildlogs and no --uds: MountAuxFunctionAtBootRCFailsafe 12/12, CameraAuxFunctionAtBoot 10/10 (the historic ~3 % flake did not reappear), plus the other three — 25/25, no flakes. CI: 102/102 passing.

Not APPROVE for one reason only, and it is not mine to clear: peterbarker's CHANGES_REQUESTED of 2026-09-16 is still the formal review decision. I re-checked all twelve of his inline points at this head and every one has matching code, so what this needs is his re-look rather than another AI round.

Admitted gaps: all timing evidence is SITL, hardware timing unverified; the MOUNT_POI_LOCK ordering note above is reasoned, not measured; I did not re-run the broader mount/camera suites beyond the five boot tests.

[Hwurzburg]

Pushed 19aaeae895. Both notes taken.

The comment is down to three lines, essentially your wording, and it is folded into the AP_Mount commit rather than added on top, so the stack is still the same four commits:

    // MOUNT_YAW_LOCK is left out: set_yaw_lock() captures a heading from the
    // gimbal attitude and AHRS yaw, neither ready this early, and only
    // recaptures while unlocked

Nothing else moved — the tree differs from eca99fad85 only in those lines.

On the +4 B CubeOrange RAM figure: you are right to doubt it, and I am not going to defend it. It is size's .bss column, and with text+data+bss bit-identical between the two trees that is a remainder, not a measurement of RAM. Treat it as unsupported. The flash numbers stand as measured and match yours: MatekF405 +200 B, CubeOrange +348 B.

Thanks also for running down the MOUNT_POI_LOCK question and refuting it — that saved me the same investigation.

[AP-Review]

Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting.

Full report: https://uav.tridgell.net/DevCallReviews/2026_09_22/devcall_pr_reviews.html#pr34413

Reviewed at head 19aaeae895. Verdict: COMMENT — no blockers. The mechanism is sound and the details are unusually well checked; one substantive finding, and two process items.

ISSUE — no "already applied" guard, so the reverse ordering causes a genuine double application

libraries/RC_Channel/RC_Channels.cpp:277

apply_aux_switch_positions() checks that the channel exists, that the function is not in the first-radio-read list, that the switch reads, and that the dispatch took — but never that read_aux() has already established this position.

Both paths are gated on has_valid_input(), but they are separate scheduler tasks and the RC read has the higher priority in Copter: read_aux_all is priority 18 at 10 Hz (ArduCopter/Copter.cpp:161) against AP_Mount::update at 108 (Copter.cpp:210) and AP_Camera::update at 111 (Copter.cpp:213). If the 50 Hz update is skipped across the 200 ms SWITCH_DEBOUNCE_TIME_MS window — and AP_Scheduler gives no deadline guarantee for low-priority tasks — read_aux() applies first and the one-shot then applies a second time, plus a duplicate RCn: ... HIGH announcement.

Your own PR body is the argument for why that matters: set_poi_lock() re-saves saved_mount_mode, a Siyi gimbal sends a second autofocus packet, and AP_RunCam::osd_option() latches a menu action. Three lines close it, after the read_3pos_switch() check:

// read_aux() already established this position; nothing owed
if (c->switch_state.current_position >= 0) {
    continue;
}

current_position starts at −1 (RC_Channel.h:574), so the test is well defined.

Being honest about the limits of this one: the starvation was modelled, not reproduced on firmware — a reduced scheduler model produced RC dispatch at 300 ms followed by INIT dispatch at 322.5 ms, which shows the ordering is permitted, not that it happens in practice. And current_position is recorded by read_aux() before dispatch and regardless of whether the dispatch succeeded, so the guard really means "RC has taken ownership of this channel" rather than strictly "the function ran". Plane is safe by accident: there read_aux_all is priority 147, below mount update at 105.

NOTE — the INIT application bypasses debounce, and can now fire in flight

RC_Channels.cpp:273-279 acts on a single raw radio_in sample where read_aux() requires 200 ms of a stable position. On master this path only ever ran at boot; because the one-shot is now held until has_valid_input(), it can fire mid-flight — a vehicle armed over GCS with no RC whose pilot later powers the transmitter, for instance.

For a stable input the eventual command is the same, but for a transient it is not: HIGH at 0 ms then LOW at 100/200/300 ms dispatches only LOW on the master path, while the new helper dispatches HIGH immediately. Worth a sentence in the PR body, or gating the one-shot on !hal.util->get_soft_armed().

NOTE — two latched mount/camera aux functions still lose their boot position

CAMERA_IMAGE_TRACKING (174) and MOUNT_LRF_ENABLE (177) are latched enable/disable state (libraries/RC_Channel/RC_Channel.cpp:1795 and :1865) but sit in the "do not need to be initialised" group at RC_Channel.cpp:779 and :782, so neither master nor this PR applies them at boot. Same defect class as this PR's premise — pre-existing, not a blocker, mentioned only so it is not assumed fixed by this change.

Process, not code

• RC_Channel: defer aux initialization until backends exist #34166 is still open although this PR's body says it replaces it.
• @peterbarker's CHANGES_REQUESTED from 2026-09-16 is stale. Every point in it is addressed at this head: rc() instead of RC_Channels::get_singleton(), the #endif labels, the trimmed comments, the one-line autotest docstrings, and his headline concern about bypassing RC_Channel::init_position_on_first_radio_read — which is now an explicit skip with a comment at RC_Channels.cpp:264-269. That review needs dismissing or re-requesting for this to move.

Checked and clean

The aux-function lists in AP_Mount.cpp:228-235 and AP_Camera.cpp:797-807 are exactly master's explicit-init set for these two libraries minus MOUNT_YAW_LOCK — enumerated on both sides and diffed, guards included (AP_MOUNT_POI_LOCK_ENABLED, AP_CAMERA_RUNCAM_ENABLED). The MOUNT_YAW_LOCK exclusion is documented and pinned by Copter.MountAuxFunctionAtBootRCFailsafe. The uninitialised bool _aux_functions_initialised is correct — these are members of statically-allocated vehicle objects. The unchecked rc() dereference — the bug class that sank #34166 via Tools/Replay — is safe here: only Copter, Plane, Rover and Sub instantiate AP_Camera/AP_Mount and all four have an RC_Channels (Blimp includes the header but has no member or scheduler entry; AntennaTracker, Replay and AP_Periph instantiate neither). The current_position/debounce_position bookkeeping stops read_aux() re-running an unmoved switch while still firing on a genuine move, and switch_state.initialised is correctly left alone. Feature guards compile out cleanly. git diff --check clean, 0 added lines with trailing whitespace, four commits one per subsystem. All 102 CI checks pass.

The design itself is not re-opened here: this is the shape @tridge asked for in #34166, and @peterbarker's separate caveat that has_valid_input() is an aggregate and does not promise any individual channel is fresh is unresolved by design — he said so himself, and you recorded it at AP_Mount.cpp:246-247.

Reviewed by Claude and independently cross-checked by a cold Codex pass shown no findings, which returned APPROVE. Codex confirmed the scheduler numbers and the mechanism of the race, and adjusted the strength of the claim — reflected above.