Adjustable land speed via RC #12220

dakejahl · 2019-06-09T07:58:04Z

Describe problem solved by the proposed pull request
This allows user input to assist in a quicker land during the land routine. This is enabled by a parameter which defaults to disabled.

Enable user assisted descent speed for autonomous land routine.
When enabled, descent speed will be equal to MPC_LAND_SPEED at half throttle,
MPC_Z_VEL_MAX_DN at zero throttle, and 0.5 m/s at full throttle.

Test data / coverage
Tested in sim using a Taranis X7 plugged in via USB (QGC Joystick)
https://review.px4.io/plot_app?log=b3208c83-0f28-4a43-ba85-0324b4f785be

Additional context
This is a feature DJI has and I have found it to be extremely convenient. Users switching from DJI to PX4 builds are not going to like losing features 😉

Edit: it would also be cool to be able to adjust the X/Y position during land. If anyone else thinks this is worth adding I can do that as well.

dakejahl · 2019-06-09T08:54:07Z

We might want a check for _sub_manual_control_setpoint->get().source == SOURCE_RC. What do you think @dagar?

dakejahl · 2019-06-11T17:56:05Z

Also it might be nice to make this a parameter that defaults to DISABLED, as people will probably have the throttle stick down while landing, and the increased descent speed would be very concerning if they were not aware of the fact that their stick positions are now contributing to the descent speed.

dakejahl · 2019-06-11T17:58:06Z

@LorenzMeier Do you have any comments/concerns? Do you think this is a feature worth pursuing?

dagar · 2019-06-11T19:51:44Z

This has come up before and high level I think it's a good idea, but we need to be extremely careful with the user interaction as well as failsafe edge cases (intermittent manual control with the stick in a weird position).

At a minimum I'd update it to respect the systems notion of manual control lost (vehicle_status.rc_signal_lost) and put it behind a parameter that's disabled by default (at least to start).

src/lib/FlightTasks/tasks/AutoMapper2/FlightTaskAutoMapper2.cpp

dagar · 2019-06-11T19:53:36Z

We might want a check for _sub_manual_control_setpoint->get().source == SOURCE_RC. What do you think @dagar?

Why treat RC differently than mavlink?

dakejahl · 2019-06-12T01:57:08Z

We might want a check for _sub_manual_control_setpoint->get().source == SOURCE_RC. What do you think @dagar?

Why treat RC differently than mavlink?

Yeah, that was my first thought. Nvm then.

dakejahl · 2019-06-12T01:58:12Z

At a minimum I'd update it to respect the systems notion of manual control lost (vehicle_status.rc_signal_lost) and put it behind a parameter that's disabled by default (at least to start).

rgr rgr

dakejahl · 2019-06-12T05:17:28Z

This is complete. Ready for final review. If you look at flight log from sim, I turned on/off the parameter as well as unplugged/replugged the RC. I probably won't make it to the dev call tomorrow.

…. Added a _getLandSpeed function the the FlightTaskAutoMapper2 class which uses the manual_control_setpoint topic to adjust landing descent speed based on user input

Co-Authored-By: Daniel Agar <[email protected]>

…status.rc_signal_lost. Added a parameter for this feature that defaults to disabled.

bresch · 2019-07-03T15:51:59Z

@PX4/testflights Can you test this PR in mission mode and set MPC_LAND_RC_HELP to 1.
During landing, try to move the throttle stick in order to modify the landing speed.

Junkim3DR · 2019-07-03T17:10:28Z

@bresch @dagar, the feature is not working at the moment.

Tested on Pixhawk 4 mini v5

Procedure

Changed parameter MPC_LAND_RC_HELP form 0 to 1
Arm and Take off in position mode, after flying for approximately one minute, trigger RTL, wait for vehicle to return, while landing test throttle stick up, middle and down, see vehicle behavior.

Notes
Vehicle landed as usual, we moved the throttle stick up then left it there for about 5 seconds, continued to middle position and left it there 5 seconds as well, same procedure for down position; vehicle kept its landing velocity and no change was noticed.

Log
https://review.px4.io/plot_app?log=396e2f3a-b372-47de-b63c-50dfeeac9692

Tested on Pixhawk Pro v4 and Pixhawk 4 v5 and same results.

bresch · 2019-07-03T20:24:56Z

@Junkim3DR , thanks for the tests. I checked the logs and it is actually working. The problem why you didn't see it is that this logic is only active in "land mode". During RTL, the drone is in land mode during the last 10 meters (RTL_DESCEND_ALT) only. Furthermore, since the maximum down speed is 1 (MPC_Z_VEL_MAX_DN) during full stick down, the land speed is 0.7 (MPC_LAND_SPEED) when stick centered and the lower speed is 0.5 (hardcoded in this PR) when stick up, the difference is not that visible.
It will be more noticeable if you increase MPC_Z_VEL_MAX_DOWN to at something like 2m/s.

Here is the plot showing that it worked in your test:

