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

Plane: added systemID as an aux function #29237

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

Conversation

tridge
Copy link
Contributor

@tridge tridge commented Feb 6, 2025

This adds VTOL systemID as an AUX function to quadplanes. It works in QSTABILIZE, QHOVER and QLOITER modes.
In QLOITER it multiplies the XY position and velocity gains by SID_XY_CTRL_MUL to reduce the impact of the XY controller on the systemID result
The AP_SystemID class is structured in a way to make it fairly easy to move this to a common library in the future if copter wants to adopt the same approach

@tridge tridge requested a review from bnsgeyer February 6, 2025 05:07
@tridge tridge force-pushed the pr-sysid-quadplane branch from ac21f5a to 1a23f2d Compare February 6, 2025 05:08
@timtuxworth
Copy link
Contributor

I'm struggling to understand why this is called "System ID". It doesn't seem to have anything to do with identifying the system - which is what we have SYSID_THISMAV for no?

@tridge
Copy link
Contributor Author

tridge commented Feb 6, 2025

I'm struggling to understand why this is called "System ID"

"system identification" is a control systems term that refers to characterising the dynamics of a system (a vehicle in this case). In copter it is a flight mode called SYSTEMID flight mode.
and yes, this means we have overlap in naming with the MAVLink SYSID. Welcome to the world of UAVs and re-used names.

// @DisplayName: System identification axis
// @Description: Controls which axis are being excited. Set to non-zero to see more parameters
// @User: Standard
// @Values: 0:None, 1:Input Roll Angle, 2:Input Pitch Angle, 3:Input Yaw Angle, 4:Recovery Roll Angle, 5:Recovery Pitch Angle, 6:Recovery Yaw Angle, 7:Rate Roll, 8:Rate Pitch, 9:Rate Yaw, 10:Mixer Roll, 11:Mixer Pitch, 12:Mixer Yaw, 13:Mixer Thrust, 14:Measured Lateral Position, 15:Measured Longitudinal Position, 16:Measured Lateral Velocity, 17:Measured Longitudinal Velocity, 18:Input Lateral Velocity, 19:Input Longitudinal Velocity
Copy link
Member

Choose a reason for hiding this comment

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

Should add "VTOL" to these, would be good to support in FW flight in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was planning to re-use the same axes (where relevant, can't do lateral vel and pos).

Copy link
Contributor

Choose a reason for hiding this comment

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

remove values 14-19. I did not implement them in my version of sysid. it doesn't look like you did either. Lets keep it to the attitude controller. We can expand to the position controller later.

gcs().send_text(MAV_SEVERITY_WARNING, "SystemID: must be armed");
return;
}
if (!plane.quadplane.enabled()) {
Copy link
Member

Choose a reason for hiding this comment

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

This case should be handled by plane.control_mode->supports_systemid()

@Hwurzburg Hwurzburg added the WikiNeeded needs wiki update label Feb 6, 2025
@timtuxworth
Copy link
Contributor

I'm struggling to understand why this is called "System ID"

"system identification" is a control systems term that refers to characterising the dynamics of a system (a vehicle in this case). In copter it is a flight mode called SYSTEMID flight mode. and yes, this means we have overlap in naming with the MAVLink SYSID. Welcome to the world of UAVs and re-used names.

I'm aware of this problem hence bringing it up. I've found it helps to provide some way to differentiate overloaded terms.

Could the new (user facing) name be something like "Dynamic System Identification" or "System Model Identification" or DSMI (Dynamic System Model Identification)?

@tridge tridge force-pushed the pr-sysid-quadplane branch 2 times, most recently from a8fd6ec to d9853b6 Compare February 7, 2025 00:49

auto *attitude_control = plane.quadplane.attitude_control;
attitude_control->bf_feedforward(restore.att_bf_feedforward);
attitude_control->rate_bf_roll_sysid(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Setting these to zero is not necessary. These are set to zero in the attitude controller at the end of every loop.
https://github.com/ArduPilot/ardupilot/blob/master/ArduPlane/quadplane.cpp#L1948

// @Field: Az: Delta velocity, Z-Axis

// log system id
void AP_SystemID::log_data() const
Copy link
Contributor

Choose a reason for hiding this comment

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

I see you removed the logging of RATE and ANG from this function. It is imperative that these all be logged at the same rate. Why don't you use the same technique as copter to allow logging at multiples of the fast loop rate. this is important to allow users to choose an appropriate rate for their use case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is imperative that these all be logged at the same rate.

unlike copter, the default setup in plane is to log all these at the full loop rate, so they are all logged at the same rate. We could force the LOG_BITMASK in case a user has changed that, or we could just document that the defaults should be used.

Why don't you use the same technique as copter to allow logging at multiples of the fast loop rate. this is important to allow users to choose an appropriate rate for their use case.

it would be good to discuss why this is needed. Is it to make logs smaller? The sysid runs for a short time (usually) and the extra complexity to support down sampling didn't seem worthwhile, but let's discuss when you have a chance

enable as an AUX switch
@tridge tridge force-pushed the pr-sysid-quadplane branch from d9853b6 to 5699218 Compare February 7, 2025 06:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
VTOL-Plane WikiNeeded needs wiki update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants