Add AP_INVERTED_FLIGHT_ENABLED #24332

peterbarker · 2023-07-17T10:58:37Z

Not so much for the space, but to avoid accidents.

Board                    AP_Periph  blimp  bootloader  copter  heli  iofirmware  plane  rover  sub
CubeOrange-periph-heavy  *
Durandal                            *      *           *       *                 *      *      *
Hitec-Airspeed           *
KakuteH7-bdshot                     *      *           *       *                 *      *      *
MatekF405                           *      *           *       *                 *      *      *
Pixhawk1-1M-bdshot                  *                  *       *                 *      *      *
f103-QiotekPeriph        *
f303-Universal           *
iomcu                                                                *
revo-mini                           *      *           *       *                 *      *      *
skyviper-v2450                                         *

... and with it disabled:

Board                    AP_Periph  blimp  bootloader  copter  heli  iofirmware  plane  rover  sub
CubeOrange-periph-heavy  *                                                                     
Durandal                            -8     *           -16     -512              -176   -8     -24
Hitec-Airspeed           *                                                                     
KakuteH7-bdshot                     -8     *           -16     -448              -392   0      -16
MatekF405                           -16    *           -32     -440              -416   -16    -32
Pixhawk1-1M-bdshot                  -16                -32     -448              -432   -16    -32
f103-QiotekPeriph        *                                                                     
f303-Universal           *                                                                     
iomcu                                                                *                         
revo-mini                           -16    *           -24     -440              -424   -16    -32
skyviper-v2450                                         -24

Hwurzburg

if you dont have the action for an RC switch shouldn't you remove the init so as to cause a prearm?

peterbarker · 2023-07-18T04:24:50Z

if you dont have the action for an RC switch shouldn't you remove the init so as to cause a prearm?

Quite right. Fixed.

peterbarker · 2023-07-18T22:37:39Z

Tested in SITL, both with and without compiled in, without issue.

tridge · 2023-07-26T08:50:48Z

be good to test inverted in manual with code compiled out, and change to FBWA and it recovers with correct elevator direction

peterbarker · 2023-08-27T05:25:14Z

flight tested and elevator does right thing

ArduCopter/Copter.h

libraries/AP_Motors/AP_MotorsHeli.h

peterbarker · 2023-08-29T02:41:36Z

@bnsgeyer I've had a look at this and I don't think we really want to not commit the Copter changes as part of this. We would end up with the Copter code trying to set state in the motors library which doesn't exist, so we'd not compile any more with the new option used (breaking CI IIRC).

Could you have a think about these changes and OK them at some stage, please?

agreed on DevCall that it's OK to lose these bits in their entirety (they're only used internally)

peterbarker · 2024-03-06T01:56:15Z

Ping @bnsgeyer @IamPete1

I've rebased this on top of the recent inverted-flight patches. There's now fewer patches in here.

IamPete1

I like it.

IamPete1 · 2024-03-06T02:24:53Z

libraries/RC_Channel/RC_Channel.h

