Implement ModuleParams inheritance and DEFINE_PARAMETERS in the LandDetector class #12209

mcsauder · 2019-06-07T18:38:36Z

Describe problem solved by the proposed pull request
This PR modifies the LandDetector class to inherit from ModuleParams and utilize the DEFINE_PARAMETERS() macro.

This PR also renames _armingSub->arming_sub and parameterSub->parameter_sub to match the most prevalent usage in the other classes.

Test data / coverage
Additional test data and flight logs will be added below.

Additional context
This PR advances the work in #9756 and #11874.

Please let me know if you have any feedback for me on this PR. Thanks!

-Mark

dagar · 2019-06-07T21:53:25Z

Why not go straight to ModuleParams if you're going to change this?
https://github.com/PX4/Firmware/blob/master/src/platforms/px4_module_params.h

mcsauder · 2019-06-07T23:26:56Z

Hi @dagar , I'd be happy to do that if that's a good direction. This PR is mostly trying to stay minimal while getting all of the land detector classes using the same design pattern. Would you want to do it in a 2-punt? This PR aligns the classes, I can submit a separate PR to move them all over to ModuleParams next unless you'd prefer me going that direction with this PR.

Thanks for the feedback! Let me know what you'd like to see!

julianoes · 2019-06-11T11:57:58Z

Works for me, thanks @mcsauder !

julianoes · 2019-06-11T11:59:08Z

src/modules/land_detector/LandDetector.cpp

 		_total_flight_time = ((uint64_t)flight_time) << 32;
-		param_get(_p_total_flight_time_low, (int32_t *)&flight_time);
+		param_get(_paramHandle.total_flight_time_low, (int32_t *)&flight_time);


Why is this duplicated with _update_params()?

Hi @julianoes , thanks for pointing that out. I think I don't actually understand what the _update_params() call at line 161 is doing right now in the current master branch, (not changed in this PR), as it appears to be doing nothing.

I've pushed up a commit to clean that up. Would you mind looking closely at that with me in the current master branch as well as this PR? Thanks!

The last commit I pushed up has an issue that shows up in flight testing. I am reverting it back momentarily.

The commit changing to ScheduleNow() caused the issue I saw. I've reverted back to my first commit, that uses ScheduleOnInterval() and flight tested.

Ok, I've got things resolved. Changing to ScheduleNow() required rescheduling in the Run() method. I don't know why ScheduleOnInterval() doesn't play well with JMavSim, something we might need to look into.

Most recent commit fixes issue 12190 and is flight tested here: https://review.px4.io/plot_app?log=70bc2060-629d-4b26-8b0f-b5d0bc00e9fb

mcsauder · 2019-06-11T15:35:58Z

Rebased on current master and re-flight tested: https://review.px4.io/plot_app?log=28669eb5-f8ab-494a-b3e1-deab987a4b17

mcsauder · 2019-06-12T00:59:51Z

Why not go straight to ModuleParams if you're going to change this?
https://github.com/PX4/Firmware/blob/master/src/platforms/px4_module_params.h

To do this, what is the correct substitution for param_set_no_notification()?

bkueng · 2019-06-14T09:50:19Z

To do this, what is the correct substitution for param_set_no_notification()?

https://github.com/PX4/Firmware/blob/master/src/platforms/px4_param.h#L131

mcsauder · 2019-06-18T18:40:01Z

Why not go straight to ModuleParams if you're going to change this?
https://github.com/PX4/Firmware/blob/master/src/platforms/px4_module_params.h

@dagar, done.

mcsauder · 2019-06-18T20:32:49Z

@dagar , @julianoes , and @bkueng ,

I have the merge conflicts cleaned up and the ModuleParams inheritance implemented and pushed up in this PR rebased and tested with today's master.

I validated that the total flight time is being set correctly by force saving the LND_FLIGHT_T_LO param to 4,284,967,295, (ten seconds below the maximum value), and LND_FLIGHT_T_HI to 0. I then armed the vehicle and allowed it to stay armed for more than 10 seconds. The results on the bench and a flight test produce this log with successful auto disarm on landing: https://review.px4.io/plot_app?log=fe84fee0-6f89-4614-aa9d-d7def8e0c50d

bkueng · 2019-06-24T11:11:51Z

src/modules/land_detector/LandDetector.h

 	hrt_abstime _takeoff_time{0};

-	perf_counter_t	_cycle_perf;
+	perf_counter_t _cycle_perf{(perf_alloc(PC_ELAPSED, "land_detector_cycle"))};


Suggested change

perf_counter_t _cycle_perf{(perf_alloc(PC_ELAPSED, "land_detector_cycle"))};

perf_counter_t _cycle_perf{perf_alloc(PC_ELAPSED, "land_detector_cycle")};

Fixed! Thanks for spotting that!

bkueng · 2019-06-24T11:15:20Z

src/modules/land_detector/LandDetector.h


-	void _check_params(bool force = false);


Why is force removed?

Hi Beat, thanks for your review! I removed force because check_params() is private, so we know nothing outside of the class is utilizing it, and the only place it now gets called is setting force to false, so it has become unneeded.

It needs to be called with force=true here: https://github.com/PX4/Firmware/pull/12209/files#diff-1fd2cab203a1ba1c362f2dd76a74542fL84. Otherwise _total_flight_time does not necessarily get initialized. This only worked because there were other param changes.

Hi Beat. So this is broken in current master? I have to admit that I'm a little confused, in the tests I show above only the LAND_FLIGHT_T_HI and LAND_FLIGHT_T_LO are changed, no others. It is also verifiable with no parameter chances in the flight logs below. I'll gladly revert this change though if it's important.

So this is broken in current master?

Yes. We always have other param changes at the end of a flight, which is why it worked.

Hi @bkueng. I pushed up a commit that calls _check_params(true) within the constructor so that it gets initialized once as used to be the case prior to 106ee28.

Let me know if you think this is appropriate, and otherwise, if it needs to be forced at each Run() call, we can deprecate the bool arg and the if statement so that _check_params() can be effectively an _update_params() call each iteration.

EDITED ABOVE - call moved to the constructor.

@bkueng , moving to the constructor absolutely breaks the detector. I am removing it and squashing that commit out of existence.

Hi @bkueng, placing the call back in the start() method works fine and I think will satisfy the concerns. Latest commit functions fine, flight tested here: https://review.px4.io/plot_app?log=cdc782c2-9999-4f49-a98a-2de786ce5c6f

Let me know if you have any other thoughts! Thanks again for your review!

src/modules/land_detector/LandDetector.h

mcsauder · 2019-06-25T01:53:58Z

@bkueng , re-tested after the latest commit and rebase against PX4:master, land detection is behaving as it does in PX4:master:
https://review.px4.io/plot_app?log=7a907eb7-dcff-42e9-9628-aa534a7ec8c1
https://review.px4.io/plot_app?log=33684687-2d06-4bd8-b7c5-6216187a43ac

- Reduce duplicate code in LandDetector _check_params() method. - Standardize naming cases. - Implement DEFINE_PARAMETERS() macro.

…nition to match current PX4:master.

… out _update_total_flight_time() method in the LandDetector class.

mcsauder changed the title ~~Implement the _paramHandle and _params structures, update_topics() and _update_params() methods in the LandDetector class.~~ Implement the _paramHandle and _params structures, update_topics() and _update_params() methods in the LandDetector class. Jun 7, 2019

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

Weekly Digest (2 June, 2019 - 9 June, 2019) #12219

Closed

mcsauder force-pushed the vehicle_imu_land_detector branch from f6cb6e0 to edc35a7 Compare June 11, 2019 00:21

mcsauder mentioned this pull request Jun 11, 2019

land_detector complains about timeout #12190

Closed

julianoes reviewed Jun 11, 2019

View reviewed changes

mcsauder force-pushed the vehicle_imu_land_detector branch 3 times, most recently from 4aec1da to 2332553 Compare June 11, 2019 15:35

mcsauder force-pushed the vehicle_imu_land_detector branch from 634ea18 to eebf15c Compare June 13, 2019 20:10

mcsauder force-pushed the vehicle_imu_land_detector branch from eebf15c to 0521a19 Compare June 14, 2019 14:35

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

Weekly Digest (9 June, 2019 - 16 June, 2019) #12290

Closed

mcsauder force-pushed the vehicle_imu_land_detector branch 3 times, most recently from 77d5c4d to 91b6c48 Compare June 18, 2019 18:37

mcsauder changed the title ~~Implement the _paramHandle and _params structures, update_topics() and _update_params() methods in the LandDetector class.~~ Implement ModuleParams inheritance and DEFINE_PARAMETERS in the LandDetector class Jun 18, 2019

mcsauder force-pushed the vehicle_imu_land_detector branch 2 times, most recently from 130851e to c8a1aff Compare June 18, 2019 19:22

mcsauder changed the title ~~Implement ModuleParams inheritance and DEFINE_PARAMETERS in the LandDetector class~~ [WIP] Implement ModuleParams inheritance and DEFINE_PARAMETERS in the LandDetector class Jun 18, 2019

mcsauder force-pushed the vehicle_imu_land_detector branch 2 times, most recently from d1c6f5d to 90d45d9 Compare June 18, 2019 20:09

mcsauder changed the title ~~[WIP] Implement ModuleParams inheritance and DEFINE_PARAMETERS in the LandDetector class~~ Implement ModuleParams inheritance and DEFINE_PARAMETERS in the LandDetector class Jun 18, 2019

mcsauder mentioned this pull request Jun 19, 2019

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

Merged

mcsauder force-pushed the vehicle_imu_land_detector branch from 90d45d9 to 1f37e67 Compare June 21, 2019 15:18

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

Weekly Digest (16 June, 2019 - 23 June, 2019) #12318

Closed

bkueng reviewed Jun 24, 2019

View reviewed changes

mcsauder force-pushed the vehicle_imu_land_detector branch 3 times, most recently from eb487df to 8b6788e Compare June 24, 2019 23:35

mcsauder added 3 commits June 25, 2019 15:42

Refactor the LandDetector class to

aabe8da

- Reduce duplicate code in LandDetector _check_params() method. - Standardize naming cases. - Implement DEFINE_PARAMETERS() macro.

Delete _measure_interval and redundant parentheses in LandDetector.h.

55d52c4

Revert changes to the check_params(const bool force) declaration/defi…

bf67344

…nition to match current PX4:master.

mcsauder force-pushed the vehicle_imu_land_detector branch 2 times, most recently from f4ad0f9 to 77e8671 Compare June 25, 2019 21:48

Add check_params(true) call to the LandDetector start() method. Break…

a0d3f51

… out _update_total_flight_time() method in the LandDetector class.

mcsauder force-pushed the vehicle_imu_land_detector branch from 6d8e015 to a0d3f51 Compare June 26, 2019 04:52

bkueng approved these changes Jun 26, 2019

View reviewed changes

bkueng merged commit d7cfebe into PX4:master Jun 26, 2019

mcsauder deleted the vehicle_imu_land_detector branch June 26, 2019 14:37

This was referenced Jun 27, 2019

Implement ModuleParams inheritance in the MultiCopterLandDetector class. #12356

Merged

Implement ModuleParams inheritance in the FixedwingLandDetector class. #12357

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement ModuleParams inheritance and DEFINE_PARAMETERS in the LandDetector class #12209

Implement ModuleParams inheritance and DEFINE_PARAMETERS in the LandDetector class #12209

mcsauder commented Jun 7, 2019 •

edited

Loading

dagar commented Jun 7, 2019

mcsauder commented Jun 7, 2019

julianoes commented Jun 11, 2019

julianoes Jun 11, 2019

mcsauder Jun 11, 2019

mcsauder Jun 11, 2019

mcsauder Jun 11, 2019

mcsauder Jun 11, 2019

mcsauder commented Jun 11, 2019

mcsauder commented Jun 12, 2019

bkueng commented Jun 14, 2019

mcsauder commented Jun 18, 2019 •

edited

Loading

mcsauder commented Jun 18, 2019

bkueng Jun 24, 2019

mcsauder Jun 24, 2019

bkueng Jun 24, 2019

mcsauder Jun 24, 2019

bkueng Jun 25, 2019

mcsauder Jun 25, 2019

bkueng Jun 25, 2019

mcsauder Jun 25, 2019 •

edited

Loading

mcsauder Jun 26, 2019

mcsauder Jun 26, 2019

mcsauder commented Jun 25, 2019

	perf_counter_t _cycle_perf{(perf_alloc(PC_ELAPSED, "land_detector_cycle"))};
	perf_counter_t _cycle_perf{perf_alloc(PC_ELAPSED, "land_detector_cycle")};

Implement ModuleParams inheritance and DEFINE_PARAMETERS in the LandDetector class #12209

Implement ModuleParams inheritance and DEFINE_PARAMETERS in the LandDetector class #12209

Conversation

mcsauder commented Jun 7, 2019 • edited Loading

dagar commented Jun 7, 2019

mcsauder commented Jun 7, 2019

julianoes commented Jun 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcsauder commented Jun 11, 2019

mcsauder commented Jun 12, 2019

bkueng commented Jun 14, 2019

mcsauder commented Jun 18, 2019 • edited Loading

mcsauder commented Jun 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcsauder Jun 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcsauder commented Jun 25, 2019

mcsauder commented Jun 7, 2019 •

edited

Loading

mcsauder commented Jun 18, 2019 •

edited

Loading

mcsauder Jun 25, 2019 •

edited

Loading