ArduPlane: convert parameter TRIM_PITCH_CD to TRIM_PITCH_DEG #25768

timtuxworth · 2023-12-13T22:40:50Z

There is a target to do some refactoring of parameters in the 4.5 release. This is mostly about targeting removing the use of centi-degrees and centimeters in favour of degrees and meters as well as some other parameter cleanups. There is a list documented in this issue: #23580 (comment)

The parameter TRIM_PITCH_CD uses centi degrees. This PR is would replace that with TRIM_PITCH_DEG in degrees.

I understand now why _DEG is important. Not so much for this parameter, but for other parameters (e.g. Q_ANGLE_MAX) which are in "centi" units but don't have a suffix now, so if we just changed them in situ it would be very dangerously easy to set a new Q_ANGLE_MAX in centi-degrees without realizing it was now degrees. So the only safe way to do this is:

change Q_ANGLE_MAX to Q_ANGLE_MAX_DEG
therefore change all "degrees" parameters being changed from centi units to _DEG
therefore as first cab off the rank - TRIM_PITCH_CD becomes TRIM_PITCH_DEG

This PR refactors all the uses of pitch_trim_cd to use pitch_trim scaled down by 100, which almost always simplifies the code and removes floating point operations, since the code needed degrees anyway.

I've added the suggestion from Tridge to create a generic convert_centi_parameter() method on parameters which I copied from some previous code. It does work, but I don't totally understand what it's doing, so please check my work on this.

libraries/AP_Param/AP_Param.cpp

ArduPlane/Parameters.cpp

IamPete1

I tend to think were better off adding the new params to the end of the g2 group rather than trying to squeeze them in the base table.

rmackay9 · 2023-12-14T00:57:02Z

Re TRIM_PITCH vs PITCH_TRIM, I think it depends upon whether you think of the trim as a feature or not. My guess is that it should remain TRIM_PITCH, TRIM_ROLL and TRIM_YAW (or whatever).

timtuxworth · 2023-12-14T01:00:13Z

Re TRIM_PITCH vs PITCH_TRIM, I think it depends upon whether you think of the trim as a feature or not. My guess is that it should remain TRIM_PITCH, TRIM_ROLL and TRIM_YAW (or whatever).

Yes that's very deliberate. I definitely want to have all the PITCH_ stuff show up together rather than grouping by TRIM as it is now, which I find counterintuitive. It also matches the variable name in the code! Very interested in other peoples thoughts.

timtuxworth · 2023-12-14T02:53:45Z

I tend to think were better off adding the new params to the end of the g2 group rather than trying to squeeze them in the base table.

There is also an aparam.pitch_trim_cd, but I can't figure out where it gets set. (there is no ASCALAR);

It looks like this might be a bug, introduced in this PR #22191 - these changes will fix the bug.

libraries/AP_Param/AP_Param.cpp

IamPete1 · 2023-12-14T19:28:23Z

libraries/AP_TECS/AP_TECS.cpp

@@ -849,7 +849,7 @@ void AP_TECS::_update_throttle_without_airspeed(int16_t throttle_nudge)
    _pitch_demand_lpf.apply(_pitch_dem, _DT);
    const float pitch_demand_hpf = _pitch_dem - _pitch_demand_lpf.get();
    _pitch_measured_lpf.apply(_ahrs.pitch, _DT);
-    const float pitch_corrected_lpf = _pitch_measured_lpf.get() - radians(0.01f * (float)aparm.pitch_trim_cd);
+    const float pitch_corrected_lpf = _pitch_measured_lpf.get() - radians(aparm.pitch_trim);


Because the old method was broken this is a change in behavior that will need testing.

ArduPlane/Parameters.cpp

libraries/AP_Vehicle/AP_FixedWing.h

ArduPlane/Parameters.cpp

timtuxworth · 2023-12-14T21:03:04Z

I tend to think were better off adding the new params to the end of the g2 group rather than trying to squeeze them in the base table.

I can't figure out how to make this a g2 parameter now it's in aparm. Is there an equivalent g2 macro to ASCALAR?

OTOH- the base table is already > 256 elements, so does it really matter?

ArduPlane/Attitude.cpp

