From ec043ad81fc47c379a02c524e9b39243f4e22a1d Mon Sep 17 00:00:00 2001 From: Matthew Kennedy Date: Thu, 12 Sep 2024 21:25:18 -0700 Subject: [PATCH] getVVTPosition returns expected<> --- .../engine_cycle/high_pressure_fuel_pump.cpp | 2 +- .../controllers/trigger/trigger_central.cpp | 14 ++++++++--- .../controllers/trigger/trigger_central.h | 6 ++++- unit_tests/tests/actuators/test_vvt.cpp | 3 ++- unit_tests/tests/test_hpfp.cpp | 12 ++++++---- .../tests/trigger/test_cam_vvt_input.cpp | 6 ++--- .../tests/trigger/test_nissan_vq_vvt.cpp | 6 ++--- unit_tests/tests/trigger/test_quad_cam.cpp | 24 +++++++++---------- .../tests/trigger/test_real_cas_24_plus_1.cpp | 6 ++--- .../test_real_cranking_nissan_vq40.cpp | 18 +++++++------- unit_tests/tests/trigger/test_real_k20.cpp | 6 ++--- .../tests/trigger/test_real_nb2_cranking.cpp | 10 ++++---- .../tests/trigger/test_toyota_3_tooth_cam.cpp | 14 +++++------ 13 files changed, 73 insertions(+), 54 deletions(-) diff --git a/firmware/controllers/engine_cycle/high_pressure_fuel_pump.cpp b/firmware/controllers/engine_cycle/high_pressure_fuel_pump.cpp index 52f92372e1..4473bbd572 100644 --- a/firmware/controllers/engine_cycle/high_pressure_fuel_pump.cpp +++ b/firmware/controllers/engine_cycle/high_pressure_fuel_pump.cpp @@ -61,7 +61,7 @@ angle_t HpfpLobe::findNextLobe() { // TODO: Is the sign correct here? + means ATDC? vvt = engine->triggerCentral.getVVTPosition( (engineConfiguration->hpfpCam - 1) / 2 & 1, // Bank - (engineConfiguration->hpfpCam - 1) & 1); // Cam + (engineConfiguration->hpfpCam - 1) & 1).value_or(0); // Cam } return engineConfiguration->hpfpPeakPos + vvt + next_index * 720 / lobes; diff --git a/firmware/controllers/trigger/trigger_central.cpp b/firmware/controllers/trigger/trigger_central.cpp index c9da673932..32b785b232 100644 --- a/firmware/controllers/trigger/trigger_central.cpp +++ b/firmware/controllers/trigger/trigger_central.cpp @@ -56,8 +56,14 @@ expected TriggerCentral::getVVTPosition(uint8_t bankIndex, uint8_t camI } // TODO: return unexpected if timed out + auto& vvt = vvtPosition[bankIndex][camIndex]; - return vvtPosition[bankIndex][camIndex]; + if (vvt.t.hasElapsedSec(1)) { + // 1s timeout on VVT angle + return unexpected; + } else { + return vvt.angle; + } } /** @@ -341,11 +347,13 @@ void hwHandleVvtCamSignal(bool isRising, efitick_t nowNt, int index) { } // Only record VVT position if we have full engine sync - may be bogus before that point + auto& vvtPos = tc->vvtPosition[bankIndex][camIndex]; if (tc->triggerState.hasSynchronizedPhase()) { - tc->vvtPosition[bankIndex][camIndex] = vvtPosition; + vvtPos.angle = vvtPosition; } else { - tc->vvtPosition[bankIndex][camIndex] = 0; + vvtPos.angle = 0; } + vvtPos.t.reset(nowNt); } int triggerReentrant = 0; diff --git a/firmware/controllers/trigger/trigger_central.h b/firmware/controllers/trigger/trigger_central.h index 80397d1a34..06bd356e94 100644 --- a/firmware/controllers/trigger/trigger_central.h +++ b/firmware/controllers/trigger/trigger_central.h @@ -145,7 +145,11 @@ class TriggerCentral final : public trigger_central_s { #endif // EFI_UNIT_TEST // synchronization event position - angle_t vvtPosition[BANKS_COUNT][CAMS_PER_BANK]; + struct VvtPos { + angle_t angle; + Timer t; + }; + VvtPos vvtPosition[BANKS_COUNT][CAMS_PER_BANK]; #if EFI_SHAFT_POSITION_INPUT PrimaryTriggerDecoder triggerState; diff --git a/unit_tests/tests/actuators/test_vvt.cpp b/unit_tests/tests/actuators/test_vvt.cpp index 776dd40b49..abab2f2e9d 100644 --- a/unit_tests/tests/actuators/test_vvt.cpp +++ b/unit_tests/tests/actuators/test_vvt.cpp @@ -27,7 +27,8 @@ TEST(Vvt, TestSetPoint) { TEST(Vvt, observePlant) { EngineTestHelper eth(engine_type_e::TEST_ENGINE); - engine->triggerCentral.vvtPosition[0][0] = 23; + engine->triggerCentral.vvtPosition[0][0].t.reset(); + engine->triggerCentral.vvtPosition[0][0].angle = 23; VvtController dut(0, 0, 0); dut.init(nullptr, nullptr); diff --git a/unit_tests/tests/test_hpfp.cpp b/unit_tests/tests/test_hpfp.cpp index 2a2e35ffcc..23bbdbe16a 100755 --- a/unit_tests/tests/test_hpfp.cpp +++ b/unit_tests/tests/test_hpfp.cpp @@ -13,10 +13,14 @@ TEST(HPFP, Lobe) { engineConfiguration->hpfpPeakPos = 123; engineConfiguration->hpfpCamLobes = 3; - engine->triggerCentral.vvtPosition[0][0] = 20; // Bank 0 - engine->triggerCentral.vvtPosition[0][1] = 40; - engine->triggerCentral.vvtPosition[1][0] = 60; // Bank 1 - engine->triggerCentral.vvtPosition[1][1] = 80; + engine->triggerCentral.vvtPosition[0][0].angle = 20; // Bank 0 + engine->triggerCentral.vvtPosition[0][0].t.reset(); + engine->triggerCentral.vvtPosition[0][1].angle = 40; + engine->triggerCentral.vvtPosition[0][1].t.reset(); + engine->triggerCentral.vvtPosition[1][0].angle = 60; // Bank 1 + engine->triggerCentral.vvtPosition[1][0].t.reset(); + engine->triggerCentral.vvtPosition[1][1].angle = 80; + engine->triggerCentral.vvtPosition[1][1].t.reset(); HpfpLobe lobe; diff --git a/unit_tests/tests/trigger/test_cam_vvt_input.cpp b/unit_tests/tests/trigger/test_cam_vvt_input.cpp index a5a9a2f922..3e0e933f75 100644 --- a/unit_tests/tests/trigger/test_cam_vvt_input.cpp +++ b/unit_tests/tests/trigger/test_cam_vvt_input.cpp @@ -126,7 +126,7 @@ TEST(trigger, testCamInput) { // asserting that error code has cleared ASSERT_EQ(0, unitTestWarningCodeState.recentWarnings.getCount()) << "warningCounter#testCamInput #3"; - EXPECT_NEAR_M3(71, engine->triggerCentral.getVVTPosition(0, 0)); + EXPECT_NEAR_M3(71, engine->triggerCentral.getVVTPosition(0, 0).value_or(0)); } TEST(trigger, testNB2CamInput) { @@ -170,7 +170,7 @@ TEST(trigger, testNB2CamInput) { eth.moveTimeForwardUs(MS2US(10)); hwHandleVvtCamSignal(true, getTimeNowNt(), 0); - ASSERT_FLOAT_EQ(0, engine->triggerCentral.getVVTPosition(0, 0)); + ASSERT_FALSE(engine->triggerCentral.getVVTPosition(0, 0)); ASSERT_EQ(totalRevolutionCountBeforeVvtSync, engine->triggerCentral.triggerState.getCrankSynchronizationCounter()); // Third gap - long @@ -180,7 +180,7 @@ TEST(trigger, testNB2CamInput) { eth.moveTimeForwardUs(MS2US( 30)); hwHandleVvtCamSignal(true, getTimeNowNt(), 0); - EXPECT_NEAR(297.5f, engine->triggerCentral.getVVTPosition(0, 0), EPS2D); + EXPECT_NEAR(297.5f, engine->triggerCentral.getVVTPosition(0, 0).value_or(0), EPS2D); // actually position based on VVT! ASSERT_EQ(totalRevolutionCountBeforeVvtSync + 3, engine->triggerCentral.triggerState.getCrankSynchronizationCounter()); diff --git a/unit_tests/tests/trigger/test_nissan_vq_vvt.cpp b/unit_tests/tests/trigger/test_nissan_vq_vvt.cpp index af8a8359ba..ef3be1ffeb 100644 --- a/unit_tests/tests/trigger/test_nissan_vq_vvt.cpp +++ b/unit_tests/tests/trigger/test_nissan_vq_vvt.cpp @@ -133,15 +133,15 @@ TEST(nissan, vq_vvt) { ASSERT_TRUE(tc->vvtState[0][0].getShaftSynchronized()); // let's celebrate that vvtPosition stays the same - ASSERT_NEAR(34, tc->vvtPosition[0][0], EPS2D) << "queueIndex=" << queueIndex; + ASSERT_NEAR(34, tc->getVVTPosition(0, 0).value_or(0), EPS2D) << "queueIndex=" << queueIndex; queueIndex++; } ASSERT_TRUE(queueIndex == 422) << "Total queueIndex=" << queueIndex; ASSERT_TRUE(tc->vvtState[1][0].getShaftSynchronized()); - ASSERT_NEAR(34, tc->vvtPosition[0][0], EPS2D); - ASSERT_NEAR(34, tc->vvtPosition[1][0], EPS2D); + ASSERT_NEAR(34, tc->getVVTPosition(0, 0).value_or(0), EPS2D); + ASSERT_NEAR(34, tc->getVVTPosition(1, 0).value_or(0), EPS2D); EXPECT_EQ(0, eth.recentWarnings()->getCount()); } diff --git a/unit_tests/tests/trigger/test_quad_cam.cpp b/unit_tests/tests/trigger/test_quad_cam.cpp index 272afe7e5e..7d1f1e7129 100644 --- a/unit_tests/tests/trigger/test_quad_cam.cpp +++ b/unit_tests/tests/trigger/test_quad_cam.cpp @@ -55,10 +55,10 @@ TEST(trigger, testQuadCam) { int secondCamSecondBank = secondBank * CAMS_PER_BANK + secondCam; // Cams should have no position yet - ASSERT_EQ(0, engine->triggerCentral.getVVTPosition(firstBank, firstCam)); - ASSERT_EQ(0, engine->triggerCentral.getVVTPosition(firstBank, secondCam)); - ASSERT_EQ(0, engine->triggerCentral.getVVTPosition(secondBank, firstCam)); - ASSERT_EQ(0, engine->triggerCentral.getVVTPosition(secondBank, secondCam)); + ASSERT_EQ(0, engine->triggerCentral.getVVTPosition(firstBank, firstCam).value_or(0)); + ASSERT_EQ(0, engine->triggerCentral.getVVTPosition(firstBank, secondCam).value_or(0)); + ASSERT_EQ(0, engine->triggerCentral.getVVTPosition(secondBank, firstCam).value_or(0)); + ASSERT_EQ(0, engine->triggerCentral.getVVTPosition(secondBank, secondCam).value_or(0)); hwHandleVvtCamSignal(true, getTimeNowNt(), firstCam); hwHandleVvtCamSignal(true, getTimeNowNt(), secondCam); @@ -68,10 +68,10 @@ TEST(trigger, testQuadCam) { float basePos = -80.2f; // All four cams should now have the same position - EXPECT_NEAR_M3(basePos, engine->triggerCentral.getVVTPosition(firstBank, firstCam)); - EXPECT_NEAR_M3(basePos, engine->triggerCentral.getVVTPosition(firstBank, secondCam)); - EXPECT_NEAR_M3(basePos, engine->triggerCentral.getVVTPosition(secondBank, firstCam)); - EXPECT_NEAR_M3(basePos, engine->triggerCentral.getVVTPosition(secondBank, secondCam)); + EXPECT_NEAR_M3(basePos, engine->triggerCentral.getVVTPosition(firstBank, firstCam).value_or(0)); + EXPECT_NEAR_M3(basePos, engine->triggerCentral.getVVTPosition(firstBank, secondCam).value_or(0)); + EXPECT_NEAR_M3(basePos, engine->triggerCentral.getVVTPosition(secondBank, firstCam).value_or(0)); + EXPECT_NEAR_M3(basePos, engine->triggerCentral.getVVTPosition(secondBank, secondCam).value_or(0)); // Now fire cam events again, but with time gaps between each eth.moveTimeForwardMs(1); @@ -85,8 +85,8 @@ TEST(trigger, testQuadCam) { // All four cams should have different positions, each retarded by 1ms from the last float oneMsDegrees = 1000 / engine->rpmCalculator.oneDegreeUs; - EXPECT_NEAR(basePos - oneMsDegrees * 1, engine->triggerCentral.getVVTPosition(firstBank, firstCam), EPS3D); - EXPECT_NEAR(basePos - oneMsDegrees * 2, engine->triggerCentral.getVVTPosition(firstBank, secondCam), EPS3D); - EXPECT_NEAR(basePos - oneMsDegrees * 3, engine->triggerCentral.getVVTPosition(secondBank, firstCam), EPS3D); - EXPECT_NEAR(basePos - oneMsDegrees * 4, engine->triggerCentral.getVVTPosition(secondBank, secondCam), EPS3D); + EXPECT_NEAR(basePos - oneMsDegrees * 1, engine->triggerCentral.getVVTPosition(firstBank, firstCam).value_or(0), EPS3D); + EXPECT_NEAR(basePos - oneMsDegrees * 2, engine->triggerCentral.getVVTPosition(firstBank, secondCam).value_or(0), EPS3D); + EXPECT_NEAR(basePos - oneMsDegrees * 3, engine->triggerCentral.getVVTPosition(secondBank, firstCam).value_or(0), EPS3D); + EXPECT_NEAR(basePos - oneMsDegrees * 4, engine->triggerCentral.getVVTPosition(secondBank, secondCam).value_or(0), EPS3D); } diff --git a/unit_tests/tests/trigger/test_real_cas_24_plus_1.cpp b/unit_tests/tests/trigger/test_real_cas_24_plus_1.cpp index 7dc4f97cd7..d8ddcc2fd5 100644 --- a/unit_tests/tests/trigger/test_real_cas_24_plus_1.cpp +++ b/unit_tests/tests/trigger/test_real_cas_24_plus_1.cpp @@ -53,10 +53,10 @@ TEST(realCas24Plus1, spinningOnBench) { EXPECT_NEAR(rpm, 915.08f, 0.1); } - float vvt = engine->triggerCentral.getVVTPosition(/*bankIndex*/0, /*camIndex*/0); - if (vvt != 0) { + auto vvt = engine->triggerCentral.getVVTPosition(/*bankIndex*/0, /*camIndex*/0); + if (vvt) { // cam position should never be reported outside of correct range - EXPECT_TRUE(vvt > -10 && vvt < -9); + EXPECT_TRUE(vvt.Value > -10 && vvt.Value < -9); } } 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 7afcdeb472..1ab0a3f58b 100644 --- a/unit_tests/tests/trigger/test_real_cranking_nissan_vq40.cpp +++ b/unit_tests/tests/trigger/test_real_cranking_nissan_vq40.cpp @@ -25,27 +25,27 @@ static void test(int engineSyncCam, float camOffsetAdd) { while (reader.haveMore()) { reader.processLine(ð); - float vvt1 = engine->triggerCentral.getVVTPosition(/*bankIndex*/0, /*camIndex*/0); - float vvt2 = engine->triggerCentral.getVVTPosition(/*bankIndex*/1, /*camIndex*/0); + auto vvt1 = engine->triggerCentral.getVVTPosition(/*bankIndex*/0, /*camIndex*/0); + auto vvt2 = engine->triggerCentral.getVVTPosition(/*bankIndex*/1, /*camIndex*/0); - if (vvt1 != 0) { + if (vvt1 && vvt1.Value != 0) { if (!hasSeenFirstVvt) { - EXPECT_NEAR(vvt1, 1.4, /*precision*/1); + EXPECT_NEAR(vvt1.Value, 1.4, /*precision*/1); hasSeenFirstVvt = true; } // cam position should never be reported outside of correct range - EXPECT_TRUE(vvt1 > -3 && vvt1 < 3); + EXPECT_TRUE(vvt1.Value > -3 && vvt1.Value < 3); } - if (vvt2 != 0) { + if (vvt2) { // cam position should never be reported outside of correct range - EXPECT_TRUE(vvt2 > -3 && vvt2 < 3); + EXPECT_TRUE(vvt2.Value > -3 && vvt2.Value < 3); } } - EXPECT_NEAR(engine->triggerCentral.getVVTPosition(/*bankIndex*/0, /*camIndex*/0), 1.351, 1e-2); - EXPECT_NEAR(engine->triggerCentral.getVVTPosition(/*bankIndex*/1, /*camIndex*/0), 1.548, 1e-2); + EXPECT_NEAR(engine->triggerCentral.getVVTPosition(/*bankIndex*/0, /*camIndex*/0).value_or(0), 1.351, 1e-2); + EXPECT_NEAR(engine->triggerCentral.getVVTPosition(/*bankIndex*/1, /*camIndex*/0).value_or(0), 1.548, 1e-2); ASSERT_EQ(102, round(Sensor::getOrZero(SensorType::Rpm)))<< reader.lineIndex(); // TODO: why warnings? diff --git a/unit_tests/tests/trigger/test_real_k20.cpp b/unit_tests/tests/trigger/test_real_k20.cpp index ecf5a919af..46cc1fa489 100644 --- a/unit_tests/tests/trigger/test_real_k20.cpp +++ b/unit_tests/tests/trigger/test_real_k20.cpp @@ -23,9 +23,9 @@ TEST(realk20, cranking) { // EXPECT_TRUE(vvtI > -10 && vvtI < 10); // } - float vvtE = engine->triggerCentral.getVVTPosition(/*bankIndex*/0, /*camIndex*/1); - if (vvtE != 0) { - EXPECT_TRUE(vvtE > -10 && vvtE < 10); + auto vvtE = engine->triggerCentral.getVVTPosition(/*bankIndex*/0, /*camIndex*/1); + if (vvtE) { + EXPECT_TRUE(vvtE.Value > -10 && vvtE.Value < 10); } } diff --git a/unit_tests/tests/trigger/test_real_nb2_cranking.cpp b/unit_tests/tests/trigger/test_real_nb2_cranking.cpp index 13c63413c6..876c9bfd47 100644 --- a/unit_tests/tests/trigger/test_real_nb2_cranking.cpp +++ b/unit_tests/tests/trigger/test_real_nb2_cranking.cpp @@ -20,7 +20,7 @@ TEST(realCrankingNB2, normalCranking) { } // VVT position nearly zero! - EXPECT_NEAR(engine->triggerCentral.getVVTPosition(0, 0), 11.2627f, 1e-4); + EXPECT_NEAR(engine->triggerCentral.getVVTPosition(0, 0).value_or(0), 11.2627f, 1e-4); // Check the number of times VVT information was used to adjust crank phase // This should happen exactly once: once we sync, we shouldn't lose it. @@ -42,10 +42,12 @@ TEST(realCrankingNB2, crankingMissingInjector) { while (reader.haveMore()) { reader.processLine(ð); - } - // VVT position nearly zero! - EXPECT_NEAR(engine->triggerCentral.getVVTPosition(0, 0), 4.476928f, 1e-4); + if (auto vvt = engine->triggerCentral.getVVTPosition(0, 0)) { + // VVT position nearly zero! + EXPECT_NEAR(vvt.Value, 0, 20); + } + } ASSERT_EQ(316, round(Sensor::getOrZero(SensorType::Rpm))); diff --git a/unit_tests/tests/trigger/test_toyota_3_tooth_cam.cpp b/unit_tests/tests/trigger/test_toyota_3_tooth_cam.cpp index 8712b03a6d..0bb521b516 100644 --- a/unit_tests/tests/trigger/test_toyota_3_tooth_cam.cpp +++ b/unit_tests/tests/trigger/test_toyota_3_tooth_cam.cpp @@ -20,22 +20,22 @@ TEST(Toyota3ToothCam, RealEngineRunning) { while (reader.haveMore()) { reader.processLine(ð); - float vvt1 = engine->triggerCentral.getVVTPosition(/*bankIndex*/0, /*camIndex*/0); + auto vvt1 = engine->triggerCentral.getVVTPosition(/*bankIndex*/0, /*camIndex*/0); - if (vvt1 != 0) { + if (vvt1) { if (!hasSeenFirstVvt) { - EXPECT_NEAR(vvt1, 0, /*precision*/1); + EXPECT_NEAR(vvt1.Value, 0, /*precision*/1); hasSeenFirstVvt = true; } // cam position should never be reported outside of correct range - EXPECT_TRUE(vvt1 > -3 && vvt1 < 3); + EXPECT_TRUE(vvt1.Value > -3 && vvt1.Value < 3); } } EXPECT_EQ(getTriggerCentral()->triggerState.camResyncCounter, 0); - EXPECT_NEAR(engine->triggerCentral.getVVTPosition(/*bankIndex*/0, /*camIndex*/0), 0, 1); + EXPECT_NEAR(engine->triggerCentral.getVVTPosition(/*bankIndex*/0, /*camIndex*/0).value_or(0), 0, 1); ASSERT_EQ(3078, round(Sensor::getOrZero(SensorType::Rpm))); // TODO: why warnings? @@ -103,14 +103,14 @@ static void test3tooth(size_t revsBeforeVvt, size_t teethBeforeVvt, bool expectS EXPECT_EQ(expectSync, getTriggerCentral()->triggerState.hasSynchronizedPhase()); if (expectSync) { - EXPECT_EQ(5, getTriggerCentral()->getVVTPosition(0, 0)); + EXPECT_EQ(5, getTriggerCentral()->getVVTPosition(0, 0).value_or(0)); // Bump ahead by one tooth eth.fireFall(1); eth.fireRise(9); ASSERT_EQ(60, getTriggerCentral()->currentEngineDecodedPhase); } else { - EXPECT_EQ(0, getTriggerCentral()->getVVTPosition(0, 0)); + EXPECT_EQ(0, getTriggerCentral()->getVVTPosition(0, 0).value_or(0)); } }