only report vvt position if we have full sync (#4547)

* only report vvt position if we have full sync

* comments

* make it selectable, since that makes the test useful

firmware/controllers/trigger/trigger_central.cpp

@@ -356,13 +356,8 @@ void hwHandleVvtCamSignal(trigger_value_e front, efitick_t nowNt, int index) {
 
 	auto vvtPosition = engineConfiguration->vvtOffsets[bankIndex * CAMS_PER_BANK + camIndex] - currentPosition;
 
-	if (index != 0) {
-		// todo: only assign initial position of not first cam once cam was synchronized
-		tc->vvtPosition[bankIndex][camIndex] = wrapVvt(vvtPosition, FOUR_STROKE_CYCLE_DURATION);
-		// at the moment we use only primary VVT to sync crank phase
-		return;
-	}
-
+	// Only do engine sync using one cam, other cams just provide VVT position.
+	if (index == engineConfiguration->engineSyncCam) {
 		angle_t crankOffset = adjustCrankPhase(camIndex);
 		// vvtPosition was calculated against wrong crank zero position. Now that we have adjusted crank position we
 		// shall adjust vvt position as well
@@ -388,8 +383,17 @@ void hwHandleVvtCamSignal(trigger_value_e front, efitick_t nowNt, int index) {
 			 */
 			warning(CUSTOM_VVT_SYNC_POSITION, "VVT sync position too close to trigger sync");
 		}
+	} else {
+		// Not using this cam for engine sync, just wrap the value in to the reasonable range
+		vvtPosition = wrapVvt(vvtPosition, FOUR_STROKE_CYCLE_DURATION);
+	}
 
+	// Only record VVT position if we have full engine sync - may be bogus before that point
+	if (tc->triggerState.hasSynchronizedPhase()) {
 		tc->vvtPosition[bankIndex][camIndex] = vvtPosition;
+	} else {
+		tc->vvtPosition[bankIndex][camIndex] = 0;
+	}
 }
 
 int triggerReentrant = 0;

firmware/integration/rusefi_config.txt

@@ -645,7 +645,7 @@ end_struct
 
 uint8_t autoscale knockRetardAggression;+Ignition timing to remove when a knock event occurs.;"%", 0.1, 0, 0, 20, 1
 uint8_t autoscale knockRetardReapplyRate;+After a knock event, reapply timing at this rate.;"deg/s", 0.1, 0, 0, 10, 1
-uint8_t unused556;;"", 1, 0, 0, 1, 0
+uint8_t engineSyncCam;Select which cam is used for engine sync. Other cams will be used only for VVT measurement, but not engine sync.;"", 1, 0, 0, 3, 0
 
 	uint8_t vssFilterReciprocal;Set this so your vehicle speed signal is responsive, but not noisy. Larger value give smoother but slower response.;"", 1, 0, 2, 200, 0
 

firmware/tunerstudio/rusefi.input

@@ -2038,6 +2038,7 @@ cmd_set_engine_type_default					= "@@TS_IO_TEST_COMMAND_char@@@@ts_command_e_TS_
 		field = "VVT offset bank 2 exhaust",			vvtOffsets4, { camInputs4 != 0 }
 		field = "Require cam/VVT sync for ignition",	isPhaseSyncRequiredForIgnition
 		field = "Maximum cam/VVT sync RPM",				maxCamPhaseResolveRpm
+		field = "Cam for engine sync resolution",		engineSyncCam
 	    field = "Print verbose VVT sync details to console",verboseVVTDecoding
 	    field = "Print verbose trigger sync to console",   verboseTriggerSynchDetails
 	    field = "Do not print messages in case of sync error",  silentTriggerError

unit_tests/tests/trigger/test_real_cranking_nissan_vq40.cpp

@@ -8,7 +8,7 @@
 #include "pch.h"
 #include "logicdata_csv_reader.h"
 
-TEST(realCrankingVQ40, normalCranking) {
+static void test(int engineSyncCam, float camOffsetAdd) {
 	CsvReader reader(1, /* vvtCount */ 2);
 	int indeces[] = {0};
 
@@ -17,16 +17,32 @@ TEST(realCrankingVQ40, normalCranking) {
 	engineConfiguration->isFasterEngineSpinUpEnabled = false;
 	engineConfiguration->alwaysInstantRpm = true;
 
+	// Different sync cam may result in different TDC point, so we might need different cam offsets.
+	engineConfiguration->vvtOffsets[0] += camOffsetAdd;
+	engineConfiguration->vvtOffsets[2] += camOffsetAdd;
+	engineConfiguration->engineSyncCam = engineSyncCam;
+
 	bool hasSeenFirstVvt = false;
 
 	while (reader.haveMore()) {
 		reader.processLine(&eth);
 		float vvt1 = engine->triggerCentral.getVVTPosition(/*bankIndex*/0, /*camIndex*/0);
+		float vvt2 = engine->triggerCentral.getVVTPosition(/*bankIndex*/1, /*camIndex*/0);
 
-		if (vvt1 != 0 && !hasSeenFirstVvt) {
+		if (vvt1 != 0) {
+			if (!hasSeenFirstVvt) {
 				EXPECT_NEAR(vvt1, -45.56, 1);
 				hasSeenFirstVvt = true;
 			}
+
+			// cam position should never be reported outside of correct range
+			EXPECT_TRUE(vvt1 > -48 && vvt1 < -43);
+		}
+
+		if (vvt2 != 0) {
+			// cam position should never be reported outside of correct range
+			EXPECT_TRUE(vvt2 > -48 && vvt2 < -43);
+		}
 	}
 
 	EXPECT_NEAR(engine->triggerCentral.getVVTPosition(/*bankIndex*/0, /*camIndex*/0), -45.64, 1e-2);
@@ -38,3 +54,13 @@ TEST(realCrankingVQ40, normalCranking) {
 	ASSERT_EQ(CUSTOM_OUT_OF_ORDER_COIL, eth.recentWarnings()->get(0).Code);	// this is from a coil being protected by overdwell protection
 	ASSERT_EQ(CUSTOM_PRIMARY_TOO_MANY_TEETH, eth.recentWarnings()->get(1).Code);
 }
+
+// On Nissan VQ, all cams have the same pattern, so all should be equally good for engine sync. Check them all!
+
+TEST(realCrankingVQ40, normalCrankingSyncCam1) {
+	test(0, 0);
+}
+
+TEST(realCrankingVQ40, normalCrankingSyncCam2) {
+	test(2, -360);
+}