From 41919a4fbde02da37005e04fd48dc87006ba9f3c Mon Sep 17 00:00:00 2001 From: rusefi Date: Mon, 14 Oct 2019 06:18:08 -0400 Subject: [PATCH] preparing for #974 code comments & refactoring --- firmware/controllers/map_averaging.cpp | 7 +++- firmware/controllers/map_averaging.h | 3 ++ .../controllers/trigger/rpm_calculator.cpp | 33 ++++++------------- firmware/controllers/trigger/rpm_calculator.h | 4 +-- firmware/controllers/trigger/spark_logic.cpp | 9 +++++ firmware/development/engine_sniffer.cpp | 4 +-- firmware/development/sensor_chart.cpp | 2 +- firmware/development/wave_analyzer.cpp | 2 +- unit_tests/tests/test_trigger_decoder.cpp | 18 +++++----- 9 files changed, 43 insertions(+), 39 deletions(-) diff --git a/firmware/controllers/map_averaging.cpp b/firmware/controllers/map_averaging.cpp index cc781f6e30..ef97130c20 100644 --- a/firmware/controllers/map_averaging.cpp +++ b/firmware/controllers/map_averaging.cpp @@ -236,6 +236,9 @@ void refreshMapAveragingPreCalc(DECLARE_ENGINE_PARAMETER_SIGNATURE) { for (int i = 0; i < engineConfiguration->specs.cylindersCount; i++) { angle_t cylinderOffset = getEngineCycle(engine->getOperationMode(PASS_ENGINE_PARAMETER_SIGNATURE)) * i / engineConfiguration->specs.cylindersCount; efiAssertVoid(CUSTOM_ERR_6692, !cisnan(cylinderOffset), "cylinderOffset"); + // part of this formula related to specific cylinder offset is never changing - we can + // move the loop into start-up calculation and not have this loop as part of periodic calculation + // todo: change the logic as described above in order to reduce periodic CPU usage? float cylinderStart = start + cylinderOffset - offsetAngle + tdcPosition(); fixAngle(cylinderStart, "cylinderStart", CUSTOM_ERR_6562); engine->engineState.mapAveragingStart[i] = cylinderStart; @@ -297,7 +300,9 @@ static void mapAveragingTriggerCallback(trigger_event_e ckpEventType, fixAngle(samplingEnd, "samplingEnd", CUSTOM_ERR_6563); // only if value is already prepared - int structIndex = engine->rpmCalculator.getRevolutionCounter() % 2; + int structIndex = getRevolutionCounter() % 2; + // at the moment we schedule based on time prediction based on current RPM and angle + // we are loosing precision in case of changing RPM - the further away is the event the worse is precision // todo: schedule this based on closest trigger event, same as ignition works scheduleByAngle(rpm, &startTimer[i][structIndex], samplingStart, startAveraging, NULL, &engine->rpmCalculator); diff --git a/firmware/controllers/map_averaging.h b/firmware/controllers/map_averaging.h index 08274f6296..7da86b64ac 100644 --- a/firmware/controllers/map_averaging.h +++ b/firmware/controllers/map_averaging.h @@ -18,7 +18,10 @@ void mapAveragingAdcCallback(adcsample_t newValue); void initMapAveraging(Logging *sharedLogger, Engine *engine); void refreshMapAveragingPreCalc(DECLARE_ENGINE_PARAMETER_SIGNATURE); + +#if EFI_TUNER_STUDIO void postMapState(TunerStudioOutputChannels *tsOutputChannels); +#endif #endif /* EFI_MAP_AVERAGING */ diff --git a/firmware/controllers/trigger/rpm_calculator.cpp b/firmware/controllers/trigger/rpm_calculator.cpp index 2927627e39..76b7d47cad 100644 --- a/firmware/controllers/trigger/rpm_calculator.cpp +++ b/firmware/controllers/trigger/rpm_calculator.cpp @@ -6,8 +6,11 @@ * Actual getRpm() is calculated once per crankshaft revolution, based on the amount of time passed * since the start of previous shaft revolution. * + * We also have 'instant RPM' logic separate from this 'cycle RPM' logic. Open question is why do we not use + * instant RPM instead of cycle RPM more often. + * * @date Jan 1, 2013 - * @author Andrey Belomutskiy, (c) 2012-2018 + * @author Andrey Belomutskiy, (c) 2012-2019 */ #include "global.h" @@ -21,8 +24,7 @@ #if EFI_PROD_CODE #include "os_util.h" -#include "engine.h" -#endif +#endif /* EFI_PROD_CODE */ #if EFI_SENSOR_CHART #include "sensor_chart.h" @@ -85,8 +87,6 @@ extern bool hasFirmwareErrorFlag; static Logging * logger; -int revolutionCounterSinceBootForUnitTest = 0; - RpmCalculator::RpmCalculator() { #if !EFI_PROD_CODE mockRpm = MOCK_UNDEFINED; @@ -96,7 +96,6 @@ RpmCalculator::RpmCalculator() { // we need this initial to have not_running at first invocation lastRpmEventTimeNt = (efitime_t) -10 * US2NT(US_PER_SECOND_LL); - revolutionCounterSinceBootForUnitTest = 0; } /** @@ -178,12 +177,9 @@ spinning_state_e RpmCalculator::getState() const { void RpmCalculator::onNewEngineCycle() { revolutionCounterSinceBoot++; revolutionCounterSinceStart++; -#if EFI_UNIT_TEST - revolutionCounterSinceBootForUnitTest = revolutionCounterSinceBoot; -#endif /* EFI_UNIT_TEST */ } -uint32_t RpmCalculator::getRevolutionCounter(void) const { +uint32_t RpmCalculator::getRevolutionCounterM(void) const { return revolutionCounterSinceBoot; } @@ -230,9 +226,7 @@ void RpmCalculator::setSpinningUp(efitime_t nowNt DECLARE_ENGINE_PARAMETER_SUFFI void rpmShaftPositionCallback(trigger_event_e ckpSignalType, uint32_t index DECLARE_ENGINE_PARAMETER_SUFFIX) { efitick_t nowNt = getTimeNowNt(); -#if EFI_PROD_CODE efiAssertVoid(CUSTOM_ERR_6632, getCurrentRemainingStack() > 256, "lowstckRCL"); -#endif RpmCalculator *rpmState = &engine->rpmCalculator; @@ -315,7 +309,7 @@ static void tdcMarkCallback(trigger_event_e ckpSignalType, (void) ckpSignalType; bool isTriggerSynchronizationPoint = index0 == 0; if (isTriggerSynchronizationPoint && ENGINE(isEngineChartEnabled)) { - int revIndex2 = engine->rpmCalculator.getRevolutionCounter() % 2; + int revIndex2 = getRevolutionCounter() % 2; int rpm = GET_RPM(); // todo: use tooth event-based scheduling, not just time-based scheduling if (isValidRpm(rpm)) { @@ -326,12 +320,6 @@ static void tdcMarkCallback(trigger_event_e ckpSignalType, } #endif -#if EFI_PROD_CODE || EFI_SIMULATOR -int getRevolutionCounter() { - return engine->rpmCalculator.getRevolutionCounter(); -} -#endif - /** * @return Current crankshaft angle, 0 to 720 for four-stroke */ @@ -361,7 +349,6 @@ void initRpmCalculator(Logging *sharedLogger DECLARE_ENGINE_PARAMETER_SUFFIX) { addTriggerEventListener(rpmShaftPositionCallback, "rpm reporter", engine); } -#if EFI_PROD_CODE || EFI_SIMULATOR /** * Schedules a callback 'angle' degree of crankshaft from now. * The callback would be executed once after the duration of time which @@ -369,14 +356,14 @@ void initRpmCalculator(Logging *sharedLogger DECLARE_ENGINE_PARAMETER_SUFFIX) { */ void scheduleByAngle(int rpm, scheduling_s *timer, angle_t angle, schfunc_t callback, void *param, RpmCalculator *calc DECLARE_ENGINE_PARAMETER_SUFFIX) { + // todo: remove 'calc' parameter UNUSED(rpm); efiAssertVoid(CUSTOM_ANGLE_NAN, !cisnan(angle), "NaN angle?"); efiAssertVoid(CUSTOM_ERR_6634, isValidRpm(rpm), "RPM check expected"); - float delayUs = calc->oneDegreeUs * angle; + float delayUs = ENGINE(rpmCalculator.oneDegreeUs) * angle; efiAssertVoid(CUSTOM_ERR_6635, !cisnan(delayUs), "NaN delay?"); - engine->executor.scheduleForLater(timer, (int) delayUs, callback, param); + ENGINE(executor.scheduleForLater(timer, (int) delayUs, callback, param)); } -#endif #else RpmCalculator::RpmCalculator() { diff --git a/firmware/controllers/trigger/rpm_calculator.h b/firmware/controllers/trigger/rpm_calculator.h index c07853d013..eec0a6ff31 100644 --- a/firmware/controllers/trigger/rpm_calculator.h +++ b/firmware/controllers/trigger/rpm_calculator.h @@ -89,7 +89,7 @@ public: * This method is invoked once per engine cycle right after we calculate new RPM value */ void onNewEngineCycle(); - uint32_t getRevolutionCounter(void) const; + uint32_t getRevolutionCounterM(void) const; void setRpmValue(int value DECLARE_ENGINE_PARAMETER_SUFFIX); /** * The same as setRpmValue() but without state change. @@ -158,7 +158,7 @@ void initRpmCalculator(Logging *sharedLogger DECLARE_ENGINE_PARAMETER_SUFFIX); float getCrankshaftAngleNt(efitime_t timeNt DECLARE_ENGINE_PARAMETER_SUFFIX); -int getRevolutionCounter(void); +#define getRevolutionCounter() ENGINE(rpmCalculator.getRevolutionCounterM()) #if EFI_ENGINE_SNIFFER #define addEngineSnifferEvent(name, msg) if (ENGINE(isEngineChartEnabled)) { waveChart.addEvent3((name), (msg)); } diff --git a/firmware/controllers/trigger/spark_logic.cpp b/firmware/controllers/trigger/spark_logic.cpp index 314973f559..3d6f0dd220 100644 --- a/firmware/controllers/trigger/spark_logic.cpp +++ b/firmware/controllers/trigger/spark_logic.cpp @@ -36,6 +36,10 @@ int isIgnitionTimingError(void) { } static void fireSparkBySettingPinLow(IgnitionEvent *event, IgnitionOutputPin *output) { +#if EFI_UNIT_TEST + Engine *engine = event->engine; +#endif /* EFI_UNIT_TEST */ + #if SPARK_EXTREME_LOGGING scheduleMsg(logger, "spark goes low %d %s %d current=%d cnt=%d id=%d", getRevolutionCounter(), output->name, (int)getTimeNowUs(), output->currentLogicValue, output->outOfOrder, event->sparkId); @@ -161,7 +165,12 @@ if (engineConfiguration->debugMode == DBG_DWELL_METRIC) { } static void startDwellByTurningSparkPinHigh(IgnitionEvent *event, IgnitionOutputPin *output) { +#if EFI_UNIT_TEST + Engine *engine = event->engine; + EXPAND_Engine; +#endif /* EFI_UNIT_TEST */ + // todo: no reason for this to be disabled in unit_test mode?! #if ! EFI_UNIT_TEST if (GET_RPM_VALUE > 2 * engineConfiguration->cranking.rpm) { diff --git a/firmware/development/engine_sniffer.cpp b/firmware/development/engine_sniffer.cpp index dca046ed26..66fa82edef 100644 --- a/firmware/development/engine_sniffer.cpp +++ b/firmware/development/engine_sniffer.cpp @@ -77,7 +77,7 @@ static uint32_t skipUntilEngineCycle = 0; #if ! EFI_UNIT_TEST extern WaveChart waveChart; static void resetNow(void) { - skipUntilEngineCycle = engine->rpmCalculator.getRevolutionCounter() + 3; + skipUntilEngineCycle = getRevolutionCounter() + 3; waveChart.reset(); } #endif @@ -169,7 +169,7 @@ void WaveChart::addEvent3(const char *name, const char * msg) { if (!ENGINE(isEngineChartEnabled)) { return; } - if (skipUntilEngineCycle != 0 && ENGINE(rpmCalculator.getRevolutionCounter()) < skipUntilEngineCycle) + if (skipUntilEngineCycle != 0 && getRevolutionCounter() < skipUntilEngineCycle) return; #if EFI_SIMULATOR // todo: add UI control to enable this for firmware if desired diff --git a/firmware/development/sensor_chart.cpp b/firmware/development/sensor_chart.cpp index b728af810a..83e26fb806 100644 --- a/firmware/development/sensor_chart.cpp +++ b/firmware/development/sensor_chart.cpp @@ -39,7 +39,7 @@ void scAddData(float angle, float value) { return; } - if (engine->rpmCalculator.getRevolutionCounter() % engineConfiguration->sensorChartFrequency != 0) { + if (getRevolutionCounter() % engineConfiguration->sensorChartFrequency != 0) { /** * We are here if we do NOT need to add an event to the analog chart */ diff --git a/firmware/development/wave_analyzer.cpp b/firmware/development/wave_analyzer.cpp index 9227fe2b64..0f5a1ee8ac 100644 --- a/firmware/development/wave_analyzer.cpp +++ b/firmware/development/wave_analyzer.cpp @@ -80,7 +80,7 @@ void WaveReader::onFallEvent() { efitick_t width = nowUs - widthEventTimeUs; last_wave_high_widthUs = width; - int revolutionCounter = engine->rpmCalculator.getRevolutionCounter(); + int revolutionCounter = getRevolutionCounter(); totalOnTimeAccumulatorUs += width; if (currentRevolutionCounter != revolutionCounter) { diff --git a/unit_tests/tests/test_trigger_decoder.cpp b/unit_tests/tests/test_trigger_decoder.cpp index 46547edc26..44119a1618 100644 --- a/unit_tests/tests/test_trigger_decoder.cpp +++ b/unit_tests/tests/test_trigger_decoder.cpp @@ -584,7 +584,7 @@ static void setTestBug299(EngineTestHelper *eth) { // inj #0 |.......#|........|.......#|........| // inj #1 |........|.......#|........|.......#| ASSERT_EQ( 4, engine->executor.size()) << "qs#00"; - ASSERT_EQ( 3, engine->rpmCalculator.getRevolutionCounter()) << "rev cnt#3"; + ASSERT_EQ( 3, getRevolutionCounter()) << "rev cnt#3"; eth->assertInjectorUpEvent("setTestBug299: 1@0", 0, MS2US(8.5), 0); eth->assertInjectorDownEvent("@1", 1, MS2US(10), 0); eth->assertInjectorUpEvent("1@2", 2, MS2US(18.5), 1); @@ -607,7 +607,7 @@ static void setTestBug299(EngineTestHelper *eth) { // inj #0 |.......#|........|.......#|........| // inj #1 |........|.......#|........|.......#| ASSERT_EQ( 8, engine->executor.size()) << "qs#0"; - ASSERT_EQ( 3, engine->rpmCalculator.getRevolutionCounter()) << "rev cnt#3"; + ASSERT_EQ( 3, getRevolutionCounter()) << "rev cnt#3"; eth->assertInjectorUpEvent("02@0", 0, MS2US(-11.5), 0); eth->assertInjectorDownEvent("@1", 1, MS2US(-10), 0); eth->assertInjectorUpEvent("@2", 2, MS2US(-1.5), 1); @@ -650,7 +650,7 @@ static void setTestBug299(EngineTestHelper *eth) { // inj #0 |.......#|........|........|........| // inj #1 |........|.......#|........|........| ASSERT_EQ( 4, engine->executor.size()) << "qs#0-2"; - ASSERT_EQ( 4, engine->rpmCalculator.getRevolutionCounter()) << "rev cnt#4"; + ASSERT_EQ( 4, getRevolutionCounter()) << "rev cnt#4"; eth->assertInjectorUpEvent("0@0", 0, MS2US(8.5), 0); eth->assertInjectorDownEvent("0@1", 1, MS2US(10), 0); eth->assertInjectorUpEvent("0@2", 2, MS2US(18.5), 1); @@ -706,9 +706,9 @@ void doTestFuelSchedulerBug299smallAndMedium(int startUpDelayMs) { ASSERT_EQ( 0, engine->executor.size()) << "qs#1#2"; - ASSERT_EQ( 4, engine->rpmCalculator.getRevolutionCounter()) << "rev cnt#4#0"; + ASSERT_EQ( 4, getRevolutionCounter()) << "rev cnt#4#0"; eth.firePrimaryTriggerRise(); - ASSERT_EQ( 5, engine->rpmCalculator.getRevolutionCounter()) << "rev cnt#4#1"; + ASSERT_EQ( 5, getRevolutionCounter()) << "rev cnt#4#1"; // time...|0.......|10......|20......|30......|40......|50......|60......| // inj #0 |########|##...###|########|.....###|########|........|........| // inj #1 |.....###|########|....####|########|........|........|........| @@ -726,7 +726,7 @@ void doTestFuelSchedulerBug299smallAndMedium(int startUpDelayMs) { // { // scheduling_s *ev = engine->executor.getForUnitTest(9); -// ASSERT_EQ( 5, engine->rpmCalculator.getRevolutionCounter()) << "rev cnt#4#2"; +// ASSERT_EQ( 5, getRevolutionCounter()) << "rev cnt#4#2"; // ASSERT_TRUE(ev == &engineConfiguration->fuelActuators[2].signalPair[1].signalTimerDown) << "down 50"; // } @@ -736,7 +736,7 @@ void doTestFuelSchedulerBug299smallAndMedium(int startUpDelayMs) { eth.fireFall(20); ASSERT_EQ( 8, engine->executor.size()) << "qs#2#1"; - ASSERT_EQ( 5, engine->rpmCalculator.getRevolutionCounter()) << "rev cnt#5"; + ASSERT_EQ( 5, getRevolutionCounter()) << "rev cnt#5"; // using old fuel schedule - but already wider pulses // time...|-20.....|-10.....|0.......|10......|20......|30......|40......| // inj #0 |........|.....###|########|.....###|########|........|........| @@ -781,7 +781,7 @@ void doTestFuelSchedulerBug299smallAndMedium(int startUpDelayMs) { eth.firePrimaryTriggerRise(); ASSERT_EQ( 4, engine->executor.size()) << "qs#2#2"; - ASSERT_EQ( 6, engine->rpmCalculator.getRevolutionCounter()) << "rev cnt6"; + ASSERT_EQ( 6, getRevolutionCounter()) << "rev cnt6"; // time...|-20.....|-10.....|0.......|10......|20......|30......|40......| // inj #0 |########|.....###|########|....####|........|........|........| // inj #1 |.....###|########|.....###|########|........|........|........| @@ -821,7 +821,7 @@ void doTestFuelSchedulerBug299smallAndMedium(int startUpDelayMs) { eth.firePrimaryTriggerFall(); ASSERT_EQ( 5, engine->executor.size()) << "qs#3"; - ASSERT_EQ( 6, engine->rpmCalculator.getRevolutionCounter()) << "rev cnt6"; + ASSERT_EQ( 6, getRevolutionCounter()) << "rev cnt6"; ASSERT_EQ( 0, eth.executeActions()) << "executed #6";