Copter: reload takeoff cmd on arm when using AllowTakeOffWithoutRaisi…

[khancyr]

…ngThrottle

USING AUTO_OPTIONS 135 we set auto mode and arm from it if we got a takeoff cmd as first nav cmd. Doing this, the takeoff cmd is loaded when we enter the auto mode and set the target alt relatively to origin https://github.com/ArduPilot/ardupilot/blob/master/ArduCopter/mode_auto.cpp#L343 This is never called again. So when we reset the home from the first arming, we don't update the target alt relativly to origin. This leads to a wrong takeoff alt settle due to the alt drift on ground.

[rmackay9]

Hi @khancyr,

Thanks very much for finding this issue. Could you provide steps to reproduce the issue in SITL?

I'd much prefer if the logic to handle this was within the _AutoTakeoff class (e.g. takeoff.cpp) and let's avoid reloading the mission command if at all possible. I think we should be able to handle this at a lower level.. a more precise fix. In particular if you look at _AutoTakeoff::run() it already has handling for when copter.ap.land_complete is true so may be in this area? I'm not totally sure but doing a complete reset by reloading the mission command seems like too large a change and I worry about future knock-on effects.

[khancyr]

@rmackay9 I don't know yet how to reproduce this on SITL. This need to offset the current position alt before taking off. On a real drone, it is quite simple as the baro drift on ground will do this.

I can probably move the logic into the takeoff class. But I need to check back the impact on the other other mode using the takeoff cmd. About reloading the takeoff cmd, unfortunately, I don't think we can avoid it for now. We need the information of the target takeoff altitude and in what frame it is. Those two info are only in the mission msg. That is also how it is done when we are using the MISSION_START cmd. We do a resume() or start() that reload the mission

[IamPete1]

I think the fundamental mistake is that were taking off relative to origin altitude. Because we make the conversion from whatever alt frame when the command it started if home vs origin changes we take off to the wrong alt. Instead we should take the location object into the auto take off controller and calculate the altitude at run time.

[rmackay9]

@IamPete1,

You may be right but it may be that we are just using the takeoff altitude as a distance to climb. Because the horizontal position doesn't change, the frame may not matter.

@khancyr, sorry to be pushy but there should be a way to do this without re-loading the command. Reloading in response to a MISSION_START makes total sense but doing it down in the auto takeoff code is something I'd like to avoid.

[khancyr]

Will look how to save all infos cleanly

[khancyr]

updated with takeoff location storage.

To reproduce on SITL. create a mission with a takeoff altitude as a relative altitude (in the example 50m). set AUTO_OPTIONS 135

Wait for home, then move it to some higher altitude : example long DO_SET_HOME 0 0 0 0 53.34642031503822 -7.714834673686349 90

Change to auto Change back the home to the correct initial position (with mavproxy , click set home (with height)) arm throttle

module load graph : graph GLOBAL_POSITION_INT.relative_alt/1000

MASTER

PR

[peterbarker]

We've discussed this before at DevCall.

Plane takes off above-current-altitude, but there's no appropriate mavlink frame for that.

I've created this for discussion: https://github.com/ArduPilot/mavlink/pull/358

[tridge]

probably worth adding a test for this, and checking the test fails without the PR and passes with

[khancyr]

Added autotest. added a small change on type + cast that reduce flash usage

[tridge]

@khancyr needs python tidy for CI pass (Tools/scripts/run_flake8.py ?)

[tridge]

@rmackay9 will take a look

[khancyr]

@rmackay9 changes done

[rmackay9]

Hi @khancyr,

Sorry for taking so long to review this.

I've reproduced the issue that you've seen and looked at the solution and while it fixes the problem I think it would be better architecturally to move the fix into the _AutoTakeoff object. The changes would include something like this:

• add new members which holds the target altitude and frame (the equivalent of the "Location takeoff_loc" you've added in this PR). Personally I would make that an target_alt_cm and target_alt_type but I guess we could use a Location but we would not use the lat,lon values.
• add a new method called something like update_complete_alt() which updates complete_alt_cm and terrain_alt from the above targets
• call the update_complete_alt() method from within _AutoTakeoff::run() when "if (!motors->armed() || !copter.ap.auto_armed) {"

What do you think?

[khancyr]

@rmackay9 I am ok with the idea to move the check into the _AutoTakeoff::run(). But this will be significant change in the code flow and it also impact Guided mode, this will lead to more testing need and write up more autotest.

I think it will hold this issue for too long until we got something merged. I would conter propose to get this current fix if you agree with it and open an issue for the upgraded version and assign me to it.

[rmackay9]

Hi, I really think with a bit more effort we could fix this in the way I've described above. Sorry!

[khancyr]

Closing then as I cannot spend more time on this. Thanks