4.2 autorotation #3

bnsgeyer · 2022-04-24T13:31:48Z

Allows me to see combined changes without putting a PR against the Ardupilot repo

MattKear · 2022-04-24T13:51:25Z

ArduCopter/Copter.cpp

@@ -125,7 +125,7 @@ const AP_Scheduler::Task Copter::scheduler_tasks[] = {
    SCHED_TASK(auto_disarm_check,     10,     50,  27),
    SCHED_TASK(auto_trim,             10,     75,  30),
 #if RANGEFINDER_ENABLED == ENABLED
-    SCHED_TASK(read_rangefinder,      20,    100,  33),
+    SCHED_TASK(read_rangefinder,      100,    100,  33),


I don't think we will get away with a straight change to 100 Hz. I think the best we can hope for is to make this a param value that defaults to 20.

MattKear · 2022-04-24T14:03:19Z

libraries/AC_Autorotation/AC_Autorotation.cpp

+{
+	 _current_rpm = get_rpm(true);
+	 if ((_param_head_speed_set_point - _current_rpm) >=0){
+				_rpm_decay = constrain_float((_param_head_speed_set_point -  _current_rpm)/400.0f, 0.0f, 1.0f);


We shouldn't assume 400Hz

MattKear · 2022-04-24T14:15:00Z

ArduCopter/mode_autorotate.cpp

+    float pitch_vel2 = MIN(fabsf(pitch_vel), 2000);
+    pilot_yaw_rate = ((float)pilot_roll/1.0f) * (1.0f - (pitch_vel2 / 5000.0f)) * g2.acro_y_rate; //****THIS NEEDS FIXED*****
+
+    roll_vel = constrain_float(roll_vel, -560.0f, 560.0f);


We shouldn't hard code constrains. We also don't need to. The user defined input shaping will be done in the attitude control library

MattKear · 2022-04-24T14:15:53Z

ArduCopter/mode_autorotate.cpp

+    float pitch_vel = vel.y * ahrs.sin_yaw() + vel.x * ahrs.cos_yaw(); // body pitch vel
+
+    // gain scheduling for yaw
+    float pitch_vel2 = MIN(fabsf(pitch_vel), 2000);


I would really like to try the weather vaneing library for doing the coordinated turns. Would reduce this code and minimise duplication.

yes, this was to do some testing of the control logic. Having the nose aligned onto velocity vector would be a great improvement both for avoiding sideslip during glide and for precise skid aligning during landing.

MattKear · 2022-04-24T14:18:07Z

ArduCopter/mode_autorotate.cpp

@@ -66,6 +61,10 @@ bool ModeAutorotate::init(bool ignore_checks)

    // The decay rate to reduce the head speed from the current to the target
    _hs_decay = ((_initial_rpm/g2.arot.get_hs_set_point()) - HEAD_SPEED_TARGET_RATIO) / AUTOROTATE_ENTRY_TIME;
+
+    //set motors to idle
+    copter.ap.motor_interlock_switch = false;


This looks like a big no no to me. We should not be setting the interlock from the code. Why do we need to do this anyway?

this is specific for ICE in case of "real" emergency. It is safer to have the ECU set to throttle-off position in case of in-flight shutdown/flameout.

MattKear · 2022-04-24T14:24:13Z

libraries/AC_Autorotation/AC_Autorotation.cpp

+    _accel_target = _vel_p ;
+
+    // filter correction acceleration
+    _accel_target_filter.set_cutoff_frequency(10.0f);


Don't think we should be hard coding filters. Depends very much on the users setup as to what cutt offs should be set.

MattKear · 2022-04-24T14:27:23Z

libraries/AC_Autorotation/AC_Autorotation.cpp

+		        _accel_target = _accel_out_last + _accel_max;
+      } else if (_accel_target < _accel_out_last - _accel_max) {
+		        _accel_target = _accel_out_last - _accel_max;
+	 }


nit pick: can just use a constrain here

MattKear · 2022-04-24T14:33:11Z

libraries/AP_Motors/AP_MotorsHeli_RSC.cpp

-                _control_output = constrain_float( _rsc_arot_bailout_pct/100.0f , 0.0f, 0.4f);
+                _control_output = constrain_float( _ext_gov_arot_pct/100.0f , 0.0f, 0.4f);
+                if(!_autorotating){
+                    gcs().send_text(MAV_SEVERITY_CRITICAL, "Autorotation");


Don't think we should be doing this at the motors level. This should be up at vehicle level

MattKear · 2022-04-24T14:36:23Z

libraries/AP_Motors/AP_MotorsHeli_RSC.cpp

-            _rotor_ramp_output = _rotor_runup_output;
+        if (_use_bailout_ramp){
+            if(!_bailing_out){
+                gcs().send_text(MAV_SEVERITY_CRITICAL, "bailing_out");


Again, not sure this is a motors level thing. We should put this stuff up in heli. Also not MAV_SEVERITY_CRITICAL

MattKear · 2022-04-24T14:38:48Z

libraries/AP_Motors/AP_MotorsHeli_Single.cpp

        if (_tail_type == AP_MOTORS_HELI_SINGLE_TAILTYPE_DIRECTDRIVE_VARPIT_EXT_GOV) {
            _tail_rotor.set_autorotation_flag(_heliflags.in_autorotation);
+            _tail_rotor.use_bailout_ramp_time(_heliflags.enable_bailout);


Surely we don't need to change tail rotor ramp? It should not have spooled down

I think the tail type is not right here. The spool down for tail motors should happen only for the direct drive fixed pitch.

bnsgeyer

My first round of reviews. Looked mainly at files changed in the Arducopter folder. Will provide follow on reviews for the remaining files.

bnsgeyer · 2022-10-02T15:26:07Z

ArduCopter/heli.cpp

@@ -205,31 +205,38 @@ void Copter::heli_update_autorotation()
        }


Is it ok that the autorotate mode is set every time this method is ran. I'm guessing the set_mode method just ignores this since the mode is already set?

bnsgeyer · 2022-10-02T15:29:16Z

ArduCopter/heli.cpp

+        int32_t gnd_dist = flightmode->get_alt_above_ground_cm();
+
+		// use rangefinder if available
+		switch (rangefinder.status_orient(ROTATION_PITCH_270)) {


would this orientation always be ROTATION_PITCH_270 for downward facing? This sounds like it could be setup specific?

I think it is always ROTATION_PITCH_270 for downward facing rangefinders ( RNGFNDx_ORIENT = 25)

bnsgeyer · 2022-10-02T16:10:17Z

ArduCopter/mode_autorotate.cpp

@@ -134,25 +193,29 @@ void ModeAutorotate::run()

            }

-            // Slowly change the target head speed until the target head speed matches the parameter defined value
-            if (g2.arot.get_rpm() > HEAD_SPEED_TARGET_RATIO*1.005f  ||  g2.arot.get_rpm() < HEAD_SPEED_TARGET_RATIO*0.995f) {
+            if(!hover_autorotation){ 


It would be good that you put a comment in here that explains what is being done in this logic for whether it is a hover autorotation or not.

bnsgeyer · 2022-10-02T16:47:35Z

ArduCopter/mode_autorotate.cpp

+               // Set target head speed in head speed controller
+               g2.arot.set_target_head_speed(_target_head_speed);
+               // Run airspeed/attitude controller
+               g2.arot.set_dt(G_Dt);


Why is Dt being set here? Why not initialize the library with Dt? just wondering if we need to set Dt on every cycle. Could put dt as a argument for the update_forward_speed_controller.

bnsgeyer · 2022-10-02T16:58:37Z

ArduCopter/mode_autorotate.cpp

+               // Retrieve pitch target
+               _pitch_target = g2.arot.get_pitch();
+               // Update controllers
+              _flags.bad_rpm = g2.arot.update_hs_glide_controller(G_Dt); //run head speed/ collective controller


Dt is passed to the Update_hs_glide_controller here. Seems like Dt should be passed in initially and then it doesn't need to be here.

bnsgeyer

Finished my review. Once we have addressed my comments then I will put in a PR and get randy involved

bnsgeyer · 2022-10-02T18:53:29Z

libraries/AC_Autorotation/AC_Autorotation.cpp

 #define AP_FW_VEL_FF                      0.15f
+#define AP_FLARE_ALT                      800
+#define AP_T_TO_G                            0.55f
+#define GUIDED                                   0


 const AP_Param::GroupInfo AC_Autorotation::var_info[] = {


ENABLE param has the wrong DisplayName
Need to discuss parameter names
consistency with abbreviation for Bail out in names (BAIL vs BLOUT)
consistent with abbreviating Target with other libraries (TGT vs TARG)
T_TO_G maybe this should be TIME_GND
FW_V_FF maybe this should be FWD_VEL_FF
FW_V_P maybe this should be FWD_VEL_P
AS_ACC_MAX maybe this should be FWD_ACC_MAX (we get 16 char so this would be 16 with the AROT_)

bnsgeyer · 2022-10-02T19:16:24Z

libraries/AP_Motors/AP_MotorsHeli_RSC.cpp

-        if (_rotor_runup_output < _rotor_ramp_output) {
-            _rotor_runup_output = _rotor_ramp_output;
-        }
+        _rotor_runup_output = _rotor_ramp_output;


This is not allowed. the Run up timer must count down to allow rotor rundown on the ground to judge when it is safe to disarm.

bnsgeyer · 2022-10-02T19:33:11Z

libraries/AP_Motors/AP_MotorsHeli_Single.cpp

+        _main_rotor.set_autorotation_flag(_heliflags.in_autorotation);
+        // set bailout ramp time
+        _main_rotor.use_bailout_ramp_time(_heliflags.enable_bailout);
+        _tail_rotor.use_bailout_ramp_time(_heliflags.enable_bailout);


why is this set here and also in the if statement below. I need to understand when this needs to be used. Is this covering practice autorotations. I guess if it is a bailout then yes. So are we assuming that the DDVP will have the tail rotor shutdown during practice autorotation?

This follows the original implementation, I don't thinkf DDVP tail rotor should be shut down in autorotation (whether practice or autonomous), directional control shall always be allowed.
The only case in which tail rotor should be shutdown is DDFP, as it relies on main rotor torque to achieve control in both directions

* Update SIM_StratoBlimp.cpp * Update SIM_StratoBlimp.h

MattKear reviewed Apr 24, 2022

View reviewed changes

bnsgeyer force-pushed the 4.2_autorotation branch 2 times, most recently from c179b89 to 17677f1 Compare April 27, 2022 02:21

bnsgeyer force-pushed the 4.2_autorotation branch from 1f5c641 to f5e9ba7 Compare July 24, 2022 02:41

bnsgeyer commented Oct 2, 2022

View reviewed changes

bnsgeyer force-pushed the 4.2_autorotation branch from dbe9afc to 5d361bd Compare January 17, 2023 04:08

Ferruccio1984 and others added 4 commits December 15, 2023 16:09

AC_Autorotation: Autorotation implementation

1c4fc25

Copter: Autorotation implementation

2709e6f

SITL: fix vertical axis dynamics for autorotation

5c593f3

Tools: update autotest to work with heli SITL model changes

2b458fb

bnsgeyer force-pushed the 4.2_autorotation branch 2 times, most recently from 3805b6f to 2b458fb Compare December 15, 2023 23:35

bnsgeyer changed the base branch from Copter-4.2 to master December 15, 2023 23:39

bnsgeyer pushed a commit that referenced this pull request May 14, 2024

SITL: modifications to lift in SIM_StratoBlimp (#3)

33892aa

* Update SIM_StratoBlimp.cpp * Update SIM_StratoBlimp.h

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

4.2 autorotation #3

4.2 autorotation #3

bnsgeyer commented Apr 24, 2022

MattKear Apr 24, 2022

MattKear Apr 24, 2022

MattKear Apr 24, 2022

MattKear Apr 24, 2022

Ferruccio1984 Apr 24, 2022

MattKear Apr 24, 2022

Ferruccio1984 Apr 24, 2022

MattKear Apr 24, 2022

MattKear Apr 24, 2022

MattKear Apr 24, 2022

MattKear Apr 24, 2022

MattKear Apr 24, 2022

Ferruccio1984 Apr 24, 2022

bnsgeyer left a comment

bnsgeyer Oct 2, 2022

bnsgeyer Oct 2, 2022

Ferruccio1984 Oct 3, 2022

bnsgeyer Oct 2, 2022

bnsgeyer Oct 2, 2022

bnsgeyer Oct 2, 2022

bnsgeyer left a comment

bnsgeyer Oct 2, 2022

bnsgeyer Oct 2, 2022

bnsgeyer Oct 2, 2022

Ferruccio1984 Oct 3, 2022

		@@ -205,31 +205,38 @@ void Copter::heli_update_autorotation()
		}

4.2 autorotation #3

Are you sure you want to change the base?

4.2 autorotation #3

Conversation

bnsgeyer commented Apr 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnsgeyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnsgeyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment