Skip to content

fix(mavlink): execute CONDITION_DELAY mission items as NAV_DELAY#27453

Merged
hamishwillee merged 1 commit into
PX4:mainfrom
anoshyn:fix/mission-condition-delay
Jun 3, 2026
Merged

fix(mavlink): execute CONDITION_DELAY mission items as NAV_DELAY#27453
hamishwillee merged 1 commit into
PX4:mainfrom
anoshyn:fix/mission-condition-delay

Conversation

@anoshyn

@anoshyn anoshyn commented May 24, 2026

Copy link
Copy Markdown
Contributor

A mission item with MAV_CMD_CONDITION_DELAY is accepted during upload but parsed into a nav_cmd value (112) that no navigator code recognizes. As a result the mission feasibility check rejects the mission ("unsupported cmd: 112"), and even if it reached execution the requested hold would never run — the delay duration is effectively lost.

PX4 already implements exactly this behavior through NAV_CMD_DELAY, which holds at the current position for param1 seconds (stored in params[0], which aliases time_inside). Since MAV_CMD_CONDITION_DELAY's param1 carries the same delay-in-seconds, this maps the command to NAV_CMD_DELAY at parse time, so the hold is honored by the existing, tested delay execution and feasibility logic without duplicating it.

Behavior is identical to a normal NAV_DELAY item: the vehicle holds position for the requested duration, then continues. Like NAV_DELAY, the item is uploaded in the MAV_FRAME_MISSION frame.

Fixes #27289

MAV_CMD_CONDITION_DELAY was accepted on upload but mapped to a nav_cmd
(112) that no navigator code recognizes, so the mission feasibility check
rejected it ("unsupported cmd") and its delay was never executed. PX4
already implements an identical seconds-hold via NAV_CMD_DELAY, whose
duration comes from param1 (params[0], aliasing time_inside). Map
CONDITION_DELAY to NAV_CMD_DELAY at parse time so the requested hold is
honored, reusing the existing, tested delay execution and feasibility
logic.

Fixes PX4#27289

Signed-off-by: Andrii Anoshyn <anoshyn.andrii@gmail.com>
@github-actions github-actions Bot added kind:bug Something is broken or behaving incorrectly. scope:mavlink MAVLink module, streams, commands, or protocol handling. labels May 24, 2026
@dakejahl
dakejahl requested a review from julianoes May 26, 2026 22:45
@julianoes
julianoes requested a review from hamishwillee May 28, 2026 00:28

@julianoes julianoes left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we rename NAV_CMD_DELAY to MAV_CMD_CONDITION_DELAY?

@hamishwillee

hamishwillee commented May 28, 2026

Copy link
Copy Markdown
Contributor

Should we rename NAV_CMD_DELAY to MAV_CMD_CONDITION_DELAY?

No.

There are two messages (MAV_CMD_NAV_DELAY and MAV_CMD_CONDITION_DELAY). The difference being that MAV_CMD_NAV_DELAY allows you to specify time to complete the command in param2,3,4.
Renaming will make this wrong in one case or the other.

This is the easiest fix in that it should work. MAV_CMD_NAV_DELAY appears to only implement param1, so it is functionally identical.

I do not love it because it promotes poor "future" MAVLink hygiene - if someone populates param2,3,4,5 on the MAV_CMD_CONDITION_DELAY these could in theory be unintentionally handled in the same way if that message is extended.

The proper MAVLink thing to do would be for MAV_CMD_NAV_DELAY to reject upload for any non-NaN value of params 2,3,4,5 (since they are unsupported) with MAV_MISSION_INVALID_PARAMn.
Then implement MAV_CMD_CONDITION_DELAY separately, rejecting incorrect params for everything other than param1.
See #27483 for more ifnormation

Given that we widely do not properly support rejecting unsupported params, this is perhaps overkill.
I'd still like it, but leave to Julian to decide if we deal with it here. Minimally it might make sense to have a second handling path.

@julianoes

Copy link
Copy Markdown
Contributor

Oh, sorry, I hadn't seen the mavlink command equivalent.

@hamishwillee

Copy link
Copy Markdown
Contributor

Oh, sorry, I hadn't seen the mavlink command equivalent.

@julianoes It was news to me too that there was an alternative MAV_CMD too - I had kind of assumed it wasn't implemented anywhere ...

Note that I left it to you to decide whether this quick fix is reasonable or we should ask for separate handling of this command.

@anoshyn

anoshyn commented May 30, 2026

Copy link
Copy Markdown
Contributor Author

I’ll keep this PR focused on making MAV_CMD_CONDITION_DELAY execute correctly by mapping it to the existing delay behavior.

I agree the stricter MAVLink handling would be to validate unsupported params separately for MAV_CMD_NAV_DELAY and MAV_CMD_CONDITION_DELAY, but that seems broader and fits better under #27483. If maintainers prefer, I can still split this into a separate CONDITION_DELAY case here without adding full unsupported-param validation.

@hamishwillee hamishwillee left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I’ll keep this PR focused on making MAV_CMD_CONDITION_DELAY execute correctly by mapping it to the existing delay behavior.

Thanks @anoshyn. Sounds good to me.

#27541 is going to do param checking, so provided we update that to check each of these commands separately it should be much harder for later updates to this to break for extensions to later break MAV_CMD_CONDITION_DELAY handling silently.

I'm going to merge as @dakejahl has approved it and @julianoes seen it too.

@hamishwillee
hamishwillee merged commit d215338 into PX4:main Jun 3, 2026
72 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind:bug Something is broken or behaving incorrectly. scope:mavlink MAVLink module, streams, commands, or protocol handling.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] MAV_CMD_CONDITION_DELAY mission items are accepted but the delay duration parameter is ignored

4 participants