Plane: Add climb slope minimum height parameter #29077
Conversation
|
needs proper library commit splits |
|
Probably needs parameter conversion for the changed params no? https://ardupilot.org/dev/docs/code-overview-adding-a-new-parameter.html#parameter-conversion-expiration |
|
This seems to make sense, what do you think @Ryanf55? |
Many thanks for the contribution! I like the naming change on first glance, perhaps that's worth splitting to it's own PR? The other change also makes sense. If we could get some exact steps to reproduce the poor behavior in SITL, so I can see a before/after from this PR, that would be fantastic. Some recommendation on which plots you are looking at would be helpful. At least for me, who's less experienced, understanding what the steps you take to reproduce the behavior and what graphs/logs you check are very useful. |
Before and after logs are included in the PR message: the difference can be seen in any altitude plot, where with master we get an undesired steep climb up to the final altitude of the problematic leg, and with this PR we only get a small steep climb up to the "safe height" (CLMB_SLOPE_HGT) and then we resume the expected slope. You can replicate them by running the flight plan in SITL with default params. It's worth noting that this wouldn't always happen in master as it depends on the height of the aircraft upon entering the leg, as can be seen in the log provided where a leg that's identical in terms of altitude difference is performed correctly. |
Maybe I'm missing something, but I don't think so? Given that it's just a name change and the place in the eeprom hasn't changed. So far I've been able to change between master and this PR with the values carrying over no problem. |
rubenp02
force-pushed
the
feature/add-param-slope-climb-hgt-min
branch
from
1c767af to
ccf01bd
Compare
|
As mentioned by Henry, separate PRs for a name change and functionality addition would be a very good idea. |
|
@peterbarker Can you confirm if param conversion is needed for the name changes? |
rubenp02
force-pushed
the
feature/add-param-slope-climb-hgt-min
branch
from
ccf01bd to
ae51af9
Compare
rubenp02
changed the title
|
Moved the slope name changes to #29087 |
ArduPlane/Parameters.cpp
Outdated
|
|
||
| // @Param: CLMB_SLOPE_HGT | ||
| // @DisplayName: Climb slope mininum height | ||
| // @Description: Sets the minimum height above home at which the aircraft will apply a climb slope between waypoints. Below it, the aircraft will ascend immediately, and will only resume the requested trajectory upon reaching this height. This prevents unsafe behavior such as attempting to slowly gain altitude near obstacles. The default value of 20m ensures safe operations in most environments, but it can be adjusted based on specific terrain or operational needs. |
There was a problem hiding this comment.
It seems like using set_altitude_offset_location() in setup_glide_slope() means this will now work correctly if terrain following. Do I have that right?
IMO that would be a very good thing, so should this comment be changed to reflect that?
There was a problem hiding this comment.
Not entirely sure if I understand, but I think it should work about the same as before for terrain following. The only differences are that now all legs are treated as altitude slopes regardless of the height at which the aircraft reaches them, and that the logic to follow altitude slopes now makes the aircraft climb as fast as possible when below CLMB_SLOPE_HGT, as a safety feature. It then resumes the correct trajectory.
There was a problem hiding this comment.
We could go to CLIMB we have two characters free.
rubenp02
force-pushed
the
feature/add-param-slope-climb-hgt-min
branch
from
ae51af9 to
a5d4bb8
Compare
rubenp02
changed the title
There was a problem hiding this comment.
Its not clear to me exactly what the expected change of behavior is here. Adding a param to replace the hard coded value is clear. But then the logic is moved into a different function? That change expands the scope from just auto mode to lots of other modes. Its also called directly in landing via a functor.
I would prefer to just update the hard coded value to the param. Otherwise we will need loads of testing.
The reason is that the old behaviour had what in my opinion was a huge design flaw where it unpredictably could take wildly different paths in terms of altitude depending on outside/non obvious factors. This can be seen in the attached log. If an user programs a leg to be flown with an altitude slope and the aircraft happens to be infinitesimally below the parameter height right before it, they won't expect the sudden climb with the aircraft not following the planned trajectory at all. They also won't expect how in a different flight, if the UAV just happens to be above that height, the behaviour becomes entirely different even though the flight plan didn't change at all. If this is a safety feature where the aircraft doesn't climb slowly below a certain height, which is very reasonable, then it should probably only climb fast up to that height, not all the way to the target height of that leg. This should be consistent throughout all modes' climb slopes, as nothing about the param. name or description indicates that is has anything to do about auto in particular. This is what this PR does. In landing this branch won't run because it checks for a climb.
I think it's worth it but if you want I can make this PR be only about the param and then make another one with the behaviour change. That being said, the behavior change is probably the key improvement here. |
ArduPlane/altitude.cpp
Outdated
| // gain height at low altitudes, potentially hitting obstacles. | ||
| if (target_altitude.offset_cm > 0 && | ||
| adjusted_relative_altitude_cm() < g2.waypoint_climb_slope_height_min * 100) { | ||
| plane.set_target_altitude_location(plane.next_WP_loc); |
There was a problem hiding this comment.
Should use the passed in location here. I also think its odd that your setting to the destination altitude here. That means that the altitude target will jump up to the destination, then when you climb above the min height it jumps back down to min height and then ramps from there. Better to constrain up to min height so that the altitude is constant before the ramp rather than having step changes. (you will also have to constrain down to the altitude of the passed in location).
There was a problem hiding this comment.
Should use the passed in location here.
True, thank you!
I also think its odd that your setting to the destination altitude here
This is intentional to exactly match the old behaviour (which effectively sets the target alt. to the next WP to achieve an immediate climb) up until we're above the min. height. It can result in a faster climb which is what we want. But I get what you're saying. What behaviour would you say is better?
There was a problem hiding this comment.
I suspect as it is now it might actually overshoot and descend slightly to get back on track when the target jumps back down. Because its a discontinuous target you might also find that it gets in to a oscillation because adjusted_relative_altitude_cm is based on true altitude. So you might have the target rapidly switch. Constraining the target up will give a continuous target so it should not cause any oscillation.
As you say that could result in a slower climb, but we could increase the default value a bit to compensate.
There was a problem hiding this comment.
You're absolutely right about the overshoot, but it's quite gentle and maybe even desirable in a safety mechanism. You can take a look at it in the logs from the tests I've just attached to see for yourself. And if you think clamping the altitude and increasing the default value for CLMB_SLOPE_HGT is a better approach, that's an immediate fix.
There was a problem hiding this comment.
I have increased the min. height by 5 m. and clamped the demanded height to the min height. But maybe this can make the aircraft get stuck just below the min. height?
There was a problem hiding this comment.
I have checked, and it does. If the aircraft is way below the safety height, it promply climbs and the behaviour is close to what was done before (just setting the destination altitude). But if it's only very slightly below the safety height, it gets stuck in a very slow climb to it and loses the climb slope in the process. In my test it eventually got above the min. height and resumed climbing normally, but depending on how the TECS is tuned I think the aircraft could get stuck forever at that height. So pretty much the opposite of the intended behavior (the cursor is at the start of the leg with the climb):
Setting target to the destination altitude (original logic of this PR):
Setting target to the safety height:
We could use a deadband around the height threshold, but maybe setting the target altitude to the destination altitude isn't all that bad? TECS seems to cope with it perfectly fine (i.e. with no discontinuities in the demanded height). After all, this is exactly what happens when no altitude slope handling is used.
There was a problem hiding this comment.
Hum, I guess the problem is using the true height as feedback. We could remove that and just shape the desired altitude.
Makes sense. What kind of tests are you thinking of? Should they be added to the testing suite or just SITL logs attached here are fine? More importantly, do you think what I was saying before makes sense in the first place? |
Yeah, it makes sense. I do wonder if we should bump up the min height a bit to make up for the shorter "max climb" it will do now. That is to say if you have your first point at 10 the next at 100. We might have missed a obstacle at 25 meters before and hit it now. If you want to write tests that would be great!, but I don't think its required. The key thing is just to make sure we don't get it completely wrong in some edge case. Tests often are not great at proving a new feature works, but they are great to make sure we don't break the feature in the future. |
rubenp02
force-pushed
the
feature/add-param-slope-climb-hgt-min
branch
from
a5d4bb8 to
1b0197e
Compare
|
I've performed the tests requested: sitl-climb-slope-min-hgt-tests.zip Would it be possible to discuss this in the dev call? I see you removed the tag and I guess that it can only be added by maintainers. I thought it made sense as this is related to #29087 which is also marked for discussion. |
|
The test logs (I looked at two) show this odd altitude demand spike (MAVExplorer: We see the demanded altitude go to 100 and then back to the ramp value when the altitude is above 20m but then there is a little spike. Any idea what is going on? Edit. NTUN.TAT shows it better due to the higher rate logging. |
|
To answer my own question, its overshooting by 5m and triggering the glide slope-recalculation.
|
Yes, it should be fixed by constraining the altitude for the immediate climb like you suggested, I think |
Yeah, I think I would prefer to increase the default threshold by 5m and have less overshoot than keep the current threshold with the overshoot. Maybe we need to look at the glide slope recalc in the future, I would have expected it to recalculate up but not come down again afterwards as it did in those logs. |
ArduPlane/Parameters.cpp
Outdated
|
|
||
| // @Param: CLMB_SLOPE_HGT | ||
| // @DisplayName: Climb slope mininum height | ||
| // @Description: Sets the minimum height above home at which the aircraft will apply a climb slope between waypoints. Below it, the aircraft will ascend immediately, and will only resume the requested trajectory upon reaching this height. This prevents unsafe behavior such as attempting to slowly gain altitude near obstacles. The default value of 20m ensures safe operations in most environments, but it can be adjusted based on specific terrain or operational needs. |
There was a problem hiding this comment.
probably should be above ground level, if we are doing terrain following currently, but should not use lidar
There was a problem hiding this comment.
Currently altitude slope handling isn't applied to legs that start with a terrain relative waypoint (an immediate climb is always used), so this PR doesn't change that behavior.
ArduPlane/altitude.cpp
Outdated
| // descending. This is meant to prevent situations where we try to slowly | ||
| // gain height at low altitudes, potentially hitting obstacles. | ||
| if (target_altitude.offset_cm > 0 && | ||
| adjusted_relative_altitude_cm() < g2.waypoint_climb_slope_height_min * 100) { |
There was a problem hiding this comment.
If terrain relative legs could use this, I'd very much agree. Would it somehow break the feature for relative alt. waypoints, or if terrain was unavailable?
|
@rubenp02 the reason for relative ground level is users doing long distance flights from point A to point B where B is much higher/lower than A. I think using relative_ground_altitude(false) would be correct if we are currently terrain following |
Slope treatment for a flight leg was conditioned to a minimum height of 20m (which was a hardcoded value and only documented in a comment in the code), below which an immediate altitude change was performed for that leg. This caused unpredictable results for flight plans with waypoints at exactly 20m followed by a climb, as the aircraft could, depending on chance, be above or below the height demand, resulting in very different trajectories. This commit addresses that issue with the following changes: - Added a parameter to control the minimum height at which a gradual climb slope can be performed. This documents the original feature and retains the safety aspect of it. - Changed the behavior when below said minimum height to only immediately climb up to the safe height, regaining the intended climb slope afterwards. This leverages existing code for recalculating climb slopes, provides a good balance between safety and following the flight plan as intended, and fixes the core issue of different trajectories being taken based on random external factors.
rubenp02
force-pushed
the
feature/add-param-slope-climb-hgt-min
branch
from
1b0197e to
b356fb3
Compare
Plane: Add climb slope minimum height parameter
Description
Plane: Add climb slope minimum height parameter:
Slope treatment for a flight leg was conditioned to a minimum height of 20m (which was a hardcoded value and only documented in a comment in the code), below which an immediate altitude change was performed for that leg. This caused unpredictable results for flight plans with waypoints at exactly 20m followed by a climb, as the aircraft could, depending on chance, be above or below the height demand, resulting in very different trajectories.
This commit addresses that issue with the following changes:
Testing flight plan and logs
testing-logs.zip