Plane: ICE governor while disarmed in non-manual

[robertlong13]

If ICE "throttle while disarmed" is enabled, and you are in any mode that will use the ICE governor while armed, then it will now also use it when disarmed too.

Basically, I'd like to start my engine while disarmed in QHover and wait for it warm up. This is nice if you have a sprung throttle and don't want to sit and wait in manual while holding the stick.

I tried to consider every edge case, and I've tested the basics in SITL, but I'm not 100% sure I've accounted for every possible setup.

[robertlong13]

~~Ah crap, this doesn't work. I'm passing current throttle in to the idle governor function, when it needs me to pass min throttle~~

Fixed, but overall, my approach is a bit of a mess. Hopefully someone says "nah, there's a much cleaner/easier way to achieve this".

[IamPete1]

Can we get it to the existing throttle override stuff here?:

https://github.com/ArduPilot/ardupilot/blob/baab8528d60baaeabd25c3ff2eaceb7c04a11ca2/ArduPlane/servos.cpp#L938-L939

The ICE stuff really is a mess with all the hooks into plane. I think a longer term fix would be to move to new servo output functions, that means we don't mess with the plane throttle output.

[robertlong13]

Can we get it to the existing throttle override stuff here?:

Ah, great call. Yeah, that's where it belongs.

I think a longer term fix would be to move to new servo output functions

This is also a great idea. I'll maybe do that in a future PR

[robertlong13]

I've moved it. Thanks @IamPete1 , this is way better.

I think the only reason this wasn't originally handled in throttle_override was that there used to be an early-return when disarmed that prevented it from ever being called. Since your series of cleanups/refactors, it's now possible to put it in the place where it really belongs.

I've tested in SITL, and everything seems to work the way I want it.

[robertlong13]

I really like the cleanup. The only disadvantage I see is that we miss telling TECS if we bump the min throttle.

Hmm, good point. It's probably not the end of the world for most applications if the governor doesn't inform the TECS, since it should only be tweaking throttle a small amount, but there's probably some edge case or weird setup out there that I don't want to break.

I don't think we can simply duplicate the call, since the two calls will fight each other and whoever gets called later (currently governor) will clobber the first. I think this means that anyone using the governor would lose the ability to set a min takeoff throttle.

I'll try to think of something.

[robertlong13]

Just pushed something that I think works, but it's pretty fresh and I haven't tested yet.

As a bonus, we're now letting the TECS know when the redline governor is limiting, which we weren't doing and is probably far more important.

[tridge]

@robertlong13 please work with @Hwurzburg on the wiki changes

[robertlong13]

One little kink to work out, the TECS min/max throttle notification stuff causes a sawtooth instead of what is intended. This has nothing to do with the existing lines 599 and 600 fighting with this (I commented them out and tested again to be sure). ~~I think this is a pre-existing issue with the set_throttle_min and set_throttle_max implementation, but I haven't gotten to the bottom of it yet.~~

Edit: I've figured it out. Once the TECS is informed about the new max, it reduces its throttle, then the governor no longer reduces the throttle, so it can't detect that it's applying a max, and stops informing the TECS, then the cycle repeats.

[robertlong13]

Okay, another little rework.

1. I've abandoned informing the TECS about redline governing. The redline governor is a bit of a mess, and unlike the idle governor, it chucks out its state as soon as you throttle back down. So, informing the TECS isn't as useful.
2. I put information about the idle governor back where it was originally in apply_throttle_limits, just instead of calling the updater for the idle governor, I added an accessor to get the minimum throttle that it's applying. This safely retains the old behavior of informing the TECS about the idle governor, while still fixing the problem I want to solve.

While doing this, I've realized that the idle governor update really belongs in AP_ICEngine::update, but that's called at 10Hz, where the idle governor was built for main-loop-rate (so is the redline governor incorrectly!). I think they should get reworked to go into update, and probably get reworked so they work the same. Not sure if that's something for this PR or another.