Skip to content
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

Feature: have the Mission Feasibility Checker check for a takeoff waypoint #11531

Merged

Conversation

TSC21
Copy link
Member

@TSC21 TSC21 commented Feb 23, 2019

Test data / coverage

  1. make px4_sitl_default gazebo_plane
  2. Create a mission without a takeoff waypoint - mission check will complain;
  3. Create a mission with a takeoff waypoint, but not being the first waypoint item - mission check will complain;
  4. Create a mission with a takeoff waypoint not as the first item, but having the previous items not being position related waypoints - mission check will pass;

Describe problem solved by the proposed pull request
This PR addresses #10657.

Describe your preferred solution

  1. Adding a check for the MC and FW takeoff on the mission feasibility checker, controlled by a parameter MIS_TAKEOFF_REQ, which allows to verify if there's a takeoff waypoint on the loaded mission;
  2. If the takeoff waypoint exists but it's not the first mission item, then a whitelist is applied to the check so to verify if the items previous to the takeoff waypoint items are not waypoint/position related points.
  3. Two different critical log messages are used to warn 1. and 2.

Describe possible alternatives
#10657 contains other approaches that may complement this solution.

@Antiheavy, here it goes.

@TSC21 TSC21 force-pushed the feature/mission_feasibility_check_check_for_takeoff_waypoint branch from 2c8ede3 to 919a097 Compare February 23, 2019 19:31
@Antiheavy
Copy link
Contributor

Thanks @TSC21 ! Not only is this a helpful reminder for users, this is an important SAFETY feature too. Requiring the user to upload a mission with a properly ordered Takeoff waypoint help mitigate the following scenario:

Our vehicle is a typical hand-launched fixed wing. We use Launch Detection to protect the motor from spooling up at unexpected times during ground handling. Too often our users will plan a mission using only a Survey Pattern and a Landing Pattern (Landing Pattern use is required since we use RTL_TYPE=1), forgetting they must place a Takeoff Waypoint at the start of the mission. This leads to an unsafe condition where using "Start Mission" in QGC switches to Mission mode and Arms the vehicle and LaunchDetection is by-passed. In this unsafe state LandDetector can easily switch to Flying by wind on the pitot tube or the user handling the vehicle on the ground. This causes the motor to spool up unexpectedly (dangerously) and even if the user manages to hand launch the vehicle, the fixed-wing takeoff logic isn't used, often resulting in a crash.

This PR is an important component to improving safety and ease of use for the above common scenario.

@Antiheavy
Copy link
Contributor

Also, I have tested extensively in SITL based on our specific use case and everything seems to be working properly from the user perspective.

Antiheavy
Antiheavy previously approved these changes Feb 23, 2019
Copy link
Contributor

@julianoes julianoes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks correct but what I don't understand is if there is actually a difference between the the multicopter and fixedwing case. If not, then that's a lot of duplicated code and should probably go in a separate function.

src/modules/navigator/mission_feasibility_checker.cpp Outdated Show resolved Hide resolved
@TSC21
Copy link
Member Author

TSC21 commented Feb 26, 2019

This looks correct but what I don't understand is if there is actually a difference between the the multicopter and fixedwing case. If not, then that's a lot of duplicated code and should probably go in a separate function.

@julianoes that's actually a fair point and I don't see a real difference here between them. Maybe we can set a common function for them?

@TSC21
Copy link
Member Author

TSC21 commented Feb 26, 2019

@julianoes I did a minor cleanup and addressed your concern regarding the repeated code. I think the feasibility checker still needs a major restructure, as some of the logic seems to be repeated and in some cases redundant, but I think we can bring that in a next PR.

Antiheavy
Antiheavy previously approved these changes Feb 27, 2019
Copy link
Contributor

@Antiheavy Antiheavy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran SITL on today's commit and everything checks out.

Copy link
Contributor

@julianoes julianoes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One change needed, then this is good to go.

src/modules/navigator/mission_feasibility_checker.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@julianoes julianoes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, this can go in!

@julianoes
Copy link
Contributor

I'm ignoring the "Hardware Test" CI, not sure what is wrong with it.

@julianoes julianoes merged commit 12d29fd into master Feb 27, 2019
@julianoes julianoes deleted the feature/mission_feasibility_check_check_for_takeoff_waypoint branch February 27, 2019 12:25
@TSC21
Copy link
Member Author

TSC21 commented Feb 27, 2019

Thanks @julianoes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants