-
Notifications
You must be signed in to change notification settings - Fork 17k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
AP_ICEngine: Add redline RPM warning message and governor #18273
Conversation
3a719b6
to
699a871
Compare
Wouldn't it be better to have an RPM meter on the GCS with a red line? This could also be done quite easily with a script so I'm not sure adding this is worth the extra parameter and code. This is just one opinion of course. |
I completely understand that. The next step to this would be adding an optional limiter to the RPM so that TECS doesn't run an engine above the redline. I backed off from that right now because the throttle_override was throwing me for a loop how how to do it correctly. It would probably end up being similar to the idle governor feature. The simple way is to limit TECS throttle percentage. Right now, this would add on having a GCS TTS the warning when it happens in non-TECS modes. |
Come to think of it, the ICE lib already does throttle overriding for idle. Perhaps this should morph into not just a non-zero minimum but also an rpm max too with an optional alarm. ICE throttle override: ICE overring throttle in Plane: |
8e8f0c4
to
74a132c
Compare
Had another think at adding a governor on the high side. I haven't had a chance to test this on a bench. Need to be at a different desk to do that. Also need to make a better fake RPM pulse signal on my arduino. I made a few assumptions with the high side integrator only calculating when over the redline RPM and limiting the throttle from increasing did not put the RPM more than 10% below the redline RPM. I think that is correctly done? I guessed at using a lowpass filter from the Filter library. An averaging filter might have issues with the varying noise in the RPM signal. Also just guessed at a 5Hz cutoff to prevent attenuating the pilot commands. And I misspelled the commit message. When do we get spellcheck on a command line? |
74a132c
to
e7fa2bd
Compare
e7fa2bd
to
ae300a6
Compare
4ea46c2
to
8d050bc
Compare
Working on a CubeBlack. Log and graph included. Removed the In this test I have the throttle input constant and I raise the RPM until it is over the ICE_REDLINE_RPM value of 5000. You can see that the RCOU.3 value starts dropping at a constant rate until the RPM is below the redline value. When the RPM is more than 10% below the parameter value, the throttle override is released and the output returns the same value as before. https://drive.google.com/drive/folders/1tgMPDCtFYH-bZ9HhoB_c_I55gX3csFOd?usp=sharing |
8d050bc
to
9b3011b
Compare
9b3011b
to
1b55ded
Compare
Added the ability for the throttle to decrease more rapidly than the integrator if commanded and the RPM is still high. Adjusts the throttle set point and the integrator to match how rapidly it is being commanded to decrease. That also removes a case of sharp throttle change. There is another case that I'm not sure I can solve with code. If the RPM goes above redline and the prop becomes more loaded causing the RPM to drop, now there's a sharp change to increase throttle. |
1b55ded
to
95aa4e3
Compare
95aa4e3
to
1b279d3
Compare
05922cd
to
fcdfd52
Compare
@magicrub can you check the requested changes? |
|
||
if (redline.flag && !(options & uint16_t(Options::DISABLE_REDLINE_GOVERNOR))) { | ||
// limit the throttle from increasing above what the current output is | ||
const float incoming_throttle = SRV_Channels::get_output_scaled(SRV_Channel::k_throttle); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a fan of this method. I'd rather see throttle_override() pass in the current throttle and let it override it. Even this bool result doesn't rub me right, just have it return the same value instead of. I think I've already done this on a branch for AION somewhere but not sure how much we ended up using and/or getting merged. (I feel like I've re-written this library like 5 times, I have so many variants of it in my head)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bool for servos.cpp? throttle_override() does get the current throttle input. update_idle_governor() changes the min_throttle value. Are you thinking this should be written to change the aparm.throttle_max value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed the throttle override to pass in current throttle value rather than zero. Costs a little more in time for all loops now.
bfd44a2
to
ab76cdb
Compare
ab76cdb
to
10ba2ed
Compare
Adds a top end governor to protect the engine from exceeding design limits. Similar to idle governor. Adds a LPF at 0.5 Hz for RPM.
41de207
to
87496b5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok.. I'm a approving it because it's a feature we need and it works OK as-is but what I'd like to see if a follow-up PR that moves rpm_filter stuff up into AP_RPM and maybe even some of the GCS/flag things. Not sure about the param.. but maybe that too. Needs discussion.
Adds warning when the RPM of the engine is above the redline RPM. Also notifies the when the RPM returns below the redline.
Tested on CubeBlack with an arduino making pulses for the RPM interrupts.