From ea733ecbf0dad833d65f4d8103c427565e6cbebe Mon Sep 17 00:00:00 2001 From: Matthew Kennedy Date: Sat, 20 Aug 2022 17:12:32 -0700 Subject: [PATCH] fix inverted vvt (#4464) * fix inverted mode * unit tests are great --- firmware/controllers/actuators/vvt.cpp | 15 ++++++------ unit_tests/tests/actuators/test_vvt.cpp | 31 +++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 8 deletions(-) diff --git a/firmware/controllers/actuators/vvt.cpp b/firmware/controllers/actuators/vvt.cpp index 7d5f3c96ea..47f4f655e5 100644 --- a/firmware/controllers/actuators/vvt.cpp +++ b/firmware/controllers/actuators/vvt.cpp @@ -82,6 +82,12 @@ static bool shouldInvertVvt(int camIndex) { } expected VvtController::getClosedLoop(angle_t target, angle_t observation) { + // User labels say "advance" and "retard" + // "advance" means that additional solenoid duty makes indicated VVT position more positive + // "retard" means that additional solenoid duty makes indicated VVT position more negative + bool isInverted = shouldInvertVvt(m_cam); + m_pid.setErrorAmplification(isInverted ? -1.0f : 1.0f); + float retVal = m_pid.getOutput(target, observation); if (engineConfiguration->isVerboseAuxPid1) { @@ -93,14 +99,7 @@ expected VvtController::getClosedLoop(angle_t target, angle_t observa m_pid.postState(engine->outputChannels.vvtStatus[index]); #endif /* EFI_TUNER_STUDIO */ - // User labels say "advance" and "retard" - // "advance" means that additional solenoid duty makes indicated VVT position more positive - // "retard" means that additional solenoid duty makes indicated VVT position more negative - if (shouldInvertVvt(m_cam)) { - return -retVal; - } else { - return retVal; - } + return retVal; } void VvtController::setOutput(expected outputValue) { diff --git a/unit_tests/tests/actuators/test_vvt.cpp b/unit_tests/tests/actuators/test_vvt.cpp index a73d247f0a..a4b1caf201 100644 --- a/unit_tests/tests/actuators/test_vvt.cpp +++ b/unit_tests/tests/actuators/test_vvt.cpp @@ -41,3 +41,34 @@ TEST(Vvt, openLoop) { // No open loop for now EXPECT_EQ(dut.getOpenLoop(10), 0); } + +TEST(Vvt, ClosedLoopNotInverted) { + EngineTestHelper eth(TEST_ENGINE); + + VvtController dut; + dut.init(0, 0, 0, nullptr); + + engineConfiguration->auxPid[0].pFactor = 1.5f; + engineConfiguration->auxPid[0].iFactor = 0; + engineConfiguration->auxPid[0].dFactor = 0; + engineConfiguration->auxPid[0].offset = 0; + + // Target of 30 with position 20 should yield positive duty, P=1.5 means 15% duty for 10% error + EXPECT_EQ(dut.getClosedLoop(30, 20).value_or(0), 15); +} + +TEST(Vvt, ClosedLoopInverted) { + EngineTestHelper eth(TEST_ENGINE); + + VvtController dut; + dut.init(0, 0, 0, nullptr); + + engineConfiguration->invertVvtControlIntake = true; + engineConfiguration->auxPid[0].pFactor = 1.5f; + engineConfiguration->auxPid[0].iFactor = 0; + engineConfiguration->auxPid[0].dFactor = 0; + engineConfiguration->auxPid[0].offset = 0; + + // Target of -30 with position -20 should yield positive duty, P=1.5 means 15% duty for 10% error + EXPECT_EQ(dut.getClosedLoop(-30, -20).value_or(0), 15); +}