Plane: most plane centi parameters to deg or m or m/s #26025

tridge · 2024-01-18T05:39:21Z

This builds on #25768 and converts:

TRIM_PITCH_CD -> PTCH_TRIM_DEG
TRIM_ARSPD_CM -> AIRSPEED_CRUISE
ARSPD_FBW_MIN -> AIRSPEED_MIN
ARSPD_FBW_MAX -> AIRSPEED_MAX
ALT_HOLD_RTL -> RTL_ALTITUDE
ALT_HOLD_FBWCM -> CRUISE_ALT_FLOOR ?
LIM_PITCH_MAX -> PTCH_LIM_MAX_DEG ?
LIM_PITCH_MIN -> PTCH_LIM_MIN_DEG ?
LIM_ROLL_CD -> ROLL_LIMIT_DEG ?
LAND_PITCH_CD -> LAND_PITCH_DEG ?
Q_LAND_SPEED -> Q_LAND_FINAL_SPD ? (matches Q_LAND_FINAL_ALT)
Q_VELZ_MAX -> Q_PILOT_SPD_UP ?
Q_VELZ_MAX_DN -> Q_PILOT_SPD_DN ?
Q_ACCEL_Z -> Q_PILOT_ACCEL_Z ?

This includes one real bug fix, in AP_Landing::type_slope_get_target_airspeed_cm() where cm and m was mixed up

tridge · 2024-01-18T05:43:19Z

ping @timtuxworth

peterbarker · 2024-01-18T05:44:47Z

So we use AIRSPEED_CRUISE in cruise mode, presumably...

Why not AIRSPEED_TRIM? Being analogous with e.g. SERVO3_MIN/MAX/TRIM

timtuxworth · 2024-01-18T15:52:37Z

So we use AIRSPEED_CRUISE in cruise mode, presumably...

Why not AIRSPEED_TRIM? Being analogous with e.g. SERVO3_MIN/MAX/TRIM

This is not just about cruise mode, it applies in all auto modes, e,g. if you flip to LOITER it will try to loiter at this speed, it's the default flying speed in AUTO if you don't set a specific speed. I think Pete's comment raises an issue though - it will definitely confuse people to have _CRUISE as the parameter name for a parameter that is NOT specifically about CRUISE mode.

I do like CRUISE - it is more understandable to new/casual users. I can sort of see Pete's point, but it really isn't the same concept, especially for plane users who might come from RC, 'trim' is about setting the surfaces' hands off location, this airspeed value is not that. The only other name I can think of that would make sense might be AIRSPEED_AUTO.

I'll make one last comment that I really think it should be ARSPD not AIRSPEED for consistency, but I won't mention it again.

timtuxworth · 2024-01-18T15:54:57Z

Do you think Q_TRIM_PITCH should be converted to Q_TRIM_PITCH_DEG @tridge ? I think it would be a good idea for consistency.

I was looking at doing that in my PR, it's a very simple change.

timtuxworth · 2024-01-18T15:55:34Z

This builds on #25768 and converts:

Does this mean my PR should be closed then?

timtuxworth

Some suggestions, some fixes from my own incomplete work. (mostly leftover PITCH_TRIM needing _DEG in param files).

ArduPlane/Parameters.cpp

ArduPlane/Parameters.h

Tools/Frame_params/QuadPlanes/MFE_StriverMini.param

Tools/autotest/default_params/plane-jsbsim.parm

Tools/autotest/default_params/vee-gull 005.param

Tools/autotest/models/plane.parm

libraries/SITL/examples/SilentWings/Params/Rolladen-Schneider-LS8b.param

libraries/SITL/examples/SilentWings/Params/Schleicher-ASW27B.param

…D_MAX

IamPete1 · 2024-01-19T14:39:02Z

I will re-state that I'm not a fan of adding _DEG all over the place. I also point out that this does not follow that pattern for speed params eg MIN_GNDSPD_CM -> MIN_GROUNDSPEED not MIN_GNDSPD_MTR. I count 5 angular conversions all getting _DEG, there are 8 speed/distance/accel conversions none of which get units in the name. Why are angular params special?

timtuxworth · 2024-01-19T16:20:34Z

I will re-state that I'm not a fan of adding _DEG all over the place. I also point out that this does not follow that pattern for speed params eg MIN_GNDSPD_CM -> MIN_GROUNDSPEED not MIN_GNDSPD_MTR. I count 5 angular conversions all getting _DEG, there are 8 speed/distance/accel conversions none of which get units in the name. Why are angular params special?

What convinced me was finding some Q_ parameters (e.g. Q_ANGLE_MAX) that were in centi-degrees that did NOT have _CD at the end, so anyone using a converted parameter might easily use centidegrees when the refactored parameter should be degrees. So Q_ANGLE_MAX needs to become Q_ANGLE_MAX_DEG otherwise - planes will fall from the sky. So by extension if you are adding _DEG for one it should be added for all.

MIN_GROUNDSPEED (should be GROUNDSPEED_MIN) is a velocity not a distance, but I note that ALT_HOLD_RTL (was centimeters) is now RTL_ALTITUDE - and yes maybe it could be RTL_ALT_M for a similar reasons (I think "M" should be ok for meters, it's a standard).

veloclity and accelerations are harder. centidegrees/s and centidegrees/s/s (yes we have both) become what? "DEGS", "DS" and "DEGSS" and "DSS"? hardly user friendly. GROUNDSPEED_MIN could be GROUNDSPEED_MIN_MS - oh wait, is that milliseconds? :-D

In essence, I think the call is - _CD becomes _DEG because the risk is higher with this one, but otherwise - no units in the name, it's in the metadata - right? Do less harm.

IamPete1 · 2024-01-19T16:57:22Z

In essence, I think the call is - _CD becomes _DEG because the risk is higher with this one, but otherwise - no units in the name, it's in the metadata - right? Do less harm.

I'm not sure that is really true that the angle params are more "risky", there are plenty of scenarios where incorrect params make bad things happen. Don't forget the risk to the vehicle it's self not the only factor. The obvious example is setting your RTL alt 100 times too high will not cause your vehicle to crash (into the ground anyway).

The whole point of the rename is to make the parameters more consistent with units and naming. We only currently have one parameter with DEG in the name (LAND_ABORT_DEG). So the new pattern of adding DEG actually makes us less consistent not more. I accept that it might be a good idea to add DEG for a few releases to make the changes less painful for users, then we can come back and remove DEG in the future and then we get our consistency. But if were doing that for DEG params then surely we should do it for the speed/distance/accel params too. If users struggle with one they will struggle with the other.

timtuxworth

Oh I had a pending review - sorry.

Tools/Frame_params/EFlight_Convergence.param

Tools/Frame_params/Parrot_Disco/Parrot_Disco.param

Tools/Frame_params/QuadPlanes/MFE_StriverMini.param

ArduPlane/Parameters.h

tridge · 2024-01-20T00:00:39Z

I count 5 angular conversions all getting _DEG, there are 8 speed/distance/accel conversions none of which get units in the name. Why are angular params special?

it is a per-parameter judgement, but it is partly because there is no obvious suffix for m/s that users will instantly recognise, whereas I think all users will understand XX_DEG as being degrees

tridge · 2024-01-20T00:09:00Z

Why not AIRSPEED_TRIM? Being analogous with e.g. SERVO3_MIN/MAX/TRIM

I discussed with @priseborough and he and I agreed it really has nothing to do with "trim". The "cruise" speed of an aircraft is standard parlance for what this variable represents, the original "trim" name was a mistake

rmackay9 · 2024-01-20T01:49:19Z

These two look suspicious because of the swapping of "Alt" and "Cruise"... also "min" and "floor" are very similar.

ALT_CRUISE_MIN
CRUISE_ALT_FLOOR

We should be careful that when MIN, MAX, TRIM etc are in the parameter name that they appear at the end except when the parameter name includes the Units. In this case it become Min/Max/Trim _ Units. I think this is all fine in this PR but I haven't checked everything carefully.

rmackay9 · 2024-01-20T01:57:58Z

ArduPlane/Parameters.cpp


-    // @Param: LIM_PITCH_MAX
+    // @Param: PTCH_LIM_MAX_DEG


In copter we've used the short form, PIT when we didn't have enough room for Pitch.

rmackay9 · 2024-01-20T01:58:25Z

ArduPlane/Parameters.cpp

@@ -637,35 +637,34 @@ const AP_Param::Info Plane::var_info[] = {
    // @User: Advanced
    GSCALAR(scaling_speed,        "SCALING_SPEED",    SCALING_SPEED),

-    // @Param: MIN_GNDSPD_CM
+    // @Param: MIN_GROUNDSPEED


MIn should not really be at the front if possible.

ArduPlane/Parameters.cpp

rmackay9 · 2024-01-20T02:00:22Z

ArduPlane/config.h

 //
-#ifndef MIN_GNDSPEED
- # define MIN_GNDSPEED                   0 // m/s (0 disables)
+#ifndef MIN_GROUNDSPEED


If the definitions being changed, I'd also put the MIN at the end.

If the definitions being changed, I'd also put the MIN at the end.

Agree

ArduPlane/is_flying.cpp

ArduPlane/quadplane.cpp

ArduPlane/Parameters.cpp

tridge · 2024-01-20T05:05:33Z

Everyone please concentrate on actual errors in this PR. It is clear we're not going to get any two people to agree on every point, so what I want if for it to meet the main objectives

peterbarker · 2024-01-23T00:11:23Z

ArduPlane/Parameters.cpp

@@ -1545,6 +1543,19 @@ void Plane::load_parameters(void)
 #if AP_FENCE_ENABLED
    AP_Param::convert_class(g.k_param_fence, &fence, fence.var_info, 0, 0, true);
 #endif
+
+    // PARAMETER_CONVERSION - Added: Dec 2023


Suggested change

// PARAMETER_CONVERSION - Added: Dec 2023

// PARAMETER_CONVERSION - Added: Jan 2023 for Plane 4.5

IamPete1

Basically looks good, I left a few comments, but no bugs I can see.

IamPete1 · 2024-01-23T02:07:16Z

ArduPlane/Parameters.cpp

@@ -1314,7 +1313,6 @@ static const AP_Param::ConversionInfo conversion_table[] = {

    { Parameters::k_param_land_slope_recalc_shallow_threshold,0,AP_PARAM_FLOAT, "LAND_SLOPE_RCALC" },
    { Parameters::k_param_land_slope_recalc_steep_threshold_to_abort,0,AP_PARAM_FLOAT, "LAND_ABORT_DEG" },
-    { Parameters::k_param_land_pitch_cd,      0,      AP_PARAM_INT16, "LAND_PITCH_CD" },


I wonder if we should just take the opportunity to remove all of these landing param conversions. With this change you get most converted but not all. Looks like there 8 years old. #5269

separate PR so I don't need to retest for the 4.5 beta

IamPete1 · 2024-01-23T02:11:28Z

ArduPlane/altitude.cpp

@@ -359,9 +359,9 @@ void Plane::check_fbwb_altitude(void)
    }
 #endif

-    if (g.FBWB_min_altitude_cm != 0) {


This is a change in behavior. I guess setting a negative altitude is a bit of a edge case, especially if 0 is a magic disable.

IamPete1 · 2024-01-23T02:22:56Z

libraries/APM_Control/AP_AutoTune.cpp

        break;
    case AUTOTUNE_PITCH:
-        att_limit_deg = MIN(abs(aparm.pitch_limit_max_cd),abs(aparm.pitch_limit_min_cd))*0.01;
+        att_limit_deg = MIN(abs(aparm.pitch_limit_max*100),abs(aparm.pitch_limit_min*100))*0.01;


Suggested change

att_limit_deg = MIN(abs(aparm.pitch_limit_max*100),abs(aparm.pitch_limit_min*100))*0.01;

att_limit_deg = MIN(fabsf(aparm.pitch_limit_max), fabsf(aparm.pitch_limit_min));

tridge added Plane WikiNeeded needs wiki update labels Jan 18, 2024

tridge force-pushed the pr-centi-param2 branch from 3e036a4 to 5de9174 Compare January 18, 2024 05:41

timtuxworth suggested changes Jan 18, 2024

View reviewed changes

timtuxworth and others added 21 commits January 19, 2024 13:41

Plane: convert parameter TRIM_PITCH_CD to TRIM_PITCH_DEG

bf3e259

AP_Param: added convert_centi_parameter()

03ca7b9

AP_TECS: use new pitch_trim in degrees

63c5afd

AP_Vehicle: use new pitch_trim in degrees in AP_FixedWing

644b11a

Tools: update Frame_params for TRIM_PITCH_DEG

f2de2b6

autotest: change default params from TRIM_PITCH_CD to TRIM_PITCH_DEG

1c86f2a

SITL: convert TRIM_PITCH_CD to TRIM_PITCH_DEG

e4c84da

AP_Landing: change TRIM_ARSPD_CM to AIRSPEED_CRUISE

180d027

AP_Soaring: change TRIM_ARSPD_CM to AIRSPEED_CRUISE

d8b5a30

AP_TECS: change TRIM_ARSPD_CM to AIRSPEED_CRUISE

a198eb3

AP_Vehicle: change TRIM_ARSPD_CM to AIRSPEED_CRUISE

10b61f0

ArduPlane: change TRIM_ARSPD_CM to AIRSPEED_CRUISE

a68b832

Tools: change TRIM_ARSPD_CM to AIRSPEED_CRUISE

ebdc279

AP_HAL_ChibiOS: converted TRIM_ARSPD_CM to AIRSPEED_CRUISE

380f7a0

SITL: converted TRIM_ARSPD_CM to AIRSPEED_CRUISE

a5de930

AP_OSD: convert remaining uses of TRIM_ARSPD_CM to AIRSPEED_CRUISE

0386750

AP_Scripting: convert remaining uses of TRIM_ARSPD_CM to AIRSPEED_CRUISE

ecdea3b

AP_Soaring: convert remaining uses of TRIM_ARSPD_CM to AIRSPEED_CRUISE

3d80aac

AP_TECS: convert remaining uses of TRIM_ARSPD_CM to AIRSPEED_CRUISE

87294cb

AP_Airspeed: convert ARSPD_FBW_MIN/MAX to AIRSPEED_MIN and AIRSPEED_MAX

1da1cc5

AP_HAL_ChibiOS: convert ARSPD_FBW_MIN/MAX to AIRSPEED_MIN and AIRSPEE…

e2a8280

…D_MAX

tridge added 6 commits January 20, 2024 10:12

Tools: added convert_param_scale.py

f7a35f6

AP_HAL_ChibiOS: fixed name of pitch limit parameters

b8000f7

SITL: fixed name of pitch limit parameters

c30b453

Tools: fixed name of pitch limit parameters

1f3cb46

Tools: fixed incorrect param conversions

a5c00bb

Plane: make the 90x factor more obvios

ff164dd

timtuxworth reviewed Jan 19, 2024

View reviewed changes

tridge added 2 commits January 20, 2024 10:57

AP_Param: whitespace fix

55d00f7

Plane: removed incorrect comment

c431945