This is unfortunately the tiny last part of the flight (because it's only active during the last 10 meters):

bresch

Thanks for that nice feature!
The XY adjustments would also be something super nice. Currently we have to switch to manual position mode and is not as nice as being able to adjust the landing while in auto mode.

dakejahl · 2019-07-03T20:43:31Z

the lower speed is 0.5 (hardcoded in this PR) when stick up

Actually I think my comment in the parameter file is wrong, looks like I changed it to

		// Allow minimum assisted land speed to be half of parameter
		if (speed < land_speed * 0.5f) {
			speed = land_speed * 0.5f;
		}

What do you think is best? A fixed small value like 0.5? A number that is a function of the land speed like the current implementation? Maybe another parameter to set this?

I'll see what I can do for XY control, look for a PR soon.

bresch · 2019-07-04T05:17:02Z

Right, I think 0.5 x land speed is a reasonable choice. Can you update the parameter description please?

dakejahl · 2019-07-04T05:54:51Z

Done

bresch

All good now

dagar · 2019-07-05T21:22:45Z

Slightly over the fmu-v2 flash limit.

MaEtUgR · 2019-08-13T15:47:31Z

src/lib/FlightTasks/tasks/AutoMapper2/FlightTaskAutoMapper2.cpp

+		float land_speed = _param_mpc_land_speed.get();
+		float head_room = _constraints.speed_down - land_speed;
+
+		speed = land_speed + 2 * (0.5f - throttle) * head_room;


I don't want to offend at all but this implementation is quite hacky and needs improvement for sure.
Issues that come to my mind:

Stick subscription and evaluation is duplicate to https://github.com/PX4/Firmware/blob/e25db0162019e5f0bb7171ec6f3db2f456672d95/src/lib/FlightTasks/tasks/Manual/FlightTaskManual.cpp#L70-L115 but misses important checks and e.g. deadzone.

It's implemented in AutoMapper2 which duplicates big parts of AutoMapper so in some configurations this feature is not available.

We should support general correction of the land location by sticks not only adjusting the land speed.

No offense taken at all, I like critical feedback. I am pretty unfamiliar with FlightTasks in general, so indeed it may be hacky as I was unsure the correct spot to put this.

We should support general correction of the land location by sticks not only adjusting the land speed.

I've got a WIP branch open right now. My plan was to move the sticks stuff into the FlightTask base class so I could use _evaluateSticks

It's implemented in AutoMapper2 which duplicates big parts of AutoMapper so in some configurations this feature is not available.

I was very confused by the naming of these two. Without getting intimate with the code, it is very unclear what their roles/responsibilities are. I actually implemented it AutoMapper at first and then realized nothing was happening.

If you've got any suggestions or just random details in general don't hesitate to share. Thanks!

I've got a WIP branch open right now. My plan was to move the sticks stuff into the FlightTask base class so I could use _evaluateSticks

I'm quite sure by now that inheriting all the data processing in the flight tasks does not scale. We should pull e.g. the stick processing into a library such that not every task that uses sticks has to inherit from Manual. (The same of course also for other reusable logic.)

I was very confused by the naming of these two.

That's definitely far from your fault. They do almost completely the same thing but for later auto task with the "AutoSmoothVel" ending (which is default) there are some small differences. We need to get rid of that duplication, here's the issue for that: #11209. The existence of Automapper and having two versions is in my eyes partly caused by the inheritance structure.

@dakejahl JFYI the duplicate AutoMapper is gone since 7e78eed 🎉

@dakejahl JFYI the stick handling was moved to a library (#15324) and I have a pr open reusing it for the auto land use case you contributed: #15325

Sweet! Have others found it useful? Haha I never did get around to allowing XY control, it would be cool if it did

I think it doesn't get used that often because of RC override which by default makes it switch out of land when you move the sticks. We'll enable x, y control soon. We can still discuss how it should work together with the mode override.
btw I changed the behavior a bit to not have such a high downwards speed on the sticks. Is that a problem? We can maybe discuss in the dev call...

dakejahl added the Admin: Enhancement (improvement) 💡 label Jun 9, 2019

dakejahl assigned dagar Jun 9, 2019

dakejahl force-pushed the pr-assisted_land_speed branch 2 times, most recently from b6c6b1d to efc2617 Compare June 9, 2019 08:47

dakejahl requested a review from MaEtUgR June 9, 2019 19:00

dakejahl unassigned dagar Jun 9, 2019

dakejahl requested a review from dagar June 9, 2019 19:00

dagar added the Multirotor 🛸 label Jun 11, 2019

dagar reviewed Jun 11, 2019

View reviewed changes

src/lib/FlightTasks/tasks/AutoMapper2/FlightTaskAutoMapper2.cpp Outdated Show resolved Hide resolved

dakejahl and others added 4 commits June 15, 2019 19:23

Added a subscription to manual_control_setpoint to the FlightTaskAuto…

7b88030

…. Added a _getLandSpeed function the the FlightTaskAutoMapper2 class which uses the manual_control_setpoint topic to adjust landing descent speed based on user input

corrected formatting

c9b518a

add float literal

fdf53b6

Co-Authored-By: Daniel Agar <[email protected]>

Fixed calculation of descent speed. Replaced time check with vehicle_…

ebd7ddf

…status.rc_signal_lost. Added a parameter for this feature that defaults to disabled.

dakejahl force-pushed the pr-assisted_land_speed branch from 3617ff4 to ebd7ddf Compare June 16, 2019 01:24

This was referenced Jun 16, 2019

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

Closed

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

Closed

bresch previously approved these changes Jul 3, 2019

View reviewed changes

Merge branch 'master' into pr-assisted_land_speed

c1e75ea

Update parameter description

387e1b9

dakejahl dismissed bresch’s stale review via 387e1b9 July 4, 2019 05:53

bresch approved these changes Jul 5, 2019

View reviewed changes

Merge branch 'master' into pr-assisted_land_speed

115c954

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

Weekly Digest (30 June, 2019 - 7 July, 2019) #12430

Closed

bresch and others added 2 commits July 8, 2019 09:20

Merge branch 'master' into pr-assisted_land_speed

8081ed6

Merge branch 'master' into pr-assisted_land_speed

e045510

dagar approved these changes Jul 9, 2019

View reviewed changes

dagar merged commit 59c555a into master Jul 9, 2019

dagar deleted the pr-assisted_land_speed branch July 9, 2019 03:55

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

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

Closed

MaEtUgR reviewed Aug 13, 2019

View reviewed changes

MaEtUgR mentioned this pull request Jul 14, 2020

FlightTasks: Stick library for auto land #15325

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adjustable land speed via RC #12220

Adjustable land speed via RC #12220

dakejahl commented Jun 9, 2019 •

edited

Loading

dakejahl commented Jun 9, 2019

dakejahl commented Jun 11, 2019 •

edited

Loading

dakejahl commented Jun 11, 2019

dagar commented Jun 11, 2019

dagar commented Jun 11, 2019

dakejahl commented Jun 12, 2019

dakejahl commented Jun 12, 2019

dakejahl commented Jun 12, 2019 •

edited

Loading

bresch commented Jul 3, 2019

Junkim3DR commented Jul 3, 2019 •

edited

Loading

bresch commented Jul 3, 2019

bresch left a comment

dakejahl commented Jul 3, 2019

bresch commented Jul 4, 2019

dakejahl commented Jul 4, 2019

bresch left a comment

dagar commented Jul 5, 2019

MaEtUgR Aug 13, 2019

dakejahl Aug 13, 2019

MaEtUgR Oct 21, 2019 •

edited

Loading

MaEtUgR Feb 19, 2020 •

edited

Loading

MaEtUgR Jul 15, 2020

dakejahl Jul 15, 2020

MaEtUgR Jul 15, 2020 •

edited

Loading

Adjustable land speed via RC #12220

Adjustable land speed via RC #12220

Conversation

dakejahl commented Jun 9, 2019 • edited Loading

dakejahl commented Jun 9, 2019

dakejahl commented Jun 11, 2019 • edited Loading

dakejahl commented Jun 11, 2019

dagar commented Jun 11, 2019

dagar commented Jun 11, 2019

dakejahl commented Jun 12, 2019

dakejahl commented Jun 12, 2019

dakejahl commented Jun 12, 2019 • edited Loading

bresch commented Jul 3, 2019

Junkim3DR commented Jul 3, 2019 • edited Loading

Tested on Pixhawk 4 mini v5

bresch commented Jul 3, 2019

bresch left a comment

Choose a reason for hiding this comment

dakejahl commented Jul 3, 2019

bresch commented Jul 4, 2019

dakejahl commented Jul 4, 2019

bresch left a comment

Choose a reason for hiding this comment

dagar commented Jul 5, 2019

MaEtUgR Aug 13, 2019

Choose a reason for hiding this comment

dakejahl Aug 13, 2019

Choose a reason for hiding this comment

MaEtUgR Oct 21, 2019 • edited Loading

Choose a reason for hiding this comment

MaEtUgR Feb 19, 2020 • edited Loading

Choose a reason for hiding this comment

MaEtUgR Jul 15, 2020

Choose a reason for hiding this comment

dakejahl Jul 15, 2020

Choose a reason for hiding this comment

MaEtUgR Jul 15, 2020 • edited Loading

Choose a reason for hiding this comment

dakejahl commented Jun 9, 2019 •

edited

Loading

dakejahl commented Jun 11, 2019 •

edited

Loading

dakejahl commented Jun 12, 2019 •

edited

Loading

Junkim3DR commented Jul 3, 2019 •

edited

Loading

MaEtUgR Oct 21, 2019 •

edited

Loading

MaEtUgR Feb 19, 2020 •

edited

Loading

MaEtUgR Jul 15, 2020 •

edited

Loading