Cherry pick the `src/modules/land_detector/` directory work from PR #9756 #11874

mcsauder · 2019-04-17T21:50:45Z

Describe problem solved by the proposed pull request
This PR captures the remaining src/modules/land_detector/ directory changes from PR #9756 to simplify that PR.

The changes in the land_detector directory are fairly straightforward:

Break out common features of the child class functionality and move to the LandDetector::initialize_topics() and LandDetector::update_topics() parent class methods.
Standardize class member variable naming, uniform initialization, and whitespace formatting across all class implementations in the land_detector module.

This PR primarily aims to reduce the scope of PR #9756.

Describe possible alternatives
Keeping the current implementations is certainly a viable option; please let me know if that is the preference!

Additional context
See PR #9756.

Much of the original work proposed in this PR has already been carried out by #12209, #12677, #12702, #13079, and #13077.

Thanks!

-Mark

mcsauder · 2019-06-04T20:30:30Z

@dagar, finally, I can help with flight testing.
This PR:
https://review.px4.io/plot_app?log=72c9c408-e52b-4b1c-b8c0-a4e21f2fb2b8
https://review.px4.io/plot_app?log=60098390-cb97-4338-8aea-0debd2c0c358
https://review.px4.io/plot_app?log=dfe950cd-854d-4c1d-be41-af1ec35824d4

PX4 master branch on the same drone:
https://review.px4.io/plot_app?log=e59fc2e3-702a-4e83-b1c1-66e08b2fa9d6
https://review.px4.io/plot_app?log=225d2309-4dee-48de-a820-e4297dc47ea2
https://review.px4.io/plot_app?log=5cadb268-8894-42ac-8bf4-c4da2a8b1734

The multicopter land detector seemed to behave the same for this PR and PX4 master with my airframe, neither reliably detects. I attribute the inability to detect the landing in this case on somewhat high levels of vibration, (something I can look into on my rig), but again, no perceptible differences between this PR and master.

Please let me know if you need more data collected against this PR.

-Mark

mcsauder · 2019-10-10T02:15:15Z

I've squashed all of the commits in this PR and corrected duplicate (inherited) code in the MulticopterLandDetector that I had missed pruning.

@PX4/testflights , now that it's passing CI, (only if you have time and its not too inconvenient), would you be able to try this PR on a fixed wing? I'll get multicopter flight logs posted here as soon as I have my hardware flight ready again; should only be a day or two.

dannyfpv · 2019-10-10T21:24:13Z

Tested on pixhawk1 v3 phantom wing
Modes Tested:
Takeoff: no issues
RTL Mode:. no issues
Mission mode: no issues
Altitude Mode:. no issues
Stabilized Mode:. no issues
Landing: no issues

log:
https://review.px4.io/plot_app?log=3672b9da-c0b9-4b53-bc70-f350517c0823
https://review.px4.io/plot_app?log=812f608d-0a99-476f-a44a-96685b82e953

Junkim3DR · 2019-10-10T21:29:16Z

Tested on Pixhawk 4mini v5:

Modes Tested

Position Mode: Good.
Altitude Mode: Good.
Stabilized Mode: Good.
Mission Plan Mode (Automated): Good.
RTL (Return To Land): 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 waypoint then trigger RTL and see landing behavior.

Notes
No issues noted, good flight in general.

Log:
https://review.px4.io/plot_app?log=818a58e3-4679-47ea-a2bf-f1b1d505c017

dannyfpv · 2019-10-10T21:33:39Z

Tested on Pixhawk 4 v5 f450
Modes Tested:
Takeoff: no issues
RTL Mode:. no issues
Mission mode: no issues
Altitude Mode:. no issues
Stabilized Mode:. no issues
Landing: no issues

log: https://review.px4.io/plot_app?log=3de37f3e-4fb2-4287-8bf6-456b6331de1d

Tony3dr · 2019-10-10T21:53:05Z

@mcsauder let us know if you need further testing.

mcsauder · 2019-10-10T22:04:02Z

Thank you so much @Tony3dr , @dannyfpv ,and @Junkim3DR !! I appreciate it very much.

My flight testing also showed good results, correct land detection 5 of 5 on a 250 quad with pixhawk 4 mini:

@MaEtUgR , this PR should be good to go!

bkueng · 2019-10-11T08:15:40Z

src/modules/land_detector/LandDetector.cpp

@@ -71,15 +63,16 @@ LandDetector::~LandDetector()

 void LandDetector::start()
 {
-	_check_params(true);
+	_force_param_update = true;


Why do you use a class attribute here? It adds another state, whereas passing an argument would be simpler.

Hi @bkueng , I changed it to meet your suggestion and flight testing is good: https://review.px4.io/plot_app?log=a7b84e5b-14dc-4324-a324-986aac6873dc

Initially I had overloaded the method to allow inheritance from the base class while preserving the original behavior. This was changed after feedback above to use a class variable. As it stands now, the MulticopterLandDetector class doesn't use the argument, but I don't see any issues with that.

Thanks for the feedback!

MaEtUgR

Thanks for fixing, now it's much clearer.

Consolidate _update_params() methods for improved inheritance from the LandDetector base class. Move common uORB::Subscriptions to the base class for inheritance. Deprecate redundant override methods.

mcsauder · 2019-10-14T17:31:02Z

Hi @MaEtUgR , the CI tools failed after the lastest master branch merge, so I rebased, squashed commits, and updated the commit comments to better reflect the changes that have occurred. No changes to the code, only the commit comments and squash.

Hopefully CI is happy this round!

UPDATE: Here is a quick flight test, everything looks good. The flight was a few minutes of hovering, climb to altitude, then descent from altitude. (I bumped the mode switch to alt on the way down, so that blip in the modes was my fault but doesn't impact the test.)

MaEtUgR

@mcsauder Awsome, thanks a lot! CI is happy. I would say it's ready.

mcsauder · 2019-10-14T19:39:46Z

Thanks @MaEtUgR ! I am excited to see this one through to the end!

I appreciate everyone's reviews and all of your feedback!

bkueng · 2019-10-16T14:34:00Z

src/modules/land_detector/MulticopterLandDetector.cpp

 {
+	LandDetector::_update_params(force);
+
 	_freefall_hysteresis.set_hysteresis_time_from(false, (hrt_abstime)(1e6f * _param_lndmc_ffall_ttri.get()));

 	param_get(_paramHandle.minThrottle, &_params.minThrottle);


@mcsauder this is not correct, it re-loads the params every cycle now, as there is no check for _parameter_update_sub here. You can move the _parameter_update_sub.updated() check out of LandDetector::_update_params around the _update_params(); in LandDetector::Run() (and get rid of the force argument).
(Alternatively you can rename this method here to void updateParams() override;, and then make LandDetector::_update_params non-virtual)
There also needs to be a call to updateParams() in LandDetector::_update_params.

Hi @bkueng , I might be mistaken, but with the default argument force = false, I don't think that passing the force argument from MulticopterLandDetector::_update_param(const bool force = false) will result in re-loading the params every cycle. It would only result in forcing a reload if somewhere else this method is being called with a true argument, which doesn't happen anywhere I am able to find. Let me know if I'm not seeing things correctly.

Ah, I think I see that your concern is with the additional 4 param_get() calls. I'll work on getting a PR opened to address that.

mcsauder force-pushed the vehicle_imu_PR_land_detector_cherry_pick branch 7 times, most recently from 2a7244c to b408a98 Compare April 24, 2019 16:26

mcsauder force-pushed the vehicle_imu_PR_land_detector_cherry_pick branch from b408a98 to 103033b Compare April 25, 2019 18:52

dagar self-requested a review April 29, 2019 00:39

dagar added this to the Release v1.10.0 milestone Apr 29, 2019

mcsauder force-pushed the vehicle_imu_PR_land_detector_cherry_pick branch 5 times, most recently from 15dd715 to c70b933 Compare May 2, 2019 15:54

weekly-digest bot mentioned this pull request May 5, 2019

Weekly Digest (28 April, 2019 - 5 May, 2019) #11965

Closed

mcsauder force-pushed the vehicle_imu_PR_land_detector_cherry_pick branch 3 times, most recently from 04a7062 to 14526c7 Compare May 10, 2019 04:02

weekly-digest bot mentioned this pull request May 12, 2019

Weekly Digest (5 May, 2019 - 12 May, 2019) #12005

Closed

mcsauder force-pushed the vehicle_imu_PR_land_detector_cherry_pick branch 2 times, most recently from d085b2d to f4bf37b Compare May 14, 2019 14:45

weekly-digest bot mentioned this pull request May 19, 2019

Weekly Digest (12 May, 2019 - 19 May, 2019) #12043

Closed

mcsauder force-pushed the vehicle_imu_PR_land_detector_cherry_pick branch from f4bf37b to 0d2919f Compare May 20, 2019 00:04

weekly-digest bot mentioned this pull request May 26, 2019

Weekly Digest (19 May, 2019 - 26 May, 2019) #12076

Closed

mcsauder force-pushed the vehicle_imu_PR_land_detector_cherry_pick branch 2 times, most recently from 51d6d7d to cd63d24 Compare May 31, 2019 21:39

weekly-digest bot mentioned this pull request Jun 2, 2019

Weekly Digest (26 May, 2019 - 2 June, 2019) #12143

Closed

mcsauder force-pushed the vehicle_imu_PR_land_detector_cherry_pick branch from cd63d24 to 36aede3 Compare June 4, 2019 21:04

TSC21 previously approved these changes Oct 10, 2019

View reviewed changes

bkueng reviewed Oct 11, 2019

View reviewed changes

mcsauder dismissed TSC21’s stale review via 4319ad5 October 11, 2019 22:25

MaEtUgR previously approved these changes Oct 12, 2019

View reviewed changes

MaEtUgR mentioned this pull request Oct 12, 2019

Land Detector Properties instead of one State #13170

Merged

weekly-digest bot mentioned this pull request Oct 13, 2019

Weekly Digest (6 October, 2019 - 13 October, 2019) #13171

Closed

Cherry pick the directory and voted_sensors_update.h from PR PX4#9756.

adb10a1

Consolidate _update_params() methods for improved inheritance from the LandDetector base class. Move common uORB::Subscriptions to the base class for inheritance. Deprecate redundant override methods.

mcsauder dismissed MaEtUgR’s stale review via adb10a1 October 14, 2019 17:28

mcsauder force-pushed the vehicle_imu_PR_land_detector_cherry_pick branch from 2eab329 to adb10a1 Compare October 14, 2019 17:28

MaEtUgR approved these changes Oct 14, 2019

View reviewed changes

MaEtUgR merged commit fb12ddb into PX4:master Oct 14, 2019

mcsauder deleted the vehicle_imu_PR_land_detector_cherry_pick branch October 14, 2019 19:40

mcsauder mentioned this pull request Oct 14, 2019

Use return INFINITY for FixedwingLandDetector::_get_max_altitude(). #12343

Merged

bkueng reviewed Oct 16, 2019

View reviewed changes

mcsauder mentioned this pull request Oct 16, 2019

Multicopter land detector: do not update parameters every cycle #13212

Merged

weekly-digest bot mentioned this pull request Oct 20, 2019

Weekly Digest (13 October, 2019 - 20 October, 2019) #13236

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cherry pick the `src/modules/land_detector/` directory work from PR #9756 #11874

Cherry pick the `src/modules/land_detector/` directory work from PR #9756 #11874

mcsauder commented Apr 17, 2019 •

edited

Loading

mcsauder commented Jun 4, 2019

mcsauder commented Oct 10, 2019

dannyfpv commented Oct 10, 2019 •

edited by Tony3dr

Loading

Junkim3DR commented Oct 10, 2019

dannyfpv commented Oct 10, 2019 •

edited by Tony3dr

Loading

Tony3dr commented Oct 10, 2019

mcsauder commented Oct 10, 2019

bkueng Oct 11, 2019

mcsauder Oct 11, 2019

MaEtUgR left a comment

mcsauder commented Oct 14, 2019 •

edited

Loading

MaEtUgR left a comment

mcsauder commented Oct 14, 2019

bkueng Oct 16, 2019

mcsauder Oct 16, 2019

mcsauder Oct 16, 2019

Cherry pick the src/modules/land_detector/ directory work from PR #9756 #11874

Cherry pick the src/modules/land_detector/ directory work from PR #9756 #11874

Conversation

mcsauder commented Apr 17, 2019 • edited Loading

mcsauder commented Jun 4, 2019

mcsauder commented Oct 10, 2019

dannyfpv commented Oct 10, 2019 • edited by Tony3dr Loading

Junkim3DR commented Oct 10, 2019

Tested on Pixhawk 4mini v5:

dannyfpv commented Oct 10, 2019 • edited by Tony3dr Loading

Tony3dr commented Oct 10, 2019

mcsauder commented Oct 10, 2019

bkueng Oct 11, 2019

Choose a reason for hiding this comment

mcsauder Oct 11, 2019

Choose a reason for hiding this comment

MaEtUgR left a comment

Choose a reason for hiding this comment

mcsauder commented Oct 14, 2019 • edited Loading

MaEtUgR left a comment

Choose a reason for hiding this comment

mcsauder commented Oct 14, 2019

bkueng Oct 16, 2019

Choose a reason for hiding this comment

mcsauder Oct 16, 2019

Choose a reason for hiding this comment

mcsauder Oct 16, 2019

Choose a reason for hiding this comment

Cherry pick the `src/modules/land_detector/` directory work from PR #9756 #11874

Cherry pick the `src/modules/land_detector/` directory work from PR #9756 #11874

mcsauder commented Apr 17, 2019 •

edited

Loading

dannyfpv commented Oct 10, 2019 •

edited by Tony3dr

Loading

dannyfpv commented Oct 10, 2019 •

edited by Tony3dr

Loading

mcsauder commented Oct 14, 2019 •

edited

Loading