Implement ModuleParams inheritance in the MultiCopterLandDetector class. #12356

mcsauder · 2019-06-27T22:41:32Z

Describe problem solved by the proposed pull request
This PR implements ModuleParams inheritance in the MultiCopterLandDetector class in following #12209.

Test data / coverage
This PR was test flown on a 250 size quad with pixhawk 4 mini, all tests performed identical to current PX4:master branch land detect behaviors:

Additional context
This PR follows work in #12209 and streamlines remaining work for #9756.

@bkueng , @dagar . Please let me know if you have any questions on this PR. Thank you!

-Mark

mcsauder · 2019-06-27T22:57:58Z

@bkueng or @dagar, would you be willing to advise me on this fmu-v2 error?
error: 'MPC_LAND_SPEED' is not a member of 'px4::params'

UPDATED: I cannot compile fmu-v2_fixedwing or fmu-v2_rover on my local machine either. I am investigating. Thank you for your advice.

NOTE: This issue is occurring because the fixedwing build is not including the mc_pos_control module AND because land_detector_main.cpp IS including "MulticopterLandDetector.h"

This is an issue now because the usage of MPC_LAND_SPEED was previously encapsulated in the translation unit by being located in the *.cpp file. With this PR it fails because DEFINE_PARAMETERS is located in the (included) *.h file.

How can this be addressed?

dagar · 2019-06-28T14:29:40Z

This is because MPC_LAND_SPEED comes from the MC position controller, which isn't included in the fixedwing only builds.

Short term the easy workaround is to keep using the old API for that particular param. One idea longer term would be to restructure this so that each vehicle type has a land detector with all the common code moving to a library.

mcsauder · 2019-06-28T18:54:32Z

Short term the easy workaround is to keep using the old API for that particular param.

@dagar, I've implemented your suggestion, squashed commits, rebased against current upstream, and re-flight tested:

EDIT: All flight tests perform as expected, identical to the current master branch.

dagar · 2019-06-29T01:40:25Z

src/modules/land_detector/MulticopterLandDetector.cpp

+	if (_param_update_sub.update(&param_update)) {
+		_update_params();
+		_freefall_hysteresis.set_hysteresis_time_from(false, (hrt_abstime)(1e6f * _param_lndmc_ffall_ttri.get()));
+		_param_lndmc_rot_max.set(math::radians(_param_lndmc_rot_max.get()));


I suppose you can get away with this, but it seems kind of shady (remote possibility of accidentally committing the value). I would do the math::radians call at runtime to keep it simple.

You're right, that was wrong. Fixed, rebased against current master and squashed into one commit.

mcsauder · 2019-07-07T17:24:32Z

src/modules/land_detector/MulticopterLandDetector.cpp

@@ -213,11 +204,11 @@ bool MulticopterLandDetector::_get_maybe_landed_state()
 	}

 	// Next look if all rotation angles are not moving.
-	float maxRotationScaled = _params.maxRotation_rad_s * landThresholdFactor;
+	float maxRotationScaled = math::radians(_param_lndmc_rot_max.get()) * landThresholdFactor;


@dagar, the issue you highlighted is addressed here.

Flight test land detection works properly and the value checks out when queried: https://review.px4.io/plot_app?log=9c628e6e-b90e-4d1b-9308-e6b5a8cc15b5

mcsauder · 2019-08-06T22:23:51Z

Rebased on today's master and flight tested again, detection works as it should and the same as current master:

bkueng · 2019-08-07T07:43:04Z

src/modules/land_detector/MulticopterLandDetector.h

 	} _params{};

 	uORB::Subscription _actuator_controls_sub{ORB_ID(actuator_controls_0)};
 	uORB::Subscription _battery_sub{ORB_ID(battery_status)};
+	uORB::Subscription _param_update_sub{ORB_ID(parameter_update)};


Why do you need this? It is handled in the base class already.

Thanks @bkueng! Removed with upstream merge while resolving conflicts.

mcsauder · 2019-08-07T14:28:40Z

Re-flight tested after latest commit: https://review.px4.io/plot_app?log=487a2f0c-d3c1-4c86-8fd4-8d23361b5811

bkueng · 2019-08-08T07:28:46Z

src/modules/land_detector/MulticopterLandDetector.cpp

-	_params.maxRotation_rad_s = math::radians(_params.maxRotation_rad_s);
+	parameter_update_s param_update;
+
+	if (_param_update_sub.update(&param_update)) {


You don't need to check for this, or call _update_params(). It is handled and called from the base class.
You can also keep _param_update_sub private.

Great, thanks! That simplifies things. I pushed up that fix.

…ss for land_detector_params only.

…tiCopterLandDetector can use it, add #include that appears to have been stripped by latest upstream merge.

… the base class implementation.

mcsauder · 2019-08-08T15:04:01Z

Flight tested with latest commit and rebase against master: https://review.px4.io/plot_app?log=3b8762bc-156a-48a5-a5da-69c0425af7a4

mcsauder · 2019-08-09T06:48:53Z

Thank you @bkueng for your review and time spent! I appreciate it!

…uires PR PX4#12356.

…uires PR #12356.

…uires PR PX4#12356.

mcsauder mentioned this pull request Jun 27, 2019

Implement ModuleParams inheritance in the FixedwingLandDetector class. #12357

Merged

mcsauder force-pushed the multicopter_land_detector_module_params branch 3 times, most recently from 88fee3f to 416da5b Compare June 28, 2019 17:56

dagar reviewed Jun 29, 2019

View reviewed changes

mcsauder force-pushed the multicopter_land_detector_module_params branch from cd634c1 to d5d13e7 Compare June 29, 2019 02:59

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

Weekly Digest (23 June, 2019 - 30 June, 2019) #12374

Closed

mcsauder mentioned this pull request Jul 1, 2019

Implement ModuleParams inheritance in the VtolLandDetector class. #12379

Merged

mcsauder force-pushed the multicopter_land_detector_module_params branch 2 times, most recently from cdfa90f to b10f096 Compare July 7, 2019 17:22

mcsauder commented Jul 7, 2019

View reviewed changes

mcsauder force-pushed the multicopter_land_detector_module_params branch from b10f096 to e3d18aa Compare July 8, 2019 18:57

mcsauder mentioned this pull request Jul 10, 2019

[WIP] introduce new vehicle_imu message #9756

Closed

weekly-digest bot mentioned this pull request Jul 14, 2019

Weekly Digest (7 July, 2019 - 14 July, 2019) #12474

Closed

mcsauder force-pushed the multicopter_land_detector_module_params branch 2 times, most recently from 90784f9 to 31817d8 Compare July 16, 2019 18:22

weekly-digest bot mentioned this pull request Jul 21, 2019

Weekly Digest (14 July, 2019 - 21 July, 2019) #12528

Closed

mcsauder force-pushed the multicopter_land_detector_module_params branch from 31817d8 to 4b44ee6 Compare July 25, 2019 22:53

weekly-digest bot mentioned this pull request Jul 28, 2019

Weekly Digest (21 July, 2019 - 28 July, 2019) #12569

Closed

mcsauder force-pushed the multicopter_land_detector_module_params branch from 4b44ee6 to ba3dc67 Compare August 1, 2019 00:22

mcsauder force-pushed the multicopter_land_detector_module_params branch 2 times, most recently from f997b83 to df644a3 Compare August 6, 2019 17:41

bkueng reviewed Aug 7, 2019

View reviewed changes

bkueng reviewed Aug 8, 2019

View reviewed changes

mcsauder added 3 commits August 8, 2019 08:41

Implement ModuleParams inheritance in the MultiCopterLandDetector cla…

ed7baed

…ss for land_detector_params only.

Make LandDetector::_param_update_sub a protected variable so that Mul…

fba6da0

…tiCopterLandDetector can use it, add #include that appears to have been stripped by latest upstream merge.

Remove param_update_sub from MulticopterLandDetector class to utilize…

33efd99

… the base class implementation.

mcsauder force-pushed the multicopter_land_detector_module_params branch from 7551aab to 33efd99 Compare August 8, 2019 15:01

bkueng approved these changes Aug 9, 2019

View reviewed changes

bkueng merged commit 056c800 into PX4:master Aug 9, 2019

mcsauder deleted the multicopter_land_detector_module_params branch August 9, 2019 06:57

mcsauder added a commit to mcsauder/PX4-Autopilot that referenced this pull request Aug 9, 2019

Implement ModuleParams inheritance in the VtolLandDetector class. Req…

20b7c51

…uires PR PX4#12356.

weekly-digest bot mentioned this pull request Aug 11, 2019

Weekly Digest (4 August, 2019 - 11 August, 2019) #12682

Closed

dagar pushed a commit that referenced this pull request Aug 14, 2019

Implement ModuleParams inheritance in the VtolLandDetector class. Req…

9e055e9

…uires PR #12356.

bozkurthan pushed a commit to bozkurthan/Firmware that referenced this pull request Sep 4, 2019

MultiCopterLandDetector: Implement ModuleParams inheritance (PX4#12356)

3bebdb7

bozkurthan pushed a commit to bozkurthan/Firmware that referenced this pull request Sep 4, 2019

Implement ModuleParams inheritance in the VtolLandDetector class. Req…

ace0705

…uires PR PX4#12356.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement ModuleParams inheritance in the MultiCopterLandDetector class. #12356

Implement ModuleParams inheritance in the MultiCopterLandDetector class. #12356

mcsauder commented Jun 27, 2019

mcsauder commented Jun 27, 2019 •

edited

Loading

dagar commented Jun 28, 2019

mcsauder commented Jun 28, 2019 •

edited

Loading

dagar Jun 29, 2019

mcsauder Jul 7, 2019

mcsauder Jul 7, 2019

mcsauder Jul 7, 2019 •

edited

Loading

mcsauder commented Aug 6, 2019

bkueng Aug 7, 2019

mcsauder Aug 7, 2019

mcsauder commented Aug 7, 2019

bkueng Aug 8, 2019

mcsauder Aug 8, 2019

mcsauder commented Aug 8, 2019

mcsauder commented Aug 9, 2019

Implement ModuleParams inheritance in the MultiCopterLandDetector class. #12356

Implement ModuleParams inheritance in the MultiCopterLandDetector class. #12356

Conversation

mcsauder commented Jun 27, 2019

mcsauder commented Jun 27, 2019 • edited Loading

dagar commented Jun 28, 2019

mcsauder commented Jun 28, 2019 • edited Loading

dagar Jun 29, 2019

Choose a reason for hiding this comment

mcsauder Jul 7, 2019

Choose a reason for hiding this comment

mcsauder Jul 7, 2019

Choose a reason for hiding this comment

mcsauder Jul 7, 2019 • edited Loading

Choose a reason for hiding this comment

mcsauder commented Aug 6, 2019

bkueng Aug 7, 2019

Choose a reason for hiding this comment

mcsauder Aug 7, 2019

Choose a reason for hiding this comment

mcsauder commented Aug 7, 2019

bkueng Aug 8, 2019

Choose a reason for hiding this comment

mcsauder Aug 8, 2019

Choose a reason for hiding this comment

mcsauder commented Aug 8, 2019

mcsauder commented Aug 9, 2019

mcsauder commented Jun 27, 2019 •

edited

Loading

mcsauder commented Jun 28, 2019 •

edited

Loading

mcsauder Jul 7, 2019 •

edited

Loading