Nb2 crank sync special case (#3727)

* extract isSyncPoint

* test because why not

* check vvt resync nb2

* custom nb decoder

* test only resyncs once!

* good job valgrind, you found a bug!

firmware/controllers/algo/engine.cpp

@@ -77,6 +77,8 @@ void Engine::resetEngineSnifferIfInTestMode() {
  */
 trigger_type_e getVvtTriggerType(vvt_mode_e vvtMode) {
 	switch (vvtMode) {
+	case VVT_INACTIVE:
+		return TT_ONE;
 	case VVT_2JZ:
 		return TT_VVT_JZ;
 	case VVT_MIATA_NB2:
@@ -105,28 +107,21 @@ trigger_type_e getVvtTriggerType(vvt_mode_e vvtMode) {
 
 static void initVvtShape(int camIndex, TriggerState &initState) {
 	vvt_mode_e vvtMode = engineConfiguration->vvtMode[camIndex];
-	TriggerWaveform *shape = &engine->triggerCentral.vvtShape[camIndex];
-
-	// not ideas but good for now code
-	engine->triggerCentral.vvtState[0][0].name = "vvt00";
-	engine->triggerCentral.vvtState[0][1].name = "vvt01";
-	engine->triggerCentral.vvtState[1][0].name = "vvt10";
-	engine->triggerCentral.vvtState[1][1].name = "vvt11";
 
 	if (vvtMode != VVT_INACTIVE) {
 		trigger_config_s config;
 		// todo: should 'vvtWithRealDecoder' be used here?
-		engine->triggerCentral.vvtTriggerType[camIndex] = config.type = getVvtTriggerType(vvtMode);
+		config.type = getVvtTriggerType(vvtMode);
 
-		shape->initializeTriggerWaveform(
+		auto& shape = engine->triggerCentral.vvtShape[camIndex];
+		shape.initializeTriggerWaveform(
 				engineConfiguration->ambiguousOperationMode,
 				engineConfiguration->vvtCamSensorUseRise, &config);
 
-		shape->initializeSyncPoint(initState,
+		shape.initializeSyncPoint(initState,
 				engine->vvtTriggerConfiguration[camIndex],
 				config);
 	}
-
 }
 
 void Engine::initializeTriggerWaveform() {
@@ -175,12 +170,15 @@ void Engine::initializeTriggerWaveform() {
 		engine->engineCycleEventCount = TRIGGER_WAVEFORM(getLength());
 	}
 
+	engine->triggerCentral.vvtState[0][0].name = "VVT B1 Int";
+	engine->triggerCentral.vvtState[0][1].name = "VVT B1 Exh";
+	engine->triggerCentral.vvtState[1][0].name = "VVT B2 Int";
+	engine->triggerCentral.vvtState[1][1].name = "VVT B2 Exh";
 
 	for (int camIndex = 0;camIndex < CAMS_PER_BANK;camIndex++) {
 		initVvtShape(camIndex, initState);
 	}
 
-
 	if (!TRIGGER_WAVEFORM(shapeDefinitionError)) {
 		prepareOutputSignals();
 	}

firmware/controllers/algo/engine.h

@@ -464,6 +464,8 @@ private:
 	void injectEngineReferences();
 };
 
+trigger_type_e getVvtTriggerType(vvt_mode_e vvtMode);
+
 void prepareShapes();
 void applyNonPersistentConfiguration();
 void prepareOutputSignals();

firmware/controllers/algo/engine2.cpp

@@ -279,7 +279,8 @@ bool VvtTriggerConfiguration::isUseOnlyRisingEdgeForTrigger() const {
 }
 
 trigger_type_e VvtTriggerConfiguration::getType() const {
-	return engine->triggerCentral.vvtTriggerType[index];
+	// Convert from VVT type to trigger type
+	return getVvtTriggerType(engineConfiguration->vvtMode[index]);
 }
 
 bool VvtTriggerConfiguration::isVerboseTriggerSynchDetails() const {

firmware/controllers/trigger/decoders/trigger_mazda.cpp

@@ -71,9 +71,9 @@ void initializeMazdaMiataNb2Crank(TriggerWaveform *s) {
 	s->tdcPosition = 60 + 655;
 
 	// Nominal gap 70/110 = 0.636
-	s->setTriggerSynchronizationGap2(0.35f, 0.98f);
+	s->setTriggerSynchronizationGap2(0.35f, 1.15f);
 	// Nominal gap 110/70 = 1.571
-	s->setSecondTriggerSynchronizationGap2(1.02f, 1.8f);
+	s->setSecondTriggerSynchronizationGap2(0.8f, 1.8f);
 
 	// todo: NB2 fronts are inverted comparing to NB1, life is not perfect :(
 	s->addEventAngle(180.0f - NB_CRANK_MAGIC - 4, T_PRIMARY, TV_FALL);

firmware/controllers/trigger/trigger_central.h

@@ -82,7 +82,6 @@ public:
 	int vvtEventRiseCounter[CAM_INPUTS_COUNT];
 	int vvtEventFallCounter[CAM_INPUTS_COUNT];
 
-	trigger_type_e vvtTriggerType[CAMS_PER_BANK];
 	angle_t getVVTPosition(uint8_t bankIndex, uint8_t camIndex);
 
 #if EFI_UNIT_TEST

firmware/controllers/trigger/trigger_decoder.cpp

@@ -704,6 +704,32 @@ void TriggerState::decodeTriggerEvent(
 }
 
 bool TriggerState::isSyncPoint(const TriggerWaveform& triggerShape, trigger_type_e triggerType) const {
+	// Miata NB needs a special decoder.
+	// The problem is that the crank wheel only has 4 teeth, also symmetrical, so the pattern
+	// is long-short-long-short for one crank rotation.
+	// A quick acceleration can result in two successive "short gaps", so we see 
+	// long-short-short-short-long instead of the correct long-short-long-short-long
+	// This logic expands the lower bound on a "long" tooth, then compares the last
+	// tooth to the current one.
+
+	// Instead of detecting short/long, this logic first checks for "maybe short" and "maybe long",
+	// then simply tests longer vs. shorter instead of absolute value.
+	if (triggerType == TT_MIATA_VVT) {
+		auto secondGap = (float)toothDurations[1] / toothDurations[2];
+
+		bool currentGapOk = isInRange(triggerShape.syncronizationRatioFrom[0], currentGap, triggerShape.syncronizationRatioTo[0]);
+		bool secondGapOk  = isInRange(triggerShape.syncronizationRatioFrom[1], secondGap,  triggerShape.syncronizationRatioTo[1]);
+
+		// One or both teeth was impossible range, this is not the sync point
+		if (!currentGapOk || !secondGapOk) {
+			return false;
+		}
+
+		// If both teeth are in the range of possibility, return whether this gap is
+		// shorter than the last or not.  If it is, this is the sync point.
+		return currentGap < secondGap;
+	}
+
 	for (int i = 0; i < triggerShape.gapTrackingLength; i++) {
 		auto from = triggerShape.syncronizationRatioFrom[i];
 		auto to = triggerShape.syncronizationRatioTo[i];

unit_tests/tests/trigger/test_real_nb2_cranking.cpp

@@ -23,8 +23,8 @@ TEST(realCrankingNB2, normalCranking) {
 	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);
+	// This should happen exactly once: once we sync, we shouldn't lose it.
+	EXPECT_EQ(engine->outputChannels.vvtSyncCounter, 1);
 
 	ASSERT_EQ(942, GET_RPM());