Skip to content

AP_Scripting: added applet to check for failed tilt servos - #34789

Open
tridge wants to merge 2 commits into
ArduPilot:masterfrom
tridge:pr-servo-check-tilt
Open

tridge wants to merge 2 commits into
ArduPilot:masterfrom
tridge:pr-servo-check-tilt

Conversation

@tridge

@tridge tridge commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Replacement lua script for this issue here: #34769

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

Description

This is an updated version of a lua script that is used on a production vendor aircraft. It should be useful for other tilt-vectored quadplanes too.
Tested in SITL with the autotest that generates CAN servo feedback using python

@github-actions

github-actions Bot commented Oct 17, 2026 •

Copy link
Copy Markdown

Global Size Summary (Total Flash delta in bytes)

Built from 9c50812 rebased onto dc78bc5

Board Periph tracker blimp bootloader copter heli iofw plane rover sub
DrotekP3Pro _ _ _ * _ _ _ _ _ _
Durandal _ 0 0 _ 0 0 _ 0 0 0
Hitec-Airspeed * _ _ _ _ _ _ _ _ _
KakuteF7 _ * * _ * * _ * * *
MambaH743v4 _ +4 +4 _ +4 +4 _ +4 +4 +4
MatekF405-CAN _ _ _ * _ _ _ _ _ _
MatekF405 _ * * _ * * _ * * *
MatekH743-bdshot _ 0 0 _ 0 0 _ 0 0 0
Pixhawk1-1M _ * * _ * * _ * * *
disco _ +4 +4 _ +4 +4 _ +8 +8 +8
f103-GPS * _ _ _ _ _ _ _ _ _

@AP-Review

AP-Review commented Oct 17, 2026 •

Copy link
Copy Markdown

Deprecated — see below for the updated review.

Previous review (2026-10-17)

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

Reviewed at head d9e797d723.
Full report: https://firmware.ardupilot.org/Tools/APReview/DevCallReviews/PRReviews/ardupilot/ardupilot/34789/1.html#pr34789

Thanks, this is a well-tested applet. Two small things are worth fixing before merge.

  1. The override lasts 40 ms (https://github.com/ardupilot/ardupilot/pull/34789/files#diff-52148f5bfade0d6370ca9290d98d3532417a4be24e00a81194daa974edd35b76R21), but a callback gap is only treated as a reset above 100 ms (https://github.com/ardupilot/ardupilot/pull/34789/files#diff-52148f5bfade0d6370ca9290d98d3532417a4be24e00a81194daa974edd35b76R20). If scripting runs late, with callbacks 40-100 ms apart, the override expires before the next callback stops the motor again (https://github.com/ardupilot/ardupilot/pull/34789/files#diff-52148f5bfade0d6370ca9290d98d3532417a4be24e00a81194daa974edd35b76R241). The motor then pulses between stopped and its commanded output instead of staying stopped. I reproduced this in your test harness for every interval from 40 to 100 ms. Making OVERRIDE_MS at least MAX_GAP_MS, or treating dt > OVERRIDE_MS as a gap, would fix it.

  2. SRV_Channels:get_safety_state() == false (https://github.com/ardupilot/ardupilot/pull/34789/files#diff-52148f5bfade0d6370ca9290d98d3532417a4be24e00a81194daa974edd35b76R264) is never true on Linux boards such as Navigator. The Linux HAL reports SAFETY_NONE, and the binding only returns false for SAFETY_ARMED, so the applet would never detect a failure there and nothing would warn the user. Could you gate on arming only, or note the HAL limitation in ServoFailDetect.md?

The failing cubeorangeplus-quadplane CI job failed while fetching Renode and looks unrelated to this PR.

@tridge
tridge force-pushed the pr-servo-check-tilt branch from d9e797d to 82dc1c7 Compare October 17, 2026 01:58

@IamPete1 IamPete1 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is significantly different to the original version which has had considerable testing. I was expecting a more minimal change to detect the long callback times and prevent triggering on multiple motors.

@tridge
tridge force-pushed the pr-servo-check-tilt branch from 82dc1c7 to a8e3904 Compare October 17, 2026 02:17
@AP-Review

AP-Review commented Oct 17, 2026 •

Copy link
Copy Markdown

Deprecated — see below for the updated review.

Previous review (2026-10-17)

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

Reviewed at head a8e39045b0.
Full report: https://firmware.ardupilot.org/Tools/APReview/DevCallReviews/PRReviews/ardupilot/ardupilot/34789/2.html#pr34789

Thanks for fixing both earlier points. The 120 ms override and the SAFETY_NONE handling both check out at a8e3904.

  1. There is one remaining timing gap. The gap check (https://github.com/ardupilot/ardupilot/pull/34789/files#diff-52148f5bfade0d6370ca9290d98d3532417a4be24e00a81194daa974edd35b76R250) and the in-callback stall check (https://github.com/ardupilot/ardupilot/pull/34789/files#diff-52148f5bfade0d6370ca9290d98d3532417a4be24e00a81194daa974edd35b76R283) each allow up to 100 ms, but the override is only renewed after both (https://github.com/ardupilot/ardupilot/pull/34789/files#diff-52148f5bfade0d6370ca9290d98d3532417a4be24e00a81194daa974edd35b76R241). Callbacks are scheduled from their start time, so a late callback that also stalls internally can renew more than 120 ms after the last one. Examples are a 60 ms gap with a 70 ms stall, or 100 ms with 30 ms. The override then expires, and the failed-side motor briefly returns to its commanded output before being stopped again. I reproduced this in your test harness. Measuring from the last renewal, and treating OVERRIDE_MS or more as a gap, would close it. The note at https://github.com/ardupilot/ardupilot/pull/34789/files#diff-71379eb4e927f01e9ac492990c43a536a7314085e5b77716a61fce42859ef8e7R122 could then stay as written.

  2. IamPete1 asked for a more minimal change from the flight-tested script. Since the original isn't in the tree, a short reply would help reviewers. That could be the delta from the flown version, or why the rewrite was needed and what flight or SITL testing it has had.

Comment thread libraries/AP_Scripting/applets/ServoFailDetect.md
Comment thread Tools/autotest/quadplane.py Outdated
@tridge
tridge force-pushed the pr-servo-check-tilt branch from a8e3904 to caf1ce4 Compare October 17, 2026 04:58
@tridge

tridge commented Oct 17, 2026

Copy link
Copy Markdown
Contributor Author

This is significantly different to the original version which has had considerable testing. I was expecting a more minimal change to detect the long callback times and prevent triggering on multiple motors.

I've reworked it to be much closer to the original, with the following changes:

  • fixed the dual disable bug
  • fixed the slow loop bug
  • moved the table to the top to be a bit easier to integrate into another aircraft (easier to find config)
  • added Q_TILT_MASK checking of the table at startup

@tridge
tridge force-pushed the pr-servo-check-tilt branch from caf1ce4 to 6fa60f4 Compare October 17, 2026 05:17
@AP-Review

AP-Review commented Oct 17, 2026 •

Copy link
Copy Markdown

Deprecated — see below for the updated review.

Previous review (2026-10-17)

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

Reviewed at head 6fa60f4920.
Full report: https://firmware.ardupilot.org/Tools/APReview/DevCallReviews/PRReviews/ardupilot/ardupilot/34789/3.html#pr34789

Thanks, the rework closes the earlier timing gap. The renewal-age check (https://github.com/ardupilot/ardupilot/pull/34789/files#diff-52148f5bfade0d6370ca9290d98d3532417a4be24e00a81194daa974edd35b76R482) holds across a full gap/stall sweep.

  1. A servo already confirmed stuck that then loses telemetry gets its motor restarted for about 2 s. disconnected() resets the state to inactive (https://github.com/ardupilot/ardupilot/pull/34789/files#diff-52148f5bfade0d6370ca9290d98d3532417a4be24e00a81194daa974edd35b76R157), so check_failsafe() sees no failure and releases the shutdown until the servo is marked disconnected (https://github.com/ardupilot/ardupilot/pull/34789/files#diff-52148f5bfade0d6370ca9290d98d3532417a4be24e00a81194daa974edd35b76R158). In the test harness the motor is released 1020 ms after telemetry loss and stopped again at 3040 ms, while the other servo stays healthy. A NaN position releases after 20 ms. This was already present before the rework. Keeping a confirmed stuck or disconnected state through telemetry loss, with a regression case, would fix it. The release list in ServoFailDetect.md (https://github.com/ardupilot/ardupilot/pull/34789/files#diff-71379eb4e927f01e9ac492990c43a536a7314085e5b77716a61fce42859ef8e7R126) would then stay accurate.

  2. peterbarker's two comments still need a reply. The hook cleanup at https://github.com/ardupilot/ardupilot/pull/34789/files#diff-f8a09249c37bc1b530e461d3f60f0c230f9edf735acbf501eb32752bf0e44675R3964 already works, so the install_message_hook_context change is just tidier. On "why CAN": Volz serial servos republish a cached angle for up to 5 s, so keeping the CAN-specific wording at https://github.com/ardupilot/ardupilot/pull/34789/files#diff-71379eb4e927f01e9ac492990c43a536a7314085e5b77716a61fce42859ef8e7R25 seems reasonable if you explain that.

@tridge
tridge requested a review from IamPete1 October 17, 2026 09:12
@tridge
tridge force-pushed the pr-servo-check-tilt branch from 6fa60f4 to c8c8c3b Compare October 18, 2026 04:18
@tridge
tridge requested a review from peterbarker October 18, 2026 04:19
@AP-Review

AP-Review commented Oct 18, 2026 •

Copy link
Copy Markdown

Deprecated — see below for the updated review.

Previous review (2026-10-18)

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

Reviewed at head c8c8c3b05a.
Full report: https://firmware.ardupilot.org/Tools/APReview/DevCallReviews/PRReviews/ardupilot/ardupilot/34789/4.html#pr34789

Thanks, this addresses most of the last round. Confirmed stuck servos now stay failed through telemetry loss (https://github.com/ardupilot/ardupilot/pull/34789/files#diff-52148f5bfade0d6370ca9290d98d3532417a4be24e00a81194daa974edd35b76R147), the hook cleanup and Volz note answer peterbarker's comments, and the renewal-age guard still holds.

  1. One case is left from the earlier telemetry-loss point: when telemetry comes back, the reconnect branch resets the servo to inactive whether or not it had already failed (https://github.com/ardupilot/ardupilot/pull/34789/files#diff-52148f5bfade0d6370ca9290d98d3532417a4be24e00a81194daa974edd35b76R201). So a servo that is still stuck has its motor released 20 ms after telemetry returns, and the motor runs about 4 s until the error is re-detected. One NaN position sample is enough, while the other servo stays healthy. This matches the documented 'telemetry recovery restarts settling' (https://github.com/ardupilot/ardupilot/pull/34789/files#diff-71379eb4e927f01e9ac492990c43a536a7314085e5b77716a61fce42859ef8e7R108), so it may be what you intend. If not, keeping a stuck or disconnected state on reconnect, and letting the position check at line 255 clear it, fixes it in the harness. With that change only the state assertion at https://github.com/ardupilot/ardupilot/pull/34789/files#diff-e2a5fde39f3d2724a74ee86bbf1eb8d9e5785af5fb2eeadca47187e18ce1357cR262 needs updating (it becomes 1 instead of 0), and the override is still released there.

@Hwurzburg Hwurzburg added the WikiNeeded needs wiki update label Oct 18, 2026
Monitor two tilt servos with configurable output channels and feedback
calibration. Use elapsed time and restart detection after scheduling gaps
so Lua starvation cannot turn healthy tilt movement into a motor shutdown.

Require a confirmed fault and a healthy opposite servo, with expiring
overrides and an interlock to prevent overlapping motor shutdowns. Include
setup documentation, the telemetry freshness limitation, and regression
fixtures for ArduPilot Lua and CAN SITL tests.
Run 17 deterministic regression cases in ArduPilot Lua with native uint32_t
timestamps, and exercise the real telemetry and motor-output bindings
through DroneCAN in an armed, stationary QuadPlane.

Check individual and simultaneous faults, telemetry loss and recovery,
override expiry during callback gaps, and resumption without a shutdown
pulse. Register both tests in the Plane CI suite.
@tridge
tridge force-pushed the pr-servo-check-tilt branch from c8c8c3b to 9c50812 Compare October 19, 2026 19:41
@AP-Review

Copy link
Copy Markdown

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

Reviewed at head 9c508129fd.
Full report: https://firmware.ardupilot.org/Tools/APReview/DevCallReviews/PRReviews/ardupilot/ardupilot/34789/5.html#pr34789

Thanks, the reconnect change fixes the last point. A stuck or disconnected servo now stays failed when telemetry returns (https://github.com/ardupilot/ardupilot/pull/34789/files#diff-52148f5bfade0d6370ca9290d98d3532417a4be24e00a81194daa974edd35b76R201), and the new subcases cover it. I re-ran the harness and the earlier reconnect probes at this head, and the previous head fails them as expected. SITL ServoFailDetect tests pass.

One note, no change needed. If both servos lose telemetry for about 1 to 3 s while one is stuck, the healthy one restarts settling on return. So the stuck side's motor is not stopped for about 2 s after telemetry comes back. That matches the documented settling rule (https://github.com/ardupilot/ardupilot/pull/34789/files#diff-71379eb4e927f01e9ac492990c43a536a7314085e5b77716a61fce42859ef8e7R121) and is half the previous window. CI is still running.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants