Repository navigation
Copter: do not treat a stale baro sample as evidence of not falling - #34033
peterbarker wants to merge 2 commits into
Conversation
|
#33823 (fixing and running the ancient Parachute test) exposed this problem. |
|
Automated review note - AI-generated (Claude), independently cross-checked by a second model and re-verified against the diff. Please sanity-check before acting. Reviewed at head Verdict: APPROVE - no blockers.
Full report, including what was checked and found clean: https://uav.tridgell.net/DevCallReviews/DevCallTopic/devcall_pr_reviews.html |
| } else if (baro_alt_m >= baro_alt_start_m) { | ||
| // exit if baro altitude change indicates we are climbing. Note | ||
| // that >= ("not falling") must not be used here | ||
| } else if (baro_alt_m > baro_alt_start_m) { |
There was a problem hiding this comment.
Let's change this to use the EKF altitude
|
@rmackay9 I think we should merge this as the simple no-side-effects bug fix, then separately address using AHRS/GPS/baro alt in another PR? Maybe we want at least one alt source to be falling? |
|
I think if we're going to go to the trouble of fixing this then we should use the EKF. That will mean that we can leave the check as it was (e.g. ">=") |
You might have deployed the parachute because of an EKF failsafe. I agree that we should be using the EKF here - you might not even have a baro on board. But what you're asking for is massive scope creep and would require vast amounts of testing and consideration of random failure paths. This is safety critical code and we need to be careful! |
|
Hi @peterbarker, Re this having happened as part of an EKF failsafe, that's possible but it's intended to be because of a loss of control (e.g. motor or ESC failure). I'm not sure that changing to use the EKF is any more dangerous than changing the ">=" to ">". Both open us up to "vast amounts of testing and consideration". If we're really keen to stick with using the baro then I think the next best solution is to also save the baro update time so that we don't consume the same baro update twice. We ensure that when we do the >= test we're using a new baro reading. |
peterbarker
moved this from ReadyForDevCall
to In Progress
in Peter's ArduPilot 4.8 Queue
8c0f20d to
77b224c
Compare
parachute_check() runs as a FAST_TASK, every main loop, but the
baro_alt_m it reads is only refreshed by update_altitude(), a 10Hz
scheduled task. The descent guard latched baro_alt_start_m from a
sample and then, on the very next loop, compared it against a
bit-identical copy of that same sample; >= was therefore true and the
loss-of-control counter was reset before it could ever reach the
second's worth of loops it needs to deploy.
Only a reading above the latched one is evidence that the vehicle is
not falling. An equal reading carries no new information, and with a
fresh, noisy baro exact float equality does not otherwise occur - so
this changes only the stale case.
Found via the Copter Parachute autotest, which failed intermittently
under load. An instrumented build showed the counter bouncing between
0 and 1 for eight consecutive seconds, reporting baro and latched
altitude equal to the metre, on a vehicle demonstrably falling 150m
with a steady 150 degree attitude error - every other condition for
deployment satisfied throughout.
Measured by running 512 copies of that test at --parallel=32, twice,
changing nothing but this comparison:
>= 17 failures in 512 runs
> 0 failures in 512 runs
with no other failures in either set.
77b224c to
d164176
Compare
Global Size Summary (Total Flash delta in bytes)Built from cc56e44 rebased onto afeb9c4
|
Previous review (2026-10-17)Automated review note — AI-generated (Claude+Codex), validated against the live diff. Please sanity-check before acting. Reviewed at head The fix at ArduCopter/crash_check.cpp:319 is correct and closes a real gap. With >=, a repeated 10 Hz baro sample reset the loss-of-control counter. In the review's SITL runs base missed the release in 3 of 8 single-motor failures and all 3 climb-then-failure runs; head released the chute in every one. Two nonblocking suggestions:
|
Previous review (2026-10-18)Automated review note — AI-generated (Claude+Codex), validated against the live diff. Please sanity-check before acting. Reviewed at head Reviewed at e77c26a, which adds the ParachuteStaleBaro autotest on top of the previously reviewed d164176. The crash_check.cpp change is unchanged. The >= to > fix at ArduCopter/crash_check.cpp:319 is still correct, and the new test separates it cleanly: with every baro frozen, the old >= never released the chute in the review's SITL runs, and head released it every time. One blocker, in the new test. ParachuteStaleBaro fails at head after the chute deploys, with "Test leaked parameters the suite could not revert: CHUTE_ENABLED 0.000000->1.000000". This looks like the red copter-tests2b job, though the CI log could not be read to confirm it. CHUTE_ENABLED is set inside the test's own context_push(). RC9 low then changes it to 0 in RAM only, so context_pop() skips the revert and its reboot reloads the stored 1. The set_parameter("CHUTE_ENABLED", 1) after it runs in the outer context and records 1 as the original. _Parachute avoids this by setting CHUTE_ENABLED outside its inner contexts. Moving that set_parameter to just before context_pop() fixes it. With that change the test passed in SITL, including Parachute,ParachuteStaleBaro,MANUAL_CONTROL in suite order, and still fails with >=. Minor, nonblocking: it would still help to say in the PR description that a baro that goes unhealthy also freezes the altitude (AP_Baro only updates altitude for healthy sensors), so a dead baro is now a permanent stale case that allows a release after 1 s of more than 30 deg attitude error. |
Copter.Parachute catches the stale-altitude bug only by luck: its loss-of-control subtest passes with the old ">=" in 12 runs of 13 here, and measuring the real rate took 512 runs to find 17 failures. A test which fails 3% of the time does not defend the fix. Freeze every simulated barometer instead. parachute_check() runs every main loop against an altitude a 10Hz task refreshes, so most loops already see a bit-identical copy of the previous sample; a frozen baro is that staleness made permanent, and the outcome deterministic. With the descent guard treating an unchanged altitude as evidence of not falling the counter can never run and the chute never deploys; with it testing only for a climb, the release happens as it should. The vehicle climbs on a working barometer first - the frozen altitude has to be a plausible one to fall from - and the chute is armed before the motors are failed.
e77c26a to
cc56e44
Compare
|
Automated review note — AI-generated (Claude+Codex), validated against the live diff. Please sanity-check before acting. Reviewed at head Reviewed at cc56e44. Since e77c26a the only change is in the autotest: the CHUTE_ENABLED reset now runs before context_pop(). crash_check.cpp is unchanged. The previous blocker is fixed. With the reset at arducopter.py:6988, ParachuteStaleBaro passed in SITL with no parameter leak, both alone and in suite order with Parachute and MANUAL_CONTROL. CI is green, including copter-tests2b. The >= to > fix at crash_check.cpp:319 is still correct, and the test still separates it: a build with >= restored never deployed the chute. For maintainers only, not a request to the author: a baro that goes unhealthy also freezes the altitude, so a dead baro now permits a release after 1 s of more than 30 deg attitude error. Whether stale data should block a release (the EKF or baro-freshness alternatives) is a design call for maintainers. |
peterbarker
moved this from In Progress
to ReadyForDevCall
in Peter's ArduPilot 4.8 Queue
5 participants
Summary
Removes 3.3% chance of Copter which has lost control/thrust of not deploying parachute due to beat frequency.
Classification & Testing (check all that apply and add your own)
Description
parachute_check() runs as a FAST_TASK, every main loop, but the baro_alt_m it reads is only refreshed by update_altitude(), a 10Hz scheduled task. The descent guard latched baro_alt_start_m from a sample and then, on the very next loop, compared it against a bit-identical copy of that same sample; >= was therefore true and the loss-of-control counter was reset before it could ever reach the second's worth of loops it needs to deploy.
Only a reading above the latched one is evidence that the vehicle is not falling. An equal reading carries no new information, and with a fresh, noisy baro exact float equality does not otherwise occur - so this changes only the stale case.
Found via the Copter Parachute autotest, which failed intermittently under load. An instrumented build showed the counter bouncing between 0 and 1 for eight consecutive seconds, reporting baro and latched altitude equal to the metre, on a vehicle demonstrably falling 150m with a steady 150 degree attitude error - every other condition for deployment satisfied throughout.
Measured by running 512 copies of that test at --parallel=32, twice, changing nothing but this comparison:
with no other failures in either set.