@@ -144,7 +145,9 @@ class RC_Channel {
        AVOID_PROXIMITY =     40, // enable object avoidance using proximity sensors (ie. horizontal lidar)
        ARMDISARM_UNUSED =    41, // UNUSED
        SMART_RTL =           42, // change to SmartRTL flight mode
+#if AP_INVERTED_FLIGHT_ENABLED


This is the first example of using a define to remove something from the enum. This would start a new pattern.

I do think it is a good idea. The "KILL_IMU" option would be a good candidate too.

True.

However, we do it a lot in other enumerations, and I'd put down a little bit of money we'll find at least one thing that's Not Quite when we gate them.

IamPete1 · 2024-03-06T02:29:24Z

ArduPlane/tailsitter.cpp

+#if AP_INVERTED_FLIGHT_ENABLED
        plane.nav_pitch_cd = constrain_float(fw_transition_initial_pitch - (quadplane.tailsitter.transition_rate_fw * dt) * 0.1f * (plane.fly_inverted()?-1.0f:1.0f), -8500, 8500);
+#else
+        plane.nav_pitch_cd = constrain_float(fw_transition_initial_pitch - (quadplane.tailsitter.transition_rate_fw * dt) * 0.1f, -8500, 8500);
+#endif


I would prefer to pull out either the sign or the fly inverted bool and keep just one line of the constrain. I think the maths is easier to understand if there is only one copy.

Maybe we could take the opportunity to break it up a little.

Suggested change

#if AP_INVERTED_FLIGHT_ENABLED

plane.nav_pitch_cd = constrain_float(fw_transition_initial_pitch - (quadplane.tailsitter.transition_rate_fw * dt) * 0.1f * (plane.fly_inverted()?-1.0f:1.0f), -8500, 8500);

#else

plane.nav_pitch_cd = constrain_float(fw_transition_initial_pitch - (quadplane.tailsitter.transition_rate_fw * dt) * 0.1f, -8500, 8500);

#endif

float pitch_ramp = quadplane.tailsitter.transition_rate_fw * dt * 0.1;

#if AP_INVERTED_FLIGHT_ENABLED

if (plane.fly_inverted()) {

// Transitioning to inverted flight, pitch up rather than down

pitch_ramp *= -1.0;

}

#endif

plane.nav_pitch_cd = constrain_float(fw_transition_initial_pitch - pitch_ramp, -8500, 8500);

Maybe we could take the opportunity to break it up a little.

Yep, I did that, and random other changes like it.

And reverted those changes, because I really like to keep these add-defines things as completely no compiler output change in the case that you have not turned the option on. It means that no existing user will ever run into problems with the code.

I've achieved that with this PR now, and updated the blocks in the opening comment appropriately.

It does uglify the code a bit, but I do plan on going back to the places and cleaning it up by factoring bits and pieces. Especially the bit in takeoff.cpp. The block that patch is in will make for a nice new method with earl returns etc, something like:

if (do_takeoff_attitude_check && !takeoff_attitude_is_good()) { gcs().send_text(MAV_SEVERITY_WARNING, "Bad launch AUTO"); takeoff_state.accel_event_counter = 0; goto no_launch; } }

// Check aircraft attitude for bad launch bool takeoff_attitude_is_good(void) const { if (ahrs.pitch_sensor <= -3000 || ahrs.pitch_sensor >= 4500) { return false; } #if AP_INVERTED_FLIGHT_ENABLED if (fly_inverted()) { // skip roll check if we're *supposed* to be upside-down return true; } #if endif if (labs(ahrs.roll_sensor) > 3000) { return false; } return true; } }

I think that's the same logic, but I'm not convinced it's good logic.... e.g. should those pitch limits be the other way around if you're inverted? Why aren't we checking roll when inverted?

Give I think we now have a partner who flies this way, we should probably give it more serious consideration now, and I think this would make for a good separate PR...

I've created a patch on top of this over here which shows things prettied up a bit:
https://github.com/ArduPilot/ardupilot/compare/master...peterbarker:ardupilot:pr/inverted-flight-tidy?expand=1

… capability

peterbarker added the NeedsTesting label Jul 17, 2023

Hwurzburg requested changes Jul 17, 2023

View reviewed changes

peterbarker force-pushed the pr/inverted-flight-enabled branch from 909d0d4 to 2fd262e Compare July 18, 2023 04:24

peterbarker requested a review from Hwurzburg July 18, 2023 04:25

peterbarker added the DevCallEU label Jul 18, 2023

Hwurzburg approved these changes Jul 18, 2023

View reviewed changes

peterbarker removed the NeedsTesting label Jul 18, 2023

tridge added NeedsTesting and removed DevCallEU labels Jul 26, 2023

peterbarker added DevCallTopic and removed NeedsTesting labels Aug 27, 2023

tridge previously requested changes Aug 27, 2023

View reviewed changes

ArduCopter/Copter.h Outdated Show resolved Hide resolved

libraries/AP_Motors/AP_MotorsHeli.h Outdated Show resolved Hide resolved

CraigElder removed the DevCallTopic label Aug 28, 2023

peterbarker requested a review from bnsgeyer August 29, 2023 02:41

peterbarker force-pushed the pr/inverted-flight-enabled branch from 2fd262e to 85ff579 Compare November 21, 2023 02:58

