From e801ffc97987bde124c9452bd7c12027e51df5b3 Mon Sep 17 00:00:00 2001 From: Matthew Kennedy Date: Mon, 7 Nov 2022 07:52:17 -0800 Subject: [PATCH] fix vvt logging logic (#4747) * fix vvt logging logic * put that back * flip condition to avoid branch on uninitialized value --- .../controllers/trigger/trigger_central.cpp | 62 ++++++++----------- .../tests/trigger/test_cam_vvt_input.cpp | 4 +- .../tests/trigger/test_real_nb2_cranking.cpp | 8 +-- 3 files changed, 33 insertions(+), 41 deletions(-) diff --git a/firmware/controllers/trigger/trigger_central.cpp b/firmware/controllers/trigger/trigger_central.cpp index b0c78809e6..3472f3deae 100644 --- a/firmware/controllers/trigger/trigger_central.cpp +++ b/firmware/controllers/trigger/trigger_central.cpp @@ -212,7 +212,7 @@ static angle_t wrapVvt(angle_t vvtPosition, int period) { return vvtPosition; } -static void logFront(bool isImportantFront, efitick_t nowNt, int index) { +static void logVvtFront(bool useOnlyRise, bool isImportantFront, TriggerValue front, efitick_t nowNt, int index) { if (isImportantFront && isBrainPinValid(engineConfiguration->camInputsDebug[index])) { #if EFI_PROD_CODE writePad("cam debug", engineConfiguration->camInputsDebug[index], 1); @@ -220,24 +220,23 @@ static void logFront(bool isImportantFront, efitick_t nowNt, int index) { getExecutorInterface()->scheduleByTimestampNt("dbg_on", &debugToggleScheduling, nowNt + DEBUG_PIN_DELAY, &turnOffAllDebugFields); } - if (engineConfiguration->displayLogicLevelsInEngineSniffer && isImportantFront) { - if (engineConfiguration->vvtCamSensorUseRise) { - // todo: unify TS composite logger code with console Engine Sniffer - // todo: better API to reduce copy/paste? -#if EFI_TOOTH_LOGGER - LogTriggerTooth(SHAFT_SECONDARY_RISING, nowNt); - LogTriggerTooth(SHAFT_SECONDARY_FALLING, nowNt); -#endif /* EFI_TOOTH_LOGGER */ - addEngineSnifferVvtEvent(index, FrontDirection::UP); - addEngineSnifferVvtEvent(index, FrontDirection::DOWN); - } else { -#if EFI_TOOTH_LOGGER - LogTriggerTooth(SHAFT_SECONDARY_FALLING, nowNt); - LogTriggerTooth(SHAFT_SECONDARY_RISING, nowNt); -#endif /* EFI_TOOTH_LOGGER */ + if (!useOnlyRise || engineConfiguration->displayLogicLevelsInEngineSniffer) { + // If we care about both edges OR displayLogicLevel is set, log every front exactly as it is + addEngineSnifferVvtEvent(index, front == TriggerValue::RISE ? FrontDirection::UP : FrontDirection::DOWN); - addEngineSnifferVvtEvent(index, FrontDirection::DOWN); +#if EFI_TOOTH_LOGGER + LogTriggerTooth(front == TriggerValue::RISE ? SHAFT_SECONDARY_RISING : SHAFT_SECONDARY_FALLING, nowNt); +#endif /* EFI_TOOTH_LOGGER */ + } else { + if (isImportantFront) { + // On the important edge, log a rise+fall pair, and nothing on the real falling edge addEngineSnifferVvtEvent(index, FrontDirection::UP); + addEngineSnifferVvtEvent(index, FrontDirection::DOWN); + +#if EFI_TOOTH_LOGGER + LogTriggerTooth(SHAFT_SECONDARY_RISING, nowNt); + LogTriggerTooth(SHAFT_SECONDARY_FALLING, nowNt); +#endif /* EFI_TOOTH_LOGGER */ } } } @@ -266,7 +265,6 @@ void hwHandleVvtCamSignal(TriggerValue front, efitick_t nowNt, int index) { extern ioportid_t criticalErrorLedPort; extern ioportmask_t criticalErrorLedPin; - for (int i = 0 ; i < 100 ; i++) { // turning pin ON and busy-waiting a bit palWritePad(criticalErrorLedPort, criticalErrorLedPin, 1); @@ -275,27 +273,21 @@ void hwHandleVvtCamSignal(TriggerValue front, efitick_t nowNt, int index) { palWritePad(criticalErrorLedPort, criticalErrorLedPin, 0); #endif // VR_HW_CHECK_MODE - if (!engineConfiguration->displayLogicLevelsInEngineSniffer) { - // todo: migrate injector_pressure_type_e to class enum, maybe merge with FrontDirection? - addEngineSnifferVvtEvent(index, front == TriggerValue::RISE ? FrontDirection::UP : FrontDirection::DOWN); + const auto& vvtShape = tc->vvtShape[camIndex]; -#if EFI_TOOTH_LOGGER -// todo: we need to start logging different VVT channels differently!!! - trigger_event_e tooth = front == TriggerValue::RISE ? SHAFT_SECONDARY_RISING : SHAFT_SECONDARY_FALLING; - - LogTriggerTooth(tooth, nowNt); -#endif /* EFI_TOOTH_LOGGER */ - } - - bool isImportantFront = (engineConfiguration->vvtCamSensorUseRise ^ (front == TriggerValue::FALL)); bool isVvtWithRealDecoder = vvtWithRealDecoder(engineConfiguration->vvtMode[camIndex]); - if (!isVvtWithRealDecoder && !isImportantFront) { - // todo: there should be a way to always use real trigger code for this logic? + + // Non real decoders only use the rising edge + bool vvtUseOnlyRise = !isVvtWithRealDecoder || vvtShape.useOnlyRisingEdges; + bool isImportantFront = !vvtUseOnlyRise || (front == TriggerValue::RISE); + + logVvtFront(vvtUseOnlyRise, isImportantFront, front, nowNt, index); + + if (!isImportantFront) { + // This edge is unimportant, ignore it. return; } - logFront(isImportantFront, nowNt, index); - // If the main trigger is not synchronized, don't decode VVT yet if (!tc->triggerState.getShaftSynchronized()) { return; @@ -306,7 +298,7 @@ void hwHandleVvtCamSignal(TriggerValue front, efitick_t nowNt, int index) { if (isVvtWithRealDecoder) { vvtDecoder.decodeTriggerEvent( "vvt", - tc->vvtShape[camIndex], + vvtShape, nullptr, tc->vvtTriggerConfiguration[camIndex], front == TriggerValue::RISE ? SHAFT_PRIMARY_RISING : SHAFT_PRIMARY_FALLING, nowNt); diff --git a/unit_tests/tests/trigger/test_cam_vvt_input.cpp b/unit_tests/tests/trigger/test_cam_vvt_input.cpp index 1a479cb2a3..335c0af612 100644 --- a/unit_tests/tests/trigger/test_cam_vvt_input.cpp +++ b/unit_tests/tests/trigger/test_cam_vvt_input.cpp @@ -110,7 +110,7 @@ TEST(trigger, testCamInput) { for (int i = 0; i < 600;i++) { eth.moveTimeForwardUs(MS2US(10)); - hwHandleVvtCamSignal(TriggerValue::FALL, getTimeNowNt(), 0); + hwHandleVvtCamSignal(TriggerValue::RISE, getTimeNowNt(), 0); eth.moveTimeForwardUs(MS2US(40)); eth.firePrimaryTriggerRise(); } @@ -177,5 +177,5 @@ TEST(trigger, testNB2CamInput) { // actually position based on VVT! ASSERT_EQ(totalRevolutionCountBeforeVvtSync + 3, engine->triggerCentral.triggerState.getCrankSynchronizationCounter()); - EXPECT_EQ(40, waveChart.getSize()); + EXPECT_EQ(39, waveChart.getSize()); } diff --git a/unit_tests/tests/trigger/test_real_nb2_cranking.cpp b/unit_tests/tests/trigger/test_real_nb2_cranking.cpp index b6c3b5b6f8..2a56209815 100644 --- a/unit_tests/tests/trigger/test_real_nb2_cranking.cpp +++ b/unit_tests/tests/trigger/test_real_nb2_cranking.cpp @@ -30,8 +30,8 @@ TEST(realCrankingNB2, normalCranking) { EXPECT_EQ(3, eth.recentWarnings()->getCount()); EXPECT_EQ(CUSTOM_OUT_OF_ORDER_COIL, eth.recentWarnings()->get(0).Code); - EXPECT_EQ(CUSTOM_CAM_TOO_MANY_TEETH, eth.recentWarnings()->get(1).Code); - EXPECT_EQ(CUSTOM_PRIMARY_NOT_ENOUGH_TEETH, eth.recentWarnings()->get(2).Code); + EXPECT_EQ(CUSTOM_PRIMARY_NOT_ENOUGH_TEETH, eth.recentWarnings()->get(1).Code); + EXPECT_EQ(CUSTOM_CAM_TOO_MANY_TEETH, eth.recentWarnings()->get(2).Code); } TEST(realCrankingNB2, crankingMissingInjector) { @@ -52,7 +52,7 @@ TEST(realCrankingNB2, crankingMissingInjector) { EXPECT_EQ(4, eth.recentWarnings()->getCount()); EXPECT_EQ(CUSTOM_OUT_OF_ORDER_COIL, eth.recentWarnings()->get(0).Code); - EXPECT_EQ(CUSTOM_CAM_TOO_MANY_TEETH, eth.recentWarnings()->get(1).Code); - EXPECT_EQ(CUSTOM_PRIMARY_NOT_ENOUGH_TEETH, eth.recentWarnings()->get(2).Code); + EXPECT_EQ(CUSTOM_PRIMARY_NOT_ENOUGH_TEETH, eth.recentWarnings()->get(1).Code); + EXPECT_EQ(CUSTOM_CAM_TOO_MANY_TEETH, eth.recentWarnings()->get(2).Code); EXPECT_EQ(CUSTOM_PRIMARY_TOO_MANY_TEETH, eth.recentWarnings()->get(3).Code); }