trigger tweaks cleanup (#4278)

* simplify some math

* we say yes to the todo

* and put back that test

* test

firmware/controllers/trigger/trigger_central.cpp

@@ -310,9 +310,9 @@ void hwHandleVvtCamSignal(trigger_value_e front, efitick_t nowNt, int index) {
 		return;
 	}
 
-	angle_t currentPosition = currentPhase.Value;
+	angle_t angleFromPrimarySyncPoint = currentPhase.Value;
-	// convert engine cycle angle into trigger cycle angle
+	// convert trigger cycle angle into engine cycle angle
-	currentPosition -= tdcPosition();
+	angle_t currentPosition = angleFromPrimarySyncPoint - tdcPosition();
 	// https://github.com/rusefi/rusefi/issues/1713 currentPosition could be negative that's expected
 
 #if EFI_UNIT_TEST
@@ -368,7 +368,7 @@ void hwHandleVvtCamSignal(trigger_value_e front, efitick_t nowNt, int index) {
 		break;
 	}
 
-	if (absF(vvtPosition - tdcPosition()) < 7) {
+	if (absF(angleFromPrimarySyncPoint) < 7) {
 		/**
 		 * we prefer not to have VVT sync right at trigger sync so that we do not have phase detection error if things happen a bit in
 		 * wrong order due to belt flex or else

firmware/controllers/trigger/trigger_decoder.cpp

@@ -83,6 +83,7 @@ void TriggerDecoderBase::resetTriggerState() {
 
 void TriggerDecoderBase::setTriggerErrorState() {
 	m_timeSinceDecodeError.reset();
+	totalTriggerErrorCounter++;
 }
 
 void TriggerDecoderBase::resetCurrentCycleState() {
@@ -518,11 +519,8 @@ expected<TriggerDecodeResult> TriggerDecoderBase::decodeTriggerEvent(
 		}
 #endif /* EFI_UNIT_TEST */
 
-		/**
+		// For less important events we simply increment the index.
-		 * For less important events we simply increment the index.
+		nextTriggerEvent();
-		 */
-		nextTriggerEvent()
-		;
 	} else {
 #if !EFI_PROD_CODE
 		if (printTriggerTrace) {
@@ -657,7 +655,6 @@ expected<TriggerDecodeResult> TriggerDecoderBase::decodeTriggerEvent(
 			// so we clear the synchronized flag.
 			if (wasSynchronized && isDecodingError) {
 				setTriggerErrorState();
-				totalTriggerErrorCounter++;
 
 				// Something wrong, no longer synchronized
 				setShaftSynchronized(false);
@@ -670,13 +667,11 @@ expected<TriggerDecodeResult> TriggerDecoderBase::decodeTriggerEvent(
 			}
 
 			// this call would update duty cycle values
-			nextTriggerEvent()
+			nextTriggerEvent();
-			;
 
 			onShaftSynchronization(wasSynchronized, nowNt, triggerShape);
 		} else {	/* if (!isSynchronizationPoint) */
-			nextTriggerEvent()
+			nextTriggerEvent();
-			;
 		}
 
 		for (int i = triggerShape.gapTrackingLength; i > 0; i--) {
@@ -694,8 +689,6 @@ expected<TriggerDecodeResult> TriggerDecoderBase::decodeTriggerEvent(
 		if (Sensor::getOrZero(SensorType::Rpm) != 0) {
 			warning(CUSTOM_SYNC_ERROR, "sync error for %s: index #%d above total size %d", name, currentCycle.current_index, triggerShape.getSize());
 			setTriggerErrorState();
-
-			// TODO: should we increment totalTriggerErrorCounter here too?
 		}
 
 		onTriggerError();

unit_tests/tests/trigger/test_trigger_decoder_2.cpp

@@ -167,6 +167,9 @@ TEST(TriggerDecoder, TooManyTeeth_CausesError) {
 	EXPECT_TRUE(dut.getShaftSynchronized());
 	EXPECT_EQ(0, dut.currentCycle.current_index);
 
+	// Fake that we have RPM so that all trigger error detection is enabled
+	Sensor::setMockValue(SensorType::Rpm, 1000);
+
 	t += MS2NT(1);
 	doTooth(dut, shape, cfg, t);
 	EXPECT_TRUE(dut.getShaftSynchronized());
@@ -176,12 +179,16 @@ TEST(TriggerDecoder, TooManyTeeth_CausesError) {
 	doTooth(dut, shape, cfg, t);
 	EXPECT_TRUE(dut.getShaftSynchronized());
 	EXPECT_EQ(4, dut.currentCycle.current_index);
+	EXPECT_EQ(0, dut.totalTriggerErrorCounter);
 
 	// This tooth is extra - expect a call to onTriggerError() and loss of sync!
 	t += MS2NT(1);
 	doTooth(dut, shape, cfg, t);
 	EXPECT_FALSE(dut.getShaftSynchronized());
 	EXPECT_EQ(6, dut.currentCycle.current_index);
+	EXPECT_EQ(1, dut.totalTriggerErrorCounter);
+
+	Sensor::resetAllMocks();
 }
 
 TEST(TriggerDecoder, NotEnoughTeeth_CausesError) {
@@ -220,6 +227,7 @@ TEST(TriggerDecoder, NotEnoughTeeth_CausesError) {
 	EXPECT_TRUE(dut.getShaftSynchronized());
 	EXPECT_EQ(2, dut.currentCycle.current_index);
 	EXPECT_FALSE(dut.someSortOfTriggerError());
+	EXPECT_EQ(0, dut.totalTriggerErrorCounter);
 
 	// Missing tooth, but it comes early - not enough teeth have happened yet!
 	t += MS2NT(2);
@@ -228,6 +236,7 @@ TEST(TriggerDecoder, NotEnoughTeeth_CausesError) {
 	// Sync is lost until we get to another sync point
 	EXPECT_FALSE(dut.getShaftSynchronized());
 	EXPECT_EQ(0, dut.currentCycle.current_index);
+	EXPECT_EQ(1, dut.totalTriggerErrorCounter);
 	EXPECT_TRUE(dut.someSortOfTriggerError());
 }
 

unit_tests/tests/trigger/test_trigger_noiseless.cpp

@@ -152,7 +152,10 @@ static void testNoiselessDecoderProcedure(EngineTestHelper &eth, int errorTolera
 	// add noise7 - 34 short spikes across the entire signal pulse
 	fireNoisyCycle60_2(&eth, 2, 1000, 2, 10, 10, failProofNumSpikes + 1);
 
-	ASSERT_EQ(errorToleranceCnt,  engine->triggerCentral.triggerState.totalTriggerErrorCounter);
+	// alas, this is a hard case even for noiseless decoder, and it fails...
+	// but still we're close to 33% signal-noise ratio threshold - not bad!
+	// so here's an error anyway!
+	ASSERT_EQ( 1,  engine->triggerCentral.triggerState.totalTriggerErrorCounter) << "testNoiselessDecoder noise#7_fail_test";
 }
 
 TEST(trigger, noiselessDecoder) {