more granular trigger error messages (#4526)

* granular trigger error messages

* adjust test expectations

* explicitly test behavior

firmware/controllers/algo/engine.cpp

@@ -383,26 +383,6 @@ void Engine::preCalculate() {
 }
 
 #if EFI_SHAFT_POSITION_INPUT
-void Engine::OnTriggerStateDecodingError() {
-	warning(CUSTOM_SYNC_COUNT_MISMATCH, "trigger not happy current %d/%d expected %d/%d",
-			triggerCentral.triggerState.currentCycle.eventCount[0],
-			triggerCentral.triggerState.currentCycle.eventCount[1],
-			TRIGGER_WAVEFORM(getExpectedEventCount(0)),
-			TRIGGER_WAVEFORM(getExpectedEventCount(1)));
-
-	if (engineConfiguration->verboseTriggerSynchDetails || (triggerCentral.triggerState.someSortOfTriggerError() && !engineConfiguration->silentTriggerError)) {
-#if EFI_PROD_CODE
-		efiPrintf("error: synchronizationPoint @ index %d expected %d/%d got %d/%d",
-				triggerCentral.triggerState.currentCycle.current_index,
-				TRIGGER_WAVEFORM(getExpectedEventCount(0)),
-				TRIGGER_WAVEFORM(getExpectedEventCount(1)),
-				triggerCentral.triggerState.currentCycle.eventCount[0],
-				triggerCentral.triggerState.currentCycle.eventCount[1]);
-#endif /* EFI_PROD_CODE */
-	}
-
-}
-
 void Engine::OnTriggerStateProperState(efitick_t nowNt) {
 	rpmCalculator.setSpinningUp(nowNt);
 }
@@ -431,18 +411,19 @@ void Engine::OnTriggerSyncronization(bool wasSynchronized, bool isDecodingError)
 		// 'triggerStateListener is not null' means we are running a real engine and now just preparing trigger shape
 		// that's a bit of a hack, a sweet OOP solution would be a real callback or at least 'needDecodingErrorLogic' method?
 		if (isDecodingError) {
-			OnTriggerStateDecodingError();
+#if EFI_PROD_CODE
+			if (engineConfiguration->verboseTriggerSynchDetails || (triggerCentral.triggerState.someSortOfTriggerError() && !engineConfiguration->silentTriggerError)) {
+				efiPrintf("error: synchronizationPoint @ index %d expected %d/%d got %d/%d",
+						triggerCentral.triggerState.currentCycle.current_index,
+						TRIGGER_WAVEFORM(getExpectedEventCount(0)),
+						TRIGGER_WAVEFORM(getExpectedEventCount(1)),
+						triggerCentral.triggerState.currentCycle.eventCount[0],
+						triggerCentral.triggerState.currentCycle.eventCount[1]);
+			}
+#endif /* EFI_PROD_CODE */
 		}
 
 		engine->triggerErrorDetection.add(isDecodingError);
-
-		if (triggerCentral.isTriggerDecoderError()) {
-			warning(CUSTOM_OBD_TRG_DECODING, "trigger decoding issue. expected %d/%d got %d/%d",
-					TRIGGER_WAVEFORM(getExpectedEventCount(0)),
-					TRIGGER_WAVEFORM(getExpectedEventCount(1)),
-					triggerCentral.triggerState.currentCycle.eventCount[0],
-					triggerCentral.triggerState.currentCycle.eventCount[1]);
-		}
 	}
 
 }

firmware/controllers/algo/engine.h

@@ -239,7 +239,6 @@ public:
 	efitick_t startStopStateLastPushTime = 0;
 
 #if EFI_SHAFT_POSITION_INPUT
-	void OnTriggerStateDecodingError();
 	void OnTriggerStateProperState(efitick_t nowNt) override;
 	void OnTriggerSyncronization(bool wasSynchronized, bool isDecodingError) override;
 	void OnTriggerSynchronizationLost() override;

firmware/controllers/algo/obd_error_codes.h

@@ -1751,9 +1751,6 @@ typedef enum {
 	CUSTOM_ZERO_DWELL = 6032,
 	CUSTOM_DWELL_TOO_LONG = 6033,
 	CUSTOM_SKIPPING_STROKE = 6034,
-	CUSTOM_OBD_TRG_DECODING = 6035,
-	// todo: looks like following two errors always happen together, it's just timing affects which one is published?
-	CUSTOM_SYNC_ERROR = 6036,
 	CUSTOM_6037 = 6037,
 	/**
 	 * This error happens if some pinout configuration changes were applied but ECU was not reset afterwards.
@@ -2140,7 +2137,13 @@ typedef enum {
 
 	CUSTOM_ERR_TRIGGER_SYNC = 9000,
 	CUSTOM_OBD_TRIGGER_WAVEFORM = 9001,
-	CUSTOM_SYNC_COUNT_MISMATCH = 9002,
+
+	CUSTOM_PRIMARY_TOO_MANY_TEETH = 9002,
+	CUSTOM_PRIMARY_NOT_ENOUGH_TEETH = 9003,
+
+	CUSTOM_CAM_TOO_MANY_TEETH = 9004,
+	CUSTOM_CAM_NOT_ENOUGH_TEETH = 9005,
+
 	/**
 	 * This is not engine miss detection - this is only internal scheduler state validation
 	 * Should not happen

firmware/controllers/trigger/trigger_decoder.cpp

@@ -407,6 +407,30 @@ void PrimaryTriggerDecoder::onTriggerError() {
 	resetHasFullSync();
 }
 
+void PrimaryTriggerDecoder::onNotEnoughTeeth(int /*actual*/, int /*expected*/) {
+	warning(CUSTOM_PRIMARY_NOT_ENOUGH_TEETH, "primary trigger error: not enough teeth between sync points: expected %d/%d got %d/%d",
+		TRIGGER_WAVEFORM(getExpectedEventCount(0)),
+		TRIGGER_WAVEFORM(getExpectedEventCount(1)),
+		currentCycle.eventCount[0],
+		currentCycle.eventCount[1]);
+}
+
+void PrimaryTriggerDecoder::onTooManyTeeth(int /*actual*/, int /*expected*/) {
+	warning(CUSTOM_PRIMARY_TOO_MANY_TEETH, "primary trigger error: too many teeth between sync points: expected %d/%d got %d/%d",
+		TRIGGER_WAVEFORM(getExpectedEventCount(0)),
+		TRIGGER_WAVEFORM(getExpectedEventCount(1)),
+		currentCycle.eventCount[0],
+		currentCycle.eventCount[1]);
+}
+
+void VvtTriggerDecoder::onNotEnoughTeeth(int actual, int expected) {
+	warning(CUSTOM_CAM_NOT_ENOUGH_TEETH, "cam %s trigger error: not enough teeth between sync points: actual %d expected %d", name, actual, expected);
+}
+
+void VvtTriggerDecoder::onTooManyTeeth(int actual, int expected) {
+	warning(CUSTOM_CAM_TOO_MANY_TEETH, "cam %s trigger error: too many teeth between sync points: %d > %d", name, actual, expected);
+}
+
 bool TriggerDecoderBase::validateEventCounters(const TriggerWaveform& triggerShape) const {
 	// We can check if things are fine by comparing the number of events in a cycle with the expected number of event.
 	bool isDecodingError = false;
@@ -661,6 +685,7 @@ expected<TriggerDecodeResult> TriggerDecoderBase::decodeTriggerEvent(
 			// so we clear the synchronized flag.
 			if (wasSynchronized && isDecodingError) {
 				setTriggerErrorState();
+				onNotEnoughTeeth(currentCycle.current_index, triggerShape.getSize());
 
 				// Something wrong, no longer synchronized
 				setShaftSynchronized(false);
@@ -693,8 +718,8 @@ expected<TriggerDecodeResult> TriggerDecoderBase::decodeTriggerEvent(
 
 		// let's not show a warning if we are just starting to spin
 		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();
+			onTooManyTeeth(currentCycle.current_index, triggerShape.getSize());
 		}
 
 		onTriggerError();

firmware/controllers/trigger/trigger_decoder.h

@@ -143,6 +143,9 @@ protected:
 	//  - Saw a sync point but the wrong number of events in the cycle
 	virtual void onTriggerError() { }
 
+	virtual void onNotEnoughTeeth(int actual, int expected) { }
+	virtual void onTooManyTeeth(int actual, int expected) { }
+
 private:
 	void resetCurrentCycleState();
 	bool isSyncPoint(const TriggerWaveform& triggerShape, trigger_type_e triggerType) const;
@@ -224,6 +227,9 @@ public:
 
 	void onTriggerError() override;
 
+	void onNotEnoughTeeth(int actual, int expected) override;
+	void onTooManyTeeth(int actual, int expected) override;
+
 private:
 	float calculateInstantRpm(
 		TriggerWaveform const & triggerShape, TriggerFormDetails *triggerFormDetails,
@@ -238,6 +244,9 @@ private:
 class VvtTriggerDecoder : public TriggerDecoderBase {
 public:
 	VvtTriggerDecoder(const char* name) : TriggerDecoderBase(name) { }
+
+	void onNotEnoughTeeth(int actual, int expected) override;
+	void onTooManyTeeth(int actual, int expected) override;
 };
 
 angle_t getEngineCycle(operation_mode_e operationMode);

unit_tests/tests/trigger/test_cam_vvt_input.cpp

@@ -54,7 +54,7 @@ TEST(trigger, testNoStartUpWarnings) {
 		eth.fireFall(150);
 	}
 	EXPECT_EQ( 1,  unitTestWarningCodeState.recentWarnings.getCount()) << "warningCounter#testNoStartUpWarnings CUSTOM_SYNC_COUNT_MISMATCH expected";
-	EXPECT_EQ(CUSTOM_SYNC_ERROR, unitTestWarningCodeState.recentWarnings.get(0).Code);
+	EXPECT_EQ(CUSTOM_PRIMARY_TOO_MANY_TEETH, unitTestWarningCodeState.recentWarnings.get(0).Code);
 }
 
 TEST(trigger, testNoisyInput) {
@@ -74,7 +74,7 @@ TEST(trigger, testNoisyInput) {
 	ASSERT_EQ(NOISY_RPM,  Sensor::getOrZero(SensorType::Rpm)) << "testNoisyInput RPM should be noisy";
 
 	ASSERT_EQ( 2,  unitTestWarningCodeState.recentWarnings.getCount()) << "warningCounter#testNoisyInput";
-	ASSERT_EQ(CUSTOM_SYNC_COUNT_MISMATCH, unitTestWarningCodeState.recentWarnings.get(0).Code) << "@0";
+	ASSERT_EQ(CUSTOM_PRIMARY_NOT_ENOUGH_TEETH, unitTestWarningCodeState.recentWarnings.get(0).Code) << "@0";
 	ASSERT_EQ(OBD_Crankshaft_Position_Sensor_A_Circuit_Malfunction, unitTestWarningCodeState.recentWarnings.get(1).Code) << "@0";
 }
 

unit_tests/tests/trigger/test_real_cranking_nissan_vq40.cpp

@@ -36,5 +36,5 @@ TEST(realCrankingVQ40, normalCranking) {
 	// TODO: why warnings?
 	ASSERT_EQ(2, eth.recentWarnings()->getCount());
 	ASSERT_EQ(CUSTOM_OUT_OF_ORDER_COIL, eth.recentWarnings()->get(0).Code);	// this is from a coil being protected by overdwell protection
-	ASSERT_EQ(CUSTOM_SYNC_ERROR, eth.recentWarnings()->get(1).Code);
+	ASSERT_EQ(CUSTOM_PRIMARY_TOO_MANY_TEETH, eth.recentWarnings()->get(1).Code);
 }

unit_tests/tests/trigger/test_real_nb2_cranking.cpp

@@ -29,10 +29,10 @@ TEST(realCrankingNB2, normalCranking) {
 
 	ASSERT_EQ(876, round(Sensor::getOrZero(SensorType::Rpm)));
 
-	ASSERT_EQ(3, eth.recentWarnings()->getCount());
+	EXPECT_EQ(3, eth.recentWarnings()->getCount());
-	ASSERT_EQ(CUSTOM_OUT_OF_ORDER_COIL, eth.recentWarnings()->get(0).Code);
+	EXPECT_EQ(CUSTOM_OUT_OF_ORDER_COIL, eth.recentWarnings()->get(0).Code);
-	ASSERT_EQ(CUSTOM_SYNC_ERROR, eth.recentWarnings()->get(1).Code);
+	EXPECT_EQ(CUSTOM_CAM_TOO_MANY_TEETH, eth.recentWarnings()->get(1).Code);
-	ASSERT_EQ(CUSTOM_SYNC_COUNT_MISMATCH, eth.recentWarnings()->get(2).Code);
+	EXPECT_EQ(CUSTOM_PRIMARY_NOT_ENOUGH_TEETH, eth.recentWarnings()->get(2).Code);
 }
 
 TEST(realCrankingNB2, crankingMissingInjector) {
@@ -52,8 +52,9 @@ TEST(realCrankingNB2, crankingMissingInjector) {
 
 	ASSERT_EQ(316, round(Sensor::getOrZero(SensorType::Rpm)));
 
-	ASSERT_EQ(3, eth.recentWarnings()->getCount());
+	EXPECT_EQ(4, eth.recentWarnings()->getCount());
-	ASSERT_EQ(CUSTOM_OUT_OF_ORDER_COIL, eth.recentWarnings()->get(0).Code);
+	EXPECT_EQ(CUSTOM_OUT_OF_ORDER_COIL, eth.recentWarnings()->get(0).Code);
-	ASSERT_EQ(CUSTOM_SYNC_ERROR, eth.recentWarnings()->get(1).Code);
+	EXPECT_EQ(CUSTOM_CAM_TOO_MANY_TEETH, eth.recentWarnings()->get(1).Code);
-	ASSERT_EQ(CUSTOM_SYNC_COUNT_MISMATCH, eth.recentWarnings()->get(2).Code);
+	EXPECT_EQ(CUSTOM_PRIMARY_NOT_ENOUGH_TEETH, eth.recentWarnings()->get(2).Code);
+	EXPECT_EQ(CUSTOM_PRIMARY_TOO_MANY_TEETH, eth.recentWarnings()->get(3).Code);
 }

unit_tests/tests/trigger/test_trigger_decoder_2.cpp

@@ -1,5 +1,6 @@
 #include "pch.h"
 
+using ::testing::_;
 using ::testing::StrictMock;
 
 class MockTriggerConfiguration : public TriggerConfiguration {
@@ -32,6 +33,8 @@ struct MockTriggerDecoder : public TriggerDecoderBase {
 	MockTriggerDecoder() : TriggerDecoderBase("mock") { }
 
 	MOCK_METHOD(void, onTriggerError, (), (override));
+	MOCK_METHOD(void, onNotEnoughTeeth, (int actual, int expected), (override));
+	MOCK_METHOD(void, onTooManyTeeth, (int actual, int expected), (override));
 };
 
 static auto makeTriggerShape(operation_mode_e mode, const TriggerConfiguration& config) {
@@ -147,6 +150,7 @@ TEST(TriggerDecoder, TooManyTeeth_CausesError) {
 	StrictMock<MockTriggerDecoder> dut;
 	// We expect one call to onTriggerError().
 	EXPECT_CALL(dut, onTriggerError());
+	EXPECT_CALL(dut, onTooManyTeeth(_, _));
 
 	// Fire a few boring evenly spaced teeth
 	t += MS2NT(1);
@@ -188,6 +192,28 @@ TEST(TriggerDecoder, TooManyTeeth_CausesError) {
 	EXPECT_EQ(6, dut.currentCycle.current_index);
 	EXPECT_EQ(1, dut.totalTriggerErrorCounter);
 
+	// Fire some normal revolutions to ensure we recover without additional error types.
+	for (size_t i = 0; i < 10; i++) {
+		// Missing tooth, 2x normal length!
+		t += MS2NT(2);
+		doTooth(dut, shape, cfg, t);
+		EXPECT_TRUE(dut.getShaftSynchronized());
+		EXPECT_EQ(0, dut.currentCycle.current_index);
+
+		// normal tooth
+		t += MS2NT(1);
+		doTooth(dut, shape, cfg, t);
+		EXPECT_EQ(2, dut.currentCycle.current_index);
+
+		// normal tooth
+		t += MS2NT(1);
+		doTooth(dut, shape, cfg, t);
+		EXPECT_EQ(4, dut.currentCycle.current_index);
+	}
+
+	// Should now have sync again
+	EXPECT_TRUE(dut.getShaftSynchronized());
+
 	Sensor::resetAllMocks();
 }
 
@@ -202,6 +228,7 @@ TEST(TriggerDecoder, NotEnoughTeeth_CausesError) {
 	StrictMock<MockTriggerDecoder> dut;
 	// We expect one call to onTriggerError().
 	EXPECT_CALL(dut, onTriggerError());
+	EXPECT_CALL(dut, onNotEnoughTeeth(_, _));
 
 	// Fire a few boring evenly spaced teeth
 	t += MS2NT(1);
@@ -222,6 +249,9 @@ TEST(TriggerDecoder, NotEnoughTeeth_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());
@@ -238,6 +268,30 @@ TEST(TriggerDecoder, NotEnoughTeeth_CausesError) {
 	EXPECT_EQ(0, dut.currentCycle.current_index);
 	EXPECT_EQ(1, dut.totalTriggerErrorCounter);
 	EXPECT_TRUE(dut.someSortOfTriggerError());
+
+	// Fire some normal revolutions to ensure we recover without additional error types.
+	for (size_t i = 0; i < 10; i++) {
+		// normal tooth
+		t += MS2NT(1);
+		doTooth(dut, shape, cfg, t);
+		EXPECT_EQ(2, dut.currentCycle.current_index);
+
+		// normal tooth
+		t += MS2NT(1);
+		doTooth(dut, shape, cfg, t);
+		EXPECT_EQ(4, dut.currentCycle.current_index);
+
+		// Missing tooth, 2x normal length!
+		t += MS2NT(2);
+		doTooth(dut, shape, cfg, t);
+		EXPECT_TRUE(dut.getShaftSynchronized());
+		EXPECT_EQ(0, dut.currentCycle.current_index);
+	}
+
+	// Should now have sync again
+	EXPECT_TRUE(dut.getShaftSynchronized());
+
+	Sensor::resetAllMocks();
 }
 
 TEST(TriggerDecoder, PrimaryDecoderNoDisambiguation) {