Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MAV_CMD_DO_SET_MISSION_CURRENT - allow mission resetting #1870

Merged
merged 8 commits into from
Aug 31, 2022

Conversation

hamishwillee
Copy link
Collaborator

This changes one of the unused params in MAV_CMD_DO_SET_MISSION_CURRENT so that it can be used to reset a mission "fully".

Prior to this the command would just update the current mission item.

  • if the mission was completed it would stay complete, with no way to restart other than re-uploading the mission or landing and taking off again (which triggers mission reset).
  • if the mission was active you could go back to the start point but the mission would not reset jump counters.

With this change you can optionally specify that setting the current mission item also clears any "completion status" and makes the mission active again, and that the mission counters are reset.

This effectively means that MAV_CMD_DO_SET_MISSION_CURRENT can be used as a hypothetical "reset mission" command by setting the mission item to 1 and the new flag.

@auturgy @julianoes Make sense?

Co-authored-by: Julian Oes <julian@oes.ch>
@hamishwillee
Copy link
Collaborator Author

@julianoes And of course PX4 does not support this :-).

Worth adding to MISSION_SET_CURRENT as well or do I fix up the command and deprecate this version (on assumption commands are better for this kind of thing?)

@hamishwillee
Copy link
Collaborator Author

hamishwillee commented Aug 10, 2022

@peterbarker Would appreciate your review/advice on this.

The current implementation of MAV_CMD_DO_SET_MISSION_CURRENT and SET_MISSION_CURRENT just resets the sequence number. If you have a completed mission the mission remains completed. If you have an active mission with jump counters it will change the mission number, but it doesn't reset the counters.

What this change does is:

  • Allow you to really reset a mission - you specify param2 to reset the counters and unset the completion status. I have stated that doing this when complete forces the mission to paused. This allows a full reset of the mission, as if you had uploaded a new mission or done an arm/disarm cycle.
  • Clearly specifies that if the mission is paused or complete calling this does not continue the mission - it just updates the mission number.
  • Clearly specifies what happens if you set a sequence value out of range MAV_RESULT_FAILED

I have looked at the ArduPilot implementation of this you did in ArduPilot/ardupilot#16687.
The definition here slightly disagrees with that in two respects.

  1. If there is no mission APM ACKs with MAV_RESULT_UNSUPPORTED. I think it should return MAV_RESULT_FAILED because this is a) a case of "out of range sequence" and b) because "unsupported" is defined as "the command is not supported", while "failed" means "this command is valid but you can't call it at this time and there is some environmental problem you will have to fix before you can call it - i.e you will have to change the mission.
  2. It looks like the general implementation just sets the mission number but does not continue, while the plane will autocontinue if it gets this:
        // if you change this you must change handle_mission_set_current
     plane.auto_state.next_wp_crosstrack = false;
     if (plane.control_mode == &plane.mode_auto && plane.mission.state() == AP_Mission::MISSION_STOPPED) {
         plane.mission.resume();
     }
    
    Is that correct? If so, then I might need to amend the text that this does not force autocontinue.

Point 2 is problematic, and the reason I am "hoping to do" this update. To give you some context, PX4 implements a normal pause just by changing the mode to hold mode (which is a separate mode, not a sub-mode of auto like in ArduPilot). But it implements a pause on autocontinue=false as a kind of sub mode. When SET_MISSION_CURRENT is called when you're in hold mode the call just updates the current mission number, but when you're in the "autocontinue=false" mode it will automatically restart the mission as well as updating the mission number.

I think this is a bit inconsistent. Should a call to this method do something different because of the type of pause, or because you happen to be flying in a plane? That's just confusing for users.

It would be good if we can make MAV_CMD_DO_SET_MISSION_CURRENT predictable. For example if it is intended to continue a mission then make it do so always. So if you call it when in another flight mode it should move to auto mode and execute. Or if it isn't intended to restart a paused mission, then it should never do so - in which case if you were paused and switched to circle mode, then back to the mission mode it should remain paused.

And the last option - we give it a flag and be explicit about whether the call should continue the mission or not.

@hamishwillee
Copy link
Collaborator Author

