-
Notifications
You must be signed in to change notification settings - Fork 18.1k
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
Plane: Attitude stall prevention adjustments #29092
Open
rubenp02
wants to merge
4
commits into
ArduPilot:master
Choose a base branch
from
Ventor-Innovations:feature/attitude-stall-prevention-adjustments
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+27
−9
Open
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
e3ac2d1
Plane: Fixed formula for load factor
abaghiyan 089f577
AP_TECS: Corrected formula for _TASmin according to fix in formula fo…
abaghiyan d5ccb9c
TECS: Add getters for pratical stall airspeed
rubenp02 f1a99ec
Plane: Use stall speed for roll stall prevention
rubenp02 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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.
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.
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.
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.
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.
@rubenp02 I think you're onto something.
But first, some clarifications.
Currently, this is what happens (to my understanding):
Attitude.cpp::update_load_factor()
will calculate a load factor usingAIRSPEED_MIN
._TASmin
(its minimum airspeed limit) by:AIRSPEED_MIN*_load_factor
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 intoupdate_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:
This is not correct. The idea is the allow maneuvers at
AIRSPEED_MIN
, as introduced here.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 inupdate_load_factor()
.And I understand that will have the effect of not exaggerating the load factor.
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.
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.
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 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).
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.
Let's see:
Attitude.cpp
indeed calculatesaerodynamic_load_factor
based ondemanded_roll
. And you also proposeConsider the following reasoning:
AIRSPEED_MIN
.aerodynamic_load_factor=1.41
.max_load_factor=1
_TASmin
bysqrt(1.41)
.demanded_roll
is the same, so isaerodynamic_load_factor
.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.aerodynamic_load_factor
does affect the calculation ofmax_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.
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.
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.
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.
Okay, let's see it!