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

@@ -106,23 +106,12 @@ static operation_mode_e lookupOperationMode() {
 	}
 }
 
-static void initVvtShape(int camIndex, TriggerDecoderBase &initState) {
+static void initVvtShape(TriggerWaveform& shape, const TriggerConfiguration& config, TriggerDecoderBase &initState) {
-	vvt_mode_e vvtMode = engineConfiguration->vvtMode[camIndex];
+	shape.initializeTriggerWaveform(
+			lookupOperationMode(),
+			engineConfiguration->vvtCamSensorUseRise, config);
 
-	if (vvtMode != VVT_INACTIVE) {
+	shape.initializeSyncPoint(initState, config);
-		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);
-	}
 }
 
 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++) {
+	for (int camIndex = 0; camIndex < CAMS_PER_BANK; camIndex++) {
-		initVvtShape(camIndex, initState);
+		// todo: should 'vvtWithRealDecoder' be used here?
+		if (engineConfiguration->vvtMode[camIndex] != VVT_INACTIVE) {
+			initVvtShape(
+				triggerCentral.vvtShape[camIndex],
+				vvtTriggerConfiguration[camIndex],
+				initState
+			);
+		}
 	}
 
 	if (!TRIGGER_WAVEFORM(shapeDefinitionError)) {

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 {

firmware/controllers/algo/engine2.cpp

@@ -203,8 +203,8 @@ bool PrimaryTriggerConfiguration::isUseOnlyRisingEdgeForTrigger() const {
 	return engineConfiguration->useOnlyRisingEdgeForTrigger;
 }
 
-trigger_type_e PrimaryTriggerConfiguration::getType() const {
+trigger_config_s PrimaryTriggerConfiguration::getType() const {
-	return engineConfiguration->trigger.type;
+	return engineConfiguration->trigger;
 }
 
 bool PrimaryTriggerConfiguration::isVerboseTriggerSynchDetails() const {
@@ -215,9 +215,9 @@ bool VvtTriggerConfiguration::isUseOnlyRisingEdgeForTrigger() const {
 	return engineConfiguration->vvtCamSensorUseRise;
 }
 
-trigger_type_e VvtTriggerConfiguration::getType() const {
+trigger_config_s VvtTriggerConfiguration::getType() const {
-	// Convert from VVT type to trigger type
+	// Convert from VVT type to trigger_config_s
-	return getVvtTriggerType(engineConfiguration->vvtMode[index]);
+	return { getVvtTriggerType(engineConfiguration->vvtMode[index]), 0, 0 };
 }
 
 bool VvtTriggerConfiguration::isVerboseTriggerSynchDetails() const {

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,
+		initializeSkippedToothTriggerWaveformExt(this, triggerConfig.TriggerType.customTotalToothCount,
-				triggerConfig->customSkippedToothCount, triggerOperationMode);
+				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'

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 TriggerConfiguration& triggerConfiguration
-			const trigger_config_s& triggerConfig
+		);
-			);
 
 	uint16_t findAngleIndex(TriggerFormDetails *details, angle_t angle) const;
 

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 TriggerConfiguration& triggerConfiguration) {
-			const trigger_config_s& triggerConfig) {
+	triggerShapeSynchPointIndex = state.findTriggerZeroEventIndex(*this, triggerConfiguration);
-	triggerShapeSynchPointIndex = state.findTriggerZeroEventIndex(*this,
-			triggerConfiguration, triggerConfig);
 }
 
 /**
@@ -127,9 +125,7 @@ void calculateTriggerSynchPoint(
 	efiAssertVoid(CUSTOM_TRIGGER_STACK, getCurrentRemainingStack() > EXPECTED_REMAINING_STACK, "calc s");
 #endif
 	engine->triggerErrorDetection.clear();
-	shape.initializeSyncPoint(state,
+	shape.initializeSyncPoint(state, engine->primaryTriggerConfiguration);
-			engine->primaryTriggerConfiguration,
-			engineConfiguration->trigger);
 
 	int length = shape.getLength();
 	engine->engineCycleEventCount = length;
@@ -534,7 +530,7 @@ expected<TriggerDecodeResult> 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<TriggerDecodeResult> 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<TriggerDecodeResult> 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<TriggerDecodeResult> 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 TriggerConfiguration& triggerConfiguration) {
-		const trigger_config_s& triggerConfig) {
-	UNUSED(triggerConfig);
 #if EFI_PROD_CODE
 	efiAssert(CUSTOM_ERR_ASSERT, getCurrentRemainingStack() > 128, "findPos", -1);
 #endif

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 TriggerConfiguration& triggerConfiguration
-			const trigger_config_s& triggerConfig
 			);
 
 	bool someSortOfTriggerError() const {

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 */

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,
+	return eth.engine.triggerCentral.triggerState.findTriggerZeroEventIndex(shape, triggerConfiguration);
-			engineConfiguration->trigger);
 }
 
 TEST(trigger, testSkipped2_0) {

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) {
+static auto makeTriggerShape(operation_mode_e mode, const TriggerConfiguration& config) {
-	// Configure a 4-1 trigger wheel
-	trigger_config_s config;
-	config.type = TT_TOOTHED_WHEEL;
-	config.customTotalToothCount = 4;
-	config.customSkippedToothCount = 1;
-
 	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, 4, 1});
-
-	MockTriggerConfiguration cfg(true, TT_TOOTHED_WHEEL);
 	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, 4, 1});
-
-	MockTriggerConfiguration cfg(true, TT_TOOTHED_WHEEL);
 	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, 4, 1});
-
-	MockTriggerConfiguration cfg(true, TT_TOOTHED_WHEEL);
 	cfg.update();
 
+	auto shape = makeTriggerShape(FOUR_STROKE_CAM_SENSOR, cfg);
+
 	efitick_t t = 0;
 
 	StrictMock<MockTriggerDecoder> 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, 4, 1});
-
-	MockTriggerConfiguration cfg(true, TT_TOOTHED_WHEEL);
 	cfg.update();
 
+	auto shape = makeTriggerShape(FOUR_STROKE_CAM_SENSOR, cfg);
+
 	efitick_t t = 0;
 
 	StrictMock<MockTriggerDecoder> dut;