4.2 autorotation by bnsgeyer · Pull Request #3 · bnsgeyer/ardupilot

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

    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

+{
+	 _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

+    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

+    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

    _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

+    _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

+		        _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

-                _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

-            _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

+        _tail_rotor.use_bailout_ramp_time(_heliflags.enable_bailout);
        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

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

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

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

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

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

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

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

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

Conversation

bnsgeyer commented Apr 24, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnsgeyer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnsgeyer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants