From ebc884e8d5d3d1907a7fbef2a1f75cf6a306ca00 Mon Sep 17 00:00:00 2001 From: Matthew Kennedy Date: Sun, 4 Oct 2020 16:29:26 -0700 Subject: [PATCH] trigger decode cleanup and perf (#1853) * trigger decode cleanup * test config ptr patchup * disable CDM by default --- firmware/config/stm32f4ems/efifeatures.h | 2 +- firmware/controllers/algo/engine.cpp | 16 ++++- firmware/controllers/algo/engine.h | 16 ++--- firmware/controllers/algo/engine2.cpp | 34 ++--------- .../controllers/trigger/trigger_central.cpp | 9 ++- .../controllers/trigger/trigger_central.h | 3 +- .../controllers/trigger/trigger_decoder.cpp | 58 +++++-------------- .../controllers/trigger/trigger_decoder.h | 6 +- 8 files changed, 53 insertions(+), 91 deletions(-) diff --git a/firmware/config/stm32f4ems/efifeatures.h b/firmware/config/stm32f4ems/efifeatures.h index 3e35b54acb..70dac954f7 100644 --- a/firmware/config/stm32f4ems/efifeatures.h +++ b/firmware/config/stm32f4ems/efifeatures.h @@ -18,7 +18,7 @@ #define EFI_FSIO TRUE #ifndef EFI_CDM_INTEGRATION -#define EFI_CDM_INTEGRATION TRUE +#define EFI_CDM_INTEGRATION FALSE #endif #ifndef EFI_TOOTH_LOGGER diff --git a/firmware/controllers/algo/engine.cpp b/firmware/controllers/algo/engine.cpp index 85ceb3db6a..03103029b9 100644 --- a/firmware/controllers/algo/engine.cpp +++ b/firmware/controllers/algo/engine.cpp @@ -94,6 +94,7 @@ trigger_type_e getVvtTriggerType(vvt_mode_e vvtMode) { void Engine::initializeTriggerWaveform(Logging *logger DECLARE_ENGINE_PARAMETER_SUFFIX) { static TriggerState initState; + INJECT_ENGINE_REFERENCE(&initState); #if EFI_ENGINE_CONTROL && EFI_SHAFT_POSITION_INPUT // we have a confusing threading model so some synchronization would not hurt @@ -245,11 +246,11 @@ void Engine::onTriggerSignalEvent(efitick_t nowNt) { lastTriggerToothEventTimeNt = nowNt; } -Engine::Engine() : primaryTriggerConfiguration(this), vvtTriggerConfiguration(this) { +Engine::Engine() { reset(); } -Engine::Engine(persistent_config_s *config) : primaryTriggerConfiguration(this), vvtTriggerConfiguration(this) { +Engine::Engine(persistent_config_s *config) { setConfig(config); reset(); } @@ -394,10 +395,21 @@ void Engine::OnTriggerSyncronization(bool wasSynchronized) { } #endif +void Engine::injectEngineReferences() { + Engine *engine = this; + EXPAND_Engine; + + INJECT_ENGINE_REFERENCE(&primaryTriggerConfiguration); + INJECT_ENGINE_REFERENCE(&vvtTriggerConfiguration); + triggerCentral.init(PASS_ENGINE_PARAMETER_SIGNATURE); +} + void Engine::setConfig(persistent_config_s *config) { this->config = config; engineConfigurationPtr = &config->engineConfiguration; memset(config, 0, sizeof(persistent_config_s)); + + injectEngineReferences(); } void Engine::printKnockState(void) { diff --git a/firmware/controllers/algo/engine.h b/firmware/controllers/algo/engine.h index a8fadf9373..e6fbfd5027 100644 --- a/firmware/controllers/algo/engine.h +++ b/firmware/controllers/algo/engine.h @@ -54,33 +54,25 @@ class IEtbController; class IFuelComputer; class IInjectorModel; -class PrimaryTriggerConfiguration : public TriggerConfiguration { +class PrimaryTriggerConfiguration final : public TriggerConfiguration { public: - PrimaryTriggerConfiguration(Engine *engine); bool isUseOnlyRisingEdgeForTrigger() const; const char * getPrintPrefix() const; - bool isSilentTriggerError() const; bool isVerboseTriggerSynchDetails() const; debug_mode_e getDebugMode() const; trigger_type_e getType() const; -private: - Engine *engine; }; -class VvtTriggerConfiguration : public TriggerConfiguration { +class VvtTriggerConfiguration final : public TriggerConfiguration { public: - VvtTriggerConfiguration(Engine *engine); bool isUseOnlyRisingEdgeForTrigger() const; const char * getPrintPrefix() const; - bool isSilentTriggerError() const; bool isVerboseTriggerSynchDetails() const; debug_mode_e getDebugMode() const; trigger_type_e getType() const; -private: - Engine *engine; }; -class Engine : public TriggerStateListener { +class Engine final : public TriggerStateListener { public: explicit Engine(persistent_config_s *config); Engine(); @@ -398,6 +390,8 @@ private: */ bool isSpinning = false; void reset(); + + void injectEngineReferences(); }; void prepareShapes(DECLARE_ENGINE_PARAMETER_SIGNATURE); diff --git a/firmware/controllers/algo/engine2.cpp b/firmware/controllers/algo/engine2.cpp index 45a1d78ffe..556d444bdc 100644 --- a/firmware/controllers/algo/engine2.cpp +++ b/firmware/controllers/algo/engine2.cpp @@ -251,24 +251,12 @@ void printCurrentState(Logging *logging, int seconds, const char *engineTypeName DELIMETER); } -PrimaryTriggerConfiguration::PrimaryTriggerConfiguration(Engine *engine) { - this->engine = engine; -} - bool PrimaryTriggerConfiguration::isUseOnlyRisingEdgeForTrigger() const { - return engine->engineConfigurationPtr->useOnlyRisingEdgeForTrigger; -} - -debug_mode_e PrimaryTriggerConfiguration::getDebugMode() const { - return engine->engineConfigurationPtr->debugMode; + return CONFIG(useOnlyRisingEdgeForTrigger); } trigger_type_e PrimaryTriggerConfiguration::getType() const { - return engine->engineConfigurationPtr->trigger.type; -} - -bool PrimaryTriggerConfiguration::isSilentTriggerError() const { - return engine->engineConfigurationPtr->silentTriggerError; + return CONFIG(trigger.type); } const char * PrimaryTriggerConfiguration::getPrintPrefix() const { @@ -276,33 +264,21 @@ const char * PrimaryTriggerConfiguration::getPrintPrefix() const { } bool PrimaryTriggerConfiguration::isVerboseTriggerSynchDetails() const { - return engine->engineConfigurationPtr->verboseTriggerSynchDetails; -} - -VvtTriggerConfiguration::VvtTriggerConfiguration(Engine *engine) { - this->engine = engine; + return CONFIG(verboseTriggerSynchDetails); } bool VvtTriggerConfiguration::isUseOnlyRisingEdgeForTrigger() const { - return engine->engineConfigurationPtr->vvtCamSensorUseRise; + return CONFIG(vvtCamSensorUseRise); } const char * VvtTriggerConfiguration::getPrintPrefix() const { return "VVT "; } -debug_mode_e VvtTriggerConfiguration::getDebugMode() const { - return engine->engineConfigurationPtr->debugMode; -} - trigger_type_e VvtTriggerConfiguration::getType() const { return engine->triggerCentral.vvtTriggerType; } -bool VvtTriggerConfiguration::isSilentTriggerError() const { - return engine->engineConfigurationPtr->silentTriggerError; -} - bool VvtTriggerConfiguration::isVerboseTriggerSynchDetails() const { - return engine->engineConfigurationPtr->verboseVVTDecoding; + return CONFIG(verboseVVTDecoding); } diff --git a/firmware/controllers/trigger/trigger_central.cpp b/firmware/controllers/trigger/trigger_central.cpp index 3a2d9956bf..0184b704a2 100644 --- a/firmware/controllers/trigger/trigger_central.cpp +++ b/firmware/controllers/trigger/trigger_central.cpp @@ -51,6 +51,11 @@ TriggerCentral::TriggerCentral() : trigger_central_s() { noiseFilter.resetAccumSignalData(); } +void TriggerCentral::init(DECLARE_ENGINE_PARAMETER_SIGNATURE) { + INJECT_ENGINE_REFERENCE(&triggerState); + INJECT_ENGINE_REFERENCE(&vvtState); +} + void TriggerNoiseFilter::resetAccumSignalData() { memset(lastSignalTimes, 0xff, sizeof(lastSignalTimes)); // = -1 memset(accumSignalPeriods, 0, sizeof(accumSignalPeriods)); @@ -429,8 +434,8 @@ void TriggerCentral::handleShaftSignal(trigger_event_e signal, efitick_t timesta if (!noiseFilter.noiseFilter(timestamp, &triggerState, signal PASS_ENGINE_PARAMETER_SUFFIX)) { return; } - const TriggerConfiguration * triggerConfiguration = &ENGINE(primaryTriggerConfiguration); - // moved here from hwHandleShaftSignal() + const TriggerConfiguration* triggerConfiguration = &ENGINE(primaryTriggerConfiguration); + if (!isUsefulSignal(signal, triggerConfiguration)) { return; } diff --git a/firmware/controllers/trigger/trigger_central.h b/firmware/controllers/trigger/trigger_central.h index edc33bc75b..9e93785245 100644 --- a/firmware/controllers/trigger/trigger_central.h +++ b/firmware/controllers/trigger/trigger_central.h @@ -34,9 +34,10 @@ public: * Probably not: we have an instance of TriggerState which is used for trigger initialization, * also composition probably better than inheritance here */ -class TriggerCentral : public trigger_central_s { +class TriggerCentral final : public trigger_central_s { public: TriggerCentral(); + void init(DECLARE_ENGINE_PARAMETER_SIGNATURE); void addEventListener(ShaftPositionListener handler, const char *name, Engine *engine); void handleShaftSignal(trigger_event_e signal, efitick_t timestamp DECLARE_ENGINE_PARAMETER_SUFFIX); int getHwEventCounter(int index) const; diff --git a/firmware/controllers/trigger/trigger_decoder.cpp b/firmware/controllers/trigger/trigger_decoder.cpp index 03861779ea..fe11025126 100644 --- a/firmware/controllers/trigger/trigger_decoder.cpp +++ b/firmware/controllers/trigger/trigger_decoder.cpp @@ -400,10 +400,10 @@ void TriggerState::onShaftSynchronization( * @param nowNt current time */ void TriggerState::decodeTriggerEvent( - const TriggerWaveform *triggerShape, + const TriggerWaveform* const triggerShape, const TriggerStateCallback triggerCycleCallback, - TriggerStateListener * triggerStateListener, - const TriggerConfiguration * triggerConfiguration, + TriggerStateListener* triggerStateListener, + const TriggerConfiguration* const triggerConfiguration, const trigger_event_e signal, const efitick_t nowNt) { ScopePerf perf(PE::DecodeTriggerEvent); @@ -468,29 +468,18 @@ void TriggerState::decodeTriggerEvent( nextTriggerEvent() ; } else { - -#if EFI_UNIT_TEST +#if !EFI_PROD_CODE if (printTriggerTrace) { printf("%s event %s %d\r\n", getTrigger_type_e(triggerConfiguration->getType()), getTrigger_event_e(signal), nowNt); - } -#endif /* EFI_UNIT_TEST */ - - isFirstEvent = false; -// todo: skip a number of signal from the beginning - -#if EFI_PROD_CODE -// scheduleMsg(&logger, "from %.2f to %.2f %d %d", triggerConfig->syncRatioFrom, triggerConfig->syncRatioTo, toothDurations[0], shaftPositionState->toothDurations[1]); -// scheduleMsg(&logger, "ratio %.2f", 1.0 * toothDurations[0]/ shaftPositionState->toothDurations[1]); -#else - if (printTriggerTrace) { printf("decodeTriggerEvent ratio %.2f: current=%d previous=%d\r\n", 1.0 * toothDurations[0] / toothDurations[1], toothDurations[0], toothDurations[1]); } #endif + isFirstEvent = false; bool isSynchronizationPoint; bool wasSynchronized = shaft_is_synchronized; @@ -521,10 +510,9 @@ void TriggerState::decodeTriggerEvent( DISPLAY(DISPLAY_FIELD(vvtCamCounter)); if (triggerShape->isSynchronizationNeeded) { - currentGap = 1.0 * toothDurations[0] / toothDurations[1]; - if (triggerConfiguration->getDebugMode() == DBG_TRIGGER_COUNTERS) { + if (CONFIG(debugMode) == DBG_TRIGGER_COUNTERS) { #if EFI_TUNER_STUDIO tsOutputChannels.debugFloatField6 = currentGap; tsOutputChannels.debugIntField3 = currentCycle.current_index; @@ -532,25 +520,24 @@ void TriggerState::decodeTriggerEvent( } bool isSync = true; - for (int i = 0;igapTrackingLength;i++) { + for (int i = 0; i < triggerShape->gapTrackingLength; i++) { bool isGapCondition = cisnan(triggerShape->syncronizationRatioFrom[i]) || (toothDurations[i] > toothDurations[i + 1] * triggerShape->syncronizationRatioFrom[i] && toothDurations[i] < toothDurations[i + 1] * triggerShape->syncronizationRatioTo[i]); isSync &= isGapCondition; - } + isSynchronizationPoint = isSync; if (isSynchronizationPoint) { - enginePins.debugTriggerSync.setValue(1); + enginePins.debugTriggerSync.toggle(); } - /** * todo: technically we can afford detailed logging even with 60/2 as long as low RPM * todo: figure out exact threshold as a function of RPM and tooth count? * Open question what is 'triggerShape->getSize()' for 60/2 is it 58 or 58*2 or 58*4? */ - bool silentTriggerError = triggerShape->getSize() > 40 && triggerConfiguration->isSilentTriggerError(); + bool silentTriggerError = triggerShape->getSize() > 40 && CONFIG(silentTriggerError); #if EFI_UNIT_TEST actualSynchGap = 1.0 * toothDurations[0] / toothDurations[1]; @@ -595,11 +582,7 @@ void TriggerState::decodeTriggerEvent( boolToString(someSortOfTriggerError)); } } - - #endif /* EFI_PROD_CODE */ - enginePins.debugTriggerSync.setValue(0); - } else { /** * We are here in case of a wheel without synchronization - we just need to count events, @@ -608,30 +591,20 @@ void TriggerState::decodeTriggerEvent( * in case of noise the counter could be above the expected number of events, that's why 'more or equals' and not just 'equals' */ -#if EFI_UNIT_TEST - if (printTriggerTrace) { - printf("decodeTriggerEvent sync=%d index=%d size=%d\r\n", - shaft_is_synchronized, - currentCycle.current_index, - triggerShape->getSize()); - } -#endif /* EFI_UNIT_TEST */ unsigned int endOfCycleIndex = triggerShape->getSize() - (triggerConfiguration->isUseOnlyRisingEdgeForTrigger() ? 2 : 1); - isSynchronizationPoint = !shaft_is_synchronized || (currentCycle.current_index >= endOfCycleIndex); #if EFI_UNIT_TEST if (printTriggerTrace) { - printf("decodeTriggerEvent decodeTriggerEvent isSynchronizationPoint=%d index=%d size=%d\r\n", - isSynchronizationPoint, - currentCycle.current_index, - triggerShape->getSize()); + printf("decodeTriggerEvent sync=%d isSynchronizationPoint=%d index=%d size=%d\r\n", + shaft_is_synchronized, + isSynchronizationPoint, + currentCycle.current_index, + triggerShape->getSize()); } #endif /* EFI_UNIT_TEST */ - } - #if EFI_UNIT_TEST if (printTriggerTrace) { printf("decodeTriggerEvent %s isSynchronizationPoint=%d index=%d %s\r\n", @@ -642,7 +615,6 @@ void TriggerState::decodeTriggerEvent( #endif /* EFI_UNIT_TEST */ if (isSynchronizationPoint) { - if (triggerStateListener) { triggerStateListener->OnTriggerSyncronization(wasSynchronized); } diff --git a/firmware/controllers/trigger/trigger_decoder.h b/firmware/controllers/trigger/trigger_decoder.h index ed78579c05..7ea26cd207 100644 --- a/firmware/controllers/trigger/trigger_decoder.h +++ b/firmware/controllers/trigger/trigger_decoder.h @@ -25,11 +25,11 @@ struct TriggerStateListener { class TriggerConfiguration { public: + DECLARE_ENGINE_PTR; + virtual bool isUseOnlyRisingEdgeForTrigger() const = 0; - virtual bool isSilentTriggerError() const = 0; virtual bool isVerboseTriggerSynchDetails() const = 0; virtual const char * getPrintPrefix() const = 0; - virtual debug_mode_e getDebugMode() const = 0; virtual trigger_type_e getType() const = 0; }; @@ -65,6 +65,8 @@ typedef struct { */ class TriggerState : public trigger_state_s { public: + DECLARE_ENGINE_PTR; + TriggerState(); /** * current trigger processing index, between zero and #size