Plane: Attitude stall prevention adjustments

[rubenp02]

Plane: Attitude stall prevention adjustments

Description

TECS: Add getters for pratical stall airspeed.

Plane: Use stall speed for roll stall prevention:

Applied the new AIRSPEED_STALL parameter to roll stall prevention, to enable tighter turns at lower airspeeds when this parameter is set. For accurate behavior, it should be set at the stall IAS on level flight at sea level, plus the desired safety margin.

This PR is branched from #29101

[IamPete1]

There is some pre-existing work here that we never did merge, but I think your proposed fix is similar. #24320 #24509

Re the second commit, I think swapping stall to speed if set is fine. Less keen on the 0.85 scale factor on airspeed min, the vast majority have no idea what there stall speed is, so we need to be conservative. I think if you do make that change you would need to remove the correction that is done in TECS here because the load factor would be lower already:

ardupilot/libraries/AP_TECS/AP_TECS.cpp

Line 418 in 25e1701

_TASmin = MAX(_TASmin, aparm.airspeed_stall*EAS2TAS*_load_factor);

[rubenp02]

Thank you for linking that because I hadn't noticed that the load factor formula itself was also wrong. I have removed the scaling of the min speed to get the stall speed, so now the second commit is just using the stall speed if set to get less restrictive roll limits.

[IamPete1]

I felt bad we had overlooked that original PR for so long so I have rebased it here: #29101

Do you agree with all the changes there? Once that is in you could rebase this to bring in the use of the stall speed param.

[rubenp02]

I agree that's the right thing to do

Once that is in you could rebase this to bring in the use of the stall speed param.

Sounds good!

[rubenp02]

@IamPete1 What do you think about this now? It centralizes the stall speed in the TECS, which means that this load factor routine and other future stall prevention logic can easily reference a roll-compensated stall speed that is based on either AIRSPEED_MIN of AIRSPEED_STALL if set.

I'm adding pitch stall prevention on top of this so I think it makes sense. Also, maybe the _TASmin member in the TECS should be renamed to _TASstall, as that's what it's treated as in the formulas.

[Georacer]

I'm not a fan of calling this the stall airspeed. AIRSPEED_MIN is explicitly meant to NOT be the stall airspeed, as per its parameter definition.

As a side-note, the resulting maximum loading factor is the maximum allowed loading factor, not the maximum loading factor supported by the airframe. That is what would correspond to the stall airspeed.

[rubenp02]

I understand, but if AIRSPEED_STALL is defined, this is going to be much closer to the actual stall speed. Since the idea behind AIRSPEED_STALL is to optionally allow for maneuvers that are closer to the stall speed, and the related math (the maximum loading factor and the scaling with bank angle) is canonically done based on Vstall, we're indeed considering AIRSPEED_MIN to be the stall airspeed in the code as it stands. And I was embracing that here because it's something that makes sense from a safety perspective and considering how poorly most AP aircraft are tuned. It makes a lot of sense to consider the minimum navigation speed (AIRSPEED_MIN) to be the stall speed in the TECS code unless the user explicitly defines an actual stall speed because that works out perfectly for both plug and play and more advanced users.

As a side-note, the resulting maximum loading factor is the maximum allowed loading factor, not the maximum loading factor supported by the airframe. That is what would correspond to the stall airspeed.

But the min. airspeed increase based on bank angle isn't something that makes any sense for an airspeed that isn't a stall speed.

[Georacer]

@rubenp02 I think you're onto something.
But first, some clarifications.

Currently, this is what happens (to my understanding):

1. Attitude.cpp::update_load_factor() will calculate a load factor using AIRSPEED_MIN.
2. It will then communicate a load factor to TECS.
3. In turn, TECS will increase _TASmin (its minimum airspeed limit) by:
4. AIRSPEED_MIN*_load_factor
5. (or if defined) max(AIRSPEED_MIN, AIRSPEED_STALL*_load_factor)`.

If I read your code correctly, you want to take the final TECS::_TASmin and feed it back into update_load_factor().
Won't this create a feedback loop, constantly increasing _TASmin?
I don't think that's right.

---

I think you have some more errors in your logic:

Since the idea behind AIRSPEED_STALL is to optionally allow for maneuvers that are closer to the stall speed

This is not correct. The idea is the allow maneuvers at AIRSPEED_MIN, as introduced here.

But the min. airspeed increase based on bank angle isn't something that makes any sense for an airspeed that isn't a stall speed.

Indeed. And as far as I understand, the PR linked above fixed that.

---

However, I think I see the merit in your idea.

Are you proposing switching the load factor calculation based on AIRSPEED_STALL, when that's available?
If yes, I think it's fine to repeat a similar if (is_positive(aparm.airspeed_stall)) structure in update_load_factor().
And I understand that will have the effect of not exaggerating the load factor.

[rubenp02]

Currently, this is what happens (to my understanding):

1. Attitude.cpp::update_load_factor() will calculate a load factor using AIRSPEED_MIN.
2. It will then communicate a load factor to TECS.
3. In turn, TECS will increase _TASmin (its minimum airspeed limit) by:
4. AIRSPEED_MIN*_load_factor
5. (or if defined) max(AIRSPEED_MIN, AIRSPEED_STALL*_load_factor)`.

If I read your code correctly, you want to take the final TECS::_TASmin and feed it back into update_load_factor().
Won't this create a feedback loop, constantly increasing _TASmin?
I don't think that's right.

Take this with a grain of salt as I'm still learning the codebase, but I don't think that's true. The load factor is based on the demanded roll so it's entirely independent from the airspeed and thus no feedback loop is possible. To put it a different way, in TECS we're essentially doing:

true_airspeed_min_or_stall = true_airspeed_min_or_stall * sqrt(1 / cos(demanded_roll))

And in the attitude controller:

roll_limit = acos(1 / (current_airspeed / airspeed_min_or_stall)^2)

One side is adjusting the min. speed and the other the max. roll, so they don't interfere with each other.

Since the idea behind AIRSPEED_STALL is to optionally allow for maneuvers that are closer to the stall speed

This is not correct. The idea is the allow maneuvers at AIRSPEED_MIN, as introduced here.

As I understand it, it does do what I said even if that wasn't the intention, and it seems quite useful. If AIRSPEED_STALL is unset it does exactly what it did before that param. existed, and if it's set, it uses it in AIRSPEED_MIN's place. Since _STALL is lower than _MIN, _TASmin will end up being less conservative. This exact behaviour is one of the things this PR attempts to apply to the roll stall prevention, based on the same AIRSPEED_STALL parameter.

But the min. airspeed increase based on bank angle isn't something that makes any sense for an airspeed that isn't a stall speed.

Indeed. And as far as I understand, the PR linked above fixed that.

But the AIRSPEED_MIN still gets compensated for bank angle if AIRSPEED_STALL isn't set.

Ultimately where I want to go with this is to have a single source of truth regarding the stall speed as far as AP is concerned (i.e. as conservative or as precise as the user has configured it). This will be very useful to improve the stall prevention functionality, and to avoid future mistakes (this part of the code was riddled with uncanonical formulas and inconsistencies when it came to compensating for bank angle).

[Georacer]

Take this with a grain of salt as I'm still learning the codebase, but I don't think that's true. The load factor is based on the demanded roll so it's entirely independent from the airspeed and thus no feedback loop is possible.

Let's see:

Attitude.cpp indeed calculates aerodynamic_load_factor based on demanded_roll. And you also propose

float max_load_factor = sq(smoothed_airspeed / MAX(TECS_controller.get_stall_airspeed(), 1));
if (max_load_factor <= 1) {
// our airspeed is below the minimum airspeed. Limit roll to 25 degrees.
[...]

Consider the following reasoning:

1. You fly level at $V_c$ that equal to AIRSPEED_MIN.
2. A 45deg bang is requested and aerodynamic_load_factor=1.41.
3. Currently, max_load_factor=1
4. At this point you're not allowed to bank more than 25deg, because of the above code.
5. You bank at 25deg.
6. TECS raises _TASmin by sqrt(1.41).
7. [next iteration]
8. The demanded_roll is the same, so is aerodynamic_load_factor.
9. max_load_factor is now even smaller, because the denominator has grown. Thus the roll will keep getting limited at 25deg. This is a steady-state point for this algorithm.
10. Thus, the desired aerodynamic_load_factor does affect the calculation of max_load_factor. Albeit, in a more conservative way, which I think is the opposite of what you wanted to achieve?

Can you check my logic above?
In any case, one of your best arguments is going to be SITL simulations with a before/after comparison.

[rubenp02]

That was what I intended (and I thought would be more accurate), but I was wrong because the denominator on the max. load factor formula should be the stall speed at 1G, i.e. without taking the increase from currently being in a bank into account, which was a pretty big mistake. Fixing that will also solve this issue you mention (it will be equivalent to what was done before, except for using the stall speed param. if available), and it will make this PR quite trivial.

[Georacer]

Okay, let's see it!