peterbarker force-pushed the pr/inverted-flight-enabled branch 3 times, most recently from 031d424 to 307bcd4 Compare March 6, 2024 00:03

IamPete1 reviewed Mar 6, 2024

View reviewed changes

peterbarker force-pushed the pr/inverted-flight-enabled branch 3 times, most recently from ead7871 to 2b89777 Compare March 6, 2024 11:49

peterbarker force-pushed the pr/inverted-flight-enabled branch from 2b89777 to 355df40 Compare April 3, 2024 01:18

Hwurzburg added the WikiNeeded needs wiki update label Apr 3, 2024

peterbarker added 6 commits April 12, 2024 12:40

RC_Channel: add compile-time option to remove inverted-flight capability

3f2e600

AC_AttitudeControl: add compile-time option to remove inverted-flight…

e0d1713

… capability

AP_HAL: add compile-time option to remove inverted-flight capability

759b8af

AP_Mission: add compile-time option to remove inverted-flight capability

10c8cc0

ArduCopter: add compile-time option to remove inverted-flight capability

91ea452

ArduPlane: add compile-time option to remove inverted-flight capability

a33c31c

peterbarker force-pushed the pr/inverted-flight-enabled branch from 355df40 to a33c31c Compare April 12, 2024 02:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add AP_INVERTED_FLIGHT_ENABLED #24332

Add AP_INVERTED_FLIGHT_ENABLED #24332

peterbarker commented Jul 17, 2023 •

edited

Loading

Hwurzburg left a comment

peterbarker commented Jul 18, 2023

peterbarker commented Jul 18, 2023

tridge commented Jul 26, 2023

peterbarker commented Aug 27, 2023

peterbarker commented Aug 29, 2023

peterbarker commented Mar 6, 2024

IamPete1 left a comment

IamPete1 Mar 6, 2024

peterbarker Mar 6, 2024

IamPete1 Mar 6, 2024

IamPete1 Mar 6, 2024

peterbarker Mar 6, 2024

peterbarker Mar 7, 2024

-#if AP_INVERTED_FLIGHT_ENABLED
-        plane.nav_pitch_cd = constrain_float(fw_transition_initial_pitch - (quadplane.tailsitter.transition_rate_fw * dt) * 0.1f * (plane.fly_inverted()?-1.0f:1.0f), -8500, 8500);
-#else
-        plane.nav_pitch_cd = constrain_float(fw_transition_initial_pitch - (quadplane.tailsitter.transition_rate_fw * dt) * 0.1f, -8500, 8500);
-#endif
+        float pitch_ramp = quadplane.tailsitter.transition_rate_fw * dt * 0.1;
+#if AP_INVERTED_FLIGHT_ENABLED
+        if (plane.fly_inverted()) {
+            // Transitioning to inverted flight, pitch up rather than down
+            pitch_ramp *= -1.0;
+        }
+#endif
+        plane.nav_pitch_cd = constrain_float(fw_transition_initial_pitch - pitch_ramp, -8500, 8500);

Add AP_INVERTED_FLIGHT_ENABLED #24332

Are you sure you want to change the base?

Add AP_INVERTED_FLIGHT_ENABLED #24332

Conversation

peterbarker commented Jul 17, 2023 • edited Loading

Hwurzburg left a comment

Choose a reason for hiding this comment

peterbarker commented Jul 18, 2023

peterbarker commented Jul 18, 2023

tridge commented Jul 26, 2023

peterbarker commented Aug 27, 2023

peterbarker commented Aug 29, 2023

peterbarker commented Mar 6, 2024

IamPete1 left a comment

Choose a reason for hiding this comment

IamPete1 Mar 6, 2024

Choose a reason for hiding this comment

peterbarker Mar 6, 2024

Choose a reason for hiding this comment

IamPete1 Mar 6, 2024

Choose a reason for hiding this comment

IamPete1 Mar 6, 2024

Choose a reason for hiding this comment

peterbarker Mar 6, 2024

Choose a reason for hiding this comment

peterbarker Mar 7, 2024

Choose a reason for hiding this comment

peterbarker commented Jul 17, 2023 •

edited

Loading