extract isSyncPoint function (#3726)

* extract isSyncPoint

* test because why not

* check vvt resync nb2

* clean up noise from test case

* clarify and comment

firmware/controllers/trigger/trigger_decoder.cpp

@@ -554,17 +554,9 @@ void TriggerState::decodeTriggerEvent(
 		bool wasSynchronized = getShaftSynchronized();
 
 		if (triggerShape.isSynchronizationNeeded) {
-			currentGap = 1.0 * toothDurations[0] / toothDurations[1];
+			currentGap = (float)toothDurations[0] / toothDurations[1];
 
-			bool isSync = true;
+			isSynchronizationPoint = isSyncPoint(triggerShape, triggerConfiguration.TriggerType);
-			for (int i = 0; i < triggerShape.gapTrackingLength; i++) {
-				bool isGapCondition = cisnan(triggerShape.syncronizationRatioFrom[i]) || (toothDurations[i] > toothDurations[i + 1] * triggerShape.syncronizationRatioFrom[i]
-					&& toothDurations[i] < toothDurations[i + 1] * triggerShape.syncronizationRatioTo[i]);
-
-				isSync &= isGapCondition;
-			}
-
-			isSynchronizationPoint = isSync;
 			if (isSynchronizationPoint) {
 				enginePins.debugTriggerSync.toggle();
 			}
@@ -577,7 +569,7 @@ void TriggerState::decodeTriggerEvent(
 			bool silentTriggerError = triggerShape.getSize() > 40 && engineConfiguration->silentTriggerError;
 
 #if EFI_UNIT_TEST
-			actualSynchGap = 1.0 * toothDurations[0] / toothDurations[1];
+			actualSynchGap = currentGap;
 #endif /* EFI_UNIT_TEST */
 
 #if EFI_PROD_CODE || EFI_SIMULATOR
@@ -705,6 +697,32 @@ void TriggerState::decodeTriggerEvent(
 	}
 }
 
+bool TriggerState::isSyncPoint(const TriggerWaveform& triggerShape, trigger_type_e triggerType) const {
+	for (int i = 0; i < triggerShape.gapTrackingLength; i++) {
+		auto from = triggerShape.syncronizationRatioFrom[i];
+		auto to = triggerShape.syncronizationRatioTo[i];
+
+		if (cisnan(from)) {
+			// don't check this gap, skip it
+			continue;
+		}
+
+		// This is transformed to avoid a division and use a cheaper multiply instead
+		// toothDurations[i] / toothDurations[i+1] > from
+		// is an equivalent comparison to
+		// toothDurations[i] > toothDurations[i+1] * from
+		bool isGapCondition = 
+			  (toothDurations[i] > toothDurations[i + 1] * from
+			&& toothDurations[i] < toothDurations[i + 1] * to);
+
+		if (!isGapCondition) {
+			return false;
+		}
+	}
+
+	return true;
+}
+
 static void onFindIndexCallback(TriggerState *state) {
 	for (int i = 0; i < PWM_PHASE_MAX_WAVE_PER_PWM; i++) {
 		// todo: that's not the best place for this intermediate data storage, fix it!

firmware/controllers/trigger/trigger_decoder.h

@@ -157,6 +157,7 @@ public:
 
 private:
 	void resetCurrentCycleState();
+	bool isSyncPoint(const TriggerWaveform& triggerShape, trigger_type_e triggerType) const;
 
 	trigger_event_e curSignal;
 	trigger_event_e prevSignal;

firmware/util/efilib.h

@@ -160,6 +160,11 @@ constexpr void copyArrayPartial(TElement (&dest)[NDest], const TElement (&src)[N
 	}
 }
 
+template <typename T>
+bool isInRange(T min, T val, T max) {
+	return val >= min && val <= max;
+}
+
 #endif /* __cplusplus */
 
 #if defined(__cplusplus) && defined(__OPTIMIZE__)

unit_tests/tests/test_util.cpp

@@ -571,3 +571,11 @@ TEST(util, WrapAround62) {
 		EXPECT_EQ(t.update(0x03453455), 0x003453455LL);
 	}
 }
+
+TEST(util, isInRange) {
+	EXPECT_FALSE(isInRange(5, 4, 10));
+	EXPECT_TRUE(isInRange(5, 5, 10));
+	EXPECT_TRUE(isInRange(5, 7, 10));
+	EXPECT_TRUE(isInRange(5, 10, 10));
+	EXPECT_FALSE(isInRange(5, 11, 10));
+}

unit_tests/tests/trigger/resources/nb2-cranking-good.csv

@@ -481,10 +481,6 @@ Time [s],crank,cam
 6.940991417,0,1
 6.941894250,1,1
 6.959307333,0,1
-6.959354333,1,1
-6.959354417,0,1
-6.959354500,1,1
-6.959354750,0,1
 6.960309083,1,1
 6.985976917,0,1
 6.986747667,1,1
@@ -980,28 +976,3 @@ Time [s],crank,cam
 9.816113833,1,1
 9.829231167,0,1
 9.829947667,1,1
-9.850056500,0,1
-9.850069917,0,0
-9.850204250,0,1
-9.850220917,1,1
-9.850724083,0,1
-9.850747167,0,0
-9.850970833,0,1
-9.851064917,1,1
-9.853498417,0,1
-9.853601333,0,0
-9.853805167,0,1
-9.853806833,0,0
-9.853833167,0,1
-9.854069167,1,1
-9.854898833,0,1
-9.854956833,0,0
-9.855161333,0,1
-9.855163667,0,0
-9.855188083,0,1
-9.855204167,0,0
-9.855218417,0,1
-9.855245667,1,1
-9.858772667,0,1
-9.858911500,0,0
-11.237376000,0,0

unit_tests/tests/trigger/test_real_nb2_cranking.cpp

@@ -22,6 +22,10 @@ TEST(realCrankingNB2, normalCranking) {
 	// VVT position nearly zero!
 	EXPECT_NEAR(engine->triggerCentral.getVVTPosition(0, 0), 3.6569f, 1e-4);
 
+	// Check the number of times VVT information was used to adjust crank phase
+	// TODO: this should be wayyyyy fewer than 8!
+	EXPECT_EQ(engine->outputChannels.vvtSyncCounter, 8);
+
 	ASSERT_EQ(942, GET_RPM());
 
 	// TODO: why warnings?