AP_Mission: Reduce the number of places _set_cmd is called from

[WickedShell]

I'm looking into making commands skippable if the vehicle will not actually preform any actions (as part of #9186) however to do this we need to honor the return value of starting a command. This PR simplifies the paths through the code that call _cmd_start. This is also worthwhile as set_current_command was missing the protection against endless do jump loops that advance_current_nav_cmd has. (Which means that a malformed mission can lock the FMU if a set_current_command points into the sequence).

This is meant to be a no functional change and presented by itself for easier review.

[rmackay9]

I don't object to this change but in Copter and (I think) Rover, if there is an unknown command the verify step immediately returns true and we move onto the next command. That takes an iteration or two but the vehicle keeps doing whatever it was doing during the interim. So if it was going to a waypoint then it will just stop or circle around the waypoint.

[WickedShell]

That takes an iteration or two but the vehicle keeps doing whatever it was doing during the interim. So if it was going to a waypoint then it will just stop or circle around the waypoint.

Yeah plane does that as well, the end state (and not in this PR) is that I'd like to not be stuck in an indeterminate amount of time like that, and on plane I actually want to ensure that the vehicle doesn't try and run the verify.

[rmackay9]

@WickedShell, OK. I wonder what would happen in the end state if there were 50 unknown commands with a do-jump going back to the first one. In any case.. I guess we shall see when we get there.

[WickedShell]

@WickedShell, OK. I wonder what would happen in the end state if there were 50 unknown commands with a do-jump going back to the first one. In any case.. I guess we shall see when we get there.

You hit the 255 command limit and it would return without doing anything, same as what it does now. This actually fixes a bug where it never returns if you used set_command_index into that list at the moment.

[peterbarker]

While @WickedShell wanted this to be NFC, some functional changes did creep in - but my understanding is that those changes are strictly beneficial.

[WickedShell]

So to expand slightly on the functional change that came in. set_current_cmd could return true even if no active navigation commands were selected. This return value was used in two places, to send a MAVLink acknowledgment to a GCS request, and inside the jump to landing logic. The former was fine, this is a quite reasonable return value over MAVLink if you only jump to a DO_ command of some form. The latter is quite problematic. What happens inside jump to landing is we execute the set_current_cmd then unconditionally restart the mission. If your last command was a DO_LAND_START this causes you to restart the mission from the first waypoint, which is almost always not what a user intended to plan, and can be extremely surprising. From a brief discussion with @rmackay9 and @tridge on mumble it was felt it was better to return false if no active command is selected with set_current_cmd as it patches the accidental mission restart, and the use case for trying to set a sequence of DO_ commands from a GCS is extremely limited.