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

[naktinis]

This is essentially an implementation of what is described in #2553 (comment).

[magicrub]

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.

[naktinis]

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).

[magicrub]

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.

[naktinis]

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)?

[naktinis]

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

[magicrub]

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

[magicrub]

Thanks for removing is_flying(). The no-statics thing is a request from @tridge because it only has drawbacks (future bugs) where moving it to the class gives flexibility. All static variables in the project need to be eventually moved to the class. Pull-requests welcome!

[magicrub]

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

[naktinis]

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

[magicrub]

into AP_Parachute

[magicrub]

the k_params are not needed if you are within a lib

[magicrub]

I'm still in favor of all of this going into AP_Parachute. Any params that start with CHUTE_ belong there too, that's why there's a "CHUTE_" prefixed one to sort them.

[naktinis]

But since AP_Parachute is used in both Copter and Plane, how do I add plane-specific code to it? Is there any convention for this?

[magicrub]

Use AP_Arming as an example

[naktinis]

Do you mean the AP_Arming_Plane class that is defined in Plane.h for plane-specific arming code?

[naktinis]

@magicrub can you please clarify how I should use AP_Arming as an example of Plane-specific code in a common library? Did you mean defining a separate class as it is done with AP_Arming_Plane in Plane.h (e.g. defining AP_Parachute_Plane in Plane.h)?

[magicrub]

Move these to line 54 of this
https://github.com/diydrones/ardupilot/blob/master/libraries/AP_Parachute/AP_Parachute.cpp

[naktinis]

I think I have addressed all issues so far. Could someone please have a look once again?

[magicrub]

Won't this message, and the release, happen every second after the first release happens?

[naktinis]

I think that parachute.released() check should kick in after parachute_release() (line 74).

[magicrub]

Ahh yes, my mistake I see it now.

[naktinis]

What's left to be done for this?

[magicrub]

would be nice for @tridge to take a look

[naktinis]

@magicrub I've made some changes after your latest suggestions. Let me know if I missed something or if there are other things that need to be fixed.

Also, somewhat relevant to these changes (although not strictly blocking) are #3837 (making CHUTE_ALT_MIN above ground, not above home), #3853 (ALT_OFFSET incorporation), and #3861 (parachute deployment timing relative to throttle suppression).

[naktinis]

Do we need to change anything else before this can be accepted?

[magicrub]

I'd like @tridge to take a look before we merge, but otherwise I think it looks good.

[naktinis]

@tridge could you have a look at this at some point?

[magicrub]

Still needs some work. Looks way too easy to trigger during normal flight by simply diving downwards

[naktinis]

Which trigger do you have in mind – altitude or sink rate? As the parachute only works in AUTO mode, the diving downwards needs to be part of a mission.

Sink rate CHUTE_AUTO_SINK should normally be set to a value greater than TECS_SINK_MAX (that means the plane is outside the sane planned boundaries when the parachute is deployed).

CHUTE_AUTO_ALT should be at least 10-20 m lower than the lowest point of the mission (except before and after take-off).

We've tested a similar functionality (the branch I've mentioned previously) with a Penguin B airframe and it seems to be working fine. One can also switch automatic parachute deployment (or one of the triggers) off.

Which scenario are you worried about?

[magicrub]

We're just concerned that this will cause chute deployments during normal operation, goal is to make this bulet-proof so that it will only deploy when you're truly in distress.

CHUTE_AUTO_ALT should be at least 10-20 m lower than the lowest point of the mission (except before and after take-off).

Waypoints can be at any altitude. Simply being below it, or even having an alt error, does mean you're in distress and need to deploy the chute. What happens if you fly auto, then switch to manual/stab/fbwa to fly around and are in a dive while switching back to auto. Boom, your chute deploys. Remember, if you have a DO_LAND_START in your mission then an RTL switches you to AUTO and switching to RTL is common when trying to recover their aircraft with loss of orientation.

Sink rate CHUTE_AUTO_SINK should normally be set to a value greater than TECS_SINK_MAX

This is good but needs to be strictly enforced in case the aircraft is not configured correctly. something like auto_state.sink_rate > MAX(CHUTE_AUTO_SINK+2, TECS_SINK_MAX).

Which trigger do you have in mind

A trigger that can only happen when we've truly lost control and are falling out of the sky in AUTO. That is not an easy thing to determine. Perhaps we can draw some ideas from #2933 .

[magicrub]

perhaps we could take advantage of the TECS controller's badDescent flag

[magicrub]

last I talked to @tridge about this, he was not happy with the concept in general. @tridge is there anything that can be done to make this work for you?

[magicrub]

This was discussed in today's developer's call. We'd like this integrated with the TECS to detect if the aircraft is not achieving the desired pitch/roll values which is a good indicator that the aircraft is in distress. There's also a is_stalled type flag in TECS. The quadplane has (or will have) similar needs where the quad motor kicks in if we begin to stall so detecting a true stall is the hard part. False positives here are bad news so we have to make sure this is done right.

[magicrub]

I think if we added a status flag in TECS of "we're in trouble and/or stalling" that could be checked by this feature then we're probably good to bring this in. I'll bring it up at the dev call tomorrow, although @tridge will not be there (he's rock'n ArduPilot at the outback Challenge this week!)

[bugalaws]

Hello guys, these feature was abandoned? It's a very important feature!!

[peterbarker]

@viniciusknabben could you give an evaluation as to whether #3703 removes the utility of this PR, or whether an additional check might be worthwhile, please?

[viniciusknabben]

@viniciusknabben could you give an evaluation as to whether #3703 removes the utility of this PR, or whether an additional check might be worthwhile, please?

@peterbarker do you mean whether #9506 removes the utility of this PR?

[viniciusknabben]

I think there is useful information like the parachute release with an altitude error.

in cases the plane is falling with stall prevention it may not trigger the critical sink and end up crashing

The general idea i think is pretty much the same

@magicrub bring up that the is_flying flag may deny the parachute opening during a stall: #3703 (comment)

the delay to remove miss triggering was suggested by @tridge and i've made the changes in 76164cd

[amilcarlucas]

does need a rebase now.

[moreba1]

What is the result ? @magicrub @naktinis @tridge

[davidbuzz]

I think this could be closed now, and maybe if there's anything in thats 'special' it could use scripting. discussed briefly with tridge and he agreed.