Skip to content

Add Tracker tracking test - #17783

Open
peterbarker wants to merge 6 commits into
ArduPilot:masterfrom
peterbarker:pr/tracker-tracking-test
Open

peterbarker wants to merge 6 commits into
ArduPilot:masterfrom
peterbarker:pr/tracker-tracking-test

Conversation

@peterbarker

@peterbarker peterbarker commented Jun 16, 2021 •

Copy link
Copy Markdown
Contributor

Summary

Adds an autotest checking that Tracker in AUTO mode points at a vehicle sending GLOBAL_POSITION_INT and holds there.

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

SITL:

  • test.Tracker: all 24 tests pass, including the new Tracking
  • test.Copter.ModeFlip and test.Plane.ThrottleFailsafe pass with the wait_attitude change; they are among its existing callers
  • flake8 and Tools/scripts/check_branch_conventions.py are clean

In Tracking, each vehicle position starts at least 45 degrees from where the tracker is pointing, the first 100 degrees away, so none of the cases is satisfied by the tracker's starting attitude.

Description

  • autotest: allow wait_attitude to wait for yaw and for a held attitude adds desyaw, compared using heading_delta so it wraps, and an optional minimum_duration the attitude must be held for. Both default to off, so existing callers are unchanged. A minimum_duration that is not less than timeout is rejected, as the wait could never succeed.
  • autotest: add test for tracker pointing at a vehicle adds Tracking. It feeds the tracker a vehicle position at 10Hz from a message hook (Tracker drops a target after 5 seconds without an update), places the vehicle at five bearings to the west, and requires the tracker to hold within 2 degrees for 5 seconds at each. The vehicle reports the tracker's own pressure, so the tracker should hold zero pitch throughout.

This is built on #34733: the first three commits are that PR. Without its fixes the simulated tracker slews while its servos are undriven, the startup gyro calibration absorbs the slew, and the EKF never becomes ready for the test to start. I'll rebase once #34733 is merged.

This PR was first opened in 2021 and has been reworked. The PID logging it used to carry is now in #17778, and its changes to the simulated tracker's yaw range and its default parameter file have been dropped.

This contribution was AI-assisted: the rework of the original branch and the new test code were written with Claude Code and reviewed by me.

@peterbarker
peterbarker force-pushed the pr/tracker-tracking-test branch from 71c420a to 1acc044 Compare December 27, 2021 04:27
@peterbarker
peterbarker marked this pull request as ready for review December 27, 2021 04:28
@peterbarker
peterbarker requested a review from IamPete1 December 27, 2021 04:28
@peterbarker
peterbarker force-pushed the pr/tracker-tracking-test branch 3 times, most recently from 9bbac19 to 614bf73 Compare December 27, 2021 21:44
The limits tested yaw_rate and pitch_rate before update_onoff_servos() or
update_position_servos() had assigned them, so they only ever saw the zeroes
the two were initialised to and never fired.  The simulated tracker could
slew past its own yaw_range and pitch_range without limit.

Move the block below the servo update, which is what sets the rates.
A PWM of zero means the channel is not being output to at all, and that is
what the tracker emits whenever its servos are not being driven.  Feeding it
through (pwm-1500)/500 turns it into a demand of -3.0, which with yaw_range
170 and pitch_range 45 asks for -510 degrees of yaw and -135 of pitch, so the
model slews at its rate limit for as long as nothing drives the servos.

A real servo with no signal holds position, and the rest of SITL already
treats a zero PWM that way - see Aircraft::filtered_servo_angle().  Do the
same here: take no new demand from an undriven channel and leave its rate at
zero.

The slew this removes was not harmless.  The tracker's simulated yaw swept at
its 9 deg/sec limit from the first physics step, and because a yaw about the
gravity vector does not disturb the accelerometers, the startup gyro
calibration's only movement check could not see it: it converged and stored
INS_GYROFFS_Z of -0.157 rad/sec, exactly the slew rate.  The EKF was then fed
a gyro denying a rotation its compass could see, drove the Z gyro bias state
out to 8.8 deg/sec to reconcile them, and so never satisfied
delAngBiasLearned - leaving readyToUseGPS() false and the filter unable to
begin absolute aiding.
@github-actions

github-actions Bot commented Oct 18, 2026 •

Copy link
Copy Markdown

Global Size Summary (Total Flash delta in bytes)

Built from dd54cb3 rebased onto dc78bc5

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

@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 c7d42950c4.
Full report: https://firmware.ardupilot.org/Tools/APReview/DevCallReviews/PRReviews/ardupilot/ardupilot/17783/1.html#pr17783

Thanks, this looks good, and nothing blocks. I rebuilt SITL at c7d4295 and Tracking passes. Earlier runs also showed it passing in suite order and at speedup 5. It fails when the firmware's bearing or baro altitude is deliberately offset, and when the position hook is removed, so it does catch real pointing errors.

One small comment fix: Tools/autotest/antennatracker.py:253 (https://github.com/ArduPilot/ardupilot/pull/17783/files#diff-d7d46e38a9dc3898d10c1828f0c345cab42344ad52194eca21eb2f12fc1beeefR253) says the simulated tracker "starts pointing west", and the docstring at line 146 (https://github.com/ArduPilot/ardupilot/pull/17783/files#diff-d7d46e38a9dc3898d10c1828f0c345cab42344ad52194eca21eb2f12fc1beeefR146) says it starts at 270 degrees yaw and 10 degrees pitch. It actually boots at the SITL home heading (about 9 degrees) with level pitch; 270/10 is the centre of its +/-170 yaw and +/-45 pitch range. The test geometry is still correct. Something like "the simulated tracker's yaw range is centred on west and spans 170 degrees either way" would be accurate. The older SIM_Tracker.h comment has the same slip.

For information: this carries #34733's three commits unchanged and Tracking needs them. It fails arming checks with the old simulator model. A note in the description that it builds on #34733 might help whoever merges.

@peterbarker
peterbarker force-pushed the pr/tracker-tracking-test branch from c7d4295 to 55a6f23 Compare October 19, 2026 16:30
@peterbarker peterbarker moved this from ReadyForDevCall to In Progress in Peter's ArduPilot 4.8 Queue Oct 19, 2026
UndrivenServosHoldPosition centres the tracker, disarms so that both
servo outputs read zero, and checks the simulated attitude does not move.
Without the undriven-channel fix in SIM_Tracker the model yaws more than
100 degrees in the 12 seconds checked.

ServoRangeLimits drives both servos to 2200 with DO_SET_SERVO, asking
for 1.4 times the model's yaw and pitch range, and checks it stops and
holds at +170 yaw and +45 pitch.  Without the limits being applied after
the servo update the model never settles there.
zero_yaw and zero_pitch are the centre of the tracker's yaw and pitch
ranges, which a centred position servo points it at.  The tracker does
not start there: it boots at the SITL home heading with level pitch.
desyaw is compared using heading_delta so that it wraps correctly.

minimum_duration requires the attitude to be held for that many seconds
rather than returning on the first message within tolerance.  It
defaults to None, so existing callers keep their current behaviour, and
must be less than timeout or the wait could never succeed.
Feed AUTO mode a vehicle position at a series of bearings from the
tracker and check it points at each one and holds there.

The vehicle reports the tracker's own pressure so the tracker should
hold zero pitch throughout.
@peterbarker
peterbarker force-pushed the pr/tracker-tracking-test branch from 55a6f23 to dd54cb3 Compare October 19, 2026 16:32
@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 dd54cb37aa.
Full report: https://firmware.ardupilot.org/Tools/APReview/DevCallReviews/PRReviews/ardupilot/ardupilot/17783/2.html#pr17783

Thanks for the fixes. The head moved from c7d4295 to dd54cb3, and the change is comment-only. The docstring at Tools/autotest/antennatracker.py:146 (https://github.com/ArduPilot/ardupilot/pull/17783/files#diff-d7d46e38a9dc3898d10c1828f0c345cab42344ad52194eca21eb2f12fc1beeefR146), the Tracking comment at Tools/autotest/antennatracker.py:254 (https://github.com/ArduPilot/ardupilot/pull/17783/files#diff-d7d46e38a9dc3898d10c1828f0c345cab42344ad52194eca21eb2f12fc1beeefR254) and libraries/SITL/SIM_Tracker.h:46 (https://github.com/ArduPilot/ardupilot/pull/17783/files#diff-4572d7ac789c0553dda5d05f738a191864606beb25f1218981567f6066976a1dR46) now all correctly describe 270/10 as the centre of the range. I rebuilt SITL at dd54cb3: the two servo tests and Tracking pass, and Tracking also passes at speedup 5. The full Tracker suite passed in an earlier pass at this head. Nothing blocks.

Two small things in the description, for information only. #34733 now has four commits, not three, and this PR carries all four. test.Tracker has 23 runnable tests rather than 24; all of them pass. The extra 'PASSED' in the log is GPSForYaw's result echoed in a later message. The first one goes away when you rebase after #34733 merges.

CI for this head was still running when checked, with no failures so far.

@peterbarker peterbarker moved this from In Progress to ReadyForDevCall in Peter's ArduPilot 4.8 Queue Oct 19, 2026

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

LGTM

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

Projects

Status: ReadyForDevCall

Development

Successfully merging this pull request may close these issues.

4 participants