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

Plane: basic automatic emergency parachute release at critical altitude error in AUTO. #3703

Closed
wants to merge 13 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 17 additions & 1 deletion ArduPlane/Parameters.cpp
Expand Up @@ -1058,10 +1058,26 @@ const AP_Param::Info Plane::var_info[] = {
GOBJECT(relay, "RELAY_", AP_Relay),

#if PARACHUTE == ENABLED
// @Group: CHUTE_
// @Group: CHUTE_
// @Path: ../libraries/AP_Parachute/AP_Parachute.cpp
GOBJECT(parachute, "CHUTE_", AP_Parachute),

// @Param: CHUTE_AUTO_ON
// @DisplayName: Parachute automatic emergency release
// @Description: Parachute automatic emergency release enabled or disabled.
// @Values: 0:Disabled,1:Enabled
// @User: Standard
GSCALAR(parachute_auto_enabled, "CHUTE_AUTO_ON", 0),
Copy link
Contributor

Choose a reason for hiding this comment

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


// @Param: CHUTE_AUTO_ERROR
// @DisplayName: Altitude deviation for parachute release
// @Description: Altitude deviation at which to release parachute if in AUTO and CHUTE_AUTO_ON.
// @Units: Meters
// @Range: 10 32767
// @Increment: 1
// @User: Standard
GSCALAR(parachute_auto_error, "CHUTE_AUTO_ERROR", 20),
Copy link
Contributor

Choose a reason for hiding this comment

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

When you move these to AP_Parachute, you can surround them with # if APM_BUILD_TYPE(APM_BUILD_ArduPlane) so they only compile for plane. Same goes for the timer variable and accessor function.


// @Param: CHUTE_CHAN
// @DisplayName: Parachute release channel
// @Description: If set to a non-zero value then this is an RC input channel number to use for manually releasing the parachute. When this channel goes above 1700 the parachute will be released
Expand Down
4 changes: 4 additions & 0 deletions ArduPlane/Parameters.h
Expand Up @@ -146,6 +146,8 @@ class Parameters {
k_param_parachute,
k_param_arming = 100,
k_param_parachute_channel,
k_param_parachute_auto_enabled,
Copy link
Contributor

Choose a reason for hiding this comment

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

the order here is extremely important. You can't push down other k_params indexes. You must use an unused index

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I see now. Where should I move these?

Copy link
Contributor

Choose a reason for hiding this comment

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

into AP_Parachute

Copy link
Contributor

Choose a reason for hiding this comment

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

the k_params are not needed if you are within a lib

k_param_parachute_auto_error,
k_param_crash_accel_threshold,
k_param_override_safety,

Expand Down Expand Up @@ -510,6 +512,8 @@ class Parameters {
#endif
AP_Int16 gcs_pid_mask;
AP_Int8 parachute_channel;
AP_Int8 parachute_auto_enabled;
AP_Int16 parachute_auto_error;

// RC channels
RC_Channel rc_1;
Expand Down
1 change: 1 addition & 0 deletions ArduPlane/Plane.h
Expand Up @@ -1021,6 +1021,7 @@ class Plane : public AP_HAL::HAL::Callbacks {
void parachute_check();
void parachute_release();
bool parachute_manual_release();
void parachute_emergency_check();
void accel_cal_update(void);

public:
Expand Down
66 changes: 64 additions & 2 deletions ArduPlane/parachute.cpp
Expand Up @@ -3,12 +3,15 @@
#include "Plane.h"


/*
/*
call parachute library update
*/
void Plane::parachute_check()
{
parachute.update();

// check if there is an emergency situation
parachute_emergency_check();
}

/*
Expand All @@ -19,7 +22,7 @@ void Plane::parachute_release()
if (parachute.released()) {
return;
}

// send message to gcs and dataflash
gcs_send_text(MAV_SEVERITY_CRITICAL,"Parachute: Released");

Expand Down Expand Up @@ -56,3 +59,62 @@ bool Plane::parachute_manual_release()

return true;
}

// Code to detect loss of control for ArduPlane
#define PARACHUTE_CHECK_TRIGGER_SEC 1 // 1 second of loss of control triggers the parachute
Copy link
Contributor

Choose a reason for hiding this comment

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

don't declare defines right in the middle of other code. Put it at the top.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. I was just replicating what was in ArduCopter/crash_check.cpp (although it probably has a define in the middle of the code because of the #if).

Choose a reason for hiding this comment

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

This message was created automatically by mail delivery software.

A message that you sent could not be delivered to one or more of its
recipients. This is a temporary error. The following address(es) deferred:

dupuyyann@gmail.com
Domain zyapps.com has exceeded the max emails per hour (454/400 (113%)) allowed. Message will be reattempted later

------- This is a copy of the message, including all the headers. ------
Received: from o3.sgmail.github.com ([192.254.112.98]:36272)
by serveur.ex2-p10.com with esmtps (TLSv1.2:ECDHE-RSA-AES128-GCM-SHA256:128)
(Exim 4.86_1)
(envelope-from bounces+848413-8f06-yann.dupuy=zyapps.com@sgmail.github.com)
id 1afmao-000dkX-52
for yann.dupuy@zyapps.com; Tue, 15 Mar 2016 11:52:58 +0100
DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=github.com;
h=from:reply-to:to:in-reply-to:references:subject:mime-version:content-type:content-transfer-encoding:list-id:list-archive:list-post:list-unsubscribe;
s=s20150108; bh=/b6sBdMZq6So4vxRhMaVo4dsq/8=; b=MjTDwnIWNgfgRXIl
nh/9kloCfSiQjqEWZCTY3PpwFs0vjbHwHyJc0sfu6twocqBxgWATD5mqJGm3H2wh
SMMV9Wv2WubDqpiYV0byC1CMra3O4GZgl3t06AIJHxGelp8aQA5wOvBQRLQtEvaF
499ZD/wdlMMNWjsia00DnyzpsrY=
Received: by filter0912p1mdw1.sendgrid.net with SMTP id filter0912p1mdw1.15600.56E7E95452
2016-03-15 10:52:04.880027198 +0000 UTC
Received: from github-smtp2a-ext-cp1-prd.iad.github.net (github-smtp2a-ext-cp1-prd.iad.github.net [192.30.253.16])
by ismtpd0001p1iad1.sendgrid.net (SG) with ESMTP id CPLwXusXRBW6ilvwUf0VsA
for yann.dupuy@zyapps.com; Tue, 15 Mar 2016 10:52:04.782 +0000 (UTC)
Date: Tue, 15 Mar 2016 03:52:04 -0700
From: Rimvydas Naktinis notifications@github.com
Reply-To: diydrones/ardupilot reply@reply.github.com
To: diydrones/ardupilot ardupilot@noreply.github.com
Message-ID: diydrones/ardupilot/pull/3703/r56144970@github.com
In-Reply-To: diydrones/ardupilot/pull/3703@github.com
References: diydrones/ardupilot/pull/3703@github.com
Subject: Re: [ardupilot] Plane: basic automatic emergency parachute release at
critical altitude error in AUTO. (#3703)
Mime-Version: 1.0
Content-Type: multipart/alternative;
boundary="--==_mimepart_56e7e9549f2f3_5c9a3fba4a0e12a01564035";
charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: list
X-GitHub-Sender: naktinis
X-GitHub-Recipient: yanndupuy
List-ID: diydrones/ardupilot <ardupilot.diydrones.github.com>
List-Archive: https://github.com/diydrones/ardupilot
List-Post: mailto:reply@reply.github.com
List-Unsubscribe: mailto:unsub+000df0dcc75a8087ef782feaa6fadc530f6ba412ab11ea4e92cf0000000112ffab5492a170ce0358b44a@reply.github.com,
https://github.com/notifications/unsubscribe/AA3w3Eu3IKrqr-1AoM-W3VkwVosm571Rks5pto9UgaJpZM4HouR4
X-Auto-Response-Suppress: All
X-GitHub-Recipient-Address: yann.dupuy@zyapps.com
X-SG-EID: /bDRkhSo/GH2gzZXMo5AKhbuTgLDPkexBjh/BYa140EGDZ0uXWBZO0LMe0b06Or59s7ROZux/f7I+n
6NxlOK2ULC9IpcBugC75z+KoVX4L9zIyj+da0CCR7uVf8jx44sxaEpeOpeLTn+Rfs0/gcoO7Flkk5H
mvuu9RFu9uTvxj5LbTtzi0wUYLLxGD7IZEAcl/W1/W6AyWv/F48BkEfz0w/y3GWE2V6f91GoqIKvAX
M=
X-Spam-Status: No, score=-1.4
X-Spam-Score: -13
X-Spam-Bar: -
X-Ham-Report: Spam detection software, running on the system "serveur.ex2-p10.com",
has NOT identified this incoming email as spam. The original
message has been attached to this so you can view it or label
similar future email. If you have any questions, see
root@localhost for details.

Content preview: > @@ -56,3 +59,55 @@ bool Plane::parachute_manual_release()
> > return true; > } > + > +// Code to detect loss of control for ArduPlane
> +#define PARACHUTE_CHECK_TRIGGER_SEC 1 // 1 second of loss of control triggers
the parachute [...]

Content analysis details: (-1.4 points, 5.0 required)

pts rule name description


-2.3 RCVD_IN_DNSWL_MED RBL: Sender listed at http://www.dnswl.org/, medium
trust
[192.254.112.98 listed in list.dnswl.org]
-0.0 RCVD_IN_MSPIKE_H3 RBL: Good reputation (+3)
[192.254.112.98 listed in wl.mailspike.net]
-0.0 RP_MATCHES_RCVD Envelope sender domain matches handover relay domain
0.0 HTML_MESSAGE BODY: HTML included in message
1.0 HTML_IMAGE_ONLY_16 BODY: HTML: images with 1200-1600 bytes of words
-0.1 DKIM_VALID_AU Message has a valid DKIM or DK signature from author's
domain
0.1 DKIM_SIGNED Message has a DKIM or DK signature, not necessarily valid
-0.1 DKIM_VALID Message has at least one valid DKIM or DK signature
-0.0 RCVD_IN_MSPIKE_WL Mailspike good senders
X-Spam-Flag: NO

----==_mimepart_56e7e9549f2f3_5c9a3fba4a0e12a01564035
Content-Type: text/plain;
charset=UTF-8
Content-Transfer-Encoding: 7bit

@@ -56,3 +59,55 @@ bool Plane::parachute_manual_release()

 return true;

}
+
+// Code to detect loss of control for ArduPlane
+#define PARACHUTE_CHECK_TRIGGER_SEC 1 // 1 second of loss of control triggers the parachute

You're right. I was just replicating what was in ArduCopter/crash_check.cpp (although it probably has a define in the middle of the code because of the #if).


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/diydrones/ardupilot/pull/3703/files#r56144970
----==_mimepart_56e7e9549f2f3_5c9a3fba4a0e12a01564035
Content-Type: text/html;
charset=UTF-8
Content-Transfer-Encoding: 7bit

In ArduPlane/parachute.cpp:

> @@ -56,3 +59,55 @@ bool Plane::parachute_manual_release()
>  
>      return true;
>  }
> +
> +// Code to detect loss of control for ArduPlane
> +#define PARACHUTE_CHECK_TRIGGER_SEC         1  // 1 second of loss of control triggers the parachute

You're right. I was just replicating what was in ArduCopter/crash_check.cpp (although it probably has a define in the middle of the code because of the #if).


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/diydrones/ardupilot/pull/3703/files#r56144970

----==_mimepart_56e7e9549f2f3_5c9a3fba4a0e12a01564035--


/*
parachute_emergency_check - trigger the release of the parachute
automatically if a critical situation is detected
*/
void Plane::parachute_emergency_check()
{
static uint32_t control_loss_ms; // time when we first lost control and never regained it
Copy link
Contributor

Choose a reason for hiding this comment

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

no statics allowed because it still creates only 1 if you have two classes of Plane (weird, I know, but maybe something will in the future). Please declare it in the class and rename it so it's global friendly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean it will be possible to create two instances of Plane class in the future? Why and how would that work – maybe there's some discussion about this somewhere I could read about it, if it has already been explained? I thought it should be some sort of singleton. Is it going to diverge from Copter architecture?

Static variables are all over the place in Plane, e.g.:
https://github.com/diydrones/ardupilot/blob/master/ArduPlane/navigation.cpp
https://github.com/diydrones/ardupilot/blob/master/ArduPlane/ArduPlane.cpp
https://github.com/diydrones/ardupilot/blob/master/ArduPlane/failsafe.cpp
https://github.com/diydrones/ardupilot/blob/master/ArduPlane/control_modes.cpp

And there are probably even more in Copter code. Is there a plan to refactor them?

I'm interested, because I was just following the existing architecture and not having a singleton Plane just sounds unintuitive (although, of course, might be wrong).

Copy link
Contributor

Choose a reason for hiding this comment

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

There is no "plan" but the concern is a relatively new one. Since ArduPilot is moving more and more towards Linux based systems you never know how the class ends up getting used! Not using static variables removes some chances of bugs. If things were to change it's easier to enforce it during development instead of all at once at the end. The other uses of static have been there a long time and should be changed too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why would Linux not allow using a singleton? Can you maybe point me to some existing Plane code where statics were replaced by globals? Is this move from static variables to globals documented anywhere in Ardupilot repository (GitHub issue, mailing list, etc)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there any example in existing code where this was already done, so I could replicate that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just make a variable in Plane.h and remove the static from here. Take a look at throttle_watt_limit_timer_ms

uint32_t now = millis();

// exit immediately if parachute or automatic release is not enabled, or already released
if (!parachute.enabled() || parachute.released() || g.parachute_auto_enabled == 0) {
control_loss_ms = 0;
return;
}

// only automatically release in AUTO mode
if (control_mode != AUTO) {
control_loss_ms = 0;
return;
}

// do not release if vehicle is not flying
if (!is_flying()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

is_flying may go false during a stall because both GPS ground speed and airspeed drop to low numbers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. I think it should be safe to remove this check, because we also have the relative_altitude check below, which ensures that we are in the air.

Also, since Plane::parachute_manual_release() also prevents parachute deployment when not is_flying() maybe it should be removed from there as well? @tridge

Copy link
Contributor

Choose a reason for hiding this comment

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

you're probably right. The normal use-case for a manual release is a planned one while flying, but if you manually do it on the way down it sure would be nice for it to work!

control_loss_ms = 0;
return;
}

// do not release if taking off or landing
if (auto_state.takeoff_complete == false || mission.get_current_nav_cmd().id == MAV_CMD_NAV_LAND) {
control_loss_ms = 0;
return;
}

if (relative_altitude() < parachute.alt_min()) {
control_loss_ms = 0;
return;
}

// do not release if we are flying within given error
if (altitude_error_cm < g.parachute_auto_error * 100.0f) {
control_loss_ms = 0;
return;
}

// at this point we consider control lost
if (control_loss_ms == 0) {
gcs_send_text_fmt(MAV_SEVERITY_WARNING, "Crash: Starting to lose control, %d m error", altitude_error_cm / 100);
control_loss_ms = now;
} else if (now - control_loss_ms > PARACHUTE_CHECK_TRIGGER_SEC * 1000) {
// we have lost control for too long
gcs_send_text_fmt(MAV_SEVERITY_WARNING, "Crash: Critical altitude error %d m", altitude_error_cm / 100);
Copy link
Contributor

Choose a reason for hiding this comment

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

use *0.01f instead of /100, it's computationally much easier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I'll change it. However, these messages are sent very very rarely and this division operation should be negligible compared to all the function calls, string formatting, and sending it to the GCS. I just thought division was more readable for me.

parachute_release();
control_loss_ms = 0;
}
}