fix(navigator): honor NAV_LAND mission item abort altitude (param1)#27454
Conversation
A NAV_CMD_LAND mission item's param1 (minimum abort altitude above the landing point) was parsed and dropped with a long-standing TODO, so an aborted landing always climbed to the global MIS_LND_ABRT_ALT regardless of the per-item value. Carry param1 in the otherwise-unused time_inside field on upload, and in do_abort_landing() use it as the climb-above-landing-point height when set (> 0), falling back to MIS_LND_ABRT_ALT otherwise. The value also now round-trips on mission download. Items without a specified abort altitude (param1 = 0), including missions stored by older firmware, keep using the parameter default. Fixes PX4#27290 Signed-off-by: Andrii Anoshyn <anoshyn.andrii@gmail.com>
🔎 FLASH Analysispx4_fmu-v5x [Total VM Diff: 104 byte (0.01 %)]px4_fmu-v6x [Total VM Diff: 136 byte (0.01 %)]Updated: 2026-05-30T12:45:40 |
There was a problem hiding this comment.
Pull request overview
Fixes a long-standing TODO so that MAV_CMD_NAV_LAND's param1 (per-item minimum abort altitude above the landing point) is no longer silently dropped. The value is stashed in the unused mission_item_s::time_inside slot on upload, read back in MissionBase::do_abort_landing() (falling back to MIS_LND_ABRT_ALT when zero/unset), and round-tripped on mission download.
Changes:
- Parse
param1fromMAV_CMD_NAV_LANDintomission_item->time_insideand emit it back fromformat_mavlink_mission_item(). - Use
_mission_item.time_inside(when> FLT_EPSILON) as the abort climb-above-landing-point height inMissionBase::do_abort_landing(). - Clarify the
MIS_LND_ABRT_ALTparameter description as the default when LAND param1 is unspecified.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/modules/mavlink/mavlink_mission.cpp | Stores/restores MAV_CMD_NAV_LAND param1 via mission_item.time_inside; removes both abort-alt TODOs. |
| src/modules/navigator/mission_base.cpp | Uses the per-item abort altitude (from time_inside) when computing the abort loiter altitude, falling back to MIS_LND_ABRT_ALT. |
| src/modules/navigator/mission_params.yaml | Documents MIS_LND_ABRT_ALT as the default when the LAND mission item doesn't specify its own abort altitude. |
There was a problem hiding this comment.
@julianoes FWIW the problem here isn't that this doesn't implement this param, it's that we don't support it and we don't fail the upload with MAV_MISSION_INVALID_PARAM1.
But anyhow, would appeciate your thoughts on #27483
EDIT - because it would be nice to also reject a non-NAN param3, or any values of PRECISION_LAND_MODE that we don't support.
|
Yes, we should fail the upload, I agree there. |
|
I’ll keep unsupported-param rejection as a follow-up. This PR now only preserves and uses Rejecting unsupported |
|
Looks good. Any objections before I merge? |
|
Looks great to me (assuming latest version tested). We should update #27541 to note the change so that it doesn't get rejected. |
A
MAV_CMD_NAV_LANDmission item carries aparam1"minimum abort altitude" (the height above the landing point to climb to if the landing is aborted;0= use the system default). PX4 parsed and dropped it — there was a long-standing// TODO: abort alt param1— so an aborted landing always climbed to the globalMIS_LND_ABRT_ALT, ignoring the per-item value.This carries
param1in the otherwise-unusedtime_insidefield on upload (nothing readstime_insidefor a LAND item), and inMissionBase::do_abort_landing()uses it as the climb-above-landing-point height when set (> 0), falling back toMIS_LND_ABRT_ALTotherwise. The value also round-trips on mission download (the reverseformat_mavlink_mission_item()TODO is resolved too).param1 = 0, including missions stored by older firmware (parsed into a zero-initialized item), keep using theMIS_LND_ABRT_ALTdefault — no behavior change for existing missions.time_insideis not consumed for LAND items elsewhere.NAV_CMD_LANDaborts, matching the existing abort behavior;VTOL_LANDis unchanged.MIS_LND_ABRT_ALTparameter docs now note it is the default when the item doesn't specify its own abort altitude.Fixes #27290