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.

Sign up for GitHub

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

Jump to bottom

Add max_altitude and _vehicle_attitude.timestamp validity checks to MulticopterLandDetector and standardize var naming #12681

Merged
merged 1 commit into from
Sep 2, 2019
Merged

Add max_altitude and _vehicle_attitude.timestamp validity checks to MulticopterLandDetector and standardize var naming #12681

merged 1 commit into from
Sep 2, 2019

Conversation

mcsauder
Copy link
Contributor

@mcsauder mcsauder commented Aug 10, 2019 •
edited
Loading

Describe problem solved by the proposed pull request
This PR adds validity checks to methods in the MulticopterLandDetector class:

  • _vehicle_local_position.v_z_valid to _get_ground_contact_state() (already accomplished through _has_altitude_lock())
  • valid_altitude_max validity check to _get_max_altitude()
  • _vehicle_attitude.timestamp check to _get_maybe_landed_state()

A few local vars are made snake_case to standardize against file convention and control_mode is renamed to vehicle_control_mode to match its typdef and the established class convention.

One unneeded #include is deleted from LandDetector.h and the #include list alphabetized.

Test data / coverage
Flight tested on a 250 quad with pixhawk 4 mini. Landing detection results are identical to PX4:master.

Additional context
These changes complete the MulticopterLandDetector logic changes introduced in #9756 . See also #11857 .

Please let me know if you'd like anything about this PR changed. Thanks!

-Mark

@mcsauder mcsauder requested review from dagar and dakejahl August 10, 2019 16:10
@weekly-digest weekly-digest bot mentioned this pull request Aug 11, 2019
Closed
dakejahl
dakejahl reviewed Aug 11, 2019
View reviewed changes
src/modules/land_detector/MulticopterLandDetector.cpp Outdated
@@ -131,42 +131,44 @@ bool MulticopterLandDetector::_get_ground_contact_state()
return true;
}

