Fix Smooth Takeoff #12001
Fix Smooth Takeoff #12001
Conversation
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.
MaEtUgR
changed the title
|
@MaEtUgR Cool, thanks for looking into this. Let me know when you want a review. |
The comments and variable names were partly misleading. I grouped all members, hopefully gave them more understandable names and adjusted the comments.
MaEtUgR
force-pushed
the
fix-smooth-takeoff
branch
from
17f0e70 to
824d07a
Compare
- 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.
MaEtUgR
force-pushed
the
fix-smooth-takeoff
branch
from
2533e97 to
7f2da66
Compare
MaEtUgR
force-pushed
the
fix-smooth-takeoff
branch
from
d231926 to
866f2be
Compare
|
@RomanBapst Thanks for the offer. I brought the basic functionality back and corrected some details such that it also works with the ...SmoothVel tasks. The important changes are in the third commit and the explanation in the commit message. Feel free to ask back. I tested in SITL which shows me that it does what it should but not how the real world takeoff user experience is: |
|
Only mildly related: What I actually spent most time debugging is a probably in reality very rare corner case where smooth takeoff is skipped:
In this case the smooth takeoff is completely skipped because:
Possible not so nice hotfix for this here: https://github.com/PX4/Firmware/compare/hotfix-manual-high-thrust-on-takeoff (EDIT: now contained in this pr) |
|
@PX4/testflights The goal of this PR is to have a good user experience when taking off from the ground in all possible position and altitude controlled modes.
|
|
@bresch Thanks for your analysis! |
|
Tested on PixRacer V4: Modes Tested: Procedure: Notes: Flight log on Position: Flight log Altitude: Flight log Stabilized: Tested on PixRacer V4 Modes Tested Takeoff: Good Pre-flight Procedure: Flash firmware through QGC.OK Flight Test: Notes: Took off in Position mode, after flying for approximately one minute, switched to Altitude Mode, then stabilized mode proceeds to switch to mission plan mode then make sure that vehicle follows all waypoints as shown in QGC, once completed all waypoints activate RTL and see landing behavior.No issues noted good flight in general. The vehicle behaved as expected. Flight log FULL Flight test: |
Tested on Pixhawk Pro v4ProModes Tested:
Procedure: Notes: Logs:
Tested on Pixhawk 4Mini v5Modes Tested:
Procedure: Notes: Logs:
|
|
Tested on Pixhawk 4 v5 Position Mode: no issue Procedure: Notes: Logs: Stabilized: https://review.px4.io/plot_app?log=c624835e-4141-4564-a706-72b75749af5c Altitude: https://review.px4.io/plot_app?log=18b11371-0ef8-4f68-bd23-e44a9caf7f4c Position: https://review.px4.io/plot_app?log=01c85f31-82a4-4e88-a710-e4d399b6ea0f Full flight test: https://review.px4.io/plot_app?log=7386dd11-f307-4ad2-b614-93e477695c91 |
| // smooth takeoff vertical velocity ramp state | ||
| bool _in_takeoff_ramp = false; /**< true while takeoff ramp is applied */ | ||
| bool _position_triggered_takeoff = false; /**< true if takeoff was triggered by a position setpoint */ | ||
| float _takeoff_ramp_velocity = -1.f; /**< current value of the smooth takeoff ramp */ |
There was a problem hiding this comment.
Just a note and not more: to me velocity agrees with the coordinate convention (vs speed, which is always positive). Hence, with that convention in mind, _takeoff_ramp_velocity of -1 means that it would start with 1m/s upward.
There was a problem hiding this comment.
Good point, I should have switched to the proper coordinate convention. Noted to do in the next iteration since this is a release blocker. 👍
There was a problem hiding this comment.
When #12014 is finished the coordinate frame will be correct.
|
Can someone test takeoffs in Offboard before this is merged? |
@okalachev is that something you could test quickly? |
| bool _in_takeoff_ramp = false; /**< true while takeoff ramp is applied */ | ||
| bool _position_triggered_takeoff = false; /**< true if takeoff was triggered by a position setpoint */ | ||
| float _takeoff_ramp_velocity = -1.f; /**< current value of the smooth takeoff ramp */ | ||
| float _takeoff_reference_z; /**< z-position when takeoff ramp was initiated */ |
There was a problem hiding this comment.
| float _takeoff_reference_z; /**< z-position when takeoff ramp was initiated */ | |
| float _takeoff_reference_z{0.0f}; /**< z-position when takeoff ramp was initiated */ |
|
@PX4/testflights Thanks a lot for the extended test coverage and feedback.
That's ok stabilized has no takeoff logic and stays raw manual input as it was. According to my tests this solution is acceptable, the smooth takeoff works again but it's not perfect. The remaining issues for the ramp itself (which also existed before!) are:
I would say the minimum viable takeoff is there and I'll work on fixing the remaining issues on #12014
I don't know how to test that. I just know that if the vehicle doesn't get off the ground in offboard CI fails (saved me in #12014 already) and since the conditions to takeoff are very similar to what they were for a long time I expect it to work exactly like the auto takeoff. |
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.
|
@dagar I fixed the zero initialization of your review comment. I also added the hotfix for the problem I described in the comment above: #12001 (comment) because I realized it's not a rare corner case but also happened to me in the real world testing.
I had the fix on my testing branch from that moment on and that showed me it's necessary for the release. I discussed with @bkueng, @RomanBapst (who found this exact problem before) and @bresch how to solve the issue and that the hotfix is viable because a proper solution requires more involved position controller changes. |
|
I went out testing again and am honestly not very happy with the takeoff user experience. It works but with the default velocity controller tuning the ramp starts too high in my tests and I also see this in the logs of the test team. Here are some of my logs: |
|
@MaEtUgR What is the next action item here? @bresch @RomanBapst Any ideas you want to contribute? |
|
Should have updated: #12014 is the follow up, it contains these changes + all the additional fixes. It's a different pr because initially I thought this minimal pr is enough to just fix what we had before but that turned out to be not the case. With only this PR
I addressed all of these in #12014 let me also update the description there. |
Describe problem solved by the proposed pull request
No takeoff in no flight task is smooth anymore.
fixes #12000 when it's finished
Describe your preferred solution
All tasks need to be able to take off with the smooth takeoff ramp.
The important changes besides some refactoring are:
Additional context
#11399