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

Implement assisted autorotation for Heli’s #22871

Open
wants to merge 61 commits into
base: master
Choose a base branch
from

Conversation

bnsgeyer
Copy link
Contributor

@bnsgeyer bnsgeyer commented Feb 8, 2023

This PR incorporates helicopter auto rotation, where it assists the pilot in the vertical axis and maintaining speed from entry all the way to touchdown. The pilot still maintains control of the lateral and directional axes to guide the aircraft to a safe touchdown point.

@bnsgeyer bnsgeyer self-assigned this Feb 8, 2023
@bnsgeyer bnsgeyer force-pushed the pr-autorotation branch 5 times, most recently from 6d4ddd6 to d87df0c Compare February 13, 2023 23:41
ArduCopter/Copter.cpp Outdated Show resolved Hide resolved
Copy link
Member

@IamPete1 IamPete1 left a comment

Choose a reason for hiding this comment

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

White space needs fixing throughout.

Guided stuff is a little odd taking roll only, what the use case?

Speeding up the rangefinder read probably has subtle unintended consequences, things like filters now having a different frequency due to the DT change.

@bnsgeyer
Copy link
Contributor Author

@IamPete1 the idea behind the guided stuff is that the autorotational controller will control the vertical and pitch axes to control forward speed and descent rate. So the pilot or autopilot only needs to control the roll and yaw axes

@IamPete1
Copy link
Member

The guided stuff is adding support for setting roll via the SET_ATTITUDE_TARGET MAVLink message? Is that how the pilot is controlling rather than via RC? As I say its a little odd.

@bnsgeyer
Copy link
Contributor Author

The guided stuff is adding support for setting roll via the SET_ATTITUDE_TARGET MAVLink message? Is that how the pilot is controlling rather than via RC? As I say its a little odd.

Ok. Good point.
@Ferruccio1984 what is the purpose of the guided mode stuff that you added?

@Ferruccio1984
Copy link
Contributor

Hi @bnsgeyer and @IamPete1 , the reason behind the guided stuff is BVLOS operation. In my use case most of the flight is performed in guided mode and the operator behind the Ground Station has a joystick for manual take-over ( which happens still in guided flight mode, joystick inputs are translated into attitude targets). Following this logic the operator is able to steer the helicopter also in case of autorotation, choosing the best landing spot through live Fpv feedback from the drone. The guided stuff is optional, so I coded it to allow traditional RC control or guided control.

@IamPete1
Copy link
Member

IamPete1 commented Feb 27, 2023

I think your probability better to drop the guided stuff from this PR and move it to a future one. As I say its a little odd and removing it make this PR more self contained in heli and auto-rotation code so will need less testing.

I think the same thing is probably true of the rangefinder speed. I think you would at least need to do something to rate limit logs. Lots of the back ends do averaging, you may be able to get it to work just as well by providing a new handle to get the min distance or just the latest in the last loop. Removing the averageing will remove 25ms worth of lag.

@Hwurzburg Hwurzburg added the WikiNeeded needs wiki update label Feb 28, 2023
@tridge
Copy link
Contributor

tridge commented Feb 28, 2023

it is worth looking at the XKF5.HAGL value as a possible height above ground

@bnsgeyer
Copy link
Contributor Author

@Ferruccio1984 See @tridge suggestion above for a EKF signal that could provide a higher quality height estimate that is based off of the rangefinder and other sources including providing some reduction in lag that you will see with rangefinders. If it looks suitable then we can add it in and remove the code that changes the the scheduler rate.

I will pull out the guided mode stuff and clean up the indentation problems.

@Ferruccio1984
Copy link
Contributor

Ferruccio1984 commented Feb 28, 2023

it is worth looking at the XKF5.HAGL value as a possible height above ground

Hi @tridge , I had a look again at last summer test flights, comparing the rangefinder data ( scheduler rate of 100Hz) and the HAGL:
RFNDvsHAGL

@bnsgeyer
Copy link
Contributor Author

bnsgeyer commented Mar 1, 2023

@tridge based on the lag that we are seeing on the HAGL signal, I don’t think it is usable for this application. Please post a link to your branch that uses a complementary filter. Thanks!

