Copter: Unchecked range of parameter PILOT_SPEED_UP may cause drone crash

[Lqs66]

Bug report

Issue details

I noticed that the range of parameter PILOT_SPEED_UP is checked in PreArm check, but after takeoff, some flight modes (e.g., loiter, alt_hold, poshold, and zigzag, etc.) are not checked with this parameter, which could lead to the drone not having enough lift and crashing.

See

ardupilot/ArduCopter/mode_loiter.cpp

Line 109 in 865cffa

target_climb_rate = constrain_float(target_climb_rate, -get_pilot_speed_dn(), g.pilot_speed_up);

ardupilot/ArduCopter/mode_poshold.cpp

Line 90 in 865cffa

target_climb_rate = constrain_float(target_climb_rate, -get_pilot_speed_dn(), g.pilot_speed_up);

ardupilot/ArduCopter/mode_sport.cpp

Line 69 in 865cffa

target_climb_rate = constrain_float(target_climb_rate, -get_pilot_speed_dn(), g.pilot_speed_up);

ardupilot/ArduCopter/mode_zigzag.cpp

Line 311 in 865cffa

target_climb_rate = constrain_float(target_climb_rate, -get_pilot_speed_dn(), g.pilot_speed_up);

ardupilot/ArduCopter/mode_althold.cpp

Line 43 in 865cffa

target_climb_rate = constrain_float(target_climb_rate, -get_pilot_speed_dn(), g.pilot_speed_up);

ardupilot/ArduCopter/mode_flowhold.cpp

Line 246 in 865cffa

target_climb_rate = constrain_float(target_climb_rate, -get_pilot_speed_dn(), copter.g.pilot_speed_up);

Replay

Run SITL and enter the following command in the mavproxy terminal:

1. mode stabilize;
2. arm throttle;
3. rc 3 1500;
4. mode poshold or another of the above modes;
5. param set PILOT_SPEED_UP -1000.
   Then drone crashes to the ground.

Version

I have only verified this on Copter-4.4, but the latest 4.6 version also has the above problem.

Platform
[ ] All
[ ] AntennaTracker
[X] Copter
[ ] Plane
[ ] Rover
[ ] Submarine

Airframe type
SITL

Hardware type
N/A

Logs
None

[rmackay9]

Hi @Lqs66,

Thanks for the investigation and report.

We could add protection but there are a lot of parameters that, when set to invalid values, could cause the vehicle to crash. There will be a lot of extra code added to AP if we check every single one. We could argue that this is a misconfiguration and it is possible to lock down parameters to stop users from changing them to invalid values. I feel this might be a better solution than adding protection everywhere. Also what is invalid depends upon the vehicle. For example, it's possible to make a vehicle crash by setting some of the PID parameters to low values, but these PID values are perfectly valid for another vehicle.

Here is the Lua script for 4.7 that helps locking down parameters. I have not actually tested it myself yet

• script
• description

[Lqs66]

Hi @Lqs66,

Thanks for the investigation and report.

We could add protection but there are a lot of parameters that, when set to invalid values, could cause the vehicle to crash. There will be a lot of extra code added to AP if we check every single one. We could argue that this is a misconfiguration and it is possible to lock down parameters to stop users from changing them to invalid values. I feel this might be a better solution than adding protection everywhere. Also what is invalid depends upon the vehicle. For example, it's possible to make a vehicle crash by setting some of the PID parameters to low values, but these PID values are perfectly valid for another vehicle.

Here is the Lua script for 4.7 that helps locking down parameters. I have not actually tested it myself yet

• script
• description

Hi,

I understand the concern about adding extensive parameter checks increasing code complexity in the AP system. However, I'm inclined to propose integrating parameter range protection directly within the AP_ParamT-related classes.

My idea is to load both the default values and their valid ranges for parameters directly from the configuration file into the flight controller. This way, if a user attempts to modify a parameter with an invalid value, the system can automatically reset it to its default value based on the predefined range.

The benefits of this approach are:

Centralized Management: This tightly couples parameter validity checks with the parameter's own data structure, avoiding dispersed and independent check code for each parameter.
User-Friendly: When a user inputs an invalid value, the system can provide immediate feedback and automatic correction, preventing potential system instability or crashes.
Prevent Runtime Issues: Even "misconfigurations" can be prevented at the earliest possible stage, significantly reducing the flight risks associated with invalid parameters.
While this approach would add some logic to the AP_ParamT classes, I believe this centralized, automated protection mechanism offers a more robust and immediate safety guarantee compared to relying solely on external lockdown scripts.

[rmackay9]

Hi @Lqs66,

OK, we've talked about this kind of thing before but we've always instead relied on the GCSs to implement the protection. Mission Planner for example has a "Password Protect Config" checkbox on the Config / Planner page (see here)

The min/max values can also be different for each vehicle and putting the table of min/max values into C++ will certainly consume a significant amount of flash space which will almost certainly mean that other features will need to be sacrificed in order for the protection to fit.

Anyway, if you want to try and implement it, the dev team will certainly review it

BTW, I think a modified version of the lua script linked above would actually have a better chance of being accepted.

[LupusTheCanine]

@rmackay9 IIRC we have about 1.2k parameters, a lot of them would benefit from simple checks like >=0 or <=0, we could simply reject arming if any parameter is out of range (unless appropriate flag is set). With 1 byte per parameter to store reference to limits and two or three arrays for limits (one per variable size 8,16,32 bits) we probably could get away with about 3kB of memory footprint, less if we can keep the tables not in RAM.

[rmackay9]

Hi @LupusTheCanine,

Thanks for that feedback. I agree that a 3kb memory cost would probably be worth it. So far we've seen 98 bytes of flash for a single check which would add up to >100k of flash which is quite a lot. If someone can implement it efficiently so it's more efficient in terms of flash cost then I think the idea has a chance.