-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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.
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
Takeoff Revision #12014
Takeoff Revision #12014
Conversation
CI was very useful and found some initialization and offboard issues. I fixed them. |
0901160
to
813f5e7
Compare
bool FlightTaskManualAltitude::_checkTakeoff() | ||
{ | ||
// stick is is deflected up | ||
return _sticks(2) > 0.65f; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in this case the flag want_to_takeoff
is always true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but it gets ignored when you're already flying.
Since the takeoff logic is anyway moved into flighttask, another alternative is to let flighttask know about the landing state. Then the takeoff-logic does no longer be split between flighttask- and mc-pos-control-main file. |
844d980
to
7d49c6d
Compare
@Stifael Thanks a lot for having a look and the valuable input! Note that the thrust ramp I'm currently testing now has the correct coordinate frame. It's a good idea to inform the task of the takeoff state since we can then probably get rid of the reactive problems. I'm continuing to further improve the takeoff on this branch since #12001 is the minimum viable solution release blocker. |
I rebased on the hotfixes #12001 (comment) I'm working on a takeoff class that has a state machine for the vehicle to stage by stage pass through (credit to @RomanBapst for that part!) and does a nice thrust ramp. Here's an example takeoff landing with the jerk limited trajectory auto implementation (default): EDIT: Works good in SITL with auto and manual in all the cases I came up with. I'll test it on a real vehicle tomorrow. |
2f6c03a
to
3876f63
Compare
3876f63
to
4b6ad6c
Compare
Real world testing revealed a problem. Please hold back of any real-world testing until I fixed them. |
There were two rather confusing function calls one to check if smooth takeoff needs to be ran and one that updates it. I combined them and documented the interface correctly making the parameters non-reference const floats.
The comments and variable names were partly misleading. I grouped all members, hopefully gave them more understandable names and adjusted the comments.
- start from a velocity setpoint pushing into the ground to ramp from idle thrust up. - start with a bit higher velocity setpoint threshold to make sure the vehicle has a chance to really get off the ground. - calculate ramp slope from initialization setpoint to the desired one instead from zero to the desired. this ramps up quicker when you demand a very small upwards velocity like the AutoLineSmoothVel and ManualPositionSmoothVel tasks do at the beginning. - don't stay in the takeoff ramp depending on the land detector, this is unnecessary.
according to @dagar's review comment.
After boot the user is in manual mode and if he has an RC but doesn't switch out the thrust gets set to the throttle stick position. When he then starts a takeoff from tablet the thrust is still high while arming and the land detector immediately sees a takeoff skiping smooth takeoff from the position controller.
The initial idea of the flight task architecture was that a task can freely set it's setpoints and doesn't have to worry about takeoff and landing. It would just takeoff when it's landed and there's a setpoint to go up and land when it puts a setpoint that pushes into the ground. With the takeoff logic there are some significant interface problems depending on the way a task is implemented: From the setpoint is not high enough to trigger to an unexpected takeoff because of some estimator fluctuation affecting the setpoint. It's easiest to solve this by allowing the task to determine when a takeoff is triggered. If no condition is implemented by default a task is not allowing a takeoff and cannot be used to get the vehicle off the ground.
…activate z axis with downward velocity to takeoff smoothly
The velocity ramp had problems with: - different vehicle tunings resulted in the start value of the resulting thrust ramp staring either higher and lower than zero thrust. lower -> delay of beginning higher -> small jump at beginning - when a task set position and velocity at the same time during takeoff (which AutoSmoothVel does) it resulted in a velocity setpoint jump at the end of the ramp because the additional velocity setpoint correction from the position controller was not considered. The thrust ramp should now be very deterministic: - always start at zero - always end at the curreant thrust setpoint output of the complete position controller
In a deterministic way with clear states to go through.
There are two local_position_setpoint in the position controller. One describing the setpoint the task gives to the position controller and a second one with the output of the position controller. I corrected the wrong one during takeoff because the new takeoff thrust ramp runs after the controller and not before.
64e0d86
to
dfe355f
Compare
Awesome, thanks! |
I wasn't entirely pleased with the pure thrust ramp because for some part of the ramp up time the vehicle ramps nicely but at some point the drone accelerates up quite quickly. Ramping the thrust while the vehicle is still on ground makes sense since it's not yet moving up anyways but as soon as the vehicle left the ground ramping further to reach the current thrust setpoint results in linearly ramping the acting vertical acceleration and hence the vertical velocity goes up quadratic. This rapid increase in velocity is audible and visible and looks like an aggressive maneuver just after leaving the ground. The velocity ramp results in a thrust ramp while the vehicle is still on the ground (zero velocity) but as soon as the vehicle is moving and the estimated vertical velocity changes the controller takes care of not shooting up but rather going up with constant acceleration until the takeoff speed is met. Early this morning I had good ideas how to resolve the issues we faced with the velocity ramp and implemented them in the last commit.
@bresch @RomanBapst Since I don't have a vehicle at hand does aynone dare to test this on the real vehicle? I tested in SITL and it all looks good. I also checked that the only horizontal thrust leading to 90 desired tilt which screwed me yesterday is not happening anywhere. |
Will test! |
I updated the description such that we are all on the same page, sorry for not doing that earlier. The test of the last commit is still pending. |
@MaEtUgR I will get to test this either today or tomorrow. |
@RomanBapst Keep us posted! The last item blocking the release! |
I'm looking for a hole in the rain to get this tested for the entire day already but there's no chance 🌧 |
But fix the two crucial problems: - When to begin the ramp? There's a calculation now for the velocity ramp initial value such that the resulting thrust is zero at the beginning. - When to end the ramp? The ramp is applied to the upwards velocity constraint and it just ramps from the initial value to the velocity constraint which is applied during flight. Slower/going down is always possible.
11cdea4
to
0ace048
Compare
Indoor flight test on Pixhawk 2 Cube V3Modes Tested Takeoff: Good Pre-flight Procedure: Flight Test: Notes: Armed and took off in stabilized mode, flew for 30 seconds approximately then proceeded to land and disarm. This process was repeated one after another to a total of 5 take offs.Flight log: Indoor flight test on PixRacer V4Modes Tested Takeoff: Good Pre-flight Procedure: Flight Test: Notes: Armed and took off in stabilized mode, flew for 30 seconds approximately then proceeded to land and disarm. This process was repeated one after another to a total of 5 take offs.Flight log: 1 Flight log: 2 |
Tested on Pixhawk 4 v5 Takeoff: Good Flight Test: Notes: Armed and took off in stabilized mode, flew for 30 seconds approximately then proceeded to land and disarm. This process was repeated one after another to a total of 5 take offs. Logs: https://review.px4.io/plot_app?log=025c0d46-cbd5-4fe4-b735-1d7aa25cd17d |
When it was not raining for a moment @RomanBapst and I went out and did a couple of tests: Generally the takeoff worked really well. We found two remaining smaller issues:
|
This reverts commit 0c81a19.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MaEtUgR Overall great job! I just have a few minor comments mostly about comments and naming.
_trajectory[i].reset(0.f, 0.f, _position(i)); | ||
} | ||
|
||
_trajectory[2].reset(0.f, 0.7f, _position(2)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was added to work nicer with the takeoff logic but should actually not matter anymore. We can do tests without it and see if it matters but I would suggest to just leave it for now.
Describe problem solved by the proposed pull request
Addressed Problems:
Describe your preferred solution
generateInitialValue()
in the code._takeoff.updateRamp(_dt, constraints.speed_up)
where the constraint is now the goal value of the ramp.It's solved by allowing the task to determine when a takeoff is triggered. If no condition is implemented by default a task is not allowing a takeoff and cannot be used to get the vehicle off the ground.
See the
constraints.want_takeoff
flag determined by the task to allow a takeoff, it's ignored in flight.Test data / coverage
I SITL tested and real world tested all the changes in between and that's also how I found all the unexpected additional problems. The last commit is not real world tested yet so that needs to be carefully done. I'll update when I arrived to test it.
Additional context
old pr to fix for smooth takeoff: #12001