-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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_GO_AROUND - isAltitude attribute discussion #1994
base: master
Are you sure you want to change the base?
Conversation
|
I think adding |
My other question still stands though:
Would something like the following be better:
It would allow for better markup on the documentation and/or better code-gen. |
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 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 Perhaps also worth making an invalid value of 0 for the altitude to indicate "use whatever value you want". |
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
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
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. |
@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
What assumptions does QGC make?
We have some proposals on the table. We can't really assess them without knowing the implications. For example, adding |
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. |
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. |
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. |
Thanks @DonLakeFlyer @sypaq-nexton What I'm taking away from this is:
I'm ambivalent as to whether it is better to have (match QGC-ish):
or
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 |
The QGC definitions leans towards the common cases requiring less doc'ing and only the un-common cases require extra tagging. So since:
Simpler, less tags needed everywhere |
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:
The low churn variant would be just to add 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. |
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:
Added the attribute to XSD in ArduPilot/pymavlink#824 FYI @sypaq-nexton |
Sounds good to me! Thanks for keeping me in the loop. |
You're welcome. Good to have someone looking at this with fresh eyes. |
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 wherehasLocation
andisDestination
arefalse
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
andisDestination
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
andisDestination
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.Falls out of discussion in #1992
@julianoes @auturgy Opinions? If you agree I will update XSD.