-
-
Notifications
You must be signed in to change notification settings - Fork 520
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
action: add RTL return altitude setting #446
Conversation
4b87f30
to
a599d35
Compare
plugins/action/action.h
Outdated
/** | ||
* @brief Get the return to launch minimum return altitude. | ||
* | ||
* When return to launch is initiated, the vehicle climbs to the return altitude if it is lower |
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 might just remove this and do a see also to the get_max_speed_m_s
(or visa versa)
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.
Right.
/** | ||
* @brief Set the return to launch minimum return altitude. | ||
* | ||
* When return to launch is initiated, the vehicle climbs to the return altitude if it is lower |
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.
- The naming in PX4 for RTL is now "Return Mode". We should perhaps rename the other API
- The behaviour on return is configurable based on parameters - e.g. it might not land. In particular if you call this on a Fixed Wing vehicle. I tend to not talk about the landing if I don't need to.
- Would it make sense to embed the setter into the return command function? Probably not, but we should certainly do a see also between this and the return command function.
Perhaps:
When Return mode is engaged the vehicle will ascend to this altitude (if below) for the return flight. The vehicle will maintain its current altitude if above this altitude.
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.
-
Good point, we should think about the naming.
-
I think the SDK needs to assume default params for everything it has not changed. Otherwise we're opening a whole can of worms and can't guarantee at all what happens when you call something. The firmware has way too many parameters that you need to know to be safe.
-
My aim is to have simple functions with as little arguments as possible. Otherwise, the function signature soon would look like this:
void Action::return_to_launch_async(action_callback_t callback, float return_altitude_m, bool include_land, bool do_smart_rtl);
_relative_takeoff_altitude_m = new_relative_altitude_m; | ||
} | ||
} | ||
|
||
float ActionImpl::get_takeoff_altitude_m() const | ||
{ | ||
// FIXME: we never actually get the param, as we should. |
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.
So that means that you need to set it from the user side in order to be able to get it?
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.
Exactly. I'll refactor the param interface next, see #440.
This adds the setting to change the RTL return altitude which is the altitude at which the drone will return if RTL is engaged if it is lower (if higher, it will return at the current higher altitude).
This adds 3 tests with different altitudes.
This adds a PathChecker which is used to check the altitudes during RTL.
e0fc67a
to
298b26a
Compare
action: add RTL return altitude setting
This adds the setting to change the RTL return altitude which is the altitude at which the drone will return if RTL is engaged if it is lower (if higher, it will return at the current higher altitude).
The param system needs some refactoring in order to get this to work properly, see #440.
Todo: an integration test for this would be nice.