Plane: ICE: fully move to aux function #28655
Conversation
There was a problem hiding this comment.
Regarding the loss of STARTCHN_MIN functionality: I know that the the control of the ICE is a crucial function. But doing away with this parameter can only mean inadvertent shutoff of the engine, not inadvertent start.
Under that light, is an inadvertent engine shutoff due to bad RC signals more dangerous than other events tied to AUX_FUNCTIONs?
E.g. switching to a different mode or enabling a parachute.
If we are concerned about minimum PWM values doing bad things, this is universal to all AUX_FUNCTIONs and we should do something about it for all of them in its their own library.
One more thing: You also need to modify the quadplane autotest MAV_CMD_DO_ENGINE_CONTROL; L1224 now fails because the AUX_FUNCTION being low doesn't block an engine start after a MAV_CMD_DO_ENGINE_CONTROL. Which I don't understand why, to be honest.
This was originally added as a result of a dodgy RC system not going into failsafe but output all channels at 900. The result was the engine stopped and could not be re-started as there was no starter. All the other options you mention don't have to be on RC, until now RC was the only option for ICE start. |
I get that.
|
|
I'm for moving this to be more consistent with our other RC Channels options. I've always wondered why this one didn't get updated with the others.
That's not true, there's already the MAVLink command for engine start/stop. I use it regularly. Granted, there's no button to send that message in Planner currently. I've always done it through plugins. But I could add it to the actions dropdown. |
There was a problem hiding this comment.
We also loose the low PWM on RC failure protection of
STARTCHN_MIN.
that protection is essential. The combination of RFD900x with ICE is very common on large quadplanes, and it is still extremely hard to get this right due to the poor UI of the RFD900x. Making the ArduPilot code neater and more consistent is nice, but is not more important than protections like this.
|
@IamPete1 note that on aircraft of this type, the start chan is likely the only "aux" function on the aircraft. |
With this change they no longer need to setup the physical switch. So they can move to no aux functions at all. How would you feel about moving to a new aux function (with no param conversion), this would force users to acknowledge the change. The only other way round it is to move the min param to rc channels and apply it to all aux functions. Or we keep the existing behaviour and add a new aux function that the gcs can use. The key goal here is to expose the aux function to the gcs. |
Is the key goal to expose an aux function to the GCS, or to give the GCS the ability to start and stop the engine? ArduPilot/MissionPlanner#3451 I suppose the aux function allows you to set the engine to a state that ignores mission command engine kills and ignore Q_LAND_ICE_CUT, but I think that's actually a recipe for more confusion. Again, I'm actually in favor of having the aux function for this (and I think the min protection is not that essential), but I think GCSs should actually use the mavlink command, not the aux function, unless there's a good reason to do otherwise. |
|
No good options really, might try again in a year or two. |
|
I have moved to to the suggested trigger structure so AP_ICE can check the triggering PWM. There remain two issues:
|
|
Why don't we just add Your ICE engine stopping is not the only RC-triggered options which can kill your vehicle! |
That results in the nastiness spreading to the out of ICE from the users point of view. This PR keeps it contained in ICE from the users side, the nastiness spreads on the dev side instead. But I can do a PR for that and we could see how we feel about it. |
To me rejecting "out of range" values seems like a sane approach. IMHO we should use RC limits instead of defining a new one, maybe add 10us margin since somebody may be using PPM or other analog protocol that may jitter out of range by few us. |
The problem with this is that we don't currently apply the min/trim/max params at all for switches. There is no requirement that there even calibrated. So if we were to start enforcing them lots of people's switches would stop working. |
…d high low thresholds
…input change before sending command
|
@peterbarker I have removed the PWM and replaced it with a source index. This means you know the RC/GCS/button/mission-item instance that triggered the function. For ICE it means we know that it was RC and the channel number so we can look up the PWM without having to pass it in the structure. I have also hacked up the logger documentation to allow |
This removes the ICE
STARTCHN_MINprotections and RC input thresholds instead moving over to those that are applied to all aux functions.Aux functions do have a min value of 800.
ardupilot/libraries/RC_Channel/RC_Channel.cpp
Line 1897 in 4d75b44
This also moves to the aux function threshold values of 1200 and 1800 rather than the 1300 and 1700 that are used now. ICE was also using a longer de-bounce time, 300ms vs 200ms in the RC lib.
This could mean a change in behavior for some users.
We also loose the low PWM on RC failure protection of
STARTCHN_MIN.We gain the ability to trigger the aux function directly from the GCS, so if RC dropout was a issue the recommendation would be to use the MAVLink command.
I don't really see any other nice way of moving forward. One option would be to add a
STARTCHN_MINparameter to the RC_Channel library and apply it to all switches.A second option would be to add another aux function that would be sent from the GCS and would be ignored if the existing option is setup. So you could do RC switch or GCS, but not both.