Skip to content
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

Adjust slope terminology #29087

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

rubenp02
Copy link
Contributor

@rubenp02 rubenp02 commented Jan 16, 2025

Adjust slope terminology

Description

Tools/SITL/autotest/Plane: Change glide slope to altitude slope:

Smooth altitude changes were always referred to as "glide slopes" despite this not being the appropriate aviation term in the case of a climb. A better option is "altitude slope", which encompasses both smooth climbs and descents.

Changed all references to glide slopes, except those that specifically refer to a single kind (like those used for landings), to the more general term. This also includes changing the GLIDE_SLOPE_MIN and GLIDE_SLOPE_THR parameters to ALT_SLOPE_MIN and ALT_SLOPE_THRESH, respectively.

This PR is related to #29077

Smooth altitude changes were always referred to as "glide slopes"
despite this not being the appropriate aviation term in the case of a
climb. A better option is "altitude slope", which encompasses both
smooth climbs and descents.

Changed all references to glide slopes, except those that specifically
refer to a single kind (like those used for landings), to the more
general term. This also includes changing the GLIDE_SLOPE_MIN and
GLIDE_SLOPE_THR parameters to ALT_SLOPE_MIN and ALT_SLOPE_THRESH,
respectively.
Smooth altitude changes were always referred to as "glide slopes"
despite this not being the appropriate aviation term in the case of a
climb. A better option is "altitude slope", which encompasses both
smooth climbs and descents.

Changed all references to glide slopes, except those that specifically
refer to a single kind (like those used for landings), to the more
general term. This also includes changing the GLIDE_SLOPE_MIN and
GLIDE_SLOPE_THR parameters to ALT_SLOPE_MIN and ALT_SLOPE_THRESH,
respectively.
Smooth altitude changes were always referred to as "glide slopes"
despite this not being the appropriate aviation term in the case of a
climb. A better option is "altitude slope", which encompasses both
smooth climbs and descents.

Changed all references to glide slopes, except those that specifically
refer to a single kind (like those used for landings), to the more
general term. This also includes changing the GLIDE_SLOPE_MIN and
GLIDE_SLOPE_THR parameters to ALT_SLOPE_MIN and ALT_SLOPE_THRESH,
respectively.
@Hwurzburg
Copy link
Collaborator

Hwurzburg commented Jan 16, 2025

@peterbarker this can go in before #29077

Copy link
Member

@IamPete1 IamPete1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know you have not made the formatting any worse, but we might as well take the opportunity to fix it while changing those lines.

ArduPlane/altitude.cpp Outdated Show resolved Hide resolved
ArduPlane/altitude.cpp Outdated Show resolved Hide resolved
Smooth altitude changes were always referred to as "glide slopes"
despite this not being the appropriate aviation term in the case of a
climb. A better option is "altitude slope", which encompasses both
smooth climbs and descents.

Changed all references to glide slopes, except those that specifically
refer to a single kind (like those used for landings), to the more
general term. This also includes changing the GLIDE_SLOPE_MIN and
GLIDE_SLOPE_THR parameters to ALT_SLOPE_MIN and ALT_SLOPE_THRESH,
respectively.
@rubenp02 rubenp02 force-pushed the chore/change-glide-slope-to-alt-slope branch from e2dbeb0 to a322f1d Compare January 16, 2025 15:49
@rubenp02 rubenp02 requested a review from IamPete1 January 16, 2025 15:51
Copy link
Member

@IamPete1 IamPete1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, Thanks!

@rubenp02
Copy link
Contributor Author

@peterbarker Can you confirm if param conversion is needed for the name changes?

@IamPete1
Copy link
Member

This is failing the MAV_CMD_GUIDED_CHANGE_ALTITUDE test, but I don't see any change of behavior in the code, looks like a clean rename. There must be something changed which I have missed.

@peterbarker Can you confirm if param conversion is needed for the name changes?

No param conversion needed. The value is stored based on the table and index within, name doesn't matter.

@rubenp02
Copy link
Contributor Author

No param conversion needed. The value is stored based on the table and index within, name doesn't matter.

Thanks, figured as much but wanted to be sure

Copy link
Contributor

@peterbarker peterbarker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

// @Param: GLIDE_SLOPE_MIN
// @DisplayName: Glide slope minimum
// @Description: This controls the minimum altitude change for a waypoint before a glide slope will be used instead of an immediate altitude change. The default value is 15 meters, which helps to smooth out waypoint missions where small altitude changes happen near waypoints. If you don't want glide slopes to be used in missions then you can set this to zero, which will disable glide slope calculations. Otherwise you can set it to a minimum number of meters of altitude error to the destination waypoint before a glide slope will be used to change altitude.
// @Param: ALT_SLOPE_MIN
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe ALT_DELTA_MIN is better? The problem with "SLOPE" is it sounds like an angle

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure but if you read the description, the word "slope" appears multiple times as it describes what the functionality is - altitude slope calculations.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that we cannot omit the ALT_SLOPE_* prefix

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ALT_SLOPE_DELTA?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

// @Param: GLIDE_SLOPE_THR
// @DisplayName: Glide slope threshold
// @Description: This controls the height above the glide slope the plane may be before rebuilding a glide slope. This is useful for smoothing out an autotakeoff
// @Param: ALT_SLOPE_THRESH
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe ALT_ERROR_THRESH ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

docs should also make it clear it is only when above, not below

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

definitely not ALT_ERROR_THRESH that could mean any ALT error that has nothing to do with this slope functionality. IMO it would be confusing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ALT_SLOPE_MAXHGT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO that doesn't convey what the parameter does. ALT_SLOPE_RECALC is the best I can think of, and the display name could be "Altitude slope recalculation threshold". I think the description already makes it clear that this only applies when above the slope but maybe it should be a part of the display name too

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps, but I think ALT_SLOPE_MAXHGT does convey what it refers to. Specifically its height (not altitude) and its above (but not below) the altitude slope, which triggers the recalculation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does make sense, I agree. Is there anything else in the display name or description that you think should be changed?

Copy link
Contributor

@timtuxworth timtuxworth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of suggestions about the parameter names.

// @Param: GLIDE_SLOPE_MIN
// @DisplayName: Glide slope minimum
// @Description: This controls the minimum altitude change for a waypoint before a glide slope will be used instead of an immediate altitude change. The default value is 15 meters, which helps to smooth out waypoint missions where small altitude changes happen near waypoints. If you don't want glide slopes to be used in missions then you can set this to zero, which will disable glide slope calculations. Otherwise you can set it to a minimum number of meters of altitude error to the destination waypoint before a glide slope will be used to change altitude.
// @Param: ALT_SLOPE_MIN
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure but if you read the description, the word "slope" appears multiple times as it describes what the functionality is - altitude slope calculations.

// @Param: GLIDE_SLOPE_THR
// @DisplayName: Glide slope threshold
// @Description: This controls the height above the glide slope the plane may be before rebuilding a glide slope. This is useful for smoothing out an autotakeoff
// @Param: ALT_SLOPE_THRESH
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

definitely not ALT_ERROR_THRESH that could mean any ALT error that has nothing to do with this slope functionality. IMO it would be confusing.

// @Param: GLIDE_SLOPE_THR
// @DisplayName: Glide slope threshold
// @Description: This controls the height above the glide slope the plane may be before rebuilding a glide slope. This is useful for smoothing out an autotakeoff
// @Param: ALT_SLOPE_THRESH
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ALT_SLOPE_MAXHGT?

Copy link
Contributor

@timtuxworth timtuxworth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're getting close :-)

// @Param: GLIDE_SLOPE_MIN
// @DisplayName: Glide slope minimum
// @Description: This controls the minimum altitude change for a waypoint before a glide slope will be used instead of an immediate altitude change. The default value is 15 meters, which helps to smooth out waypoint missions where small altitude changes happen near waypoints. If you don't want glide slopes to be used in missions then you can set this to zero, which will disable glide slope calculations. Otherwise you can set it to a minimum number of meters of altitude error to the destination waypoint before a glide slope will be used to change altitude.
// @Param: ALT_SLOPE_MIN
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

// @Param: GLIDE_SLOPE_THR
// @DisplayName: Glide slope threshold
// @Description: This controls the height above the glide slope the plane may be before rebuilding a glide slope. This is useful for smoothing out an autotakeoff
// @Param: ALT_SLOPE_THRESH
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps, but I think ALT_SLOPE_MAXHGT does convey what it refers to. Specifically its height (not altitude) and its above (but not below) the altitude slope, which triggers the recalculation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants