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

Copter: further explain even if RTL_ALT set to zero, copter still climb to 2m… #8799

Closed
wants to merge 1 commit into from

Conversation

chobitsfan
Copy link
Contributor

@chobitsfan chobitsfan commented Jul 3, 2018

… above ground

In current description of RTL_ALT, it states "Set to zero to return at current altitude". But if current altitude of a copter is below 2m above ground, it will climb up to 2m (defined as RTL_ALT_MIN in config.h) rather than "current altitude"). It works well outdoor, but when a copter fly indoor, it may hit ceiling. And user will feel confused about why copter climb up with RTL_ALT = 0.

@chobitsfan chobitsfan force-pushed the pr_rtl_alt_comment branch 2 times, most recently from 60eb8e3 to ce5a1ff Compare July 3, 2018 08:14
Copy link
Contributor

@amilcarlucas amilcarlucas left a comment

Choose a reason for hiding this comment

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

I think you should refer to the RTL_ALT_MIN name at some point. So that if someone wants to change it, he has a chance at finding that constant.

@chobitsfan
Copy link
Contributor Author

chobitsfan commented Jul 4, 2018

Yes, you are right

Copy link
Member

@OXINARF OXINARF left a comment

Choose a reason for hiding this comment

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

@amilcarlucas Referring code constants in user descriptions is a bad idea, please don't ask people to do that.

@chobitsfan What do you think of my inline suggestion? Am I missing something?

@@ -108,7 +108,7 @@ const AP_Param::Info Copter::var_info[] = {
#if MODE_RTL_ENABLED == ENABLED
// @Param: RTL_ALT
// @DisplayName: RTL Altitude
// @Description: The minimum relative altitude the model will move to before Returning to Launch. Set to zero to return at current altitude.
// @Description: The minimum relative altitude the model will move to before Returning to Launch. Set to zero to return at current altitude. Copter still climb to 2m above ground if its current altitude lower than 2m (defined as RTL_ALT_MIN in config.h)
// @Units: cm
// @Range: 0 8000
Copy link
Member

Choose a reason for hiding this comment

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

Why don't we change the range to start at 200 instead (and remove the part about zero)?

The description could probably be improved to something like The minimum altitude above home the vehicle will move to before Returning to Launch. If TERRAIN_FOLLOW is enabled and terrain data is available, vehicle will follow terrain at the altitude set here.

@rmackay9
Copy link
Contributor

rmackay9 commented Jul 5, 2018

Consolidated changes and merged, thanks!

@rmackay9 rmackay9 closed this Jul 5, 2018
@amilcarlucas
Copy link
Contributor

@OXINARF If the value of the constant changes in the code.. the description will go out of sync with the real value (i.e. 3m instead of 2m)
Having a reference in there allows the developers to at least do a search and replace in the code and find all the spots where the code constant is used/mentioned, and adapt to that change.

What is the rationale behind your request?

@rmackay9
Copy link
Contributor

rmackay9 commented Jul 5, 2018

I actually agree with @OXINARF. The parameter descriptions are really for regular users, not for developers.

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

4 participants