From ae91b2aea88d49d12f4c2aa5c2a95c21efbc6613 Mon Sep 17 00:00:00 2001 From: Matthew Kennedy Date: Fri, 13 May 2022 15:48:26 -0700 Subject: [PATCH] don't rely on triggerStateListener to detect trigger errors (#4164) * Write proper TriggerDecoder tests * Improve logic around sync loss * these tests I understand * these I sort of understand.... * the one error in the noiseless decoder is gone! --- .../controllers/trigger/trigger_decoder.cpp | 53 +++++++++++-------- .../tests/trigger/test_cam_vvt_input.cpp | 11 ++-- .../test_real_cranking_nissan_vq40.cpp | 2 +- .../tests/trigger/test_real_nb2_cranking.cpp | 10 ++-- .../tests/trigger/test_trigger_decoder_2.cpp | 12 ++--- .../tests/trigger/test_trigger_noiseless.cpp | 6 +-- 6 files changed, 47 insertions(+), 47 deletions(-) diff --git a/firmware/controllers/trigger/trigger_decoder.cpp b/firmware/controllers/trigger/trigger_decoder.cpp index 00eba1051e..1189ceb2f9 100644 --- a/firmware/controllers/trigger/trigger_decoder.cpp +++ b/firmware/controllers/trigger/trigger_decoder.cpp @@ -422,20 +422,18 @@ void PrimaryTriggerDecoder::onTriggerError() { 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; for (int i = 0;i < PWM_PHASE_MAX_WAVE_PER_PWM;i++) { isDecodingError |= (currentCycle.eventCount[i] != triggerShape.getExpectedEventCount(i)); } - #if EFI_UNIT_TEST - printf("sync point: isDecodingError=%d\r\n", isDecodingError); - if (isDecodingError) { - for (int i = 0;i < PWM_PHASE_MAX_WAVE_PER_PWM;i++) { - printf("count: cur=%d exp=%d\r\n", currentCycle.eventCount[i], triggerShape.getExpectedEventCount(i)); - } - } + printf("validateEventCounters: isDecodingError=%d\n", isDecodingError); + if (isDecodingError) { + for (int i = 0;i < PWM_PHASE_MAX_WAVE_PER_PWM;i++) { + printf("count: cur=%d exp=%d\n", currentCycle.eventCount[i], triggerShape.getExpectedEventCount(i)); + } + } #endif /* EFI_UNIT_TEST */ return isDecodingError; @@ -670,22 +668,32 @@ void TriggerDecoderBase::decodeTriggerEvent( #endif /* EFI_UNIT_TEST */ if (isSynchronizationPoint) { + bool isDecodingError = validateEventCounters(triggerShape); + if (triggerStateListener) { - bool isDecodingError = validateEventCounters(triggerShape); - triggerStateListener->OnTriggerSyncronization(wasSynchronized, isDecodingError); - - // If we got a sync point, but the wrong number of events since the last sync point - if (wasSynchronized && isDecodingError) { - setTriggerErrorState(); - totalTriggerErrorCounter++; - - // This is a decoding error - onTriggerError(); - } } - setShaftSynchronized(true); + // If we got a sync point, but the wrong number of events since the last sync point + // One of two things has happened: + // - We missed a tooth, and this is the real sync point + // - Due to some mistake in timing, we found what looks like a sync point but actually isn't + // In either case, we should wait for another sync point before doing anything to try and run an engine, + // so we clear the synchronized flag. + if (wasSynchronized && isDecodingError) { + setTriggerErrorState(); + totalTriggerErrorCounter++; + + // Something wrong, no longer synchronized + setShaftSynchronized(false); + + // This is a decoding error + onTriggerError(); + } else { + // If this was the first sync point OR no decode error, we're synchronized! + setShaftSynchronized(true); + } + // this call would update duty cycle values nextTriggerEvent() ; @@ -703,8 +711,7 @@ void TriggerDecoderBase::decodeTriggerEvent( toothed_previous_time = nowNt; } - // TODO: should we include triggerStateListener here? That seems vestigial from when it called a listener, but it changes the behavior to remove it. - if (getShaftSynchronized() && !isValidIndex(triggerShape) && triggerStateListener) { + if (getShaftSynchronized() && !isValidIndex(triggerShape)) { // We've had too many events since the last sync point, we should have seen a sync point by now. // This is a trigger error. @@ -718,6 +725,8 @@ void TriggerDecoderBase::decodeTriggerEvent( onTriggerError(); + setShaftSynchronized(false); + return; } diff --git a/unit_tests/tests/trigger/test_cam_vvt_input.cpp b/unit_tests/tests/trigger/test_cam_vvt_input.cpp index d0ebc78553..c63c5540d4 100644 --- a/unit_tests/tests/trigger/test_cam_vvt_input.cpp +++ b/unit_tests/tests/trigger/test_cam_vvt_input.cpp @@ -53,9 +53,8 @@ TEST(trigger, testNoStartUpWarnings) { eth.fireRise(50); eth.fireFall(150); } - ASSERT_EQ( 2, unitTestWarningCodeState.recentWarnings.getCount()) << "warningCounter#testNoStartUpWarnings CUSTOM_SYNC_COUNT_MISMATCH expected"; - ASSERT_EQ(CUSTOM_SYNC_ERROR, unitTestWarningCodeState.recentWarnings.get(0)); - ASSERT_EQ(CUSTOM_SYNC_COUNT_MISMATCH, unitTestWarningCodeState.recentWarnings.get(1)); + EXPECT_EQ( 1, unitTestWarningCodeState.recentWarnings.getCount()) << "warningCounter#testNoStartUpWarnings CUSTOM_SYNC_COUNT_MISMATCH expected"; + EXPECT_EQ(CUSTOM_SYNC_ERROR, unitTestWarningCodeState.recentWarnings.get(0)); } TEST(trigger, testNoisyInput) { @@ -141,8 +140,8 @@ TEST(trigger, testNB2CamInput) { int totalRevolutionCountBeforeVvtSync = 5; // need to be out of VVT sync to see VVT sync in action - eth.fireRise(25); - eth.fireRise(25); + eth.fireRise(25 * 70 / 180); + eth.fireRise(25 * 110 / 180); ASSERT_EQ(totalRevolutionCountBeforeVvtSync, engine->triggerCentral.triggerState.getTotalRevolutionCounter()); ASSERT_TRUE((totalRevolutionCountBeforeVvtSync % SYMMETRICAL_CRANK_SENSOR_DIVIDER) != 0); @@ -174,7 +173,7 @@ TEST(trigger, testNB2CamInput) { eth.moveTimeForwardUs(MS2US( 30)); hwHandleVvtCamSignal(TV_RISE, getTimeNowNt(), 0); - EXPECT_NEAR(93.000f, engine->triggerCentral.getVVTPosition(0, 0), EPS2D); + EXPECT_NEAR(288.0f, engine->triggerCentral.getVVTPosition(0, 0), EPS2D); // actually position based on VVT! ASSERT_EQ(totalRevolutionCountBeforeVvtSync + 3, engine->triggerCentral.triggerState.getTotalRevolutionCounter()); diff --git a/unit_tests/tests/trigger/test_real_cranking_nissan_vq40.cpp b/unit_tests/tests/trigger/test_real_cranking_nissan_vq40.cpp index e94c51ac6b..98c9462fc6 100644 --- a/unit_tests/tests/trigger/test_real_cranking_nissan_vq40.cpp +++ b/unit_tests/tests/trigger/test_real_cranking_nissan_vq40.cpp @@ -23,7 +23,7 @@ TEST(realCrankingVQ40, normalCranking) { float vvt1 = engine->triggerCentral.getVVTPosition(/*bankIndex*/0, /*camIndex*/0); if (vvt1 != 0 && !hasSeenFirstVvt) { - EXPECT_NEAR(vvt1, 24.91, 1); + EXPECT_NEAR(vvt1, -33.204, 1); hasSeenFirstVvt = true; } } diff --git a/unit_tests/tests/trigger/test_real_nb2_cranking.cpp b/unit_tests/tests/trigger/test_real_nb2_cranking.cpp index 145f86c3b2..36cd22aca8 100644 --- a/unit_tests/tests/trigger/test_real_nb2_cranking.cpp +++ b/unit_tests/tests/trigger/test_real_nb2_cranking.cpp @@ -24,14 +24,14 @@ TEST(realCrankingNB2, normalCranking) { // Check the number of times VVT information was used to adjust crank phase // This should happen exactly once: once we sync, we shouldn't lose it. - EXPECT_EQ(engine->triggerCentral.triggerState.vvtSyncCounter, 1); + EXPECT_EQ(engine->triggerCentral.triggerState.vvtSyncCounter, 2); ASSERT_EQ(942, round(Sensor::getOrZero(SensorType::Rpm))); ASSERT_EQ(3, eth.recentWarnings()->getCount()); ASSERT_EQ(CUSTOM_OUT_OF_ORDER_COIL, eth.recentWarnings()->get(0)); - ASSERT_EQ(CUSTOM_SYNC_COUNT_MISMATCH, eth.recentWarnings()->get(1)); - ASSERT_EQ(CUSTOM_SYNC_ERROR, eth.recentWarnings()->get(2)); + ASSERT_EQ(CUSTOM_SYNC_ERROR, eth.recentWarnings()->get(1)); + ASSERT_EQ(CUSTOM_SYNC_COUNT_MISMATCH, eth.recentWarnings()->get(2)); } TEST(realCrankingNB2, crankingMissingInjector) { @@ -52,6 +52,6 @@ TEST(realCrankingNB2, crankingMissingInjector) { ASSERT_EQ(3, eth.recentWarnings()->getCount()); ASSERT_EQ(CUSTOM_OUT_OF_ORDER_COIL, eth.recentWarnings()->get(0)); - ASSERT_EQ(CUSTOM_SYNC_COUNT_MISMATCH, eth.recentWarnings()->get(1)); - ASSERT_EQ(CUSTOM_SYNC_ERROR, eth.recentWarnings()->get(2)); + ASSERT_EQ(CUSTOM_SYNC_ERROR, eth.recentWarnings()->get(1)); + ASSERT_EQ(CUSTOM_SYNC_COUNT_MISMATCH, eth.recentWarnings()->get(2)); } diff --git a/unit_tests/tests/trigger/test_trigger_decoder_2.cpp b/unit_tests/tests/trigger/test_trigger_decoder_2.cpp index aa7b5ecd63..d53cb86639 100644 --- a/unit_tests/tests/trigger/test_trigger_decoder_2.cpp +++ b/unit_tests/tests/trigger/test_trigger_decoder_2.cpp @@ -143,8 +143,7 @@ TEST(TriggerDecoder, TooManyTeeth_CausesError) { StrictMock dut; // We expect one call to onTriggerError(). - // TODO: uncomment this once decoder is fixed - // EXPECT_CALL(dut, onTriggerError()); + EXPECT_CALL(dut, onTriggerError()); // Fire a few boring evenly spaced teeth t += MS2NT(1); @@ -178,8 +177,7 @@ TEST(TriggerDecoder, TooManyTeeth_CausesError) { // This tooth is extra - expect a call to onTriggerError() and loss of sync! t += MS2NT(1); doTooth(dut, shape, cfg, t); - // TODO: uncomment this once decoder is fixed - // EXPECT_FALSE(dut.getShaftSynchronized()); + EXPECT_FALSE(dut.getShaftSynchronized()); EXPECT_EQ(6, dut.currentCycle.current_index); } @@ -193,8 +191,7 @@ TEST(TriggerDecoder, NotEnoughTeeth_CausesError) { StrictMock dut; // We expect one call to onTriggerError(). - // TODO: uncomment this once decoder is fixed - // EXPECT_CALL(dut, onTriggerError()); + EXPECT_CALL(dut, onTriggerError()); // Fire a few boring evenly spaced teeth t += MS2NT(1); @@ -225,7 +222,6 @@ TEST(TriggerDecoder, NotEnoughTeeth_CausesError) { doTooth(dut, shape, cfg, t); // Sync is lost until we get to another sync point - // TODO: uncomment this once decoder is fixed - // EXPECT_FALSE(dut.getShaftSynchronized()); + EXPECT_FALSE(dut.getShaftSynchronized()); EXPECT_EQ(0, dut.currentCycle.current_index); } diff --git a/unit_tests/tests/trigger/test_trigger_noiseless.cpp b/unit_tests/tests/trigger/test_trigger_noiseless.cpp index 0d8dd85257..75cc2e7231 100644 --- a/unit_tests/tests/trigger/test_trigger_noiseless.cpp +++ b/unit_tests/tests/trigger/test_trigger_noiseless.cpp @@ -152,11 +152,7 @@ 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); - // 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"; - + ASSERT_EQ(errorToleranceCnt, engine->triggerCentral.triggerState.totalTriggerErrorCounter); } TEST(trigger, noiselessDecoder) {