-
Notifications
You must be signed in to change notification settings - Fork 4.3k
Add support for multiple landing sequences #12313
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
Add support for multiple landing sequences #12313
Conversation
So it's the lat/lon in the DO_LAND_START which is used to find the nearest landing pattern sequence for an RTL if there are multiple patterns. Is that correct? If that's the case, is there a reason why setting this with lat/lon/alt should not always be done?
I can see how maybe a commercial vehicle creator may want to create a custom version of QGC where this is not allowed. But for general use I wonder if you should be able to turn off this behavior. Any reason why this shouldn't be allowed by default? Maybe a popup, with a "Don't show again" type thing which tells the user what will happen if they add a second pattern. Then they can Yes/No it if they did it by accident? |
I wonder if an option is needed for this as well? I would change the handling of this to tweak DO_LAND_START in the scan code for the complex item on the way in. This way it only takes effect for landing patterns which were created using the Plan/Pattern support. It specifically does not touch DO_LAND_STARTs which may have been manually added to by the user in which case they control what they want themselves. |
At least in ArduPilot, its not necessary. The DO_LAND_START seems to get picked based on the coords of the next item, at least when no DO_LAND_START items specify any coords. However the MAVLink spec seems to suggest having it, so I assume other firmwares might need it, which is why I'm adding it as an option.
Fair point, I just figured I shouldn't change the existing behavior by default. No option in settings + a popup seems like a good idea but I think from a UX perspective it breaks the philosophy of mission planning in QGC which is meant to be fully intuitive. Maybe change the tool name to "Alternate Land"? Seems a bit too long. It can be kept as a fact so that custom builds can disable it if they want, like you say.
2647246 always generates the DO_LAND_START of a LandingComplexItem with the coords of the next item: ce1f246 only affects standalone DO_LAND_STARTS. It's meant as a helpful feature for when you often use manually defined landing sequences, to avoid having to copy the coords of the next item every time (and it also avoids relying on "Show all values" which is needed to do this, and is an advanced mode feature). I assume that the 2 most common use cases are default DO_LAND_STARTS, and having their coords match the next item with coords. If someone wants something else they can just disable this feature and get the current master behavior. |
Since PX4 doesn't support multiple landing points it doesn't look at the coordinate in here. To me the mavlink spec is bad. The spec should say that closest landing points are determined by the first coordinate item found after the DO_LAND_START. Otherwise the work a GCS has to go through to keep this up to date is crazy. If you create a manual landing path and then move the item after the DO_LAND_START then you need to go backwards and update the coordinate in the DO_LAND_START to keep things working. Makes no sense. My suggestion is to remove the DO_LAND_START futzing code. And then I'll get a discussion started on changing the spec to be something useful. |
I've just been testing this with PX4 SITL and it fails upload of fixed wing landing sequences. Not sure why yet. I verified that Stable still works fine. |
I also did the following:
@rubenp02 Can you test this with PX4 SITL and figure out what is going on? I'm guess this sequence I've detailed here in this comment would break with ArduPilot as well. |
I’m guessing the issue is PX4 only and related to the frame of reference of
the DO_LAND_START items, but I’ll have to test it. I don’t have a PX4 SITL
environment set up and I’m not familiar with it, is it Gazebo you’re
talking about? Is there a setup guide anywhere?
PX4 doesn’t support multiple landings? That needs to be taken into account
for this PR and currently isn’t.
|
Might be easier to just flash a board with PX4 than setup SITL: https://docs.px4.io/main/en/dev_setup/dev_env.html.
I don't think so. Hoping to get some verification in that issue I created for DO_LAND_START |
What's the roadmap like for 5.0? Do you reckon this (once it works for all firmwares and versions) and a fix for #12236 can get in? |
Sure this will be in 5.0. 5.0 will be whatever daily is in a couple months. |
ce1f246
to
9487682
Compare
9487682
to
8c38fb5
Compare
@DonLakeFlyer I've implemented your suggestions from mavlink/mavlink#2201, fixed the PX4 bugs you mentioned here, and explicitly disabled the ability to add multiple landing sequences for PX4, as it doesn't support them. For APM 4.2 and above, I’ve opted not to include coordinates in DO_LAND_START mission items despite the specifiesCoordinate metadata being true, since they are unnecessary. |
Also the discussion going on in the other issue about coords in DO_LAND_START is still clear as mud to me. Need to wait until that settles out as well. |
8c38fb5
to
8734ce9
Compare
I can live with this the code stands now so we can merge this in after the json fix. Then depending on what happens on the conclusion of that discussion in the other issue can you then come back and remove the coords in DO_LAND_START after the fact if that is where that discussion ends up. That way we don't wait on that to get this in. |
The JSON is fixed now. Have you tested this after the changes and does it look good to you now? I have tested it in both ArduPilot and PX4 (where the main feature is disabled as it's unsupported), and everything appears to be working. However, I’d appreciate it if you could also confirm that. To summarize how this has ended up, just to confirm it's all OK:
It's enabled by default now, but it remains a Fact so that custom builds can disable it. The GUI option remains in case some user wants to disable it as well.
For simplicity and consistency, I've opted to simply change the tool name from "Land" to "Alt Land" once there's a valid landing and another one can be added. This keeps it simple and avoids pop-ups.
It has been removed. If at all possible, this keeps the DO_LAND_START items as they are in master (w/o coords.). The only exception is ArduPilot pre-4.2 where the DO_LAND_STARTs generated by the Landing Pattern item get the coordinates of the approach loiter/waypoint, because as @peterbarker said on mavlink/mavlink#2201, they used to be necessary. It would be very easy to add other firmware/version combos to this alternate handling as well. |
I'm trying to test in PX4 SITL but the current SITL plane won't land because it doesn't have lidar. Trying to get a version that will work for me. As it stands now I can't test with ArduPilot SITL. I can't figure out how to get SITL running in a VM to communicate over UDP to QGC on my main OS. |
On Sun, 2 Mar 2025, Don Gagne wrote:
Have you tested this after the changes and does it look good to you now?
I'm trying to test in PX4 SITL but the current SITL plane won't land because it doesn't have lidar. Trying to get a version that will work for me. As it stands now I can't test with
ArduPilot SITL. I can't figure out how to get SITL running in a VM to communicate over UDP to QGC on my main OS.
Adding a udp output is "output add 17.18.19.20:9876" for a UDP stream..
But with a more likely IP address. There obviously has to be a route from
the VM to the main OS for that to work.
Happy to do a VN / screen share to help you out with that.
|
@peterbarker Yeah I've done the |
On Tue, 4 Mar 2025, Don Gagne wrote:
@peterbarker Yeah I've done the output add thing many times before. What happens is that the QGC to ArduPilot connection seems to only pass traffic in the ArduPilot->QGC direction. QGC
connects, but then when QGC asks for things like the parameter list it gets no response back and times out.
Probably worth running tcpdump on the VM to see if ArduPilot is sending
back the relevant response - and which address it is sending to.
|
Updated the Fly View's flight plan drawing to match the Plan View, enabling support for discontinuities in missions (unconnected flight legs) in the former. Key changes: - Fly View now uses simpleFlightPathSegments, the same model used by Plan View, to draw flight paths. This enables proper handling of disconnected mission legs, which the previous _waypointPath approach did not support. - Flight plan arrows now appear in the same places between the Fly and the Plan Views. - Removed the old Fly View drawing logic and related members and properties.
Added an option to configure multiple landing sequences in a mission, if the firmware supports it. This feature allows users to define alternative landing sites, which can be useful for contingency planning. When multiple landing sequences are present, the first one is used for the primary mission, but in the event of an RTL, the closest landing sequence will be used, as outlined in the MAVLink spec: https://mavlink.io/en/messages/common.html#MAV_CMD_DO_LAND_START. Key changes: - Disabled the restriction to only one landing sequence. - Modified LandingComplexItem to support scanning for multiple complex items. - Updated MissionController to handle multiple landing sequences, including proper flight path segment calculation and handling of discontinuities after landing commands. - The TerrainProfile displays the entire mission with any possible alternative landing sequences, but the mission stats (distance and time) represent the primary mission, up to the default landing. Any alternate landings are labelled as such. - Added a Fact and its corresponding option in the Plan View settings to allow users and custom builds to disable this feature.
8734ce9
to
c2df7c3
Compare
Done. I thought the solution would need to be quite hacky but it turned out alright. I made it simply say "Alternate Landing" because I think the context makes it clear that it refers to a fixed-wing landing (or VTOL, since both are supported) but let me know if you prefer it to be more specific. |
Add support for multiple landing sequences
Description
Make isLandCommand true for LandingComplexItems.
Change plan drawing in Fly View to match Plan View:
Updated the Fly View's flight plan drawing to match the Plan View, enabling support for discontinuities in missions (unconnected flight legs) in the former.
Key changes:
Add support for multiple landing sequences:
Added an option to configure multiple landing sequences in a mission. This feature allows users to define alternative landing sites, which can be useful for contingency planning. When multiple landing sequences are present, the first one is used for the primary mission, but in the event of an RTL, the closest landing sequence will be used, as outlined in the MAVLink spec: https://mavlink.io/en/messages/common.html#MAV_CMD_DO_LAND_START.
Key changes:
Add support to set Land Start coords to next item:
Added an option in the Plan View settings to automatically set the coordinates of DO_LAND_START mission items to match the next waypoint in the mission. This is used to help find the closest landing sequence in the event of an RTL. This option is disabled by default to match the previous behavior.
Checklist:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.