From 711e90f3d38ab6dc30507a729741b27dbcadfb3e Mon Sep 17 00:00:00 2001 From: Matthew Kennedy Date: Thu, 23 Feb 2023 11:27:56 -0800 Subject: [PATCH] remove old trigger noise detector (#44) * remove old noiseless decoder layer * test --- .../engine_cycle/rpm_calculator.cpp | 2 - .../controllers/trigger/trigger_central.cpp | 93 +-------- .../controllers/trigger/trigger_central.h | 14 -- firmware/integration/rusefi_config.txt | 1 - firmware/tunerstudio/rusefi.input | 1 - unit_tests/tests/tests.mk | 1 - .../tests/trigger/test_trigger_noiseless.cpp | 189 ------------------ 7 files changed, 3 insertions(+), 298 deletions(-) delete mode 100644 unit_tests/tests/trigger/test_trigger_noiseless.cpp diff --git a/firmware/controllers/engine_cycle/rpm_calculator.cpp b/firmware/controllers/engine_cycle/rpm_calculator.cpp index 73a2ec8e4a..193b3d6256 100644 --- a/firmware/controllers/engine_cycle/rpm_calculator.cpp +++ b/firmware/controllers/engine_cycle/rpm_calculator.cpp @@ -223,8 +223,6 @@ void RpmCalculator::setStopped() { if (cachedRpmValue != 0) { assignRpmValue(0); - // needed by 'useNoiselessTriggerDecoder' - engine->triggerCentral.noiseFilter.resetAccumSignalData(); efiPrintf("engine stopped"); } state = STOPPED; diff --git a/firmware/controllers/trigger/trigger_central.cpp b/firmware/controllers/trigger/trigger_central.cpp index 1cf9ed2a53..101ee8fe24 100644 --- a/firmware/controllers/trigger/trigger_central.cpp +++ b/firmware/controllers/trigger/trigger_central.cpp @@ -49,13 +49,6 @@ TriggerCentral::TriggerCentral() : { memset(&hwEventCounters, 0, sizeof(hwEventCounters)); triggerState.resetState(); - noiseFilter.resetAccumSignalData(); -} - -void TriggerNoiseFilter::resetAccumSignalData() { - memset(lastSignalTimes, 0xff, sizeof(lastSignalTimes)); // = -1 - memset(accumSignalPeriods, 0, sizeof(accumSignalPeriods)); - memset(accumSignalPrevPeriods, 0, sizeof(accumSignalPrevPeriods)); } int TriggerCentral::getHwEventCounter(int index) const { @@ -437,15 +430,9 @@ void handleShaftSignal(int signalIndex, bool isRising, efitick_t timestamp) { #endif /* EFI_TOOTH_LOGGER */ - // for effective noise filtering, we need both signal edges, - // so we pass them to handleShaftSignal() and defer this test - if (!engineConfiguration->useNoiselessTriggerDecoder) { - if (!isUsefulSignal(signal, getTriggerCentral()->triggerShape)) { - /** - * no need to process VR falls further - */ - return; - } + if (!isUsefulSignal(signal, getTriggerCentral()->triggerShape)) { + // This is a falling edge in rise-only mode (etc), drop it + return; } if (engineConfiguration->triggerInputDebugPins[signalIndex] != Gpio::Unassigned) { @@ -503,69 +490,6 @@ static void reportEventToWaveChart(trigger_event_e ckpSignalType, int triggerEve } } -/** - * This is used to filter noise spikes (interference) in trigger signal. See - * The basic idea is to use not just edges, but the average amount of time the signal stays in '0' or '1'. - * So we update 'accumulated periods' to track where the signal is. - * And then compare between the current period and previous, with some tolerance (allowing for the wheel speed change). - * @return true if the signal is passed through. - */ -bool TriggerNoiseFilter::noiseFilter(efitick_t nowNt, - TriggerDecoderBase * triggerState, - trigger_event_e signal) { - // todo: find a better place for these defs - static const trigger_event_e opposite[4] = { SHAFT_PRIMARY_RISING, SHAFT_PRIMARY_FALLING, SHAFT_SECONDARY_RISING, SHAFT_SECONDARY_FALLING }; - static const TriggerWheel triggerIdx[4] = { TriggerWheel::T_PRIMARY, TriggerWheel::T_PRIMARY, TriggerWheel::T_SECONDARY, TriggerWheel:: T_SECONDARY }; - // we process all trigger channels independently - TriggerWheel ti = triggerIdx[signal]; - // falling is opposite to rising, and vise versa - trigger_event_e os = opposite[signal]; - - // todo: currently only primary channel is filtered, because there are some weird trigger types on other channels - if (ti != TriggerWheel::T_PRIMARY) - return true; - - // update period accumulator: for rising signal, we update '0' accumulator, and for falling - '1' - if (lastSignalTimes[signal] != -1) - accumSignalPeriods[signal] += nowNt - lastSignalTimes[signal]; - // save current time for this trigger channel - lastSignalTimes[signal] = nowNt; - - // now we want to compare current accumulated period to the stored one - efitick_t currentPeriod = accumSignalPeriods[signal]; - // the trick is to compare between different - efitick_t allowedPeriod = accumSignalPrevPeriods[os]; - - // but first check if we're expecting a gap - bool isGapExpected = TRIGGER_WAVEFORM(isSynchronizationNeeded) && triggerState->getShaftSynchronized() && - (triggerState->currentCycle.eventCount[(int)ti] + 1) == TRIGGER_WAVEFORM(getExpectedEventCount(ti)); - - if (isGapExpected) { - // usually we need to extend the period for gaps, based on the trigger info - allowedPeriod *= TRIGGER_WAVEFORM(syncRatioAvg); - } - - // also we need some margin for rapidly changing trigger-wheel speed, - // that's why we expect the period to be no less than 2/3 of the previous period (this is just an empirical 'magic' coef.) - efitick_t minAllowedPeriod = 2 * allowedPeriod / 3; - // but no longer than 5/4 of the previous 'normal' period - efitick_t maxAllowedPeriod = 5 * allowedPeriod / 4; - - // above all, check if the signal comes not too early - if (currentPeriod >= minAllowedPeriod) { - // now we store this period as a reference for the next time, - // BUT we store only 'normal' periods, and ignore too long periods (i.e. gaps) - if (!isGapExpected && (maxAllowedPeriod == 0 || currentPeriod <= maxAllowedPeriod)) { - accumSignalPrevPeriods[signal] = currentPeriod; - } - // reset accumulator - accumSignalPeriods[signal] = 0; - return true; - } - // all premature or extra-long events are ignored - treated as interference - return false; -} - void TriggerCentral::decodeMapCam(efitick_t timestamp, float currentPhase) { if (engineConfiguration->vvtMode[0] == VVT_MAP_V_TWIN && Sensor::getOrZero(SensorType::Rpm) < engineConfiguration->cranking.rpm) { @@ -678,16 +602,6 @@ void TriggerCentral::handleShaftSignal(trigger_event_e signal, efitick_t timesta return; } - // This code gathers some statistics on signals and compares accumulated periods to filter interference - if (engineConfiguration->useNoiselessTriggerDecoder) { - if (!noiseFilter.noiseFilter(timestamp, &triggerState, signal)) { - return; - } - if (!isUsefulSignal(signal, triggerShape)) { - return; - } - } - if (!isToothExpectedNow(timestamp)) { triggerIgnoredToothCount++; return; @@ -947,7 +861,6 @@ void onConfigurationChangeTriggerCallback() { if (changed) { #if EFI_ENGINE_CONTROL engine->updateTriggerWaveform(); - getTriggerCentral()->noiseFilter.resetAccumSignalData(); #endif } #if EFI_DEFAILED_LOGGING diff --git a/firmware/controllers/trigger/trigger_central.h b/firmware/controllers/trigger/trigger_central.h index c31cb7bc09..b6a3aa8f46 100644 --- a/firmware/controllers/trigger/trigger_central.h +++ b/firmware/controllers/trigger/trigger_central.h @@ -29,18 +29,6 @@ typedef void (*ShaftPositionListener)(trigger_event_e signal, uint32_t index, ef #define HAVE_CAM_INPUT() (isBrainPinValid(engineConfiguration->camInputs[0])) -class TriggerNoiseFilter { -public: - void resetAccumSignalData(); - bool noiseFilter(efitick_t nowNt, - TriggerDecoderBase* triggerState, - trigger_event_e signal); - - efitick_t lastSignalTimes[HW_EVENT_TYPES]; - efitick_t accumSignalPeriods[HW_EVENT_TYPES]; - efitick_t accumSignalPrevPeriods[HW_EVENT_TYPES]; -}; - /** * Maybe merge TriggerCentral and TriggerState classes into one class? * Probably not: we have an instance of TriggerState which is used for trigger initialization, @@ -146,8 +134,6 @@ public: return engineMovedRecently(getTimeNowNt()); } - TriggerNoiseFilter noiseFilter; - int vvtEventRiseCounter[CAM_INPUTS_COUNT]; int vvtEventFallCounter[CAM_INPUTS_COUNT]; diff --git a/firmware/integration/rusefi_config.txt b/firmware/integration/rusefi_config.txt index ba90a5a5e4..9d26ea8655 100644 --- a/firmware/integration/rusefi_config.txt +++ b/firmware/integration/rusefi_config.txt @@ -744,7 +744,6 @@ bit is_enabled_spi_2 bit isFasterEngineSpinUpEnabled;If enabled, try to fire the engine before a full engine cycle has been completed using RPM estimated from the last 90 degrees of engine rotation. As soon as the trigger syncs plus 90 degrees rotation, fuel and ignition events will occur. If disabled, worst case may require up to 4 full crank rotations before any events are scheduled. bit coastingFuelCutEnabled;This setting disables fuel injection while the engine is in overrun, this is useful as a fuel saving measure and to prevent back firing. bit useIacTableForCoasting;Override the IAC position during overrun conditions to help reduce engine breaking, this can be helpful for large engines in light weight cars or engines that have trouble returning to idle. - bit useNoiselessTriggerDecoder bit useIdleTimingPidControl bit disableEtbWhenEngineStopped;Allows disabling the ETB when the engine is stopped. You may not like the power draw or PWM noise from the motor, so this lets you turn it off until it's necessary. bit is_enabled_spi_4 diff --git a/firmware/tunerstudio/rusefi.input b/firmware/tunerstudio/rusefi.input index 120b2e6363..2a6524ca14 100644 --- a/firmware/tunerstudio/rusefi.input +++ b/firmware/tunerstudio/rusefi.input @@ -2223,7 +2223,6 @@ cmd_set_engine_type_default = "@@TS_IO_TEST_COMMAND_char@@@@ts_command_e_TS_ field = "Display logic signals", displayLogicLevelsInEngineSniffer field = "Do not print messages in case of sync error", silentTriggerError field = "Focus on inputs in engine sniffer", engineSnifferFocusOnInputs - field = "Enable noise filtering", useNoiselessTriggerDecoder, {trigger_type == @@trigger_type_e_TT_TOOTHED_WHEEL_60_2@@ || trigger_type == @@trigger_type_e_TT_TOOTHED_WHEEL_36_1@@} panel = triggerInputComparator @@if_ts_show_trigger_comparator panel = triggerConfiguration_gap diff --git a/unit_tests/tests/tests.mk b/unit_tests/tests/tests.mk index fc4c93aca5..17716b5efa 100644 --- a/unit_tests/tests/tests.mk +++ b/unit_tests/tests/tests.mk @@ -3,7 +3,6 @@ TESTS_SRC_CPP = \ tests/trigger/test_symmetrical_crank.cpp \ tests/trigger/test_trigger_decoder.cpp \ tests/trigger/test_trigger_decoder_2.cpp \ - tests/trigger/test_trigger_noiseless.cpp \ tests/trigger/test_trigger_multi_sync.cpp \ tests/trigger/test_trigger_input_adc.cpp \ tests/trigger/test_miata_na_tdc.cpp \ diff --git a/unit_tests/tests/trigger/test_trigger_noiseless.cpp b/unit_tests/tests/trigger/test_trigger_noiseless.cpp deleted file mode 100644 index bed81393a4..0000000000 --- a/unit_tests/tests/trigger/test_trigger_noiseless.cpp +++ /dev/null @@ -1,189 +0,0 @@ -/** - * @file test_trigger_noiseless.cpp - * - * @date Apr 20, 2018 - */ - -#include "pch.h" - -#include "trigger_decoder.h" -#include "event_queue.h" -#include "trigger_central.h" -#include "main_trigger_callback.h" -#include "advance_map.h" -#include "speed_density.h" -#include "fuel_math.h" -#include "spark_logic.h" -#include "trigger_universal.h" - -extern bool printTriggerDebug; - -static void fireEvent(EngineTestHelper *eth, bool isRise) { - // mostly we fire only rise events (useOnlyRisingEdgeForTrigger=true). - // but for noise filtering, both edges should be processed, so we fire falling events too - if (isRise) - eth->firePrimaryTriggerRise(); - else if (engineConfiguration->useNoiselessTriggerDecoder) - eth->firePrimaryTriggerFall(); -} - -/* ________ __ _ _ __ - * __|_|_|__| OR | | | |________ - * spikes signal spikes signal - */ -static void noisyPulse(EngineTestHelper *eth, int idx, int durationUs, bool isRise, int noiseIdx, int durationNoiseUs, int offsetNoiseUs, int numSpikes) { - // skip some time at the beginning - eth->moveTimeForwardUs(offsetNoiseUs); - durationUs -= offsetNoiseUs; - // add noise spikes - if (idx == noiseIdx) { - // calculate the distance between noise spikes (evenly spaced) - int noiseIntervalUs = (durationUs - durationNoiseUs * numSpikes) / numSpikes; - - for (int i = 0; i < numSpikes; i++) { - // start spike - fireEvent(eth, isRise); - eth->moveTimeForwardUs(durationNoiseUs); - durationUs -= durationNoiseUs; - // end spike - fireEvent(eth, !isRise); - - // add space between spikes - eth->moveTimeForwardUs(noiseIntervalUs); - durationUs -= noiseIntervalUs; - } - } - - // add the rest of pulse period - eth->moveTimeForwardUs(durationUs); - fireEvent(eth, isRise); -} - -static void fireNoisyCycle60_2(EngineTestHelper *eth, int numCycles, int durationUs, int noiseIdx, int durationNoiseUs, int offsetNoiseUs, int numSpikes) { - int idx = 0; - for (int cycles = 0; cycles < numCycles; cycles++) { - int count = 60 - 2 - 1; - for (int i = 0; i < count; i++) { - // rising - noisyPulse(eth, idx++, durationUs, true, noiseIdx, durationNoiseUs, offsetNoiseUs, numSpikes); - // falling - noisyPulse(eth, idx++, durationUs, false, noiseIdx, durationNoiseUs, offsetNoiseUs, numSpikes); - } - // skip 2 teeth (durationUs * 4) - // and add 1st rising teeth of the next cycle - noisyPulse(eth, idx++, (durationUs * 4) + durationUs, true, noiseIdx, durationNoiseUs, offsetNoiseUs, numSpikes); - // falling - noisyPulse(eth, idx++, durationUs, false, noiseIdx, durationNoiseUs, offsetNoiseUs, numSpikes); - } -} - -static void resetTrigger(EngineTestHelper ð) { - eth.applyTriggerWaveform(); - eth.engine.triggerCentral.noiseFilter.resetAccumSignalData(); - // reset error counter - eth.engine.triggerCentral.triggerState.totalTriggerErrorCounter = 0; -} - -static void testNoiselessDecoderProcedure(EngineTestHelper ð, int errorToleranceCnt) { - printf("*** (bc->useNoiselessTriggerDecoder = %s)\r\n", - engineConfiguration->useNoiselessTriggerDecoder ? "true" : "false"); - - resetTrigger(eth); - - // first, no noise - fireNoisyCycle60_2(ð, 2, 1000, -1, 0, 0, 0); - - // should be no errors anyway - ASSERT_EQ( 0, engine->triggerCentral.triggerState.totalTriggerErrorCounter) << "testNoiselessDecoderProcedure totalTriggerErrorCounter"; - // check if we're imitating the 60-2 signal correctly - ASSERT_EQ( 0, eth.engine.triggerCentral.triggerState.getCurrentIndex()) << "index #1"; - // check rpm (60secs / (1000us * 60teeth)) = 1000rpm - ASSERT_EQ( 1000, Sensor::getOrZero(SensorType::Rpm)) << "testNoiselessDecoder RPM"; - - // add noise1 - 1 spike in the middle of the 2nd rising pulse - fireNoisyCycle60_2(ð, 2, 1000, 2, 10, 500, 1); - - assertEqualsM("testNoiselessDecoder noise#1", errorToleranceCnt, engine->triggerCentral.triggerState.totalTriggerErrorCounter); - - resetTrigger(eth); - - // add noise2 - 1 spike in the middle of the 2nd falling pulse - fireNoisyCycle60_2(ð, 2, 1000, 3, 10, 500, 1); - - //assertEqualsM("noise#2", errorToleranceCnt, engine->triggerCentral.triggerState.totalTriggerErrorCounter); - - resetTrigger(eth); - - // add noise3 - in the middle of the sync.gap, - // so that we cannot tell for sure if it's a start of another 'extra' tooth or just a noise inside the gap, - // that's why we used expectedEventCount[] in our filtering algo to make a prediction about gap - fireNoisyCycle60_2(ð, 2, 1000, 114, 10, 1500, 1); - - // so everything runs smoothly! - assertEqualsM("noise#3", errorToleranceCnt, engine->triggerCentral.triggerState.totalTriggerErrorCounter); - - resetTrigger(eth); - - // add noise4 - too close to the start of the real next signal, so the noise spike is accepted as a signal - // but when the real signal comes shortly afterwards, it we be treated as a noise spike, - fireNoisyCycle60_2(ð, 2, 1000, 4, 10, 980, 1); - - // and we won't get out of sync! - assertEqualsM("noise#4", errorToleranceCnt, engine->triggerCentral.triggerState.totalTriggerErrorCounter); - - resetTrigger(eth); - - // add noise5 - one very long 333us noise spike - fireNoisyCycle60_2(ð, 2, 1000, 4, 333, 10, 1); - // still ok - assertEqualsM("noise#5", errorToleranceCnt, engine->triggerCentral.triggerState.totalTriggerErrorCounter); - - resetTrigger(eth); - - // add noise6 - 10 short spikes across the entire signal pulse - const int failProofNumSpikes = 10; - fireNoisyCycle60_2(ð, 2, 1000, 4, 5, 10, failProofNumSpikes); - - // we barely survived this time - assertEqualsM("testNoiselessDecoder noise#6", errorToleranceCnt, engine->triggerCentral.triggerState.totalTriggerErrorCounter); - - resetTrigger(eth); - - // 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"; -} - -TEST(trigger, noiselessDecoder) { - printf("====================================================================================== testNoiselessDecoder\r\n"); - - EngineTestHelper eth(TEST_ENGINE); - - engineConfiguration->ignitionMode = IM_WASTED_SPARK; - - // we'll test on 60-2 wheel - eth.setTriggerType(TT_TOOTHED_WHEEL_60_2); - - ASSERT_EQ(0, engine->triggerCentral.triggerState.totalTriggerErrorCounter); - ASSERT_EQ( 0, Sensor::getOrZero(SensorType::Rpm)) << "testNoiselessDecoder RPM"; - - //printTriggerDebug = true; - -#if 0 - // try normal trigger mode, no noise filtering - engineConfiguration->useNoiselessTriggerDecoder = false; - // for test validation, it should be 1 trigger error - testNoiselessDecoderProcedure(eth, 1); -#endif - - // now enable our noise filtering algo - engineConfiguration->useNoiselessTriggerDecoder = true; - // should be 0 errors! - testNoiselessDecoderProcedure(eth, 0); - - //printTriggerDebug = false; -}