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

Remove LNDMC_ALT_MAX #22376

Merged
merged 1 commit into from
Nov 20, 2023
Merged

Remove LNDMC_ALT_MAX #22376

merged 1 commit into from
Nov 20, 2023

Conversation

MaEtUgR
Copy link
Member

@MaEtUgR MaEtUgR commented Nov 15, 2023

Solved Problem

When discussing with @sfuhrer about geofence improvements he mentioned again the overlap between LNDMC_ALT_MAX and GF_MAX_VER_DIST. I know @dagar was bringing this up many times.

I think now is the right time to remove it since I'm not aware who is using that functionality right now and @sfuhrer is working on geofence improvements which should allow us to forward users to using GF_MAX_VER_DIST.

Fixes #13345

Solution

I remove LNDMC_ALT_MAX completely.
It was introduced here: #6679
extended to shift down mission items here: #8048

Changelog Entry

Cleanup: Remove LNDMC_ALT_MAX, use GF_MAX_VER_DIST instead

Alternatives

We could use GF_MAX_VER_DIST in place of how LNDMC_ALT_MAX is currently used but:

  • On multicopter it prevents breaching the altitude in any climb rate enabled mode which we should achieve with the geofence.
  • On any vehicle, all mission items just get silently repositioned to be lower which was a hack to still allow RTL even if the return altitude is higher than the allowed altitude but flying lower than planned is not generally a safe thing to do. The check for the return altitude being lower than the maximum altitude should be done explicitly.

Test coverage

I did not specifically test this but the functionality was removed and it still compiles.

This should be replaced with the maximum geofence
altitude GF_MAX_VER_DIST. I'm not aware anyone is using
this functionality as is.
Copy link
Contributor

@sfuhrer sfuhrer left a comment

Choose a reason for hiding this comment

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

For me this is a second implementation of a geofence feature, and thus support its removal.
If manufacturers want to implement a hard altitude limit that the user cannot overwrite then they can set GF_MAX_VER_DIST and GF_ACTION and do not give the user the ability to change them.

@sfuhrer
Copy link
Contributor

sfuhrer commented Nov 15, 2023

Note: this would require a docs update (remove deprecated param).

@MaEtUgR MaEtUgR merged commit 9bed8f4 into main Nov 20, 2023
84 of 89 checks passed
@MaEtUgR MaEtUgR deleted the remove-lndmc-alt-max branch November 20, 2023 13:52
@MaEtUgR
Copy link
Member Author

MaEtUgR commented Nov 20, 2023

this would require a docs update

It was actually never documented explicitly but only in the autogenerated reference 😅

image

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

Successfully merging this pull request may close these issues.

Max altitude parameter does weird things
2 participants