MAV_CMD_DO_GO_AROUND - isAltitude attribute discussion

[hamishwillee]

MAV_CMD_DO_GO_AROUND has an altitude with units in metres but no specified frame.

While I suspect that most flight stacks would assume this to be AMSL because they should have a barometer, but some might have terrain info, rangefinder, or assume AGL from home. If sent in a COMMAND_LONG the frame is ambiguous.

For discussion, this adds a new attribute hasAltitude that should be applied to any item where hasLocation and isDestination are false but there is an altitude setting that does not have a fixed and specified frame.
This is needed so we can auto-add notes to indicate what command this should be sent in.

Initially I intended to change the attributes hasLocation and isDestination from false to true, which would allow me to auto-add a note that the command should be send in COMMAND_INT.

However @sypaq-nexton had a reasonable question - should hasLocation and isDestination only apply to things that have a position in the ground plane (like lat/lon).
The answer is yes. isDestination was invented so a GCS could identify waypoints for drawing lines through automatically. If we apply this to things without a location, the GCS might screw up rendering.
I don't know for sure, but I suspect that hasLocation might have similar semantics.

1. Does adding this make sense.
2. Does adding it only if the other attributes make sense, or is it better to add it to everything that has an altitude, including those that have location.
3. You probably only add it if the frame is fixed in the description. The only case you would want it would be if param 5/6 was used for altitude. We would avoid this, so I think we don't add it for those cases.

Falls out of discussion in #1992

@julianoes @auturgy Opinions? If you agree I will update XSD.

[nexton-winjeel]

While I suspect that most flight stacks would assume this to be AMSL because they should have a barometer, but some might have terrain info, rangefinder, or assume AGL from home.

• PX4 doesn't use the altitude: FixedwingPositionControl.cpp#L227
• ArduPilot uses the altitude as height above home:
  • GCS_Mavlink.cpp#L1038, which calls into ArduPlane.cpp#L685

[nexton-winjeel]

I think adding hasAltitude makes sense. And unless there's already an established meaning for hasLocation, I think specifying that as lat/lon only makes sense. So most commands would get tagged with both hasLocation and hasAltitude.

[nexton-winjeel]

My other question still stands though:

• Does hasLocation imply that the latitude and longitude are in params 5 and 6, or just somewhere in the command?
• Ditto for the new hasAltitude -- in param 7, or anywhere?

Would something like the following be better:

• latitudeParam="5", longitudeParam="6", altitudeParam="7"

It would allow for better markup on the documentation and/or better code-gen.

[hamishwillee]

While I suspect that most flight stacks would assume this to be AMSL because they should have a barometer, but some might have terrain info, rangefinder, or assume AGL from home.

• PX4 doesn't use the altitude: FixedwingPositionControl.cpp#L227

• ArduPilot uses the altitude as height above home:

  • GCS_Mavlink.cpp#L1038, which calls into ArduPlane.cpp#L685

As you say - PX4 uses the greater of a parameter or the current height: https://docs.px4.io/main/en/flight_modes/mission.html#operator-abort
I guess we could specify as "AGL in whatever way calculated by the vehicle". Not sure we need to be more specific.

It's fine by me that PX4 ignores the altitude - I'd hope that ArduPilot would below some threshold too. What isn't so fine is that there is no way for a recipient to know that. I wonder if we might accept the command (because if someone does DOAROUND they really want it to happen) but allow feedback on the particular params that were ignored in the command ack. This might be a genuine use for COMMAND_ACK.result_param2.

Perhaps also worth making an invalid value of 0 for the altitude to indicate "use whatever value you want".

[hamishwillee]

I think adding hasAltitude makes sense. And unless there's already an established meaning for hasLocation, I think specifying that as lat/lon only makes sense. So most commands would get tagged with both hasLocation and hasAltitude.

That is a good idea too, and if we were starting "from scratch" I'd say for sure. Whether it is viable depends on whether ground stations or anyone are using hasLocation for anything.

My other question still stands though:

• Does hasLocation imply that the latitude and longitude are in params 5 and 6, or just somewhere in the command?
• Ditto for the new hasAltitude -- in param 7, or anywhere?

Its still an open question. IMO we should make that assumption (param 5, 6) and use the convention to enforce it. Mostly because of the desire for higher resolution data, and for the ground station to be able to just assume that these values are the ones it should use.

Not so sure for hasAltitude, mostly because there are cases with multiple altitudes.

Would something like the following be better:

• latitudeParam="5", longitudeParam="6", altitudeParam="7"

It would allow for better markup on the documentation and/or better code-gen.

Yes it would. But you still wouldn't know if the thing was a destination or an ROI target.

I also have a leaning to make it hard to allow lat/lon generally to be outside of param 5/6 to force the convention, and ease cognitive load for users.

Last of all, I'm not sure what a ground station would do with this additional information if it differed.

[hamishwillee]

@DonLakeFlyer We're trying to better instrument MAVLink positional information so that we can auto-add documentation about whether commands should be sent in command_int. We've got some problems because there are cases where various values are not in the expected positions.

Minimally we would like to

• be able to differentiate cases that have only an altitude.
• know what we can assume about the isDestination/hasLocation

What assumptions does QGC make?

• Does it use isDestination? [I think it does, to draw lines between waypoints]
• Does it actually use hasLocation? [not sure]
• For both of these, do the commands assume that lat, lon, altitude, are in param 5, 6, 7?

We have some proposals on the table. We can't really assess them without knowing the implications. For example, adding hasAltitude, and making hasLocation just mean lat/lon.

[DonLakeFlyer]

QGC has the json formation for mission items doc'ed here: https://github.com/mavlink/qgroundcontrol/blob/master/src/MissionManager/MissionCommandUIInfo.h#L88

The original proposal was based on that QGC code. I'm not exactly sure if there is one to one mapping or no with the mavlink spec. But it does tell you all the stuff that QGC needs to know about to creates it's ui for planning missions.

The generic mavlink version of QGC mavlink ui mission info are in MissionManager directory an all start with MavCmdInfo*.json. The firmware specific overrides are in for example: FirmwarePlugin/PX4/PX4-MavCmdInfo.json. You can use these as examples. I thought I had docmented all of this somewhere in the dev guide but can't seem to find it any more.

Can you take a look at that and see if you still have questions.

From a standpoint of param 5/6/7 being lat/lon/alt, QGC does not use the MissionCommandUInfo json to know where those are. That is hardwired into the code and various UI bits.

[DonLakeFlyer]

Also keep in mind that QGC still uses it's own json description files for all of this. It does not currently use the mavlink spec metadata directly. Although if there is a one to one correspondence it could be changed to use this with some coding and build system work.

[DonLakeFlyer]

Although if there is a one to one correspondence it could be changed to use this with some coding and build system work.

Sorry haven't been in this space for a while. That statement is incorrect. It is way more complicated than having a single definition of the mavlink messages. If anyone wants the gory details on why that it is it would easier to explain in a call.

[hamishwillee]

Thanks @DonLakeFlyer

@sypaq-nexton

What I'm taking away from this is:

1. The position co-ordinate metadata QGC uses is
   • specifiesCoordinate - Command specifies a lat/lon/alt coordinate
   • specifiesAltitudeOnly - Command specifies an altitude only (no coordinate)
   • standaloneCoordinate Vehicle does not fly through coordinate associated with command (exampl: ROI)
2. hasLocation = specifiesCoordinate, but then standaloneCoordinate is the opposite of isDestination - i.e. mavlink marks the locations that are specifically destinations, while QGC marks the locations that are specifically ROI-like.
3. I suspect that QGC always deals in positions (lat, lon, alt) or altitudes (alt), so in that case it is easiest to have attributes that represent those cases.
4. We can change any of this without breaking QGC.

I'm ambivalent as to whether it is better to have (match QGC-ish):

• hasLocation - has lat/lon/alt
• altitudeOnly - has alt

or

• hasLocation - has lat/lon
• hasAltitude - has alt

OR latitudeParam="5", longitudeParam="6", altitudeParam="7"

The first is the closest to actual use, the last is the most flexible. I lean towards one or two. I slightly prefer 2

[DonLakeFlyer]

The QGC definitions leans towards the common cases requiring less doc'ing and only the un-common cases require extra tagging. So since:

• Majority or lat/lon mission items include alt so specifiesCoordinate means lat/lon/alt so less flagging to do to indicate items which are only alt, or only lat/lon
• Majority of lat/lon items are also for flying through hence only a few items require standaloneCoordinate

Simpler, less tags needed everywhere

[hamishwillee]

Thanks - I appreciate your reasoning. It makes sense for flying drones, and that is the case that MAVLink messages have been mostly evolved for. However it isn't quite so reasonable for ground systems/any command where you don't care about the altitude. I'm also not concerned about a few extra tags here. We can focus on air systems and how the command set looks now, or try make something general. Or we can decide we don't like the churn.

The good compromise general format would be:

• hasLocation - has lat/lon
• hasAltitude - has alt
• isStandaloneCoordinate - is an ROI and such. Otherwise assumed to be a location.

The low churn variant would be just to add altitudeOnly

Either case I think we assume that these only apply to cases where the values apply to param5, 6, 7.

I'll see what the mavlink call people think.

[hamishwillee]

We discussed in dev call. The general feeling was that this is pretty pointless unless someone is using them or indicates they want to use them and can be a stakeholder in the discussion. For now, the main point is to be clear about what the set do mean. Since we want to reduce churn, and keep it possible for QGC to use this data:

1. Add altitudeOnly, a synonym for QGC specifiesAltitudeOnly.
2. Clarify that the tagging ONLY applies altitude in param7 and lat/lon (x/y) in param5, 6. The tags would not be applied otherwise.

Added the attribute to XSD in ArduPilot/pymavlink#824

FYI @sypaq-nexton

[nexton-winjeel]

1. Add altitudeOnly, a synonym for QGC specifiesAltitudeOnly.
2. Clarify that the tagging ONLY applies altitude in param7 and lat/lon (x/y) in param5, 6. The tags would not be applied otherwise.

Sounds good to me! Thanks for keeping me in the loop.

[hamishwillee]

You're welcome. Good to have someone looking at this with fresh eyes.