-
Notifications
You must be signed in to change notification settings - Fork 19.9k
Plane: add acceleration based height control option in TECS #31698
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
base: master
Are you sure you want to change the base?
Conversation
93b7ab3 to
47f7a69
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have yet to run the simulation to look at the performance, I will do that soon.
The control logic seems sound.
But I really don't like how this new control scheme is merged as part of TECS. This is no longer a Total Energy controller. It's two separate controllers for altitude and airspeed errors. Embedding them in TECS makes an already complicated file even less maintainable.
Additionally, all of the traditional TECS functions are still run (e.g. pitch demand calculation), even though their result is never actually used. This seems wasteful, resource-wise.
I would much prefer if this new controller was a new subclass of longitudinal control, much like AP_L1_Control is a subclass of AP_Navigation. I know this is a much larger refactoring, but I think it's necessary, especially if we are to develop other longitudinal controllers in the future.
I see how switching between the two controllers will be much harder in this case, but I still think it's the right choice.
I'm happy to help along with this.
| Vector3f vel_NED, vel_wind_NED; | ||
| if (_ahrs.get_velocity_NED(vel_NED) && _ahrs.wind_estimate(vel_wind_NED)) { | ||
| // Calculate a wind relative flight path unit vector | ||
| Vector3f flight_path_NED = (vel_NED - vel_wind_NED); | ||
| flight_path_NED.normalize(); | ||
| // Get rate of change of velocity vector along wind relative flight path | ||
| Vector3f vel_dot_NED = _ahrs.get_accel_ef(); | ||
| vel_dot_NED.z += GRAVITY_MSS; | ||
| // * operator is overloaded as a dot product | ||
| const float temp = vel_dot_NED * flight_path_NED; | ||
| // take 5 point moving average | ||
| _vel_dot = _vdot_filter.apply(temp); | ||
| } else { | ||
| // If no wind relative velocity data is available, use less accurate method | ||
| // that uses acceleration along X axis and is subject to pitch coupling | ||
| const Matrix3f &rotMat = _ahrs.get_rotation_body_to_ned(); | ||
| // Calculate speed rate of change | ||
| const float temp = rotMat.c.x * GRAVITY_MSS + AP::ins().get_accel().x; | ||
| // take 5 point moving average | ||
| _vel_dot = _vdot_filter.apply(temp); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't we bring this in in another PR recently?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see it in master, I've split it out as a separate commit
@priseborough did you intend this change to be part of this PR?
| _SEBdot_dem_clip = clipStatus::MIN; | ||
| // The vertical accln control method is incompatible with the height demand | ||
| // profile mechanism controlled by _SEBdot_dem_clip | ||
| if (!option_is_set(Option::HGT_ACCLN_CTRL) && | ||
| !option_is_set(Option::INHIBIT_CLIPSTATUS)) { | ||
| _SEBdot_dem_clip = clipStatus::MIN; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I read this correctly, you are preventing clipping in the climb/descent rate here: The only consumer of _SEBdot_de_clip is _update_height_demand().
This is the only modification in _update_pitch(), right?
Then my question is, why run _update_pitch() at all? Its result is not used during acceleration control mode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are correct. We don't need to run it in this mode. Will fix.
| // @Param: _NOR_GLIM | ||
| // @DisplayName: Normal accln limit | ||
| // @Description: Maximum allowed normal g when airspeed = SCALING_SPEED. When flying faster or slower than SCALING_SPEED the limit is adjusted for dynamic pressure. Use this to prevent wing stall. Use this to stay within angle of attack limits. | ||
| // @Units: g | ||
| // @Range: 1.5 5.0 | ||
| // @User: Standard | ||
| AP_GROUPINFO("_NOR_GLIM", 12, AP_PitchController, acro_normal_g_lim, 2.0f), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we try to reuse/merge the TECS acceleration limit? Or is this a controller performance vs "pilot feel" situation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure, @priseborough ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The TECS limit is a vertical acceleration limit, this is a normal g limit and needs to be higher because it includes g buildup in constant height turns, whereas the TECS limit is more about limiting TECS authority and smoothing out response to steps in demands, not a vehicle limit.
| */ | ||
| float AP_PitchController::get_servo_out_accln(float up_accln, float scaler, float pitch_angle_min, float pitch_angle_max, float pitch_trim_deg) | ||
| { | ||
| Vector3f vdot_dem_clipped = Vector3f(0.0f, 0.0f, -constrain_float(up_accln, -7.0f, 7.0f)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the arbitrary 7 here and not reuse NOR_GLIM?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a vertical accln limit like the TECS one. It probably should be harmonised with the TECS limit. It is there to prevent small positive and below g pushovers which cause problems with horizontal tracking
47f7a69 to
d5c862b
Compare
| // @DisplayName: Extra TECS options | ||
| // @Description: This allows the enabling of special features in the speed/height controller. | ||
| // @Bitmask: 0:GliderOnly,1:AllowDescentSpeedup | ||
| // @Bitmask: 0:GliderOnly,1:AllowDescentSpeedup,2:DoAcclnControl,3:InhibitClipStatus |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty sure we need spaces here. This will appear in the GCS UI and this is what is displayed to the user so:
// @bitmask: 0:Glider Only,1:Allow Descent Speedup,2:Enable Height Acceleration Control,3:Inhibit Clip Status
I guess this fixes the two existing options which should have had spaces I'm thinking.
7ca38eb to
d9c6145
Compare
this gives more accurate velocity information and less pitch coupling
this gives an option for more precise height control when flying close to constraints (complex terrain, power-lines, landing etc)
for testing the new functionality
test mission with and without TECS_OPTIONS=4
55afc82 to
4fa8201
Compare
| } else { | ||
| return pitchController.get_servo_out(demanded_pitch - ahrs.pitch_sensor, speed_scaler, disable_integrator, | ||
| ground_mode && !(plane.flight_option_enabled(FlightOptions::DISABLE_GROUND_PID_SUPPRESSION))); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| } |
| // Update and average speed rate of change | ||
|
|
||
| // calculate a low pass filtered _vel_dot | ||
| if (_flags.reset) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update_vel_dot_lpf()?
The if/else logic is getting worse over time
| // @Field: CR: current climb rate | ||
| // @Field: VAL: vertical acceleration limit | ||
| AP::logger().WriteStreaming("TEC6","TimeUS,HAD,UEL,DEL,HD,H,HRD,CR,VAL", | ||
| "Qffffffff", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Units and multipliers
| // get throttle demand setpoint, then adjust using a PID control scheme if airspeed available | ||
| _update_throttle_without_airspeed(throttle_nudge, pitch_trim_deg); | ||
|
|
||
| if (_ahrs.using_airspeed_sensor() || _use_synthetic_airspeed) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't we have case somewhere where we needed to check the "once" flag or something here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is now covered by the new use_airspeed() private method.
| // @Field: TRD: true airspeed rate demand (low-pass filtered) | ||
| // @Field: Tdem: final throttle demand | ||
| AP::logger().WriteStreaming("TEC5","TimeUS,FF,P,D,I,TD,TM,TRD,Tdem", | ||
| "Qffffffff", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
units and multipliers
| Vector3f velRateDemBF; // demanded velocity rate of change - body frame | ||
| Vector3f acclnDemBF; // demanded specific force - body frame |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
intiialise as part of declaration
| // @Field: clip: rate limit clip indicator | ||
| #if HAL_LOGGING_ENABLED | ||
| AP::logger().WriteStreaming("ACC1","TimeUS,VRDBF,ADBF,AEBF,gain,PRDFF,AEI,PRmin,PRmax,DR,clip", | ||
| "Qfffffffffb", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
units and multipliers
|
|
||
| def TECSAccelControl(self): | ||
| '''Test TECS height acceleration control via scripting''' | ||
| self.context_push() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't need this
This adds an acceleration based height controller as an alternative to the current TECS height control system.
The maths and most of the code was developed by @priseborough - I am just bringing it as a PR and coordinating testing.
To try this system you need to set TECS_OPTIONS=4 to enable the new controller.
Issues: