Skip to content

Add support for MAV_CMD_DO_ORBIT - #1702

Open
peterbarker wants to merge 1 commit into
ArduPilot:masterfrom
peterbarker:pr/MAV_CMD_DO_ORBIT
Open

Add support for MAV_CMD_DO_ORBIT#1702
peterbarker wants to merge 1 commit into
ArduPilot:masterfrom
peterbarker:pr/MAV_CMD_DO_ORBIT

Conversation

@peterbarker

Copy link
Copy Markdown
Contributor

@peterbarker
peterbarker force-pushed the pr/MAV_CMD_DO_ORBIT branch 2 times, most recently from 52b2a46 to 0b24deb Compare July 13, 2026 00:31
@peterbarker
peterbarker force-pushed the pr/MAV_CMD_DO_ORBIT branch from 0b24deb to ed5714d Compare July 27, 2026 10:01
@tridge
tridge force-pushed the pr/MAV_CMD_DO_ORBIT branch from ed5714d to 145e62a Compare August 30, 2026 00:05
@tridge

tridge commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

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_01_AIReview/devcall_pr_reviews.html#prMAVProxy-1702

Reviewed at head 145e62aa3e. COMMENT — no blockers.

The parameter mapping is correct, which is the thing most worth getting right here and the easiest to get wrong. Checked field by field against the fetched common.xml: p1 radius in metres with "Positive: orbit clockwise. Negative: orbit counter-clockwise." passed through signed and documented that way in your help text; p2 velocity m/s with NaN meaning vehicle default; p3 mapped to the ORBIT_YAW_BEHAVIOUR enum values; p4 declared units="rad" and correctly converted from circuits as orbits * 2 * pi (verified: 3 circuits → 18.8495559 = 6π); p5/p6 scaled to degE7 in a COMMAND_INT. COMMAND_INT is right given the lat/lon, and the non-_INT frame constants are correct — the spec marks the _INT variants superseded since 2024-03.

Two things worth fixing:

  1. velocity= and yaw= raise an uncaught ValueError from a GUI-reachable path. Both are declared type=str in orbit_arg_spec, so the arg parser's try/except ValueError never sees them and the later float(...) raises. Reachable by simply clearing the free-text "Velocity (m/s)" box in the Orbit Here dialog; the user gets ERROR in command [...] : could not convert string to float: '' with no usage. (mavproxy_mode.py:248, :258)
  2. No mode change is requested and the NACK isn't surfaced. cmd_guided sends DO_REPOSITION with MAV_DO_REPOSITION_FLAGS_CHANGE_MODE; cmd_orbit has no equivalent and nothing reads COMMAND_ACK, so from Loiter it just fails with a generic console line a map-menu user is unlikely to be watching.

That second point matters more than usual because no released ArduPilot firmware implements this command yet — a search of a full checkout finds DO_ORBIT only in the bundled mavlink XML, zero source files. The only implementation is the still-open ArduPilot/ardupilot#32434 (Copter-only, Guided-only, size-gated), so today every vehicle NACKs this. Worth cross-linking the two PRs in the description.

Smaller: negative orbits emits a negative p4, below the spec's minValue="0" (ArduPilot's handler fabsf()s it, but another stack may not); yaw=Default sends NaN for p3, which the spec doesn't annotate as NaN-capable — ORBIT_YAW_BEHAVIOUR_UNCHANGED is its actual "leave it alone" value; and frame= permanently rewrites the shared flytoframe setting, which the Orbit Here popup always emits, so every map orbit silently reassigns the frame Fly To will use next.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants