From f945e6efc9253ae346e68366b1d349331ff71a5f Mon Sep 17 00:00:00 2001 From: Matthew Kennedy Date: Mon, 30 May 2022 16:36:47 -0700 Subject: [PATCH] trigger configuration refactoring for clarity (#4212) * This field was ignored. * move pad out * gone * make trigger configuration a little clearer * even simpler! * format * test fix --- firmware/controllers/algo/engine.cpp | 34 +++++++--------- firmware/controllers/algo/engine.h | 4 +- firmware/controllers/algo/engine2.cpp | 10 ++--- .../trigger/decoders/trigger_structure.cpp | 12 +++--- .../trigger/decoders/trigger_structure.h | 7 ++-- .../controllers/trigger/trigger_decoder.cpp | 22 ++++------ .../controllers/trigger/trigger_decoder.h | 7 ++-- .../controllers/trigger/trigger_simulator.cpp | 4 +- .../tests/trigger/test_trigger_decoder.cpp | 3 +- .../tests/trigger/test_trigger_decoder_2.cpp | 40 ++++++++----------- 10 files changed, 62 insertions(+), 81 deletions(-) diff --git a/firmware/controllers/algo/engine.cpp b/firmware/controllers/algo/engine.cpp index c4cac4807e..78175bd6bd 100644 --- a/firmware/controllers/algo/engine.cpp +++ b/firmware/controllers/algo/engine.cpp @@ -106,23 +106,12 @@ static operation_mode_e lookupOperationMode() { } } -static void initVvtShape(int camIndex, TriggerDecoderBase &initState) { - vvt_mode_e vvtMode = engineConfiguration->vvtMode[camIndex]; +static void initVvtShape(TriggerWaveform& shape, const TriggerConfiguration& config, TriggerDecoderBase &initState) { + shape.initializeTriggerWaveform( + lookupOperationMode(), + engineConfiguration->vvtCamSensorUseRise, config); - if (vvtMode != VVT_INACTIVE) { - trigger_config_s config; - // todo: should 'vvtWithRealDecoder' be used here? - config.type = getVvtTriggerType(vvtMode); - - auto& shape = engine->triggerCentral.vvtShape[camIndex]; - shape.initializeTriggerWaveform( - lookupOperationMode(), - engineConfiguration->vvtCamSensorUseRise, &config); - - shape.initializeSyncPoint(initState, - engine->vvtTriggerConfiguration[camIndex], - config); - } + shape.initializeSyncPoint(initState, config); } void Engine::updateTriggerWaveform() { @@ -140,7 +129,7 @@ void Engine::updateTriggerWaveform() { TRIGGER_WAVEFORM(initializeTriggerWaveform( lookupOperationMode(), - engineConfiguration->useOnlyRisingEdgeForTrigger, &engineConfiguration->trigger)); + engineConfiguration->useOnlyRisingEdgeForTrigger, primaryTriggerConfiguration)); /** * this is only useful while troubleshooting a new trigger shape in the field @@ -180,8 +169,15 @@ void Engine::updateTriggerWaveform() { engine->triggerCentral.vvtState[1][0].name = "VVT B2 Int"; engine->triggerCentral.vvtState[1][1].name = "VVT B2 Exh"; - for (int camIndex = 0;camIndex < CAMS_PER_BANK;camIndex++) { - initVvtShape(camIndex, initState); + for (int camIndex = 0; camIndex < CAMS_PER_BANK; camIndex++) { + // todo: should 'vvtWithRealDecoder' be used here? + if (engineConfiguration->vvtMode[camIndex] != VVT_INACTIVE) { + initVvtShape( + triggerCentral.vvtShape[camIndex], + vvtTriggerConfiguration[camIndex], + initState + ); + } } if (!TRIGGER_WAVEFORM(shapeDefinitionError)) { diff --git a/firmware/controllers/algo/engine.h b/firmware/controllers/algo/engine.h index af887dea78..2a4c16fe69 100644 --- a/firmware/controllers/algo/engine.h +++ b/firmware/controllers/algo/engine.h @@ -96,7 +96,7 @@ public: protected: bool isUseOnlyRisingEdgeForTrigger() const override; bool isVerboseTriggerSynchDetails() const override; - trigger_type_e getType() const override; + trigger_config_s getType() const override; }; class VvtTriggerConfiguration final : public TriggerConfiguration { @@ -109,7 +109,7 @@ public: protected: bool isUseOnlyRisingEdgeForTrigger() const override; bool isVerboseTriggerSynchDetails() const override; - trigger_type_e getType() const override; + trigger_config_s getType() const override; }; class PrimeController : public EngineModule { diff --git a/firmware/controllers/algo/engine2.cpp b/firmware/controllers/algo/engine2.cpp index 65d00b9b11..e689438a1d 100644 --- a/firmware/controllers/algo/engine2.cpp +++ b/firmware/controllers/algo/engine2.cpp @@ -203,8 +203,8 @@ bool PrimaryTriggerConfiguration::isUseOnlyRisingEdgeForTrigger() const { return engineConfiguration->useOnlyRisingEdgeForTrigger; } -trigger_type_e PrimaryTriggerConfiguration::getType() const { - return engineConfiguration->trigger.type; +trigger_config_s PrimaryTriggerConfiguration::getType() const { + return engineConfiguration->trigger; } bool PrimaryTriggerConfiguration::isVerboseTriggerSynchDetails() const { @@ -215,9 +215,9 @@ bool VvtTriggerConfiguration::isUseOnlyRisingEdgeForTrigger() const { return engineConfiguration->vvtCamSensorUseRise; } -trigger_type_e VvtTriggerConfiguration::getType() const { - // Convert from VVT type to trigger type - return getVvtTriggerType(engineConfiguration->vvtMode[index]); +trigger_config_s VvtTriggerConfiguration::getType() const { + // Convert from VVT type to trigger_config_s + return { getVvtTriggerType(engineConfiguration->vvtMode[index]), 0, 0 }; } bool VvtTriggerConfiguration::isVerboseTriggerSynchDetails() const { diff --git a/firmware/controllers/trigger/decoders/trigger_structure.cpp b/firmware/controllers/trigger/decoders/trigger_structure.cpp index e344465852..52bb1b6141 100644 --- a/firmware/controllers/trigger/decoders/trigger_structure.cpp +++ b/firmware/controllers/trigger/decoders/trigger_structure.cpp @@ -447,26 +447,26 @@ void TriggerWaveform::setThirdTriggerSynchronizationGap(float syncRatio) { /** * External logger is needed because at this point our logger is not yet initialized */ -void TriggerWaveform::initializeTriggerWaveform(operation_mode_e triggerOperationMode, bool useOnlyRisingEdgeForTrigger, const trigger_config_s *triggerConfig) { +void TriggerWaveform::initializeTriggerWaveform(operation_mode_e triggerOperationMode, bool useOnlyRisingEdgeForTrigger, const TriggerConfiguration& triggerConfig) { #if EFI_PROD_CODE efiAssertVoid(CUSTOM_ERR_6641, getCurrentRemainingStack() > EXPECTED_REMAINING_STACK, "init t"); - efiPrintf("initializeTriggerWaveform(%s/%d)", getTrigger_type_e(triggerConfig->type), (int) triggerConfig->type); + efiPrintf("initializeTriggerWaveform(%s/%d)", getTrigger_type_e(triggerConfig.TriggerType.type), (int)triggerConfig.TriggerType.type); #endif shapeDefinitionError = false; this->useOnlyRisingEdgeForTriggerTemp = useOnlyRisingEdgeForTrigger; - switch (triggerConfig->type) { + switch (triggerConfig.TriggerType.type) { case TT_TOOTHED_WHEEL: /** * huh? why all know skipped wheel shapes use 'setToothedWheelConfiguration' method * which touches 'useRiseEdge' flag while here we do not touch it?! */ - initializeSkippedToothTriggerWaveformExt(this, triggerConfig->customTotalToothCount, - triggerConfig->customSkippedToothCount, triggerOperationMode); + initializeSkippedToothTriggerWaveformExt(this, triggerConfig.TriggerType.customTotalToothCount, + triggerConfig.TriggerType.customSkippedToothCount, triggerOperationMode); break; case TT_MAZDA_MIATA_NA: @@ -775,7 +775,7 @@ void TriggerWaveform::initializeTriggerWaveform(operation_mode_e triggerOperatio default: setShapeDefinitionError(true); - warning(CUSTOM_ERR_NO_SHAPE, "initializeTriggerWaveform() not implemented: %d", triggerConfig->type); + warning(CUSTOM_ERR_NO_SHAPE, "initializeTriggerWaveform() not implemented: %d", triggerConfig.TriggerType.type); } /** * Feb 2019 suggestion: it would be an improvement to remove 'expectedEventCount' logic from 'addEvent' diff --git a/firmware/controllers/trigger/decoders/trigger_structure.h b/firmware/controllers/trigger/decoders/trigger_structure.h index d6616de6fa..10770a7de0 100644 --- a/firmware/controllers/trigger/decoders/trigger_structure.h +++ b/firmware/controllers/trigger/decoders/trigger_structure.h @@ -74,7 +74,7 @@ class TriggerWaveform { public: TriggerWaveform(); void initializeTriggerWaveform(operation_mode_e triggerOperationMode, - bool useOnlyRisingEdgeForTrigger, const trigger_config_s *triggerConfig); + bool useOnlyRisingEdgeForTrigger, const TriggerConfiguration& triggerConfig); void setShapeDefinitionError(bool value); /** @@ -265,9 +265,8 @@ public: void initializeSyncPoint( TriggerDecoderBase& state, - const TriggerConfiguration& triggerConfiguration, - const trigger_config_s& triggerConfig - ); + const TriggerConfiguration& triggerConfiguration + ); uint16_t findAngleIndex(TriggerFormDetails *details, angle_t angle) const; diff --git a/firmware/controllers/trigger/trigger_decoder.cpp b/firmware/controllers/trigger/trigger_decoder.cpp index 1f0f85b58a..de5840f9b3 100644 --- a/firmware/controllers/trigger/trigger_decoder.cpp +++ b/firmware/controllers/trigger/trigger_decoder.cpp @@ -109,10 +109,8 @@ float actualSynchGap; #endif /* ! EFI_PROD_CODE */ void TriggerWaveform::initializeSyncPoint(TriggerDecoderBase& state, - const TriggerConfiguration& triggerConfiguration, - const trigger_config_s& triggerConfig) { - triggerShapeSynchPointIndex = state.findTriggerZeroEventIndex(*this, - triggerConfiguration, triggerConfig); + const TriggerConfiguration& triggerConfiguration) { + triggerShapeSynchPointIndex = state.findTriggerZeroEventIndex(*this, triggerConfiguration); } /** @@ -127,9 +125,7 @@ void calculateTriggerSynchPoint( efiAssertVoid(CUSTOM_TRIGGER_STACK, getCurrentRemainingStack() > EXPECTED_REMAINING_STACK, "calc s"); #endif engine->triggerErrorDetection.clear(); - shape.initializeSyncPoint(state, - engine->primaryTriggerConfiguration, - engineConfiguration->trigger); + shape.initializeSyncPoint(state, engine->primaryTriggerConfiguration); int length = shape.getLength(); engine->engineCycleEventCount = length; @@ -534,7 +530,7 @@ expected TriggerDecoderBase::decodeTriggerEvent( #if EFI_UNIT_TEST if (printTriggerTrace) { printf("%s isLessImportant %s now=%d index=%d\r\n", - getTrigger_type_e(triggerConfiguration.TriggerType), + getTrigger_type_e(triggerConfiguration.TriggerType.type), getTrigger_event_e(signal), (int)nowNt, currentCycle.current_index); @@ -550,7 +546,7 @@ expected TriggerDecoderBase::decodeTriggerEvent( #if !EFI_PROD_CODE if (printTriggerTrace) { printf("%s event %s %d\r\n", - getTrigger_type_e(triggerConfiguration.TriggerType), + getTrigger_type_e(triggerConfiguration.TriggerType.type), getTrigger_event_e(signal), nowNt); printf("decodeTriggerEvent ratio %.2f: current=%d previous=%d\r\n", 1.0 * toothDurations[0] / toothDurations[1], @@ -565,7 +561,7 @@ expected TriggerDecoderBase::decodeTriggerEvent( if (triggerShape.isSynchronizationNeeded) { triggerSyncGapRatio = (float)toothDurations[0] / toothDurations[1]; - isSynchronizationPoint = isSyncPoint(triggerShape, triggerConfiguration.TriggerType); + isSynchronizationPoint = isSyncPoint(triggerShape, triggerConfiguration.TriggerType.type); if (isSynchronizationPoint) { enginePins.debugTriggerSync.toggle(); } @@ -660,7 +656,7 @@ expected TriggerDecoderBase::decodeTriggerEvent( #if EFI_UNIT_TEST if (printTriggerTrace) { printf("decodeTriggerEvent %s isSynchronizationPoint=%d index=%d %s\r\n", - getTrigger_type_e(triggerConfiguration.TriggerType), + getTrigger_type_e(triggerConfiguration.TriggerType.type), isSynchronizationPoint, currentCycle.current_index, getTrigger_event_e(signal)); } @@ -810,9 +806,7 @@ static void onFindIndexCallback(TriggerDecoderBase *state) { */ uint32_t TriggerDecoderBase::findTriggerZeroEventIndex( TriggerWaveform& shape, - const TriggerConfiguration& triggerConfiguration, - const trigger_config_s& triggerConfig) { - UNUSED(triggerConfig); + const TriggerConfiguration& triggerConfiguration) { #if EFI_PROD_CODE efiAssert(CUSTOM_ERR_ASSERT, getCurrentRemainingStack() > 128, "findPos", -1); #endif diff --git a/firmware/controllers/trigger/trigger_decoder.h b/firmware/controllers/trigger/trigger_decoder.h index f24e2bbaf2..673bcea82f 100644 --- a/firmware/controllers/trigger/trigger_decoder.h +++ b/firmware/controllers/trigger/trigger_decoder.h @@ -31,12 +31,12 @@ public: const char* const PrintPrefix; bool UseOnlyRisingEdgeForTrigger; bool VerboseTriggerSynchDetails; - trigger_type_e TriggerType; + trigger_config_s TriggerType; protected: virtual bool isUseOnlyRisingEdgeForTrigger() const = 0; virtual bool isVerboseTriggerSynchDetails() const = 0; - virtual trigger_type_e getType() const = 0; + virtual trigger_config_s getType() const = 0; }; typedef void (*TriggerStateCallback)(TriggerDecoderBase*); @@ -149,8 +149,7 @@ public: uint32_t findTriggerZeroEventIndex( TriggerWaveform& shape, - const TriggerConfiguration& triggerConfiguration, - const trigger_config_s& triggerConfig + const TriggerConfiguration& triggerConfiguration ); bool someSortOfTriggerError() const { diff --git a/firmware/controllers/trigger/trigger_simulator.cpp b/firmware/controllers/trigger/trigger_simulator.cpp index b1a0724a27..7f1d76cc31 100644 --- a/firmware/controllers/trigger/trigger_simulator.cpp +++ b/firmware/controllers/trigger/trigger_simulator.cpp @@ -115,7 +115,7 @@ void TriggerStimulatorHelper::assertSyncPositionAndSetDutyCycle( int revolutionCounter = state.getTotalRevolutionCounter(); if (revolutionCounter != TEST_REVOLUTIONS) { warning(CUSTOM_OBD_TRIGGER_WAVEFORM, "sync failed/wrong gap parameters trigger=%s revolutionCounter=%d", - getTrigger_type_e(triggerConfiguration.TriggerType), + getTrigger_type_e(triggerConfiguration.TriggerType.type), revolutionCounter); shape.setShapeDefinitionError(true); return; @@ -124,7 +124,7 @@ void TriggerStimulatorHelper::assertSyncPositionAndSetDutyCycle( #if EFI_UNIT_TEST if (printTriggerTrace) { printf("Happy %s revolutionCounter=%d\r\n", - getTrigger_type_e(triggerConfiguration.TriggerType), + getTrigger_type_e(triggerConfiguration.TriggerType.type), revolutionCounter); } #endif /* EFI_UNIT_TEST */ diff --git a/unit_tests/tests/trigger/test_trigger_decoder.cpp b/unit_tests/tests/trigger/test_trigger_decoder.cpp index a23e379083..200747a016 100644 --- a/unit_tests/tests/trigger/test_trigger_decoder.cpp +++ b/unit_tests/tests/trigger/test_trigger_decoder.cpp @@ -42,8 +42,7 @@ static int getTriggerZeroEventIndex(engine_type_e engineType) { const auto& triggerConfiguration = engine->primaryTriggerConfiguration; TriggerWaveform& shape = eth.engine.triggerCentral.triggerShape; - return eth.engine.triggerCentral.triggerState.findTriggerZeroEventIndex(shape, triggerConfiguration, - engineConfiguration->trigger); + return eth.engine.triggerCentral.triggerState.findTriggerZeroEventIndex(shape, triggerConfiguration); } TEST(trigger, testSkipped2_0) { diff --git a/unit_tests/tests/trigger/test_trigger_decoder_2.cpp b/unit_tests/tests/trigger/test_trigger_decoder_2.cpp index 8e740e7b79..9265e3ba6f 100644 --- a/unit_tests/tests/trigger/test_trigger_decoder_2.cpp +++ b/unit_tests/tests/trigger/test_trigger_decoder_2.cpp @@ -4,7 +4,7 @@ using ::testing::StrictMock; class MockTriggerConfiguration : public TriggerConfiguration { public: - MockTriggerConfiguration(bool useOnlyRise, trigger_type_e type) + MockTriggerConfiguration(bool useOnlyRise, trigger_config_s type) : TriggerConfiguration("Mock") , m_useOnlyRise(useOnlyRise) , m_type(type) @@ -19,28 +19,22 @@ protected: return false; } - trigger_type_e getType() const override { + trigger_config_s getType() const override { return m_type; } private: const bool m_useOnlyRise; - const trigger_type_e m_type; + const trigger_config_s m_type; }; struct MockTriggerDecoder : public TriggerDecoderBase { MOCK_METHOD(void, onTriggerError, (), (override)); }; -static TriggerWaveform makeTriggerShape(operation_mode_e mode) { - // Configure a 4-1 trigger wheel - trigger_config_s config; - config.type = TT_TOOTHED_WHEEL; - config.customTotalToothCount = 4; - config.customSkippedToothCount = 1; - +static auto makeTriggerShape(operation_mode_e mode, const TriggerConfiguration& config) { TriggerWaveform shape; - shape.initializeTriggerWaveform(mode, true, &config); + shape.initializeTriggerWaveform(mode, true, config); return shape; } @@ -48,11 +42,11 @@ static TriggerWaveform makeTriggerShape(operation_mode_e mode) { #define doTooth(dut, shape, cfg, t) dut.decodeTriggerEvent("", shape, nullptr, nullptr, cfg, SHAFT_PRIMARY_RISING, t) TEST(TriggerDecoder, FindsFirstSyncPoint) { - auto shape = makeTriggerShape(FOUR_STROKE_CAM_SENSOR); - - MockTriggerConfiguration cfg(true, TT_TOOTHED_WHEEL); + MockTriggerConfiguration cfg(true, {TT_TOOTHED_WHEEL, 4, 1}); cfg.update(); + auto shape = makeTriggerShape(FOUR_STROKE_CAM_SENSOR, cfg); + efitick_t t = 0; // Strict so it complains about unexpected calls to onTriggerError() @@ -91,11 +85,11 @@ TEST(TriggerDecoder, FindsFirstSyncPoint) { TEST(TriggerDecoder, FindsSyncPointMultipleRevolutions) { - auto shape = makeTriggerShape(FOUR_STROKE_CAM_SENSOR); - - MockTriggerConfiguration cfg(true, TT_TOOTHED_WHEEL); + MockTriggerConfiguration cfg(true, {TT_TOOTHED_WHEEL, 4, 1}); cfg.update(); + auto shape = makeTriggerShape(FOUR_STROKE_CAM_SENSOR, cfg); + efitick_t t = 0; // Strict so it complains about unexpected calls to onTriggerError() @@ -137,11 +131,11 @@ TEST(TriggerDecoder, FindsSyncPointMultipleRevolutions) { } TEST(TriggerDecoder, TooManyTeeth_CausesError) { - auto shape = makeTriggerShape(FOUR_STROKE_CAM_SENSOR); - - MockTriggerConfiguration cfg(true, TT_TOOTHED_WHEEL); + MockTriggerConfiguration cfg(true, {TT_TOOTHED_WHEEL, 4, 1}); cfg.update(); + auto shape = makeTriggerShape(FOUR_STROKE_CAM_SENSOR, cfg); + efitick_t t = 0; StrictMock dut; @@ -185,11 +179,11 @@ TEST(TriggerDecoder, TooManyTeeth_CausesError) { } TEST(TriggerDecoder, NotEnoughTeeth_CausesError) { - auto shape = makeTriggerShape(FOUR_STROKE_CAM_SENSOR); - - MockTriggerConfiguration cfg(true, TT_TOOTHED_WHEEL); + MockTriggerConfiguration cfg(true, {TT_TOOTHED_WHEEL, 4, 1}); cfg.update(); + auto shape = makeTriggerShape(FOUR_STROKE_CAM_SENSOR, cfg); + efitick_t t = 0; StrictMock dut;