Mission item - document values for autocontinue #1868
Conversation
|
I didn't know about MAV_CMD_DO_PAUSE_CONTINUE or MAV_CMD_OVERRIDE_GOTO. How did you know that? I would have used MISSION_SET_CURRENT current instead to move on. |
|
I think DO_JUMP would work too |
|
MISSION_SET_CURRENT sets the current mission item to whatever you want but it should not restart a paused mission. So it isn't something you can use if autocontinue was previously set as That is also probably true for MAV_CMD_DO_JUMP. It is hard to be sure because the description implies it is used in missions. I will fix both those to make it more clear ^^^. Only MAV_CMD_OVERRIDE_GOTO and MAV_CMD_DO_PAUSE_CONTINUE are correct here because they are the only ones that have the semantics of "restart" or "continue". Make sense? If so, this OK to merge? |
|
PS I know about the Pause already and I found override by searching when doing due diligence for this issue @julianoes :-). |
And will continue from there, right? I'm pretty sure this is how it worked in QGC around 2013 😄. There was a checkbox in front of every mission item, and it would switch as it progressed, and you could move the current item by clicking on it, and from my recollection it would send MISSION_SET_CURRENT.
This is not implemented by PX4 nor ArduPilot, from what I can tell, so not sure we should be mention it. It is also sometimes confused with DO_JUMP - from my understanding - is very much a command you use while doing a mission as you go through the mission items and jump around within them. It's new to me that you would use it "from the outside" to move along a mission. It can probably be implemented that way but I would probably rather not do that, given there is MISSION_SET_CURRENT for exactly that purpose. |
Can we remove it? At least deprecate? @auturgy |
Depends. All this should do is update the mission number. It shouldn't change the pause/running state. So if you call the method while the the mission is running it will continue at the new item. If you call it when paused it will move to the new item, but still be paused. That's clearly the mavlink intent. If you can show that the implementation is not doing that then we have a problem. I hope what you saw was only moving the mission number when the mission is already active. Even so though, it could under the hood have been implemented as setting the mission number, then continuing. |
|
And yes, for DO_JUMP it doesn't make so much sense out of missions, though it could used. We should be explicit. |
That makes sense. Now the question is what does it mean to have a "paused mission". I don't think there is a notion of this in PX4, it's just that we're either in mission mode or not. If we're not, then that's usually Hold/Loiter mode. QGC actually sends the command DO_REPOSITION with lat/lon NAN if I remember it right to go into Hold mode and "pause" the mission. MAVSDK just switches to Hold/Loiter mode directly. Now the interesting part is when the mission is "finished". I think PX4 stays in mission mode, so if you then click on a previous item, it will go there instead. However, it would also be intuitive to switch to Hold mode once done which means that sending a SET_CURRENT would then not have an effect. |
|
@julianoes "hold-as-pause" is a valid alternative implementation, provided the system can comply with the mavlink interface. My take is that SET_CURRENT on PX4 should set the current mission item but not change the mode. If it does something different it would be non-compliant.
I think going into hold on mission completion is fine too, and might be more consistent. But even with this what I'm not so fine with is the fact that:
So going into hold immediately at the end of a mission might be obvious, but the reason that trying to put a complete mission back to running fails might not be obvious to a user. So we would need to provide a reason for this. The discussion in #1867 about how to implement in-mission land, disarm, arm, continue is relevant to this. Does any of this change the PR being correct? (genuine question)? |
We somewhat do by sending out the current index. If the current index is equal to the size, or -1, that can be used as a signal that it's done.
You can reset it by setting it to item 0. However, this does not reset DO_JUMPS with counters that go down every time they are executed. You can also reset it by uploading it again. Disarm only sets it back to 0, in the same way as when you do that manually. Regarding the PR: I think it's fine except that MAV_CMD_OVERRIDE_GOTO is not implemented, so I wouldn't recommend it. MAV_CMD_DO_PAUSE_CONTINUE might work but then I'd also mention MISSION_SET_CURRENT. |
|
Hmmm. So MAV_CMD_OVERRIDE_GOTO is not implemented, but you never know what is implemented in mavlink. I can't recommend MISSION_SET_CURRENT because it shouldn't work for continuing (as per this discussion). If it does it is a bug. I think it is worth having a simpler way to reset the mission. Uploading a whole mission is costly, and resetting to item 0 is not resetting if it doesn't fix the counter. |
I don't think you're right here. So we have to try it out for both, in my opinion. In PX4 there is no notion of a paused mission. You're either in mission mode and so changing the set current, would jump to that item and go there, or you're not in mission but Hold and yes, it will switch it but not do anything. For ArduPilot I don't know. |
|
@julianoes Fortunately PX4 does as MAVLink implies - MISSION_SET_CURRENT just changes the waypoint number - it doesn't continue-by-default if paused. Verified on current master using QGC and also using Pymavlink to change the mission number. I'll remove overridegoto though. We can confirm this is OK with James this evening. |
|
@hamishwillee and how did you pause the mission? Using |
|
@julianoes So now we are in violent agreement, this makes sense as documented right? |
|
@hamishwillee yes and no. The comment is now technically not wrong in terms of MAVLink spec, however, it's confusing when you would apply it to PX4 as MAV_CMD_DO_PAUSE_CONTINUE is not implemented and the way to get it to "continue" is to use SET_MISSION_CURRENT to the next one. |
|
What I think you are saying is that
Then you said
Testing shows this does not work. If you're in hold mode you can change the current waypoint without changing into mission mode by calling SET_MISSION_CURRENT. So SET_MISSION_CURRENT does not cause things to continue. I "think" only changing back into the mission mode will restart it. Unfortunately I'm unable to verify the info flow on window because QGC isn't letting me upload a mission. Once I understand how PX4 works I can comment on how this might be updated. However my gut feeling is that PX4 might be technically compliant with the spec but is woefully implementing it. Who would expect do_reposition to be the method you use to pause? |
Correct.
That's my guess, yes.
Correct.
Oh, I missed this one. Yes, it doesn't look like it's implemented. I guess that's almost the same as MISSION_SET_CURRENT except that it has proper acks/nacks. So we should probably deprecate MISSION_SET_CURRENT which is of course the one implemented everywhere. Here is where it was added: #940 We basically added it without discouraging or deprecating the old one that does the same thing.
This does work for "pause with autocontinue=false" but not for paused to a different mode which makes sense to me. To test it:
Oh, you can't upload missions at all 😲, hmmm 🤔... You could use mavsdk :)
That's wrong, I think, but it doesn't matter because otherwise "pause" would just translate to "switch mode to Loiter/Hold", same outcome. |
|
Thanks @julianoes
How can that make sense???? The semantics everywhere else are that this just sets the sequence - independent of all other settings - i.e. you can set the current index when you're landed, or when you're completed and that works fine. Why should the one case of I can't verify what ArduPilot does - it appears to ignore the autocontinue setting altogether. Perhaps we can let the old version behave oddly and fix up the new command? Another question on this. What should happen if the value is set to the same sequence ID?
What I'm kind of getting at is that I expect MISSION_CURRENT to be emitted when the number changes. But it could be that it also gets emitted when you call this command. |
It does "just set the sequence". But when you're in an active mission stuck at autocontinue=false, then changing the sequence (or in this case it could be "manually setting it to next", it means the mission will now continue again.
It gets emitted at a regular rate, so yes it's a bit odd. Which is why the command would be more appropriate. |
I understand what it does thanks. I just think it's inconsistent and therefore likely unexpected. Not technically a spec violation though, since the spec was not written clearly enough. I will think on this now we have a more full picture of what things are doing.
I think it probably should be emitted after the command is sent too. But not at all sure if this should emit only if the number changed. Also it is a spec change to say this so I think I'll leave that aspect for now. |
That as well. I think that's the behavior. |
|
@julianoes OK, I have removed the mention of how this might be paused/restarted. I'm still concerned about this, but perhaps we can agree consistent behaviour in #1870 and deprecate SET_MISSION_CURRENT. IN any case this should be "OK" now. |
Co-authored-by: Julian Oes <julian@oes.ch>
|
@julianoes I have reverted the change to That leaves this with just defining what Autocontinue should do. That's crystal clear, in particular since ArduPilot don't implement it. Merging :-) |
Autocontinue field can be set true or false. If true when the mission item is "achieved" it will start executing the next item. If false, it will pause until explicitly commanded to proceed using MAV_CMD_DO_PAUSE_CONTINUE or MAV_CMD_OVERRIDE_GOTO.
This makes it clear how the command should be used - since it wasn't clear what values would cause what behaviour.