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

@@ -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

firmware/controllers/trigger/decoders/sync_edge.h

@@ -1,7 +1,8 @@
 #pragma once
 
 enum class SyncEdge : uint8_t {
-	Rise,
+	Rise,		// Sync on rising edges, use rise+fall for phase info
-	Fall,
+	Fall,		// Sync on falling edges, use rise+fall for phase info
-	Both
+	RiseOnly,	// Completely ignore all falling edges (this used to be useOnlyRisingEdgeForTrigger=true etc)
+	Both		// Sync on both edges, use both for phase
 };

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) {
+void TriggerWaveform::calculateExpectedEventCounts() {
-	if (!useOnlyRisingEdgeForTrigger) {
+	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->useOnlyRisingEdges = triggerConfig.UseOnlyRisingEdgeForTrigger;
-	this->useOnlyRisingEdgeForTriggerTemp = 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

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

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);
 	}
 }
 

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<TriggerDecodeResult> 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<TriggerDecodeResult> 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);
 

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) {
+bool isUsefulSignal(trigger_event_e signal, const TriggerWaveform& shape) {
-	return !triggerConfiguration.UseOnlyRisingEdgeForTrigger || isRisingEdge[(int) signal];
+	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,

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);

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);

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);

unit_tests/tests/trigger/test_trigger_multi_sync.cpp

@@ -8,11 +8,11 @@
 #include "pch.h"
 #include "trigger_mazda.h"
 
-TEST(trigger, miataNA) {
+// TEST(trigger, miataNA) {
-	TriggerWaveform naShape;
+// 	TriggerWaveform naShape;
-	naShape.useOnlyRisingEdgeForTriggerTemp = false;
+// 	naShape.useOnlyRisingEdgeForTriggerTemp = false;
-	initializeMazdaMiataNaShape(&naShape);
+// 	initializeMazdaMiataNaShape(&naShape);
 
-	EngineTestHelper eth(FRANKENSO_MIATA_NA6_MAP);
+// 	EngineTestHelper eth(FRANKENSO_MIATA_NA6_MAP);
-	// todo: https://github.com/rusefi/rusefi/issues/679
+// 	// todo: https://github.com/rusefi/rusefi/issues/679
-}
+// }