-
Notifications
You must be signed in to change notification settings - Fork 16.8k
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
Plane: Emergency Parachute Feature #9506
Conversation
Hi @viniciusknabben, thank you for your first contribution and welcome to ArduPilot! Please read our contribution rules at http://ardupilot.org/dev/docs/submitting-patches-back-to-master.html, especially the parts about the commit message rules and the commit per-library/vehicle. |
eceab07
to
58387b6
Compare
Should i rename the PR to Plane: Emergency Parachute Feature ? |
ArduPlane/parachute.cpp
Outdated
gcs().send_text(MAV_SEVERITY_CRITICAL,"Emergency Parachute: Stall detected"); | ||
parachute_release(); | ||
} | ||
else if (battery.voltage() < parachute.min_batt_voltage()) { |
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.
This batt check needs hysteresis. Battery is notoriously noisy and fluctuates a lot depending on throttle and you don't want a single check to trigger the parachute. Perhaps what we need is a "super low-pass" version in the battery library because doing checks like are not the parachute's library duty to deal with.
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.
Agreed, I will make a deep read on AP_BattMonitor and work on 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.
I'll take care of the battery stuff right now
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.
Here's a PR #9508
please give it a try on your branch
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.
Cool, i will do it
These are generic checks that should get moved into AP_Parachute. Both copter and plane would benefit from:
They should all be in the single parachute::update() The easiest way to do this is to add accessors in the chute library for an isFlying flag because those are determined by different mechanisms in plave vs copter. For plane, just pass it the is_flying flag when it gets set in is_flying.cpp like how frsky and adsb does. Same with sink rate, although I think we could be smarter with that through ahrs singleton but for now simply passing it the auto_state.sink_rate is fine. |
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 battery one is actually much easier to handle. Unless there is a strong reason not to, you should simply be adding release parachute as a battery failsafe action. The battery library will manage the triggers for you, add the time requirement, and will allow you to trigger off of capacity as well. All you have to do to make this work is add it to the list of enums (and priorities) here https://github.com/ArduPilot/ardupilot/blob/master/ArduPlane/Plane.h#L1043
Then handle the action here: https://github.com/ArduPilot/ardupilot/blob/master/ArduPlane/events.cpp#L143 (this would just be calling parachute_release()
)
d3ba65d
to
8ad1196
Compare
@WickedShell @magicrub , code updated |
@@ -20,6 +20,10 @@ | |||
|
|||
#define AP_PARACHUTE_ALT_MIN_DEFAULT 10 // default min altitude the vehicle should have before parachute is released | |||
|
|||
#define AP_PARACHUTE_EMERGENCY_DEFAULT 0 // default value to emergency parachute | |||
#define AP_PARACHUTE_MAX_SINK_DEFAULT 10.0 // default maximun sink speed in m/s to trigger emergency parachute | |||
#define AP_PARACHUTE_MIN_BATT_VOLTAGE_DEFAULT 10.0 // default minimum battery voltage to trigger emergency parachute |
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.
This is unneeded now that you are leveraging the failsafe system.
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.
We should keep emergency and max sink default values, i will remove battery for now.
f19bbf1
to
a56742b
Compare
1 similar comment
4df17f9
to
cc80551
Compare
void set_is_flying(const bool is_flying) { _is_flying = is_flying; } | ||
|
||
// get_sink_rate - gets vehicle sink rate | ||
void get_sink_rate(float sink_rate) { _sink_rate = sink_rate; } |
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.
this should be set_sink_rate() not get_sink_rate()
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
i think this looks good now, except for minor things:
|
if you don't know how to fix commit msgs (and squash commits) then let me know and I can do it for you |
git rebase -i HEAD~x right? then squash and and change commit msg. can you explain me about the commit msg? should i squash all commits into a single one and write a msg like: Plane: Added parachute release for dangerous sinking and battery failsafe |
I'm a bit confused where copter calcs its altitude variation to call set_sink_rate() |
On Tue, 19 Feb 2019, Vinicius Knabben wrote:
if you don't know how to fix commit msgs (and squash commits) then let me know and I can do it for you
git rebase -i HEAD~x right? then squash and and change commit msg.
Assuming you are on a topic branch, something like this:
git rebase -i remotes/origin/master
(this is much the same thing, asking git to rebase onto a different
commit; the last argument differs as there are *many* ways to specify
which commit to rebase onto).
As always when learning git - make a backup first :-)
can you explain me about the commit msg? should i squash all commits into a single one and write a msg like: Plane: Added parachute release for dangerous
sinking and battery failsafe
Yes. So long as all the commits are in the ArduPlane directory, otherwise
you will need to split the commits into one per vehicle/library.
Peter
|
f7ffcc8
to
bca10cb
Compare
I've added in the sink rate to copter. |
Agreed, we could also make this a parameter. In some cases the plane/copter could have time to recover, the user change the value |
e738109
to
307d0eb
Compare
Rebase required. Last commit needs fixup'ing into another. |
b5e0d6b
to
f6e1b6b
Compare
@peterbarker Rebase done |
@viniciusknabben Do you think you have addressed @WickedShell 's concerns in his review? |
@peterbarker, i think i did, the concern was about where to handle the parachute release when the battery reaches a critical voltage. The triggers were already there and i simply added the parachute action (3bdf81b and 9b2611d) as suggested by @WickedShell. |
@@ -172,6 +172,10 @@ void Plane::handle_battery_failsafe(const char *type_str, const int8_t action) | |||
afs.gcs_terminate(true, battery_type_str); | |||
break; | |||
|
|||
case Failsafe_Action_Parachute: | |||
parachute_release(); |
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.
Should the vehicle be disarmed here? I haven't investigated the parachute code in the past, but what keeps us from spinning motors once the parachute has been released?
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 throttle is suppressed when the parachute release is initiated.
ardupilot/ArduPlane/servos.cpp
Lines 54 to 62 in 12e62d0
bool Plane::suppress_throttle(void) | |
{ | |
#if PARACHUTE == ENABLED | |
if (auto_throttle_mode && parachute.release_initiated()) { | |
// throttle always suppressed in auto-throttle modes after parachute release initiated | |
throttle_suppressed = true; | |
return true; | |
} | |
#endif |
This needs a rebase. There's a regression test for it here: https://github.com/peterbarker/ardupilot/tree/emergency_parachute - it would be nice to cherry-pick that on top. |
…afe actions Signed-off-by: Vinicius Knabben <viniciusknabben@hotmail.com>
The user can now set the parachute release as a failsafe action Signed-off-by: Vinicius Knabben <viniciusknabben@hotmail.com>
Signed-off-by: Vinicius Knabben <viniciusknabben@hotmail.com>
Signed-off-by: Vinicius Knabben <viniciusknabben@hotmail.com>
f6e1b6b
to
245345d
Compare
Releases parachute if a maximum vertical speed is reached
Releases parachute if a minumum battery voltage is reached
Created parameter to enable emergency parachute
Created parameters to set up emergency threshold values
Signed-off-by: Vinicius Knabben viniciusknabben@hotmail.com