hamishwillee commented Aug 10, 2022

PS We might further clarify this adding a continue param to decide whether the mission continues or not after calling the command.

Perhaps the bigger question is should this always make the mission continue? If I was in hold mode, would you expect this to change the mode back to auto and continue?

@hamishwillee
Copy link
Collaborator Author

@julianoes FYI I now know a bit more about how ArduPilot does "pause" and works with this MAV_CMD_DO_SET_MISSION_CURRENT

  • Neither PX4 or ArduPilot allows MAV_CMD_DO_SET_MISSION_CURRENT to change the main mode. So if you're in hold and call it, you won't change mode and resume automatically.
  • ArduCopter runs all functionality in sub modes of auto. So if you pause you are pausing in auto.
    • MAV_CMD_DO_SET_MISSION_CURRENT does not change the mode from paused. The only mode like this in PX4 is start from autocontinue=false pause - I would say PX4 is incompatible since autocontinue is a late spec addition and their implementation predates it.
    • does not autostart if you call this after boot when landed. I think this differs from PX4 which will autostart from landed - right?
  • ArduPlane does not support pause "semantically".
    • If you want to pause you just change to the mode you want. They don't implement a pause instruction or call it pause.
    • If it runs out of items it performs an RTL. If you call MAV_CMD_DO_SET_MISSION_CURRENT it will reset the mission sequence and unset the "stopped" status. However it will not resume until you manually change back to auto (i.e. not auto change from calling this).
    • if you call MAV_CMD_DO_SET_MISSION_CURRENT from the ground when it is in auto it will take off. THis is more like PX4 copter.

I'm still trying to work out whether we can align. I think the best thing would be if PX4 implements "not resuming following autocontinue=false" I can tell a good story. The other behaviours I can live with the inconsitency

@hamishwillee
Copy link
Collaborator Author

@tridge @rmackay9 @peterbarker @julianoes Can I please have a new review of this MAV_CMD_DO_SET_MISSION_CURRENT/MISSION_SET_CURRENT update.

This aligns them with how ArduCopter, PX4 and ArduPlane actually work using the terminology we used for the MISSION_CURRENT update. The main purpose is the jump counter addition, which has not changed since your last review.

Specifically:

  • It explicitly states that by default jump counters are not reset (that was previously inferred)
  • It states that this command may change the mission-state on some systems, and that in mission mode this might resume a mission.
  • it states that this command must not change the flight mode - so if you're loitering this command will change the state machine, but not resume the mission.

What it doesn't do is explain which systems allow the commands to change the state, and why - for example, ArduPlane uses a model where MAV_CMD_DO_SET_MISSION_CURRENT is used to set mission-state active when the mission is suspended, and then move the mission to auto to take off. While ArduCopter would might sit in AUTO and then use MISSION_START to start a mission. That would have to be in flight stack docs, though I hope to capture some of it in the mavlink devguide once all this is settled.

@peterbarker I think the ideas are now baked enough to implement.

Copy link
Collaborator

@julianoes julianoes left a comment

Choose a reason for hiding this comment

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

Thanks, I think this is pretty clear now!

@hamishwillee
Copy link
Collaborator Author

Thanks @julianoes . Just waiting on ArduPilot review.

The command will ACK with MAV_RESULT_FAILED if the sequence number is out of range (including if there is no mission item).
</description>
<param index="1" label="Number" minValue="-1" increment="1">Mission sequence value to set. -1 for the current mission item (use to reset mission without changing current mission item).</param>
<param index="2" label="Reset Mission" minValue="0" maxValue="1" increment="1">Resets mission. 1: true, 0: false. Resets jump counters to initial values and changes mission state "completed" to be "active" or "paused".</param>
Copy link
Member

Choose a reason for hiding this comment

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

I like the counter reset idea.

@hamishwillee
Copy link
Collaborator Author

Thanks for reviews! Merging.

@hamishwillee hamishwillee merged commit 5043a1e into master Aug 31, 2022
@hamishwillee hamishwillee deleted the hamishwillee-MAV_CMD_DO_SET_MISSION_CURRENT branch August 31, 2022 23:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants