From 715c3efb186fd0fc0645a2c9f997ef2c6568a2e4 Mon Sep 17 00:00:00 2001 From: Matthew Kennedy Date: Thu, 8 Sep 2022 13:15:36 -0700 Subject: [PATCH] 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 --- .../controllers/trigger/trigger_central.cpp | 66 ++++++++++--------- firmware/integration/rusefi_config.txt | 2 +- firmware/tunerstudio/rusefi.input | 1 + .../test_real_cranking_nissan_vq40.cpp | 34 ++++++++-- 4 files changed, 67 insertions(+), 36 deletions(-) diff --git a/firmware/controllers/trigger/trigger_central.cpp b/firmware/controllers/trigger/trigger_central.cpp index 55f625233a..96fad0b10c 100644 --- a/firmware/controllers/trigger/trigger_central.cpp +++ b/firmware/controllers/trigger/trigger_central.cpp @@ -356,40 +356,44 @@ 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 + vvtPosition -= crankOffset; + vvtPosition = wrapVvt(vvtPosition, FOUR_STROKE_CYCLE_DURATION); + + // this could be just an 'if' but let's have it expandable for future use :) + switch(engineConfiguration->vvtMode[camIndex]) { + case VVT_HONDA_K: + // honda K has four tooth in VVT intake trigger, so we just wrap each of those to 720 / 4 + vvtPosition = wrapVvt(vvtPosition, 180); + break; + default: + // else, do nothing + break; + } + + if (absF(angleFromPrimarySyncPoint) < 7) { + /** + * we prefer not to have VVT sync right at trigger sync so that we do not have phase detection error if things happen a bit in + * wrong order due to belt flex or else + * https://github.com/rusefi/rusefi/issues/3269 + */ + 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); } - 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 - vvtPosition -= crankOffset; - vvtPosition = wrapVvt(vvtPosition, FOUR_STROKE_CYCLE_DURATION); - - // this could be just an 'if' but let's have it expandable for future use :) - switch(engineConfiguration->vvtMode[camIndex]) { - case VVT_HONDA_K: - // honda K has four tooth in VVT intake trigger, so we just wrap each of those to 720 / 4 - vvtPosition = wrapVvt(vvtPosition, 180); - break; - default: - // else, do nothing - break; + // 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; } - - if (absF(angleFromPrimarySyncPoint) < 7) { - /** - * we prefer not to have VVT sync right at trigger sync so that we do not have phase detection error if things happen a bit in - * wrong order due to belt flex or else - * https://github.com/rusefi/rusefi/issues/3269 - */ - warning(CUSTOM_VVT_SYNC_POSITION, "VVT sync position too close to trigger sync"); - } - - tc->vvtPosition[bankIndex][camIndex] = vvtPosition; } int triggerReentrant = 0; diff --git a/firmware/integration/rusefi_config.txt b/firmware/integration/rusefi_config.txt index d2f5a34f9b..c29c95c88f 100644 --- a/firmware/integration/rusefi_config.txt +++ b/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 diff --git a/firmware/tunerstudio/rusefi.input b/firmware/tunerstudio/rusefi.input index 7eb0db8a62..343d7e8038 100644 --- a/firmware/tunerstudio/rusefi.input +++ b/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 diff --git a/unit_tests/tests/trigger/test_real_cranking_nissan_vq40.cpp b/unit_tests/tests/trigger/test_real_cranking_nissan_vq40.cpp index 4a3dcb3a51..72ca891eff 100644 --- a/unit_tests/tests/trigger/test_real_cranking_nissan_vq40.cpp +++ b/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,15 +17,31 @@ 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(ð); float vvt1 = engine->triggerCentral.getVVTPosition(/*bankIndex*/0, /*camIndex*/0); + float vvt2 = engine->triggerCentral.getVVTPosition(/*bankIndex*/1, /*camIndex*/0); - if (vvt1 != 0 && !hasSeenFirstVvt) { - EXPECT_NEAR(vvt1, -45.56, 1); - hasSeenFirstVvt = true; + 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); } } @@ -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); +}