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

RTL check alt_max before using #8272

Merged
merged 2 commits into from Nov 14, 2017
Merged

RTL check alt_max before using #8272

merged 2 commits into from Nov 14, 2017

Conversation

dagar
Copy link
Member

@dagar dagar commented Nov 12, 2017

Partial fix for #8257.

@dagar dagar added the bug label Nov 12, 2017
@dagar dagar added this to the Release v1.7.0 milestone Nov 12, 2017
@dagar dagar requested a review from MaEtUgR November 12, 2017 17:19
@MaEtUgR MaEtUgR requested a review from Stifael November 13, 2017 16:55

static constexpr float DELAY_SIGMA = 0.01f;
using math::max;
Copy link
Member

Choose a reason for hiding this comment

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

probably you can't take away #include <mathlib/mathlib.h> and use the function defined in it...
or is it still in the include chain?

@dagar dagar force-pushed the pr-rtl_min_alt branch 3 times, most recently from d60edd7 to a9ed78e Compare November 14, 2017 01:07
@dagar
Copy link
Member Author

dagar commented Nov 14, 2017

In a quick test the climb issue and multiple RTL in the same flight strangeness seem to be resolved, but there are many edge cases - https://logs.px4.io/plot_app?log=49ec4967-a06d-4115-8470-02287cd2c01e

@Stifael
Copy link
Contributor

Stifael commented Nov 14, 2017

it looks ok, but the get_rtl_function is not really needed anymore.
The function here is a duplicate of here, which is checked at the end of each navigation mode (in rtl here). That is why I removed that function here.

The point is that the altitude limit with -1 was introduced, but rtl pr was never merged, which resulted in a conflict.

@Stifael
Copy link
Contributor

Stifael commented Nov 14, 2017

also to mention is that this PR is a pr that comes from a fork that was tested more than half a year. My point is that this PR is good, but maybe we should combine this PR with #8101.

@dagar
Copy link
Member Author

dagar commented Nov 14, 2017

Updated to remove get_rtl_altitude().

@dagar dagar merged commit 84f07c6 into master Nov 14, 2017
@dagar dagar deleted the pr-rtl_min_alt branch November 14, 2017 19:57
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.

None yet

3 participants