// land speed threshold
float land_speed_threshold = 0.9f * math::max(_params.landSpeed, 0.1f);
if (_vehicle_local_position.v_z_valid) {
Copy link
Contributor

@dakejahl dakejahl Aug 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

What happens if this is not valid? (Tested?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the z velocity is not valid it will never show a _state = LandDetectionState::GROUND_CONTACT; state and the vehicle_land_detected_s uORB topic will never show a true ground contacted state.

In studying the code to answer your question, for the ground contact state to be true, _vehicle_local_position.xy_valid must also be true, (checked in the _has_position_lock(), and this is also being checked in _has_altitude_lock() so I think this change can be reverted. Thanks so much for pointing this out! I'll push up a commit that reverts this change.

src/modules/land_detector/MulticopterLandDetector.cpp Show resolved Hide resolved
src/modules/land_detector/MulticopterLandDetector.h
@@ -42,22 +42,19 @@

#pragma once

#include "LandDetector.h"
#include <math.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this just for the INFINITY?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's correct.

@mcsauder mcsauder changed the title Add _vehicle_local_position.v_z_valid and max_altitude validity checks to MulticopterLandDetector Add max_altitude validity check and _vehicle_attitude.timestamp to MulticopterLandDetector and standardize var naming Aug 14, 2019
@mcsauder mcsauder changed the title Add max_altitude validity check and _vehicle_attitude.timestamp to MulticopterLandDetector and standardize var naming Add max_altitude and _vehicle_attitude.timestamp validity checks to MulticopterLandDetector and standardize var naming Aug 14, 2019
@mcsauder mcsauder force-pushed the multicopter_land_detector branch from b659346 to 66bcf6b Compare August 14, 2019 16:31
@dagar dagar requested a review from a team August 14, 2019 16:36
@jorge789
Copy link

jorge789 commented Aug 15, 2019 •
edited by Tony3dr
Loading

Tested on PixRacer V4:
Modes Tested

Position Mode: Good.
Altitude Mode: Good.
Stabilized Mode: Good.
Mission Plan Mode (Automated): Good.
RTL: Good.

- Procedure
Arm and Take off in position mode, after flying for approximately one minute, switched to altitude then stabilized mode proceed to switch to mission plan mode then make sure that vehicle follows all waypoints as shown in QGC, once completed all waypoin activate RTL and see landing behaviour.

Notes:
No issues noted, good flight in general.

Log:
https://review.px4.io/plot_app?log=bb2b4455-5b0c-477b-8453-b1fd3a35ed28

Master comparison:
https://review.px4.io/plot_app?log=b4b80186-0346-441e-bfea-89d9c39f63f1

Tested on CUAV V5+:
Modes Tested

Position Mode: Good.
Altitude Mode: Good.
Stabilized Mode: Good.
Mission Plan Mode (Automated): Good.
RTL: Good.

- Procedure
Arm and Take off in position mode, after flying for approximately one minute, switched to altitude then stabilized mode proceeds to switch to mission plan mode then make sure that vehicle follows all waypoints as shown in QGC, once completed all waypoin activate RTL and see landing behavior.

Notes:
Upon landing, the vehicle deviated approximately 10 meters from the takeoff site and could not get off, it was changed to stabilized to be able to land manually (7.0 wind speed)

Log:
https://review.px4.io/plot_app?log=3ced7e73-5320-4341-b487-07b75fb66a80

Master comparison:
https://review.px4.io/plot_app?log=fcbefcfc-6ddd-4def-b6e9-b5bf721ba256

@mcsauder
Copy link
Contributor Author

mcsauder commented Aug 15, 2019 •
edited
Loading

Thank you @jorge789 !

@weekly-digest weekly-digest bot mentioned this pull request Aug 18, 2019
Closed
@mcsauder mcsauder force-pushed the multicopter_land_detector branch from 66bcf6b to 5468d5a Compare August 19, 2019 20:40
@mcsauder
Copy link
Contributor Author

Flight tested on a Tarot T960 hexa (~6kg takeoff weight), pixhawk 4 (fmu-v5), one short takeoff/land, one 28 minute flight, good land detection:

Additional flight testing on a 250 quad racer, pixhawk 4 mini:

@weekly-digest weekly-digest bot mentioned this pull request Aug 25, 2019
Closed
dagar
dagar reviewed Aug 26, 2019
View reviewed changes
src/modules/land_detector/MulticopterLandDetector.cpp
@@ -251,6 +251,10 @@ float MulticopterLandDetector::_get_max_altitude()
/* TODO: add a meaningful altitude */
float valid_altitude_max = _param_lndmc_alt_max.get();

if (valid_altitude_max < 0.0f) {
return INFINITY;
Copy link
Member

@dagar dagar Aug 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Does this cause vehicle_land_detected to be published constantly (~ 50 Hz)? Check uorb top.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @dagar, It doesn't appear to:
image

@weekly-digest weekly-digest bot mentioned this pull request Sep 1, 2019
Closed
@dagar
Copy link
Member

dagar commented Sep 1, 2019

@mcsauder can you resolve the merge conflict?

@mcsauder
Add and max_altitude and timestamp validity checks to MulticopterLand…
1131828 
…Detector class.

Rename local camelCase vars to snake_case and control_mode -> vehicle_control_mode to match typdef with established class convention.
@mcsauder mcsauder force-pushed the multicopter_land_detector branch from 65202ec to 1131828 Compare September 1, 2019 23:17
@mcsauder
Copy link
Contributor Author

mcsauder commented Sep 1, 2019 •
edited
Loading

Rebased on current PX4:master and flight tested on a 250 quad, pixhawk 4 mini. Flight testing checks out fine, as previous:

@dagar dagar merged commit 60e5e08 into PX4:master Sep 2, 2019
@mcsauder
Copy link
Contributor Author

mcsauder commented Sep 2, 2019 •
edited
Loading

Thank you for the time you spent reviewing @dagar , @jorge789 , and @dakejahl . I appreciate it.

@mcsauder mcsauder deleted the multicopter_land_detector branch September 2, 2019 04:45
bozkurthan pushed a commit to bozkurthan/Firmware that referenced this pull request Sep 4, 2019
@mcsauder @bozkurthan
land_detector: Add and max_altitude and timestamp validity checks to …
f857ba8 
…MulticopterLandDetector class. (PX4#12681)

 * Rename local camelCase vars to snake_case and control_mode -> vehicle_control_mode to match typdef with established class convention.
@weekly-digest weekly-digest bot mentioned this pull request Sep 8, 2019
Closed
@mcsauder mcsauder mentioned this pull request Oct 2, 2019
Merged
dagar pushed a commit that referenced this pull request Oct 2, 2019
@mcsauder @dagar
land_detector: Remove recently added vehicle_attitude.timestamp check…
508ffa1 
… from MulticopterLandDetector::_get_maybe_landed_state(). (#13072)

PR #12681 added a check to the MulticopterLandDetector::_get_maybe_landed_state() method for a valid vehicle_attitude.timstamp value to finish work in PR #9756. It was discovered that the addition of that check leaves it possible to fly in acro mode without a valid attitude and auto-disarm can engage, allowing the multicopter to fall out of the sky.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dakejahl dakejahl dakejahl left review comments

@dagar dagar dagar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mcsauder @jorge789 @dagar @dakejahl