ardupilot icon indicating copy to clipboard operation
ardupilot copied to clipboard

Dangerous change in behaviour of TECS_PITCH_MIN

Open LupusTheCanine opened this issue 7 months ago • 5 comments

Bug report

Issue details

Prior to commit https://github.com/ArduPilot/ardupilot/commit/880ebbcdad7bcbee92c8dbef197535c94bc70979 TECS_PITCH_MIN set to positive value was treated as do not use, after the commit positive values are used with no sanity checks.

Version 4.6.0 Platform [ ] All [ ] AntennaTracker [ ] Copter [ x ] Plane [ ] Rover [ ] Submarine

Airframe type Any airframe

Hardware type Cube Orange + and SITL Logs

00000012.zip

LupusTheCanine avatar May 24 '25 11:05 LupusTheCanine

Thanks for the report, I've added it to the 4.6 issues list

rmackay9 avatar May 26 '25 00:05 rmackay9

@Georacer we could discuss

tridge avatar May 27 '25 00:05 tridge

I can join EU dev call.

LupusTheCanine avatar May 27 '25 04:05 LupusTheCanine

Hello! Thanks for the report! I'll try to be there too.

Georacer avatar May 27 '25 19:05 Georacer

So, looking at the changes, this is what was before:

if (_pitch_min >= 0) {
    _PITCHminf = aparm.pitch_limit_min;
} else {
    _PITCHminf = MAX(_pitch_min, aparm.pitch_limit_min);
}

and this is how it is now:

if (_pitch_min == 0) {
    _PITCHminf = aparm.pitch_limit_min;
} else {
    _PITCHminf = _pitch_min;
}

External pitch limits are applied afterwards and can always constrain more, not relax.

It's been 8 months since I've made this change, so it's not very fresh in my mind. On the one hand it seems quite purposeful that I've converted >= into ==, judging by the change in the else branch.

But I don't see any benefit in doing so. Except perhaps my own satisfaction that the parameter should be ignored if it's left unset. And if the user wants to set it positive, then I should let them, trusting they have a reason.

This falls onto the same bin as the other discussion with the stick nudging during AUTO modes. What if the pilot keeps the nose high during AUTO mode? The behaviour will be the same as in this ticket. Should we not allow that as well?

Georacer avatar May 27 '25 19:05 Georacer