tridge · 2023-12-16T04:29:26Z

ArduPlane/Parameters.cpp

+    // @Units: deg
+    // @Range: -45 45
+    // @User: Standard
+    ASCALAR(pitch_trim,        "PITCH_TRIM",  0.0f),


I would greatly prefer TRIM_PITCH_DEG so users looking for the old parameter can find the new one easily

I thought part of the point of getting rid of the "CD" was that the units are already in the metadata, adding _DEG is inconsistent with any other parameter usage and will cost 4 characters without adding any value. Users can easily find the parameter by typing "TRIM" or "PITCH" in the search box.

See also this comment in the related issue: #23580 (comment)

Oh and I just noticed this comment in the related issue - maybe it would be better to use PTCH because it's the most consistently used tag for "pitch" which often needs to be abbreviated. Right now it appears as PITCH, PTCH and PIT
#23580 (comment)

I agree with Tim, I don't think we need to add deg for si units. Adding deg also sets up a pattern we may not be able to follow as deg is one character longer than cd.

We could add a magnitude arming check, anything above say 10 we could assume the user copied the cd value.

that shouldn't stop us adding _DEG when we can. It will save users a lot of pain and most params can get the _DEG

There are already at least 13 PITCH parameters that are in degrees, but do NOT have "DEG" in the name. Not to mention all the YAW and ROLL parameters that are in degrees with no "DEG" at the end. I can only find one parameter in plane with _DEG at the end (LAND_ABORT_DEG), whereas there are dozens with degrees as their @units.

IMO it's inconsistency that is most painful to users, and again - it's in the metadata, which is displayed in both Mission Planner and QGroundControl.

Just noticed this one: Q_TRIM_PITCH: Quadplane AHRS trim pitch (in degrees, no _DEG) so TRIM_PITCH would be consistent with this. without it we will have

TRIM_PITCH_DEG (in degrees)

Q_TRIM_PITCH (in degrees)

Ok - I get it _DEG is necessary. So I've reworked this now.

tridge · 2023-12-16T04:30:55Z

ArduPlane/Parameters.cpp

