From 9cd55536171330e302327070cc761c0dc28d5334 Mon Sep 17 00:00:00 2001 From: Matthew Kennedy Date: Sun, 25 Sep 2022 15:49:54 -0700 Subject: [PATCH] plumbing parts of 4621 (#4622) * plumbing parts of 4621 * show error list * exit on error too! * this test has some unhealthy undefined behavior * no uninitialized values * no that doesn't work * remove the invalid data and the bug becomes obvious --- .github/workflows/build-unit-tests.yaml | 2 +- .../controllers/trigger/decoders/sync_edge.h | 7 ++++--- .../trigger/decoders/trigger_structure.cpp | 18 +++++++++--------- .../trigger/decoders/trigger_structure.h | 7 ++++--- .../controllers/trigger/trigger_central.cpp | 13 +++++++------ .../controllers/trigger/trigger_decoder.cpp | 7 ++++--- .../controllers/trigger/trigger_simulator.cpp | 6 +++--- .../controllers/trigger/trigger_simulator.h | 2 +- .../tests/ignition_injection/test_fuel_map.cpp | 2 +- .../tests/trigger/test_trigger_decoder.cpp | 2 +- .../tests/trigger/test_trigger_multi_sync.cpp | 14 +++++++------- 11 files changed, 42 insertions(+), 38 deletions(-) diff --git a/.github/workflows/build-unit-tests.yaml b/.github/workflows/build-unit-tests.yaml index e0747de141..5c26567675 100644 --- a/.github/workflows/build-unit-tests.yaml +++ b/.github/workflows/build-unit-tests.yaml @@ -78,4 +78,4 @@ jobs: - name: Run Tests (Valgrind) if: ${{ matrix.os != 'macos-latest' }} working-directory: ./unit_tests/ - run: valgrind --error-exitcode=1 --leak-check=no build/rusefi_test + run: valgrind --error-exitcode=1 --exit-on-first-error=yes --leak-check=no --show-error-list=yes build/rusefi_test diff --git a/firmware/controllers/trigger/decoders/sync_edge.h b/firmware/controllers/trigger/decoders/sync_edge.h index bb34823472..78ea900563 100644 --- a/firmware/controllers/trigger/decoders/sync_edge.h +++ b/firmware/controllers/trigger/decoders/sync_edge.h @@ -1,7 +1,8 @@ #pragma once enum class SyncEdge : uint8_t { - Rise, - Fall, - Both + Rise, // Sync on rising edges, use rise+fall for phase info + Fall, // Sync on falling edges, use rise+fall for phase info + RiseOnly, // Completely ignore all falling edges (this used to be useOnlyRisingEdgeForTrigger=true etc) + Both // Sync on both edges, use both for phase }; diff --git a/firmware/controllers/trigger/decoders/trigger_structure.cpp b/firmware/controllers/trigger/decoders/trigger_structure.cpp index 10ae28d57d..f5ba66cc1f 100644 --- a/firmware/controllers/trigger/decoders/trigger_structure.cpp +++ b/firmware/controllers/trigger/decoders/trigger_structure.cpp @@ -191,8 +191,8 @@ size_t TriggerWaveform::getExpectedEventCount(TriggerWheel channelIndex) const { return expectedEventCount[(int)channelIndex]; } -void TriggerWaveform::calculateExpectedEventCounts(bool useOnlyRisingEdgeForTrigger) { - if (!useOnlyRisingEdgeForTrigger) { +void TriggerWaveform::calculateExpectedEventCounts() { + if (!useOnlyRisingEdges) { for (size_t i = 0; i < efi::size(expectedEventCount); i++) { if (getExpectedEventCount((TriggerWheel)i) % 2 != 0) { firmwareError(ERROR_TRIGGER_DRAMA, "Trigger: should be even number of events index=%d count=%d", i, getExpectedEventCount((TriggerWheel)i)); @@ -200,7 +200,7 @@ void TriggerWaveform::calculateExpectedEventCounts(bool useOnlyRisingEdgeForTrig } } - bool isSingleToothOnPrimaryChannel = useOnlyRisingEdgeForTrigger ? getExpectedEventCount(TriggerWheel::T_PRIMARY) == 1 : getExpectedEventCount(TriggerWheel::T_PRIMARY) == 2; + bool isSingleToothOnPrimaryChannel = useOnlyRisingEdges ? getExpectedEventCount(TriggerWheel::T_PRIMARY) == 1 : getExpectedEventCount(TriggerWheel::T_PRIMARY) == 2; // todo: next step would be to set 'isSynchronizationNeeded' automatically based on the logic we have here if (!shapeWithoutTdc && isSingleToothOnPrimaryChannel != !isSynchronizationNeeded) { firmwareError(ERROR_TRIGGER_DRAMA, "shapeWithoutTdc isSynchronizationNeeded isSingleToothOnPrimaryChannel constraint violation"); @@ -259,7 +259,7 @@ void TriggerWaveform::addEvent(angle_t angle, TriggerWheel const channelIndex, T // todo: the whole 'useOnlyRisingEdgeForTrigger' parameter and logic should not be here // todo: see calculateExpectedEventCounts // related calculation should be done once trigger is initialized outside of trigger shape scope - if (!useOnlyRisingEdgeForTriggerTemp || state == TriggerValue::RISE) { + if (!useOnlyRisingEdges || state == TriggerValue::RISE) { expectedEventCount[(int)channelIndex]++; } @@ -388,7 +388,7 @@ uint16_t TriggerWaveform::findAngleIndex(TriggerFormDetails *details, angle_t ta } } while (left <= right); left -= 1; - if (useOnlyRisingEdgeForTriggerTemp) { + if (useOnlyRisingEdges) { left &= ~1U; } return left; @@ -474,8 +474,7 @@ void TriggerWaveform::initializeTriggerWaveform(operation_mode_e triggerOperatio shapeDefinitionError = false; - bool useOnlyRisingEdgeForTrigger = triggerConfig.UseOnlyRisingEdgeForTrigger; - this->useOnlyRisingEdgeForTriggerTemp = useOnlyRisingEdgeForTrigger; + this->useOnlyRisingEdges = triggerConfig.UseOnlyRisingEdgeForTrigger; switch (triggerConfig.TriggerType.type) { @@ -789,18 +788,19 @@ void TriggerWaveform::initializeTriggerWaveform(operation_mode_e triggerOperatio setShapeDefinitionError(true); warning(CUSTOM_ERR_NO_SHAPE, "initializeTriggerWaveform() not implemented: %d", triggerConfig.TriggerType.type); } + /** * Feb 2019 suggestion: it would be an improvement to remove 'expectedEventCount' logic from 'addEvent' * and move it here, after all events were added. */ - calculateExpectedEventCounts(useOnlyRisingEdgeForTrigger); + calculateExpectedEventCounts(); version++; if (!shapeDefinitionError) { wave.checkSwitchTimes(getCycleDuration()); } - if (syncEdge == SyncEdge::Both && useOnlyRisingEdgeForTrigger) { + if (syncEdge == SyncEdge::Both && useOnlyRisingEdges) { #if EFI_PROD_CODE || EFI_SIMULATOR firmwareError(CUSTOM_ERR_BOTH_FRONTS_REQUIRED, "trigger: both fronts required"); #else diff --git a/firmware/controllers/trigger/decoders/trigger_structure.h b/firmware/controllers/trigger/decoders/trigger_structure.h index 1cf0611266..bd67c8eea9 100644 --- a/firmware/controllers/trigger/decoders/trigger_structure.h +++ b/firmware/controllers/trigger/decoders/trigger_structure.h @@ -146,7 +146,10 @@ public: // Which edge(s) to consider for finding the sync point: rise, fall, or both SyncEdge syncEdge; - void calculateExpectedEventCounts(bool useOnlyRisingEdgeForTrigger); + // If true, falling edges should be fully ignored on this trigger shape. + bool useOnlyRisingEdges; + + void calculateExpectedEventCounts(); size_t getExpectedEventCount(TriggerWheel channelIndex) const; @@ -180,8 +183,6 @@ public: bool isRiseEvent[PWM_PHASE_MAX_COUNT]; - bool useOnlyRisingEdgeForTriggerTemp; - /* (0..1] angle range */ void addEvent(angle_t angle, TriggerWheel const channelIndex, TriggerValue const state); /* (0..720] angle range diff --git a/firmware/controllers/trigger/trigger_central.cpp b/firmware/controllers/trigger/trigger_central.cpp index 6c98d19903..d8155cf872 100644 --- a/firmware/controllers/trigger/trigger_central.cpp +++ b/firmware/controllers/trigger/trigger_central.cpp @@ -279,6 +279,7 @@ void hwHandleVvtCamSignal(TriggerValue front, efitick_t nowNt, int index) { #endif /* EFI_TOOTH_LOGGER */ } + const auto& vvtShape = tc->vvtShape[camIndex]; bool isImportantFront = (engineConfiguration->vvtCamSensorUseRise ^ (front == TriggerValue::FALL)); bool isVvtWithRealDecoder = vvtWithRealDecoder(engineConfiguration->vvtMode[camIndex]); @@ -472,7 +473,7 @@ void handleShaftSignal(int signalIndex, bool isRising, efitick_t timestamp) { // 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()->primaryTriggerConfiguration)) { + if (!isUsefulSignal(signal, getTriggerCentral()->triggerShape)) { /** * no need to process VR falls further */ @@ -519,7 +520,7 @@ void TriggerCentral::resetCounters() { static const bool isUpEvent[4] = { false, true, false, true }; static const int wheelIndeces[4] = { 0, 0, 1, 1}; -static void reportEventToWaveChart(trigger_event_e ckpSignalType, int triggerEventIndex) { +static void reportEventToWaveChart(trigger_event_e ckpSignalType, int triggerEventIndex, bool addOppositeEvent) { if (!getTriggerCentral()->isEngineSnifferEnabled) { // this is here just as a shortcut so that we avoid engine sniffer as soon as possible return; // engineSnifferRpmThreshold is accounted for inside getTriggerCentral()->isEngineSnifferEnabled } @@ -529,7 +530,7 @@ static void reportEventToWaveChart(trigger_event_e ckpSignalType, int triggerEve bool isUp = isUpEvent[(int) ckpSignalType]; addEngineSnifferCrankEvent(wheelIndex, triggerEventIndex, isUp ? FrontDirection::UP : FrontDirection::DOWN); - if (engineConfiguration->useOnlyRisingEdgeForTrigger) { + if (addOppositeEvent) { // let's add the opposite event right away addEngineSnifferCrankEvent(wheelIndex, triggerEventIndex, isUp ? FrontDirection::DOWN : FrontDirection::UP); } @@ -659,7 +660,7 @@ void TriggerCentral::handleShaftSignal(trigger_event_e signal, efitick_t timesta if (!noiseFilter.noiseFilter(timestamp, &triggerState, signal)) { return; } - if (!isUsefulSignal(signal, primaryTriggerConfiguration)) { + if (!isUsefulSignal(signal, triggerShape)) { return; } } @@ -692,7 +693,7 @@ void TriggerCentral::handleShaftSignal(trigger_event_e signal, efitick_t timesta int crankInternalIndex = triggerState.getCrankSynchronizationCounter() % crankDivider; int triggerIndexForListeners = decodeResult.Value.CurrentIndex + (crankInternalIndex * triggerShape.getSize()); - reportEventToWaveChart(signal, triggerIndexForListeners); + reportEventToWaveChart(signal, triggerIndexForListeners, triggerShape.useOnlyRisingEdges); // Look up this tooth's angle from the sync point. If this tooth is the sync point, we'll get 0 here. auto currentPhaseFromSyncPoint = getTriggerCentral()->triggerFormDetails.eventAngles[triggerIndexForListeners]; @@ -772,7 +773,7 @@ void TriggerCentral::handleShaftSignal(trigger_event_e signal, efitick_t timesta decodeMapCam(timestamp, currentPhase); } else { // We don't have sync, but report to the wave chart anyway as index 0. - reportEventToWaveChart(signal, 0); + reportEventToWaveChart(signal, 0, triggerShape.useOnlyRisingEdges); } } diff --git a/firmware/controllers/trigger/trigger_decoder.cpp b/firmware/controllers/trigger/trigger_decoder.cpp index be614f2d6a..73106541bd 100644 --- a/firmware/controllers/trigger/trigger_decoder.cpp +++ b/firmware/controllers/trigger/trigger_decoder.cpp @@ -195,7 +195,7 @@ void TriggerFormDetails::prepareEventAngles(TriggerWaveform *shape) { // Wrap the angle back in to [0, 720) fixAngle(angle, "trgSync", CUSTOM_TRIGGER_SYNC_ANGLE_RANGE); - if (engineConfiguration->useOnlyRisingEdgeForTrigger) { + if (shape->useOnlyRisingEdges) { efiAssertVoid(OBD_PCM_Processor_Fault, triggerDefinitionIndex < triggerShapeLength, "trigger shape fail"); assertIsInBounds(triggerDefinitionIndex, shape->isRiseEvent, "isRise"); @@ -524,6 +524,7 @@ static bool shouldConsiderEdge(const TriggerWaveform& triggerShape, TriggerWheel switch (triggerShape.syncEdge) { case SyncEdge::Both: return true; + case SyncEdge::RiseOnly: case SyncEdge::Rise: return edge == TriggerValue::RISE; case SyncEdge::Fall: return edge == TriggerValue::FALL; } @@ -562,7 +563,7 @@ expected TriggerDecoderBase::decodeTriggerEvent( } } - bool useOnlyRisingEdgeForTrigger = triggerConfiguration.UseOnlyRisingEdgeForTrigger; + bool useOnlyRisingEdgeForTrigger = triggerShape.useOnlyRisingEdges; efiAssert(CUSTOM_TRIGGER_UNEXPECTED, signal <= SHAFT_SECONDARY_RISING, "unexpected signal", unexpected); @@ -704,7 +705,7 @@ expected TriggerDecoderBase::decodeTriggerEvent( * in case of noise the counter could be above the expected number of events, that's why 'more or equals' and not just 'equals' */ - unsigned int endOfCycleIndex = triggerShape.getSize() - (triggerConfiguration.UseOnlyRisingEdgeForTrigger ? 2 : 1); + unsigned int endOfCycleIndex = triggerShape.getSize() - (useOnlyRisingEdgeForTrigger ? 2 : 1); isSynchronizationPoint = !getShaftSynchronized() || (currentCycle.current_index >= endOfCycleIndex); diff --git a/firmware/controllers/trigger/trigger_simulator.cpp b/firmware/controllers/trigger/trigger_simulator.cpp index 866980c697..9ef27192ae 100644 --- a/firmware/controllers/trigger/trigger_simulator.cpp +++ b/firmware/controllers/trigger/trigger_simulator.cpp @@ -21,8 +21,8 @@ static const bool isRisingEdge[HW_EVENT_TYPES] = { false, true, false, true, fal * todo: should this method be invoked somewhere deeper? at the moment we have too many usages too high * @return true if front should be decoded further, false if we are not interested */ -bool isUsefulSignal(trigger_event_e signal, const TriggerConfiguration& triggerConfiguration) { - return !triggerConfiguration.UseOnlyRisingEdgeForTrigger || isRisingEdge[(int) signal]; +bool isUsefulSignal(trigger_event_e signal, const TriggerWaveform& shape) { + return !shape.useOnlyRisingEdges || isRisingEdge[(int) signal]; } #if EFI_UNIT_TEST @@ -77,7 +77,7 @@ void TriggerStimulatorHelper::feedSimulatedEvent( if (needEvent(stateIndex, multiChannelStateSequence, i)) { pin_state_t currentValue = multiChannelStateSequence.getChannelState(/*phaseIndex*/i, stateIndex); trigger_event_e event = (currentValue == TriggerValue::RISE ? riseEvents : fallEvents)[i]; - if (isUsefulSignal(event, triggerConfiguration)) { + if (isUsefulSignal(event, shape)) { state.decodeTriggerEvent( "sim", shape, diff --git a/firmware/controllers/trigger/trigger_simulator.h b/firmware/controllers/trigger/trigger_simulator.h index f7c0bce13a..893158ebf1 100644 --- a/firmware/controllers/trigger/trigger_simulator.h +++ b/firmware/controllers/trigger/trigger_simulator.h @@ -37,4 +37,4 @@ public: int i); }; -bool isUsefulSignal(trigger_event_e signal, const TriggerConfiguration& triggerConfiguration); +bool isUsefulSignal(trigger_event_e signal, const TriggerWaveform& shape); diff --git a/unit_tests/tests/ignition_injection/test_fuel_map.cpp b/unit_tests/tests/ignition_injection/test_fuel_map.cpp index d59e921ec7..a0a89d2d4c 100644 --- a/unit_tests/tests/ignition_injection/test_fuel_map.cpp +++ b/unit_tests/tests/ignition_injection/test_fuel_map.cpp @@ -86,7 +86,7 @@ TEST(misc, testFuelMap) { static void configureFordAspireTriggerWaveform(TriggerWaveform * s) { s->initialize(FOUR_STROKE_CAM_SENSOR, SyncEdge::Rise); - s->useOnlyRisingEdgeForTriggerTemp = false; + s->useOnlyRisingEdges = false; s->addEvent720(53.747, TriggerWheel::T_SECONDARY, TriggerValue::RISE); s->addEvent720(121.90, TriggerWheel::T_SECONDARY, TriggerValue::FALL); diff --git a/unit_tests/tests/trigger/test_trigger_decoder.cpp b/unit_tests/tests/trigger/test_trigger_decoder.cpp index 7cc51901f9..fc1ab2fa55 100644 --- a/unit_tests/tests/trigger/test_trigger_decoder.cpp +++ b/unit_tests/tests/trigger/test_trigger_decoder.cpp @@ -415,7 +415,7 @@ TEST(trigger, testTriggerDecoder) { EngineTestHelperBase base(&e, &c.engineConfiguration, &c); TriggerWaveform * s = &e.triggerCentral.triggerShape; - s->useOnlyRisingEdgeForTriggerTemp = false; + s->useOnlyRisingEdges = false; initializeSkippedToothTriggerWaveformExt(s, 2, 0, FOUR_STROKE_CAM_SENSOR); assertEqualsM("shape size", s->getSize(), 4); ASSERT_EQ(s->wave.getSwitchTime(0), 0.25); diff --git a/unit_tests/tests/trigger/test_trigger_multi_sync.cpp b/unit_tests/tests/trigger/test_trigger_multi_sync.cpp index a5dfc368ef..aa07ffd615 100644 --- a/unit_tests/tests/trigger/test_trigger_multi_sync.cpp +++ b/unit_tests/tests/trigger/test_trigger_multi_sync.cpp @@ -8,11 +8,11 @@ #include "pch.h" #include "trigger_mazda.h" -TEST(trigger, miataNA) { - TriggerWaveform naShape; - naShape.useOnlyRisingEdgeForTriggerTemp = false; - initializeMazdaMiataNaShape(&naShape); +// TEST(trigger, miataNA) { +// TriggerWaveform naShape; +// naShape.useOnlyRisingEdgeForTriggerTemp = false; +// initializeMazdaMiataNaShape(&naShape); - EngineTestHelper eth(FRANKENSO_MIATA_NA6_MAP); - // todo: https://github.com/rusefi/rusefi/issues/679 -} +// EngineTestHelper eth(FRANKENSO_MIATA_NA6_MAP); +// // todo: https://github.com/rusefi/rusefi/issues/679 +// }