From d6471a84bca260b2f129279fbab1b7f60bd36954 Mon Sep 17 00:00:00 2001 From: rusefi Date: Fri, 6 Dec 2019 01:57:11 -0500 Subject: [PATCH] Refactor Trigger System #635 injecting callback via parameters instead of nasty "bool isInitializingTrigger" field --- firmware/controllers/algo/engine.cpp | 13 ++++++++++++ firmware/controllers/algo/engine.h | 9 ++++---- .../controllers/trigger/trigger_central.cpp | 2 +- .../controllers/trigger/trigger_decoder.cpp | 19 +++++++++-------- .../controllers/trigger/trigger_decoder.h | 7 +++++++ .../controllers/trigger/trigger_simulator.cpp | 21 +++++++++++++------ unit_tests/tests/test_trigger_decoder.cpp | 16 +++++++------- 7 files changed, 59 insertions(+), 28 deletions(-) diff --git a/firmware/controllers/algo/engine.cpp b/firmware/controllers/algo/engine.cpp index 337409f13d..040656d5b9 100644 --- a/firmware/controllers/algo/engine.cpp +++ b/firmware/controllers/algo/engine.cpp @@ -227,6 +227,19 @@ void Engine::preCalculate(DECLARE_ENGINE_PARAMETER_SIGNATURE) { #endif } +void Engine::OnTriggerStateDecodingError() { + Engine *engine = this; + EXPAND_Engine; + triggerCentral.triggerState.handleTriggerError(PASS_ENGINE_PARAMETER_SIGNATURE); +} + +void Engine::OnTriggerStateProperState(efitick_t nowNt) { + Engine *engine = this; + EXPAND_Engine; + rpmCalculator.setSpinningUp(nowNt PASS_ENGINE_PARAMETER_SUFFIX); +} + + void Engine::setConfig(persistent_config_s *config) { this->config = config; engineConfigurationPtr = &config->engineConfiguration; diff --git a/firmware/controllers/algo/engine.h b/firmware/controllers/algo/engine.h index 30d3a9db12..a303652a8a 100644 --- a/firmware/controllers/algo/engine.h +++ b/firmware/controllers/algo/engine.h @@ -47,10 +47,14 @@ class RpmCalculator; #define CYCLE_ALTERNATION 2 -class Engine { +class Engine : public TriggerStateListener { public: explicit Engine(persistent_config_s *config); Engine(); + + void OnTriggerStateDecodingError() override; + void OnTriggerStateProperState(efitick_t nowNt) override; + void setConfig(persistent_config_s *config); injection_mode_e getCurrentInjectionMode(DECLARE_ENGINE_PARAMETER_SIGNATURE); @@ -140,9 +144,6 @@ public: // timestamp of most recent time RPM hard limit was triggered efitime_t rpmHardLimitTimestamp = 0; - // todo: should be a field on some other class, not Engine? - bool isInitializingTrigger = false; - /** * This flag indicated a big enough problem that engine control would be * prohibited if this flag is set to true. diff --git a/firmware/controllers/trigger/trigger_central.cpp b/firmware/controllers/trigger/trigger_central.cpp index b6bb3d5281..0adecf84a5 100644 --- a/firmware/controllers/trigger/trigger_central.cpp +++ b/firmware/controllers/trigger/trigger_central.cpp @@ -364,7 +364,7 @@ void TriggerCentral::handleShaftSignal(trigger_event_e signal DECLARE_ENGINE_PAR /** * This invocation changes the state of triggerState */ - triggerState.decodeTriggerEvent(nullptr, signal, nowNt PASS_ENGINE_PARAMETER_SUFFIX); + triggerState.decodeTriggerEvent(nullptr, engine, signal, nowNt PASS_ENGINE_PARAMETER_SUFFIX); /** * If we only have a crank position sensor with four stroke, here we are extending crank revolutions with a 360 degree diff --git a/firmware/controllers/trigger/trigger_decoder.cpp b/firmware/controllers/trigger/trigger_decoder.cpp index 7be8588060..d6d557a3ab 100644 --- a/firmware/controllers/trigger/trigger_decoder.cpp +++ b/firmware/controllers/trigger/trigger_decoder.cpp @@ -343,7 +343,7 @@ bool TriggerState::validateEventCounters(DECLARE_ENGINE_PARAMETER_SIGNATURE) con || currentCycle.eventCount[2] != TRIGGER_SHAPE(expectedEventCount[2]); #if EFI_UNIT_TEST - printf("sync point: isDecodingError=%d isInit=%d\r\n", isDecodingError, engine->isInitializingTrigger); + printf("sync point: isDecodingError=%d\r\n", isDecodingError); if (isDecodingError) { printf("count: cur=%d exp=%d\r\n", currentCycle.eventCount[0], TRIGGER_SHAPE(expectedEventCount[0])); printf("count: cur=%d exp=%d\r\n", currentCycle.eventCount[1], TRIGGER_SHAPE(expectedEventCount[1])); @@ -417,6 +417,7 @@ void TriggerState::onShaftSynchronization(const TriggerStateCallback triggerCycl * @param nowNt current time */ void TriggerState::decodeTriggerEvent(const TriggerStateCallback triggerCycleCallback, + TriggerStateListener * triggerStateListener, trigger_event_e const signal, efitime_t nowNt DECLARE_ENGINE_PARAMETER_SUFFIX) { ScopePerf perf(PE::DecodeTriggerEvent, static_cast(signal)); @@ -449,6 +450,7 @@ void TriggerState::decodeTriggerEvent(const TriggerStateCallback triggerCycleCal toothDurations[0] = currentDurationLong > 10 * US2NT(US_PER_SECOND_LL) ? 10 * US2NT(US_PER_SECOND_LL) : currentDurationLong; + bool haveListener = triggerStateListener != NULL; bool isPrimary = triggerWheel == T_PRIMARY; if (needToSkipFall(type) || needToSkipRise(type) || (!considerEventForGap())) { @@ -647,8 +649,10 @@ void TriggerState::decodeTriggerEvent(const TriggerStateCallback triggerCycleCal enginePins.triggerDecoderErrorPin.setValue(isDecodingError); - if (isDecodingError && !engine->isInitializingTrigger) { - handleTriggerError(PASS_ENGINE_PARAMETER_SIGNATURE); + // 'haveListener' means we are running a real engine and now just preparing trigger shape + // that's a bit of a hack, a sweet OOP solution would be a real callback or at least 'needDecodingErrorLogic' method? + if (isDecodingError && haveListener) { + triggerStateListener->OnTriggerStateDecodingError(); } errorDetection.add(isDecodingError); @@ -674,7 +678,7 @@ void TriggerState::decodeTriggerEvent(const TriggerStateCallback triggerCycleCal toothed_previous_time = nowNt; } - if (!isValidIndex(PASS_ENGINE_PARAMETER_SIGNATURE) && !engine->isInitializingTrigger) { + if (!isValidIndex(PASS_ENGINE_PARAMETER_SIGNATURE) && haveListener) { // let's not show a warning if we are just starting to spin if (GET_RPM_VALUE != 0) { warning(CUSTOM_SYNC_ERROR, "sync error: index #%d above total size %d", currentCycle.current_index, getTriggerSize()); @@ -691,8 +695,8 @@ void TriggerState::decodeTriggerEvent(const TriggerStateCallback triggerCycleCal runtimeStatistics(nowNt PASS_ENGINE_PARAMETER_SUFFIX); // Needed for early instant-RPM detection - if (!engine->isInitializingTrigger) { - engine->rpmCalculator.setSpinningUp(nowNt PASS_ENGINE_PARAMETER_SUFFIX); + if (haveListener) { + triggerStateListener->OnTriggerStateProperState(nowNt); } } @@ -724,14 +728,12 @@ uint32_t findTriggerZeroEventIndex(TriggerState *state, TriggerShape * shape, return 0; } - engine->isInitializingTrigger = true; // todo: should this variable be declared 'static' to reduce stack usage? TriggerStimulatorHelper helper; uint32_t syncIndex = helper.findTriggerSyncPoint(shape, state PASS_ENGINE_PARAMETER_SUFFIX); if (syncIndex == EFI_ERROR_CODE) { - engine->isInitializingTrigger = false; return syncIndex; } efiAssert(CUSTOM_ERR_ASSERT, state->getTotalRevolutionCounter() == 1, "findZero_revCounter", EFI_ERROR_CODE); @@ -751,7 +753,6 @@ uint32_t findTriggerZeroEventIndex(TriggerState *state, TriggerShape * shape, helper.assertSyncPositionAndSetDutyCycle(onFindIndexCallback, syncIndex, state, shape PASS_ENGINE_PARAMETER_SUFFIX); - engine->isInitializingTrigger = false; return syncIndex % shape->getSize(); } diff --git a/firmware/controllers/trigger/trigger_decoder.h b/firmware/controllers/trigger/trigger_decoder.h index cc1c6579ad..9bd939c792 100644 --- a/firmware/controllers/trigger/trigger_decoder.h +++ b/firmware/controllers/trigger/trigger_decoder.h @@ -14,6 +14,12 @@ class TriggerState; +class TriggerStateListener { + public: + virtual void OnTriggerStateDecodingError() = 0; + virtual void OnTriggerStateProperState(efitick_t nowNt) = 0; +}; + typedef void (*TriggerStateCallback)(TriggerState *); typedef struct { @@ -60,6 +66,7 @@ public: efitime_t getTotalEventCounter() const; void decodeTriggerEvent(const TriggerStateCallback triggerCycleCallback, + TriggerStateListener * triggerStateListener, trigger_event_e const signal, efitime_t nowUs DECLARE_ENGINE_PARAMETER_SUFFIX); bool validateEventCounters(DECLARE_ENGINE_PARAMETER_SIGNATURE) const; diff --git a/firmware/controllers/trigger/trigger_simulator.cpp b/firmware/controllers/trigger/trigger_simulator.cpp index cf0f5fbef6..6cd0021674 100644 --- a/firmware/controllers/trigger/trigger_simulator.cpp +++ b/firmware/controllers/trigger/trigger_simulator.cpp @@ -63,22 +63,31 @@ void TriggerStimulatorHelper::feedSimulatedEvent(const TriggerStateCallback trig if (needEvent(stateIndex, size, multiChannelStateSequence, 0)) { pin_state_t currentValue = multiChannelStateSequence->getChannelState(/*phaseIndex*/0, stateIndex); trigger_event_e s = currentValue ? SHAFT_PRIMARY_RISING : SHAFT_PRIMARY_FALLING; - if (isUsefulSignal(s PASS_ENGINE_PARAMETER_SUFFIX)) - state->decodeTriggerEvent(triggerCycleCallback, s, time PASS_ENGINE_PARAMETER_SUFFIX); + if (isUsefulSignal(s PASS_ENGINE_PARAMETER_SUFFIX)) { + state->decodeTriggerEvent(triggerCycleCallback, + /* override */ nullptr, + s, time PASS_ENGINE_PARAMETER_SUFFIX); + } } if (needEvent(stateIndex, size, multiChannelStateSequence, 1)) { pin_state_t currentValue = multiChannelStateSequence->getChannelState(/*phaseIndex*/1, stateIndex); trigger_event_e s = currentValue ? SHAFT_SECONDARY_RISING : SHAFT_SECONDARY_FALLING; - if (isUsefulSignal(s PASS_ENGINE_PARAMETER_SUFFIX)) - state->decodeTriggerEvent(triggerCycleCallback, s, time PASS_ENGINE_PARAMETER_SUFFIX); + if (isUsefulSignal(s PASS_ENGINE_PARAMETER_SUFFIX)) { + state->decodeTriggerEvent(triggerCycleCallback, + /* override */ nullptr, + s, time PASS_ENGINE_PARAMETER_SUFFIX); + } } if (needEvent(stateIndex, size, multiChannelStateSequence, 2)) { pin_state_t currentValue = multiChannelStateSequence->getChannelState(/*phaseIndex*/2, stateIndex); trigger_event_e s = currentValue ? SHAFT_3RD_RISING : SHAFT_3RD_FALLING; - if (isUsefulSignal(s PASS_ENGINE_PARAMETER_SUFFIX)) - state->decodeTriggerEvent(triggerCycleCallback, s, time PASS_ENGINE_PARAMETER_SUFFIX); + if (isUsefulSignal(s PASS_ENGINE_PARAMETER_SUFFIX)) { + state->decodeTriggerEvent(triggerCycleCallback, + /* override */ nullptr, + s, time PASS_ENGINE_PARAMETER_SUFFIX); + } } } diff --git a/unit_tests/tests/test_trigger_decoder.cpp b/unit_tests/tests/test_trigger_decoder.cpp index d5401ac1f1..0dadb5b87e 100644 --- a/unit_tests/tests/test_trigger_decoder.cpp +++ b/unit_tests/tests/test_trigger_decoder.cpp @@ -118,28 +118,28 @@ TEST(misc, testSomethingWeird) { ASSERT_FALSE(sta->shaft_is_synchronized) << "shaft_is_synchronized"; int r = 10; - sta->decodeTriggerEvent(nullptr, SHAFT_PRIMARY_FALLING, r PASS_ENGINE_PARAMETER_SUFFIX); + sta->decodeTriggerEvent(nullptr, /* override */ nullptr,SHAFT_PRIMARY_FALLING, r PASS_ENGINE_PARAMETER_SUFFIX); ASSERT_FALSE(sta->shaft_is_synchronized) << "shaft_is_synchronized"; // still no synchronization - sta->decodeTriggerEvent(nullptr, SHAFT_PRIMARY_RISING, ++r PASS_ENGINE_PARAMETER_SUFFIX); + sta->decodeTriggerEvent(nullptr, /* override */ nullptr,SHAFT_PRIMARY_RISING, ++r PASS_ENGINE_PARAMETER_SUFFIX); ASSERT_TRUE(sta->shaft_is_synchronized); // first signal rise synchronize ASSERT_EQ(0, sta->getCurrentIndex()); - sta->decodeTriggerEvent(nullptr, SHAFT_PRIMARY_FALLING, r++ PASS_ENGINE_PARAMETER_SUFFIX); + sta->decodeTriggerEvent(nullptr, /* override */ nullptr,SHAFT_PRIMARY_FALLING, r++ PASS_ENGINE_PARAMETER_SUFFIX); ASSERT_EQ(1, sta->getCurrentIndex()); for (int i = 2; i < 10;) { - sta->decodeTriggerEvent(nullptr, SHAFT_PRIMARY_RISING, r++ PASS_ENGINE_PARAMETER_SUFFIX); + sta->decodeTriggerEvent(nullptr, /* override */ nullptr,SHAFT_PRIMARY_RISING, r++ PASS_ENGINE_PARAMETER_SUFFIX); assertEqualsM("even", i++, sta->getCurrentIndex()); - sta->decodeTriggerEvent(nullptr, SHAFT_PRIMARY_FALLING, r++ PASS_ENGINE_PARAMETER_SUFFIX); + sta->decodeTriggerEvent(nullptr, /* override */ nullptr,SHAFT_PRIMARY_FALLING, r++ PASS_ENGINE_PARAMETER_SUFFIX); assertEqualsM("odd", i++, sta->getCurrentIndex()); } - sta->decodeTriggerEvent(nullptr, SHAFT_PRIMARY_RISING, r++ PASS_ENGINE_PARAMETER_SUFFIX); + sta->decodeTriggerEvent(nullptr, /* override */ nullptr,SHAFT_PRIMARY_RISING, r++ PASS_ENGINE_PARAMETER_SUFFIX); ASSERT_EQ(10, sta->getCurrentIndex()); - sta->decodeTriggerEvent(nullptr, SHAFT_PRIMARY_FALLING, r++ PASS_ENGINE_PARAMETER_SUFFIX); + sta->decodeTriggerEvent(nullptr, /* override */ nullptr,SHAFT_PRIMARY_FALLING, r++ PASS_ENGINE_PARAMETER_SUFFIX); ASSERT_EQ(11, sta->getCurrentIndex()); - sta->decodeTriggerEvent(nullptr, SHAFT_PRIMARY_RISING, r++ PASS_ENGINE_PARAMETER_SUFFIX); + sta->decodeTriggerEvent(nullptr, /* override */ nullptr,SHAFT_PRIMARY_RISING, r++ PASS_ENGINE_PARAMETER_SUFFIX); ASSERT_EQ(0, sta->getCurrentIndex()); // new revolution }