MAVLink: allow mission upload and clear in flight when the mission is not being executed - #11851
Open
raphaelhunziker1202-stack wants to merge 1 commit into
Conversation
… not being executed Aligns the MAVLink mission path with the MSP policy introduced in iNavFlight#10273, where setWaypoint() accepts mission uploads while armed as long as the WP mission is not actively being flown. The MAVLink handlers denied every mission transfer and clear outright while armed. Changes: - New mavlinkMissionEditBlocked() gate used by MISSION_COUNT, MISSION_CLEAR_ALL, MISSION_ITEM and MISSION_ITEM_INT: edits are refused while armed AND (WP mode active OR the mission's own RTH leg is running OR the on-the-fly mission planner is active). The RTH-leg term protects the land/loiter decision at home, which reads the live list; the planner term prevents two writers on the same list. The ARMED term keeps the disarmed path provably unchanged. - When the refused sender owns the receiving transfer, the transfer is aborted via mavlinkAbortMissionUpload(MAV_MISSION_DENIED) so the retry engine stops soliciting items from a partner that was just denied. - Guided fly-to-here / altitude-target items are now dispatched on the item itself (NAV_WAYPOINT with current == 2 or 3) instead of on transfer state, so a guided click can never be absorbed into a running upload. - mavlinkCommitMissionUpload() / mavlinkClearPersistedMission(): while armed the commit or clear applies to RAM only and skips persistence - saveNonVolatileWaypointList() refuses to run while armed, and a flash write mid-flight would stall the main loop. The uploaded mission survives disarm but not a reboot; persisting after landing remains the GCS's responsibility. An in-flight upload also collapses a loaded multi-mission set to the uploaded mission for the rest of the session. - mavlinkResolveUploadedMissionJumps(): armed commits now enforce the same JUMP rules as the arm-time validation they bypass (no JUMP as first item, no self/adjacent targets, sane repeat count, geo-referenced target). Ground uploads are left to the arm-time check. - navigation.c: setWaypoint()'s post-upload clamp of activeWaypointIndex uses >= instead of > (the index is 0-based, so index == waypointCount is already out of range); new public isWpMissionPlannerActive() accessor. - Unit tests: the old MissionCountWhileArmedIsRejected asserts the new policy as MissionCountWhileArmedStartsTransfer; new tests cover the WP-mode and mission-RTH rejections, RAM-only armed commit and clear (persist not called), and the preserved clear rejection during WP mode. The staged upload buffer with atomic commit and snapshot rollback means an in-flight upload never exposes a partially written list to the navigation state machine within a main-loop tick. Note the deliberate MSP-parity semantics carried over from iNavFlight#10273: with nav_wp_mission_restart = RESUME, replacing the mission mid-flight keeps the waypoint index when the new mission is at least as long as the index.
Contributor
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
PR Summary by QodoAllow safe MAVLink mission edits while armed
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
Code Review by Qodo
1. Stale planner corrupts missions
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+101
to
+102
| return ARMING_FLAG(ARMED) && | ||
| (FLIGHT_MODE(NAV_WP_MODE) || isWaypointMissionRTHActive() || isWpMissionPlannerActive()); |
There was a problem hiding this comment.
1. Stale planner corrupts missions 🐞 Bug ≡ Correctness
The new gate permits armed MAVLink upload or clear after the planner is disabled, but those operations do not reset wpPlannerActiveWPIndex, which planner deactivation also preserves. Reactivating the planner then writes at the stale index and derives the mission count from it, resurrecting cleared waypoints or overwriting/truncating the newly uploaded mission.
Agent Prompt
## Issue description
Armed MAVLink mission upload and clear are now allowed after the on-the-fly planner is disabled, but the planner's progress index remains stale. Reset planner progress whenever an external operation replaces or clears the waypoint list so later planner activation starts from a consistent list.
## Issue Context
Planner deactivation does not clear `wpPlannerActiveWPIndex`. MAVLink clear and commit reset the mission metadata without resetting that index, while `missionPlannerSetWaypoint()` uses any nonzero index directly and then sets the mission count from it.
## Fix Focus Areas
- src/main/mavlink/mavlink_mission.c[203-219]
- src/main/mavlink/mavlink_mission.c[446-477]
- src/main/navigation/navigation.c[5502-5514]
- src/main/navigation/navigation.c[6440-6480]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Since #10273 (INAV 8.0) the MSP path accepts mission uploads while armed as long as the WP mission is not actively being flown - that is what enables in-flight mission management over a serial telemetry radio. The MAVLink mission handlers still deny every transfer and clear outright while armed (
MAV_MISSION_DENIED), so the transport that ELRS and Crossfire bridge natively cannot do what MSP can. This PR aligns the two policies.What changes
mavlinkMissionEditBlocked()gate replaces the blanketARMING_FLAG(ARMED)rejections inMISSION_COUNT,MISSION_CLEAR_ALL,MISSION_ITEMandMISSION_ITEM_INT. Edits are refused while armed AND (WP mode active OR the mission's own RTH leg running OR the on-the-fly mission planner active). The RTH-leg term protects the land/loiter decision at home, which reads the live list; the planner term prevents two writers on one list; the ARMED term keeps the disarmed path provably unchanged.mavlinkAbortMissionUpload(MAV_MISSION_DENIED)so the retry engine stops soliciting items from a partner that was just denied.NAV_WAYPOINTwithcurrent == 2/3) instead of on transfer state, so a guided click can never be absorbed into a running upload.mavlinkCommitMissionUpload()/mavlinkClearPersistedMission(): while armed, commit and clear apply to RAM only and skip persistence -saveNonVolatileWaypointList()refuses while armed, and a flash write mid-flight would stall the main loop. The uploaded mission survives disarm but not a reboot; persisting after landing remains the GCS's responsibility. An in-flight upload also collapses a loaded multi-mission set to the uploaded mission for the session.mavlinkResolveUploadedMissionJumps(), since an in-flight upload bypassesnavigationIsBlockingArming(). Ground uploads are left to the arm-time check, keeping disarmed behaviour unchanged.navigation.c:setWaypoint()'s post-upload clamp ofactiveWaypointIndexuses>=instead of>(0-based index, soindex == waypointCountis already out of range), and a publicisWpMissionPlannerActive()accessor is added.MissionCountWhileArmedIsRejectedbecomesMissionCountWhileArmedStartsTransfer; new tests cover the WP-mode and mission-RTH rejections, the RAM-only armed commit and clear (persist not called), and the preserved rejection of clears during WP mode.Why it is safe
The staged upload buffer with atomic commit and snapshot rollback means an in-flight upload never exposes a partially written list to the navigation state machine. One MSP-parity semantic is deliberately carried over from #10273 and worth an explicit maintainer sign-off: with
nav_wp_mission_restart = RESUME, replacing the mission mid-flight keeps the waypoint index when the new mission is at least as long as that index.Testing
Parameter Group Version Checkfailure is unrelated -check-pg-versions.shcurrently errors withlocal: can only be used in a functionon any PR; this change touches no parameter group.)--pathonly, MSP receiver + fake GPS, pymavlink GCS): disarmed upload/download roundtrip byte-identical to the unpatched nightly; arming via MSP RC; upload while armed: ACCEPTED with the mission verified by download; clear while armed: ACCEPTED, RAM only. On the unpatched nightly the same suite showsMAV_MISSION_DENIEDfor both armed cases - a full A/B.Motivation (measured)
Mission transfer over a radio link is latency-bound, not bandwidth-bound: over an RFD868x at 57600 baud (SiK 3.57, MAVLink framing off) a 40-waypoint MSP mission uploads in 10.7 s at 0.3 % link utilisation, with the cost per waypoint tracking twice the modem's MAX_WINDOW. Enabling the same in-flight capability on the MAVLink transport removes the need for a second radio on ELRS/Crossfire aircraft entirely.
A 9.x backport of the policy (without the staging machinery, matching the one-item-at-a-time semantics MSP has there) is prepared on
raphaelhunziker1202-stack:mavlink-inflight-mission-uploadwith green CI, should it be wanted for master.