From d5f1235a43c01bb2d73acbf437b37e1bb8f783e3 Mon Sep 17 00:00:00 2001 From: Matthew Kennedy Date: Thu, 23 Jun 2022 20:11:29 -0700 Subject: [PATCH] trigger tweaks cleanup (#4278) * simplify some math * we say yes to the todo * and put back that test * test --- .../controllers/trigger/trigger_central.cpp | 10 +++++----- .../controllers/trigger/trigger_decoder.cpp | 17 +++++------------ .../tests/trigger/test_trigger_decoder_2.cpp | 9 +++++++++ .../tests/trigger/test_trigger_noiseless.cpp | 5 ++++- 4 files changed, 23 insertions(+), 18 deletions(-) diff --git a/firmware/controllers/trigger/trigger_central.cpp b/firmware/controllers/trigger/trigger_central.cpp index ec6b200eb4..cf8df0c69f 100644 --- a/firmware/controllers/trigger/trigger_central.cpp +++ b/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; - // convert engine cycle angle into trigger cycle angle - currentPosition -= tdcPosition(); + angle_t angleFromPrimarySyncPoint = currentPhase.Value; + // convert trigger cycle angle into engine cycle angle + angle_t currentPosition = angleFromPrimarySyncPoint - tdcPosition(); // https://github.com/rusefi/rusefi/issues/1713 currentPosition could be negative that's expected #if EFI_UNIT_TEST @@ -366,9 +366,9 @@ void hwHandleVvtCamSignal(trigger_value_e front, efitick_t nowNt, int index) { default: // else, do nothing 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 diff --git a/firmware/controllers/trigger/trigger_decoder.cpp b/firmware/controllers/trigger/trigger_decoder.cpp index 455de2019d..cd392c5c39 100644 --- a/firmware/controllers/trigger/trigger_decoder.cpp +++ b/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 TriggerDecoderBase::decodeTriggerEvent( } #endif /* EFI_UNIT_TEST */ - /** - * For less important events we simply increment the index. - */ - nextTriggerEvent() - ; + // For less important events we simply increment the index. + nextTriggerEvent(); } else { #if !EFI_PROD_CODE if (printTriggerTrace) { @@ -657,7 +655,6 @@ expected 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 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 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(); diff --git a/unit_tests/tests/trigger/test_trigger_decoder_2.cpp b/unit_tests/tests/trigger/test_trigger_decoder_2.cpp index e93d4abf1f..dc31c9699c 100644 --- a/unit_tests/tests/trigger/test_trigger_decoder_2.cpp +++ b/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()); } diff --git a/unit_tests/tests/trigger/test_trigger_noiseless.cpp b/unit_tests/tests/trigger/test_trigger_noiseless.cpp index 75cc2e7231..0897b34872 100644 --- a/unit_tests/tests/trigger/test_trigger_noiseless.cpp +++ b/unit_tests/tests/trigger/test_trigger_noiseless.cpp @@ -152,7 +152,10 @@ static void testNoiselessDecoderProcedure(EngineTestHelper ð, int errorTolera // add noise7 - 34 short spikes across the entire signal pulse fireNoisyCycle60_2(ð, 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) {