Add cranking taper duration multiplier based on CLT #370
Conversation
| @@ -981,6 +981,7 @@ bit skippedWheelOnCam,"On camshaft","On crankshaft";Where is your primary skippe | |||
| bit complexWallModel,"Advanced (tables)","Basic (constants)";Should we use tables to vary tau/beta based on CLT/MAP, or just with fixed values? | |||
| bit alwaysInstantRpm | |||
| bit isMapAveragingEnabled | |||
| bit overrideCrankingTaperDurationSetting; If enabled, use separate temperature multiplier table for cranking taper duration. | |||
There was a problem hiding this comment.
I'm not sure override is the right language here - maybe useCrankingIdleTaperTable or something would be clearer?
There was a problem hiding this comment.
Will change, just looked add overrideCrankingIacSetting, maybe change that one too
|
I'd love to see a unit test for the new behavior :) |
| (engineConfiguration->afterCrankingIACtaperDuration * | ||
| interpolate2d(clt, config->cltCrankingTaperCorrBins, config->cltCrankingTaperCorr)); | ||
| } | ||
| return (float)engine->rpmCalculator.getRevolutionCounterSinceStart() / engineConfiguration->afterCrankingIACtaperDuration; |
There was a problem hiding this comment.
I would avoid duplicating this code if you can - it's not obvious to the reader why these two cases are different when they look so similar on first glance.
prefer something like
float x = config.taperDuration;
if (new_behavior) {
x *= table_lookup();
}
return getRevCounter() / x;There was a problem hiding this comment.
that style of writing it makes it very clear which parts are the same, and which parts are different when the setting is enabled
| @@ -1556,6 +1557,9 @@ uint8_t[PEDAL_TO_TPS_SIZE] autoscale pedalToTpsRpmBins;;"RPM", 100, 0, 0, 25000, | |||
| float[CLT_CRANKING_CURVE_SIZE] cltCrankingCorrBins;CLT-based cranking position multiplier for simple manual idle controller;"C", 1, 0, -100, 250, 2 | |||
| float[CLT_CRANKING_CURVE_SIZE] cltCrankingCorr ;CLT-based cranking position multiplier for simple manual idle controller;"%", 1, 0, 0, 500, 2 | |||
|
|
|||
| float[CLT_CRANKING_CURVE_SIZE] cltCrankingTaperCorrBins;CLT-based taper duration multiplier for simple manual idle controller;"C", 1, 0, -100, 250, 2 | |||
| float[CLT_CRANKING_CURVE_SIZE] cltCrankingTaperCorr ;CLT-based taper duration multiplier for simple manual idle controller;"%", 1, 0, 0, 500, 2 | |||
There was a problem hiding this comment.
- is 500 a reasonable maximum value for this parameter? maybe 5 is safer?
- using float here is a waste of storage, maybe int8_t for temp, and uint8_t with scale 0.02 for multiplier would be better?
There was a problem hiding this comment.
Maybe a good idea to also make these change this for the cltCrankingCorr
There was a problem hiding this comment.
yes, finding other tables we can compact would be a great follow-on task #371
…w one to test multiplier table for cranking taper duration.
Added a unit test |
Krakert
changed the title
Fixes #369