@bnsgeyer bnsgeyer force-pushed the pr-autorotation branch 2 times, most recently from b9871c9 to 3870277 Compare June 30, 2023 03:27
…ion, change all measurements to height above ground to be the same variable.
…der measurement for heigh above ground in the autorotation lib. We don't need to check for range finder out of range because inertial nave will interpolate for this case and we check against ground clearance in the autorotation lib.
…read and to ensure flight phase progression based on heights. Also flip flag bools to be init checks.
// driven the collective high to cushion the landing so the landing detector will not trip until we drive the collective back to zero thrust and below.
if ((_time_on_ground > 0) && ((AP_HAL::millis() - _time_on_ground) > MIN_TIME_ON_GROUND)) {
// On ground, smoothly lower collective to just bellow zero thrust, to make sure we trip the landing detector
float desired_col = _motors_heli->get_coll_mid() * 0.95;
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs changed to use H_COL_LAND_MIN instead of zero thrust collective, since the landing detector looks for "motors->get_below_land_min_coll()"

Copy link
Member

@IamPete1 IamPete1 left a comment

Choose a reason for hiding this comment

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

There are quite a few "lucky dip" units varables. As a rule if its not SI units we append the unit the variable/define. Also knowing we will be making some effort to more to SI units in the future it would be nice to bring this in using SI as much as possible.

Comment on lines +618 to 620
if (!pos_control->is_active_xy() && use_psc_case) {
return copter.current_loc.alt;
}
Copy link
Member

Choose a reason for hiding this comment

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

I suspect we can just remove this whole section. I don't understand why it would just return current loc rather than trying to get the terrain value. As far as I can tell the only user that is not in position control is landing gear.

In anycase, removing this section means you can also remove the new bool.


//--- Internal functions ---
void warning_message(uint8_t message_n); //Handles output messages to the terminal
uint32_t _last_bad_rpm_ms;
Copy link
Member

Choose a reason for hiding this comment

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

Its a little odd having this bad rpm time stamp in a different structure to the bad_rpm flag. Maybe combine:

    struct {
        uint32_t last_ms;
        bool bad;
    } _rpm;

// Update the touchdown controller
void touchdown_controller(void);

void set_ground_distance(float gnd_dist) { _gnd_hgt = (float)gnd_dist; }
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
void set_ground_distance(float gnd_dist) { _gnd_hgt = (float)gnd_dist; }
void set_ground_distance(float gnd_dist) { _gnd_hgt = gnd_dist; }

g2.arot.init_hs_controller();
g2.arot.init_fwd_spd_controller();
// Init autorotation controllers object
g2.arot.init(motors, copter.rangefinder.ground_clearance_cm_orient(ROTATION_PITCH_270));
Copy link
Member

Choose a reason for hiding this comment

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

Passing in ground clearance here is quite odd. It means that we use the range finder ground clearance for a rangefinder that we may not even be using for the altitude estimate.

I suspect we can just remove for now. Then we can come back and include ground clearance at the copter level before we pass the height above ground estimate down to the arot lib. That way it can only apply the the offset when using the rangefinder.

Copy link
Member

Choose a reason for hiding this comment

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

Also its a bit mad passing in motors in the mode init. You should be able to pass a const reference in the constructor.

_flags.straight_ahead_initial = true;
_flags.bail_out_initial = true;
_msg_flags.bad_rpm = true;
// Set all intial flags to on
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Set all intial flags to on
// Set all intial flags off

@@ -251,54 +321,121 @@ float AC_Autorotation::get_rpm(bool update_counter)
return current_rpm;
}

void AC_Autorotation::initial_flare_estimate()
{
Copy link
Member

Choose a reason for hiding this comment

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

This bit is much better, but still quite scary.

(double)_accel_target,
(double)_pitch_target);
"TimeUS,hsp,hse,co,cff,sf,dsf,vp,vff,az,dvz,h",
"Qfffffffffff",
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to add units and multipliers too.


// Check to see if we think the aircraft is on the ground
if(_current_sink_rate < 10.0 && _gnd_hgt <= _ground_clearance && _time_on_ground == 0){
_time_on_ground = AP_HAL::millis();
Copy link
Member

Choose a reason for hiding this comment

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

you could const uint32_t now = AP_HAL::millis(); Its used again on 673

}


void AC_Autorotation::update_avg_acc_z(void)
Copy link
Member

Choose a reason for hiding this comment

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

This makes me nervous, it will work differently with different loop rates. I would prefer a low pass. If average is better there is a AverageFilter class that you should use. https://github.com/ArduPilot/ardupilot/blob/master/libraries/Filter/AverageFilter.h

//--------References to Other Libraries--------
AP_InertialNav& _inav;
AP_AHRS& _ahrs;
AP_MotorsHeli* _motors_heli;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
AP_MotorsHeli* _motors_heli;
AP_MotorsHeli*& _motors_heli;

You can refence the copter pointer and set it in the constructor. You will have to check for nullprt before use.

MattKear and others added 8 commits February 15, 2024 22:58
…ermine unhealthy. Col defaults to ang of -2 if no RPM
post touchdown condition requires collective to be lowered below land_col_min_pct in order to get the disarm logic to disarm the aircraft
-collective gets lowered below land_col_min_pct in order to get the aircraft disarmed
-adjusted thresholds of lfare update and flare complete check
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants