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.
|
ⓘ 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.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
|
RAM / Flash usage vs. base commit
See RAM/flash optimization guide for techniques to reduce usage. |
|
Test firmware build ready — commit Download firmware for PR #11851 249 targets built. Find your board's
|
|
@raphaelhunziker1202-stack what do you think of that Qodo comment? Is that real? |
|
@sensei-hacker I checked it against the actual navigation code, and I don't think that Qodo finding is real. Both paths it calls out already go through
So after either an external clear or a replacement upload, re-enabling the on-the-fly planner cannot resume from the previous planner index; it starts from index 0. The failure sequence Qodo describes would require I can add a focused regression test for this interaction if you think it would be useful as documentation/guard coverage, but I don't think a production-code change is needed for this finding. |
An upload or clear (MSP, MAVLink, EEPROM load) goes through resetWaypointList(), but the on-the-fly planner kept its write index and status across it. Re-enabling the planner afterwards wrote at the stale index inside the new mission and derived the waypoint count from it, or refused new waypoints if the old status was still FULL.
|
Addressed the review finding on the mission planner index in ea0577c: The upstream workflow for this commit is waiting for approval; the same commit built green on my fork: https://github.com/Raffi1202/inav/actions/runs/34327738409 (all targets, SITL, unit tests). |
|
@sensei-hacker I have to correct my reply from 6 September — it was wrong. At Fixed in |
Thanks! |
Those two files belong to iNavFlight#11885, which replaces check-pg-versions.sh with a Python checker. Carrying a second, older edit of the same file here only produces a conflict once either lands, and it is unrelated to the MAVLink mission change.
Four statements no longer held: the blanket armed rejection, the clear that always removed the saved mission, and the claim that every completed upload reaches nonvolatile storage. Adds what RAM-only means for the pilot - the mission survives disarming but not a reboot - and that an upload while armed collapses a loaded multi-mission set.
|
Thanks for catching the off-by-one in A few thoughts from my side, mostly suggestions to consider rather than requirements — happy to hear what you and @sensei-hacker think: Shared edit gate for MSP and MAVLink JUMP validation Smaller things
Testing Thanks again! |
3 participants
Problem
A ground station connected over MAVLink cannot change the mission once the aircraft is armed:
MISSION_COUNTandMISSION_CLEAR_ALLare answered withMAV_MISSION_DENIED, and anyMISSION_ITEM/MISSION_ITEM_INTthat is not a guided waypoint withMAV_MISSION_ERROR, even when no waypoint mission is running. The same upload over MSP has been accepted since #10273 (INAV 8.0) as long as WP mode is not active. No issue is filed for this; nothing crashes or misbehaves, the two transports simply apply different rules.Cause
src/main/mavlink/mavlink_mission.c:986(MISSION_COUNT),:953(MISSION_CLEAR_ALL),:1051-1058and:1254-1261(MISSION_ITEM/_INT) onmaintenance-10.xreject onARMING_FLAG(ARMED)alone, while the MSP path insrc/main/navigation/navigation.c:5468only checks!FLIGHT_MODE(NAV_WP_MODE). Lifting the gate alone would not work: commit and clear (mavlink_mission.c:430,:197) always callmavlinkPersistMission(), andsaveNonVolatileWaypointList()(navigation.c:5668) returns false while armed. Two related spots:navigation.c:5492clampsactiveWaypointIndexwith>although the index is 0-based, andresetWaypointList()(navigation.c:5501) keeps the on-the-fly planner's write index and status when the list is replaced (Qodo finding on this PR).Change
A
mavlinkMissionEditBlocked()gate replaces the fourARMEDchecks: edits are refused only while armed and WP mode is active, the mission's own RTH leg is running, or the on-the-fly mission planner is active; if the refused sender owns a receiving transfer, it is aborted withMAV_MISSION_DENIED. Guided fly-to-here / altitude-target items are recognised bycurrent == 2 || 3on the item instead of by transfer state. While armed, commit and clear apply to RAM only and skip persistence, and armed commits apply the arm-time JUMP rules (no JUMP as first item, no self/adjacent target, sane repeat count, geo-referenced target) inmavlinkResolveUploadedMissionJumps().setWaypoint()clamps with>=,resetWaypointList()resetswpPlannerActiveWPIndexandwpMissionPlannerStatus, andisWpMissionPlannerActive()is exported.Test
Not run on hardware and no SITL session. Cause verified by reading the lines above on
maintenance-10.x. Unit tests insrc/test/unit/mavlink_unittest.cc:MissionCountWhileArmedIsRejectedbecomesMissionCountWhileArmedStartsTransfer; new tests cover rejection during WP mode and during the mission RTH leg, RAM-only commit and clear while armed (saveNonVolatileWaypointListnot called), and the clear rejection during WP mode. Fork CI for the head commit,testjob plus all targets and the four SITL builds green: https://github.com/Raffi1202/inav/actions/runs/34374150507. Upstream CI ran for the first commit only: https://github.com/iNavFlight/inav/actions/runs/33616820819; the runs for the two later commits are held at "action required".Flash / RAM
Not measured yet. The upstream firmware CI has not been released for this PR, so no size report exists.
Docs
docs/Mavlink.md: the capability summary, theMISSION_ITEMandMISSION_CLEAR_ALLentries and the mission-storage paragraph described the old blanket armed rejection and unconditional persistence. They now state when an armed edit is refused, that a clear while armed leaves stored waypoints alone, and what RAM-only means for the pilot (survives disarming, lost on reboot, an armed upload collapses a loaded multi-mission set).