+    // PARAMETER_CONVERSION - Added: Dec 2023
+    // Convert _CM (centimeter) parameters to meters and _CD (centidegrees) parameters to meters
+    {
+        const AP_Param::ConversionInfo centimeters_conversion_info[] {


we need to create a special method for this in AP_Param, so we can do:
myparam.convert_centi_parameter();
and it is done

Not really. That way you have to make multiple calls one for each parameter. This way you can pass in a whole table of parameters using the existing function - which is what I'm expecting next ...

const AP_Param::ConversionInfo centimeters_conversion_info[] { { Parameters::k_param_pitch_trim_cd_old, 0, AP_PARAM_INT16, "PITCH_TRIM" }, { Parameters::k_param_lim_pitch_max_cd_old, 0, AP_PARAM_INT16, "PITCH_LIM_MAX" }, { Parameters::k_param_lim_pitch_min_cd_old, 0, AP_PARAM_INT16, "PITCH_LIM_MIN" }, { Parameters::k_param_lim_roll_old, 0, AP_PARAM_INT16, "ROLL_LIM" }, { Parameters::k_param_airspeed_cruise_old, 0, AP_PARAM_INT16, "AIRSPEED_CRUISE" }, }; AP_Param::convert_old_parameters_scaled(&centimeters_conversion_info[0], ARRAY_SIZE(centimeters_conversion_info), 0.01f, 0);

we need to create a special method for this in AP_Param, so we can do: myparam.convert_centi_parameter(); and it is done

I took a stab at it by copying the existing convert_parameter_width() method in AP_Param, it seems to work, but it's very low level so I wonder if you could take a look. I had to pass in the old type to get it to work.

Tested in SITL

ArduPlane/Parameters.h

timtuxworth · 2023-12-17T01:55:49Z

PITCH_TRIM auto converted from TRIM_PITCH_CD = 250 on a Durandal

// @Param: PITCH_TRIM
// @DisplayName: Pitch angle offset
//@Description: Offset used for in-flight pitch trimming for level flight. Correct ground leveling is an alternative to changing this parameter.
// @Units: deg
// @Range: -45 45
// @User: Standard

Note @Units: deg

tridge · 2023-12-17T04:13:38Z

I tend to think were better off adding the new params to the end of the g2 group rather than trying to squeeze them in the base table.

we don't need to move them at all, they can keep the same ID

davidbuzz · 2023-12-19T01:02:29Z

I think this is as close to what tridge wanted, based on today's discussion: ( $ means end-of-string)
I took a full list of all possible plane and copter params, and wroe a quick tool to replace _CD or CD$ with _DEG or DEG, and then _CM or CM$ with _M or M, and here's the results in a columnar/csv kinda format:

PLANE , RNGFND1_MIN_CM, LEN, 14, newname, RNGFND1_MIN_M, newlen, 13,
PLANE , RNGFND1_MAX_CM, LEN, 14, newname, RNGFND1_MAX_M, newlen, 13,
PLANE , RNGFND2_MIN_CM, LEN, 14, newname, RNGFND2_MIN_M, newlen, 13,
PLANE , RNGFND2_MAX_CM, LEN, 14, newname, RNGFND2_MAX_M, newlen, 13,
PLANE , RNGFND3_MIN_CM, LEN, 14, newname, RNGFND3_MIN_M, newlen, 13,
PLANE , RNGFND3_MAX_CM, LEN, 14, newname, RNGFND3_MAX_M, newlen, 13,
PLANE , RNGFND4_MIN_CM, LEN, 14, newname, RNGFND4_MIN_M, newlen, 13,
PLANE , RNGFND4_MAX_CM, LEN, 14, newname, RNGFND4_MAX_M, newlen, 13,
PLANE , RNGFND5_MIN_CM, LEN, 14, newname, RNGFND5_MIN_M, newlen, 13,
PLANE , RNGFND5_MAX_CM, LEN, 14, newname, RNGFND5_MAX_M, newlen, 13,
PLANE , RNGFND6_MIN_CM, LEN, 14, newname, RNGFND6_MIN_M, newlen, 13,
PLANE , RNGFND6_MAX_CM, LEN, 14, newname, RNGFND6_MAX_M, newlen, 13,
PLANE , RNGFND7_MIN_CM, LEN, 14, newname, RNGFND7_MIN_M, newlen, 13,
PLANE , RNGFND7_MAX_CM, LEN, 14, newname, RNGFND7_MAX_M, newlen, 13,
PLANE , RNGFND8_MIN_CM, LEN, 14, newname, RNGFND8_MIN_M, newlen, 13,
PLANE , RNGFND8_MAX_CM, LEN, 14, newname, RNGFND8_MAX_M, newlen, 13,
PLANE , RNGFND9_MIN_CM, LEN, 14, newname, RNGFND9_MIN_M, newlen, 13,
PLANE , RNGFND9_MAX_CM, LEN, 14, newname, RNGFND9_MAX_M, newlen, 13,
PLANE , RNGFNDA_MIN_CM, LEN, 14, newname, RNGFNDA_MIN_M, newlen, 13,
PLANE , RNGFNDA_MAX_CM, LEN, 14, newname, RNGFNDA_MAX_M, newlen, 13,
PLANE , LIM_ROLL_CD, LEN, 11, newname, LIM_ROLL_DEG, newlen, 12,
PLANE , TRIM_ARSPD_CM, LEN, 13, newname, TRIM_ARSPD_M, newlen, 12,
PLANE , MIN_GNDSPD_CM, LEN, 13, newname, MIN_GNDSPD_M, newlen, 12,
PLANE , TRIM_PITCH_CD, LEN, 13, newname, TRIM_PITCH_DEG, newlen, 14,
PLANE , ALT_HOLD_FBWCM, LEN, 14, newname, ALT_HOLD_FBWM, newlen, 13,
PLANE , LAND_PITCH_CD, LEN, 13, newname, LAND_PITCH_DEG, newlen, 14,
PLANE , RNGFND1_MIN_CM, LEN, 14, newname, RNGFND1_MIN_M, newlen, 13,
PLANE , RNGFND1_MAX_CM, LEN, 14, newname, RNGFND1_MAX_M, newlen, 13,
PLANE , RNGFND2_MIN_CM, LEN, 14, newname, RNGFND2_MIN_M, newlen, 13,
PLANE , RNGFND2_MAX_CM, LEN, 14, newname, RNGFND2_MAX_M, newlen, 13,
PLANE , RNGFND3_MIN_CM, LEN, 14, newname, RNGFND3_MIN_M, newlen, 13,
PLANE , RNGFND3_MAX_CM, LEN, 14, newname, RNGFND3_MAX_M, newlen, 13,
PLANE , RNGFND4_MIN_CM, LEN, 14, newname, RNGFND4_MIN_M, newlen, 13,
PLANE , RNGFND4_MAX_CM, LEN, 14, newname, RNGFND4_MAX_M, newlen, 13,
PLANE , RNGFND5_MIN_CM, LEN, 14, newname, RNGFND5_MIN_M, newlen, 13,
PLANE , RNGFND5_MAX_CM, LEN, 14, newname, RNGFND5_MAX_M, newlen, 13,
PLANE , RNGFND6_MIN_CM, LEN, 14, newname, RNGFND6_MIN_M, newlen, 13,
PLANE , RNGFND6_MAX_CM, LEN, 14, newname, RNGFND6_MAX_M, newlen, 13,
PLANE , RNGFND7_MIN_CM, LEN, 14, newname, RNGFND7_MIN_M, newlen, 13,
PLANE , RNGFND7_MAX_CM, LEN, 14, newname, RNGFND7_MAX_M, newlen, 13,
PLANE , RNGFND8_MIN_CM, LEN, 14, newname, RNGFND8_MIN_M, newlen, 13,
PLANE , RNGFND8_MAX_CM, LEN, 14, newname, RNGFND8_MAX_M, newlen, 13,
PLANE , RNGFND9_MIN_CM, LEN, 14, newname, RNGFND9_MIN_M, newlen, 13,
PLANE , RNGFND9_MAX_CM, LEN, 14, newname, RNGFND9_MAX_M, newlen, 13,
PLANE , RNGFNDA_MIN_CM, LEN, 14, newname, RNGFNDA_MIN_M, newlen, 13,
PLANE , RNGFNDA_MAX_CM, LEN, 14, newname, RNGFNDA_MAX_M, newlen, 13,
PLANE , SOAR_POLAR_CD0, LEN, 14, newname, SOAR_POLAR_DEG0, newlen, 15,
_CD count: 4
_CM count: 43
plane params: 4195
copter params: 4194
total params reviewed: 8389
@tridge - is this close to what u were thinking..? there's a couple of gotchas in here like 'SOAR_POLAR_CD0' that doesn't end in _CD, but is included , and ALT_HOLD_FBWCM->ALT_HOLD_FBWM that is missing the underscrore before and after, and u might want to use _M in the new version instead of what i did here..?

davidbuzz · 2023-12-19T01:20:13Z

as discussed, the above list also doesn't include params that have centimeters in them, but don't end in _CM ... those are available here:
#23580 (comment)

davidbuzz · 2023-12-19T23:36:50Z

This was force-pushed, so just noting that it still isn't aligned with the renaming that tridge said he prefers in the dev call. TRIM_PITCH_CD => TRIM_PITCH_DEG , so I suspect it won't be mergable.

timtuxworth · 2023-12-19T23:40:07Z

This was force-pushed, so just noting that it still isn't aligned with the renaming that tridge said he prefers in the dev call. TRIM_PITCH_CD => TRIM_PITCH_DEG , so I suspect it won't be mergable.

I was a bit premature with my push, I need to do more testing.

tridge · 2024-01-18T02:11:39Z

@timtuxworth I've rebased and cleaned this up for TRIM_PITCH_DEG

timtuxworth · 2024-01-18T03:32:46Z

@timtuxworth I've rebased and cleaned this up for TRIM_PITCH_DEG

Thanks @tridge this looks much cleaner. I think the only thing left are a few .param files so the CI will pass.

tridge · 2024-01-19T02:46:03Z

replaced by #26025

peterbarker added the DevCallTopic label Dec 13, 2023