cam/crank sync improvements (#4152)

* clarify getOperationMode

* wait for phase synchronize to transition to sequential mode

* one test

* encapsulate vvtSyncCounter

* test

* Add option to stop VVT sync above some RPM

* getOrZero

* Revert "synchronized Phase handling improvements fix #4099"

This reverts commit 32d46d1f09.

* Revert "m_hasSynchronizedSymmetrical handling improvements #4099"

This reverts commit d5e131b1d0.

* remove synchronizedPhase

* only crank needs this

* changelog

* needsDisambiguation()

* test

* s

* bad merge

* put the timer back

* s

* s

* changelog

* test for new behavior

* correct parameter order

* spelling

* s

* s

* s

* tests merge happy

* bad merge

Co-authored-by: Matthew Kennedy <makenne@microsoft.com>

firmware/CHANGELOG.md

@@ -26,6 +26,7 @@ Release template (copy/paste this for new release):
 ### Added
 
 ### Fixed
+ - Improved logic used to disambiguate trigger sync using cam/VVT information. Engine now runs in wasted spark until cam sync is achieved, at which point it switches to fully sequential. #4099
 
 ## June 2022 Release - "Day 98"
 

firmware/controllers/algo/engine.cpp

@@ -170,6 +170,9 @@ void Engine::updateTriggerWaveform() {
 		}
 	}
 
+	// This is not the right place for this, but further refactoring has to happen before it can get moved.
+	engine->triggerCentral.triggerState.setNeedsDisambiguation(engine->triggerCentral.triggerShape.needsDisambiguation());
+
 	if (!TRIGGER_WAVEFORM(shapeDefinitionError)) {
 		prepareOutputSignals();
 	}

firmware/controllers/math/engine_math.cpp

@@ -410,9 +410,15 @@ void prepareIgnitionPinIndices(ignition_mode_e ignitionMode) {
 ignition_mode_e getCurrentIgnitionMode() {
 	ignition_mode_e ignitionMode = engineConfiguration->ignitionMode;
 #if EFI_SHAFT_POSITION_INPUT
-	// In spin-up cranking mode we don't have full phase sync. info yet, so wasted spark mode is better
-	if (ignitionMode == IM_INDIVIDUAL_COILS && engine->rpmCalculator.isSpinningUp())
-		ignitionMode = IM_WASTED_SPARK;
+	// In spin-up cranking mode we don't have full phase sync info yet, so wasted spark mode is better
+	if (ignitionMode == IM_INDIVIDUAL_COILS) {
+		bool missingPhaseInfoForSequential = 
+			!engine->triggerCentral.triggerState.hasSynchronizedPhase();
+
+		if (engine->rpmCalculator.isSpinningUp() || missingPhaseInfoForSequential) {
+			ignitionMode = IM_WASTED_SPARK;
+		}
+	}
 #endif /* EFI_SHAFT_POSITION_INPUT */
 	return ignitionMode;
 }

firmware/controllers/trigger/decoders/trigger_structure.cpp

@@ -117,6 +117,22 @@ angle_t TriggerWaveform::getCycleDuration() const {
 	}
 }
 
+bool TriggerWaveform::needsDisambiguation() const {
+	switch (getOperationMode()) {
+		case FOUR_STROKE_CRANK_SENSOR:
+		case FOUR_STROKE_SYMMETRICAL_CRANK_SENSOR:
+		case FOUR_STROKE_THREE_TIMES_CRANK_SENSOR:
+		case FOUR_STROKE_TWELVE_TIMES_CRANK_SENSOR:
+			return true;
+		case FOUR_STROKE_CAM_SENSOR:
+		case TWO_STROKE:
+			return false;
+		default:
+			firmwareError(OBD_PCM_Processor_Fault, "bad operationMode() in needsDisambiguation");
+			return true;
+	}
+}
+
 /**
  * Trigger event count equals engine cycle event count if we have a cam sensor.
  * Two trigger cycles make one engine cycle in case of a four stroke engine If we only have a cranksensor.

firmware/controllers/trigger/decoders/trigger_structure.h

@@ -248,6 +248,9 @@ public:
 
 	angle_t getCycleDuration() const;
 
+	// Returns true if this trigger alone can fully sync the current engine for sequential mode.
+	bool needsDisambiguation() const;
+
 	/**
 	 * index of synchronization event within TriggerWaveform
 	 * See findTriggerZeroEventIndex()

firmware/controllers/trigger/trigger_decoder.cpp

@@ -216,7 +216,7 @@ void PrimaryTriggerDecoder::resetTriggerState() {
 	prevInstantRpmValue = 0;
 	m_instantRpm = 0;
 
-	m_hasSynchronizedPhase = false;
+	resetHasFullSync();
 }
 
 void PrimaryTriggerDecoder::movePreSynchTimestamps() {
@@ -401,7 +401,7 @@ void TriggerDecoderBase::incrementTotalEventCounter() {
 
 void PrimaryTriggerDecoder::onTriggerError() {
 	// On trigger error, we've lost full sync
-	m_hasSynchronizedPhase = false;
+	resetHasFullSync();
 }
 
 bool TriggerDecoderBase::validateEventCounters(const TriggerWaveform& triggerShape) const {

firmware/controllers/trigger/trigger_decoder.h

@@ -168,6 +168,11 @@ public:
 	PrimaryTriggerDecoder(const char* name);
 	void resetTriggerState() override;
 
+	void resetHasFullSync() {
+		// If this trigger doesn't need disambiguation, we already have phase sync
+		m_hasSynchronizedPhase = !m_needsDisambiguation;
+	}
+
 	angle_t syncEnginePhase(int divider, int remainder, angle_t engineCycle);
 
 	float getInstantRpm() const {
@@ -210,6 +215,12 @@ public:
 		return m_hasSynchronizedPhase;
 	}
 
+	void setNeedsDisambiguation(bool needsDisambiguation) {
+		m_needsDisambiguation = needsDisambiguation;
+
+		resetHasFullSync();
+	}
+
 	void onTriggerError() override;
 
 private:
@@ -220,6 +231,7 @@ private:
 	float m_instantRpm = 0;
 	float m_instantRpmRatio = 0;
 
+	bool m_needsDisambiguation = false;
 	bool m_hasSynchronizedPhase = false;
 };
 

firmware/tunerstudio/rusefi.input

@@ -1997,7 +1997,7 @@ cmd_set_engine_type_default					= "@@TS_IO_TEST_COMMAND_char@@\x00\x31\x00\x00"
 		field = "VVT offset bank 1 exhaust",			vvtOffsets2, { camInputs2 != 0 }
 		field = "VVT offset bank 2 intake",				vvtOffsets3, { camInputs3 != 0 }
 		field = "VVT offset bank 2 exhaust",			vvtOffsets4, { camInputs4 != 0 }
-		field = "Require CAM/VVT sync for ignition",	isPhaseSyncRequiredForIgnition
+		field = "Require cam/VVT sync for ignition",	isPhaseSyncRequiredForIgnition
 		field = "Maximum cam/VVT sync RPM",				maxCamPhaseResolveRpm
 	    field = "Print verbose VVT sync details to console",verboseVVTDecoding
 	    field = "Print verbose trigger sync to console",   verboseTriggerSynchDetails

unit_tests/tests/trigger/test_fasterEngineSpinningUp.cpp

@@ -19,8 +19,10 @@ TEST(cranking, testFasterEngineSpinningUp) {
 	engineConfiguration->cranking.rpm = 999;
 	// set sequential injection mode to test auto-change to simultaneous when spinning-up
 	setupSimpleTestEngineWithMafAndTT_ONE_trigger(&eth, IM_SEQUENTIAL);
+	// Lie that this trigger requires disambiguation
+	engine->triggerCentral.triggerState.setNeedsDisambiguation(true);
 
-	ASSERT_EQ(IM_INDIVIDUAL_COILS, getCurrentIgnitionMode());
+	ASSERT_EQ(IM_WASTED_SPARK, getCurrentIgnitionMode());
 
 	eth.fireRise(1000 /*ms*/);
 
@@ -67,14 +69,18 @@ TEST(cranking, testFasterEngineSpinningUp) {
 	ASSERT_EQ( 200,  round(Sensor::getOrZero(SensorType::Rpm))) << "RPM#2";
 	// test if they are simultaneous in cranking mode too
 	ASSERT_EQ(IM_SIMULTANEOUS, engine->getCurrentInjectionMode());
-	// test if ignition mode is restored to ind.coils
-	ASSERT_EQ(IM_INDIVIDUAL_COILS, getCurrentIgnitionMode());
+	// Should still be in wasted spark since we don't have cam sync yet
+	ASSERT_EQ(IM_WASTED_SPARK, getCurrentIgnitionMode());
 	// two simultaneous injections
 	ASSERT_EQ( 4,  engine->executor.size()) << "plain#2";
 	// check real events
 	eth.assertEvent5("inj start#2", 0, (void*)startSimultaniousInjection, 148375);
 	eth.assertEvent5("inj end#2", 1, (void*)endSimultaniousInjection, 149999);
 
+	// Now perform a fake VVT sync and check that ignition mode changes to sequential
+	engine->triggerCentral.triggerState.syncEnginePhase(1, 0, 720);
+	ASSERT_EQ(IM_SEQUENTIAL, getCurrentIgnitionMode());
+
 	// skip, clear & advance 1 more revolution at higher RPM
 	eth.fireFall(60);
 

unit_tests/tests/trigger/test_trigger_decoder.cpp

@@ -1067,6 +1067,8 @@ TEST(big, testSparkReverseOrderBug319) {
 
 	setConstantDwell(45);
 
+	engine->triggerCentral.triggerState.syncEnginePhase(1, 0, 720);
+
 	// this is needed to update injectorLag
 	engine->updateSlowSensors();
 
@@ -1078,6 +1080,8 @@ TEST(big, testSparkReverseOrderBug319) {
 	eth.fireRise(20);
 	eth.fireFall(20);
 
+	engine->triggerCentral.triggerState.syncEnginePhase(1, 0, 720);
+
 	eth.executeActions();
 
 	eth.fireRise(20);

unit_tests/tests/trigger/test_trigger_decoder_2.cpp

@@ -110,6 +110,7 @@ TEST(TriggerDecoder, FindsSyncPointMultipleRevolutions) {
 	doTooth(dut, shape, cfg, t);
 	EXPECT_TRUE(dut.getShaftSynchronized());
 	EXPECT_EQ(0, dut.currentCycle.current_index);
+	EXPECT_EQ(0, dut.getTotalRevolutionCounter());
 
 	// Do 100 turns and make sure we stay synchronized
 	for (int i = 0; i < 100; i++) {
@@ -129,6 +130,9 @@ TEST(TriggerDecoder, FindsSyncPointMultipleRevolutions) {
 		EXPECT_TRUE(dut.getShaftSynchronized());
 		EXPECT_EQ(0, dut.currentCycle.current_index);
 		EXPECT_FALSE(dut.someSortOfTriggerError());
+
+		// We do one revolution per loop iteration
+		EXPECT_EQ(i + 1, dut.getTotalRevolutionCounter());
 	}
 }
 
@@ -226,3 +230,76 @@ TEST(TriggerDecoder, NotEnoughTeeth_CausesError) {
 	EXPECT_EQ(0, dut.currentCycle.current_index);
 	EXPECT_TRUE(dut.someSortOfTriggerError());
 }
+
+TEST(TriggerDecoder, PrimaryDecoderNoDisambiguation) {
+	MockTriggerConfiguration cfg(true, {TT_TOOTHED_WHEEL, 4, 1});
+	cfg.update();
+
+	auto shape = makeTriggerShape(FOUR_STROKE_CAM_SENSOR, cfg);
+
+	efitick_t t = 0;
+
+	PrimaryTriggerDecoder dut("test");
+
+	// This is not the right place for this, but further refactoring has to happen before it can get moved.
+	dut.setNeedsDisambiguation(shape.needsDisambiguation());
+
+	// Fire a few boring evenly spaced teeth
+	t += MS2NT(1);
+	doTooth(dut, shape, cfg, t);
+	t += MS2NT(1);
+	doTooth(dut, shape, cfg, t);
+	t += MS2NT(1);
+	doTooth(dut, shape, cfg, t);
+
+	// Missing tooth, 2x normal length!
+	t += MS2NT(2);
+	doTooth(dut, shape, cfg, t);
+
+	// Is synchronized
+	EXPECT_TRUE(dut.getShaftSynchronized());
+	// Has "full" sync, doesn't need cam information to sync
+	EXPECT_TRUE(dut.hasSynchronizedPhase());
+}
+
+TEST(TriggerDecoder, PrimaryDecoderNeedsDisambiguation) {
+	MockTriggerConfiguration cfg(true, {TT_TOOTHED_WHEEL, 4, 1});
+	cfg.update();
+
+	auto shape = makeTriggerShape(FOUR_STROKE_CRANK_SENSOR, cfg);
+
+	efitick_t t = 0;
+
+	PrimaryTriggerDecoder dut("test");
+
+	// This is not the right place for this, but further refactoring has to happen before it can get moved.
+	dut.setNeedsDisambiguation(shape.needsDisambiguation());
+
+	// Fire a few boring evenly spaced teeth
+	t += MS2NT(1);
+	doTooth(dut, shape, cfg, t);
+	t += MS2NT(1);
+	doTooth(dut, shape, cfg, t);
+	t += MS2NT(1);
+	doTooth(dut, shape, cfg, t);
+
+	// Missing tooth, 2x normal length!
+	t += MS2NT(2);
+	doTooth(dut, shape, cfg, t);
+
+	// Is synchronized
+	EXPECT_TRUE(dut.getShaftSynchronized());
+	// Does not have full sync, this trigger is ambiguous
+	EXPECT_FALSE(dut.hasSynchronizedPhase());
+
+	// Provide cam assist information to the primary trigger
+	dut.syncEnginePhase(2, 0, 720);
+
+	// We now have full sync!
+	EXPECT_TRUE(dut.hasSynchronizedPhase());
+
+	// If there's a trigger error, we lose full sync
+	// Tests above ensure onTriggerError() is called properly
+	dut.onTriggerError();
+	EXPECT_FALSE(dut.hasSynchronizedPhase());
+}