Skip to content

Conversation

rubenp02
Copy link
Contributor

@rubenp02 rubenp02 commented Jan 9, 2025

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:

  • 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.

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:

  • Disabled the restriction to only one landing sequence.
  • Modified LandingComplexItem to support scanning for multiple complex items, and configuring the DO_LAND_START mission items to contain the coordinates of the first element in the landing sequence.
  • 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.
  • Added an option in the Plan View settings to control this feature, defaulting to match the previous behavior.

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.

@HTRamsey HTRamsey requested a review from DonLakeFlyer January 9, 2025 12:47
@DonLakeFlyer
Copy link
Contributor

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.

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?

Added an option to configure multiple landing sequences in a mission.

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?

@DonLakeFlyer
Copy link
Contributor

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.

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.

@rubenp02
Copy link
Contributor Author

rubenp02 commented Jan 16, 2025

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?

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.

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?

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.

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.

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.

@DonLakeFlyer
Copy link
Contributor

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.

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.

@DonLakeFlyer
Copy link
Contributor

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.

@DonLakeFlyer
Copy link
Contributor

I also did the following:

  • Use PX4 SITL and the cessna for testing
  • Use Stable to upload a simple mission with a landing sequence to the vehicle
  • Quit QGC and start the version from this pull
  • Go to Plan. The landing sequence degenerates into individual items instead of being sucked back into a landing pattern complex item

@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.

@rubenp02
Copy link
Contributor Author

rubenp02 commented Jan 19, 2025 via email

@DonLakeFlyer
Copy link
Contributor

Might be easier to just flash a board with PX4 than setup SITL: https://docs.px4.io/main/en/dev_setup/dev_env.html.

PX4 doesn’t support multiple landings?

I don't think so. Hoping to get some verification in that issue I created for DO_LAND_START

@rubenp02 rubenp02 marked this pull request as draft January 24, 2025 08:56
@rubenp02
Copy link
Contributor Author

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?

@DonLakeFlyer
Copy link
Contributor

Sure this will be in 5.0. 5.0 will be whatever daily is in a couple months.

@rubenp02 rubenp02 force-pushed the feature/multiple-landing-sequences-support branch from ce1f246 to 9487682 Compare February 14, 2025 14:44
@rubenp02 rubenp02 force-pushed the feature/multiple-landing-sequences-support branch from 9487682 to 8c38fb5 Compare February 24, 2025 10:50
@rubenp02 rubenp02 marked this pull request as ready for review February 24, 2025 10:51
@rubenp02
Copy link
Contributor Author

rubenp02 commented Feb 24, 2025

@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.

@DonLakeFlyer
Copy link
Contributor

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.

@rubenp02 rubenp02 force-pushed the feature/multiple-landing-sequences-support branch from 8c38fb5 to 8734ce9 Compare February 28, 2025 17:22
@DonLakeFlyer
Copy link
Contributor

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.

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.

@rubenp02
Copy link
Contributor Author

rubenp02 commented Mar 2, 2025

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:

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?

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.

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?

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.

My suggestion is to remove the DO_LAND_START futzing code.

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.

@rubenp02 rubenp02 requested a review from DonLakeFlyer March 2, 2025 16:09
@DonLakeFlyer
Copy link
Contributor

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.

@peterbarker
Copy link
Contributor

peterbarker commented Mar 3, 2025 via email

@DonLakeFlyer
Copy link
Contributor

@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.

@peterbarker
Copy link
Contributor

peterbarker commented Mar 5, 2025 via email

@DonLakeFlyer
Copy link
Contributor

I don't remember quite how Terrain Profile works. But here is an example with an alternate landing:
Screenshot 2025-03-05 at 8 29 13 AM

Could you see if it's possible to make the second landing pattern say Alternate FW Landing or something?

As it stands now I'm good to merge this. If you want I can merge and you can look at this in a separate pull?

rubenp02 added 3 commits March 6, 2025 08:37
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.
@rubenp02 rubenp02 force-pushed the feature/multiple-landing-sequences-support branch from 8734ce9 to c2df7c3 Compare March 6, 2025 11:21
@rubenp02
Copy link
Contributor Author

rubenp02 commented Mar 6, 2025

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.

@DonLakeFlyer DonLakeFlyer merged commit 1d17321 into mavlink:master Mar 9, 2025
10 checks passed
@rubenp02 rubenp02 deleted the feature/multiple-landing-sequences-support branch March 9, 2025 22:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants