From 339b906aa96d1cc97415f816c75ac86affba4988 Mon Sep 17 00:00:00 2001 From: rusefillc Date: Mon, 10 Jan 2022 20:12:11 -0500 Subject: [PATCH] RE usability: live data for idle controller static bad, class fields better --- .../controllers/actuators/idle_thread.cpp | 31 ++++--------------- firmware/controllers/actuators/idle_thread.h | 22 +++++++++++-- unit_tests/tests/test_idle_controller.cpp | 12 ++++--- 3 files changed, 33 insertions(+), 32 deletions(-) diff --git a/firmware/controllers/actuators/idle_thread.cpp b/firmware/controllers/actuators/idle_thread.cpp index 91551bf508..ea667589ec 100644 --- a/firmware/controllers/actuators/idle_thread.cpp +++ b/firmware/controllers/actuators/idle_thread.cpp @@ -40,24 +40,6 @@ #include "stepper.h" #endif -static efitimeus_t restoreAfterPidResetTimeUs = 0; - -static PidIndustrial industrialWithOverrideIdlePid; - -#if EFI_IDLE_PID_CIC -// Use PID with CIC integrator -static PidCic idleCicPid; -#endif //EFI_IDLE_PID_CIC - -Pid * getIdlePid() { -#if EFI_IDLE_PID_CIC - if (engineConfiguration->useCicPidForIdle) { - return &idleCicPid; - } -#endif /* EFI_IDLE_PID_CIC */ - return &industrialWithOverrideIdlePid; -} - #if ! EFI_UNIT_TEST void idleDebug(const char *msg, percent_t value) { @@ -134,11 +116,12 @@ void setManualIdleValvePosition(int positionPercent) { #endif /* EFI_UNIT_TEST */ -void IdleController::init(pid_s* idlePidConfig) { +void IdleController::init() { shouldResetPid = false; mightResetPid = false; wasResetPid = false; - m_timingPid.initPidClass(idlePidConfig); + m_timingPid.initPidClass(&engineConfiguration->idleTimingPid); + getIdlePid()->initPidClass(&engineConfiguration->idleRpmPid); } int IdleController::getTargetRpm(float clt) const { @@ -357,7 +340,7 @@ float IdleController::getClosedLoop(IIdleController::Phase phase, float tpsPos, } // increase the errorAmpCoef slowly to restore the process correctly after the PID reset // todo: move restoreAfterPidResetTimeUs to idle? - efitimeus_t timeSincePidResetUs = nowUs - /*engine->idle.*/restoreAfterPidResetTimeUs; + efitimeus_t timeSincePidResetUs = nowUs - restoreAfterPidResetTimeUs; // todo: add 'pidAfterResetDampingPeriodMs' setting errorAmpCoef = interpolateClamped(0, 0, MS2US(/*engineConfiguration->pidAfterResetDampingPeriodMs*/1000), errorAmpCoef, timeSincePidResetUs); // If errorAmpCoef > 1.0, then PID thinks that RPM is lower than it is, and controls IAC more aggressively @@ -485,7 +468,7 @@ void IdleController::onSlowCallback() { } static void applyPidSettings() { - getIdlePid()->updateFactors(engineConfiguration->idleRpmPid.pFactor, engineConfiguration->idleRpmPid.iFactor, engineConfiguration->idleRpmPid.dFactor); + engine->module().unmock().getIdlePid()->updateFactors(engineConfiguration->idleRpmPid.pFactor, engineConfiguration->idleRpmPid.iFactor, engineConfiguration->idleRpmPid.dFactor); } void setDefaultIdleParameters() { @@ -559,9 +542,7 @@ void startIdleBench(void) { #endif /* EFI_UNIT_TEST */ void startIdleThread() { - engine->module().unmock().init(&engineConfiguration->idleTimingPid); - - getIdlePid()->initPidClass(&engineConfiguration->idleRpmPid); + engine->module().unmock().init(); #if ! EFI_UNIT_TEST // todo: we still have to explicitly init all hardware on start in addition to handling configuration change via diff --git a/firmware/controllers/actuators/idle_thread.h b/firmware/controllers/actuators/idle_thread.h index 345bbef74a..16c66b8918 100644 --- a/firmware/controllers/actuators/idle_thread.h +++ b/firmware/controllers/actuators/idle_thread.h @@ -35,7 +35,7 @@ class IdleController : public IIdleController, public EngineModule, public idle_ public: typedef IIdleController interface_t; - void init(pid_s* idlePidConfig); + void init(); float getIdlePosition(); @@ -65,10 +65,29 @@ public: return m_lastPhase == Phase::Idling || (engineConfiguration->useSeparateIdleTablesForCrankingTaper && m_lastPhase == Phase::CrankToIdleTaper); } + PidIndustrial industrialWithOverrideIdlePid; + + #if EFI_IDLE_PID_CIC + // Use PID with CIC integrator + PidCic idleCicPid; + #endif //EFI_IDLE_PID_CIC + + Pid * getIdlePid() { + #if EFI_IDLE_PID_CIC + if (engineConfiguration->useCicPidForIdle) { + return &idleCicPid; + } + #endif /* EFI_IDLE_PID_CIC */ + return &industrialWithOverrideIdlePid; + } + + private: + // These are stored by getIdlePosition() and used by getIdleTimingAdjustment() Phase m_lastPhase = Phase::Cranking; int m_lastTargetRpm = 0; + efitimeus_t restoreAfterPidResetTimeUs = 0; // This is stored by getClosedLoop and used in case we want to "do nothing" float m_lastAutomaticPosition = 0; @@ -90,6 +109,5 @@ void setIdleIFactor(float value); void setIdleDFactor(float value); void setIdleMode(idle_mode_e value); void setTargetIdleRpm(int value); -Pid * getIdlePid(); void startPedalPins(); void stopPedalPins(); diff --git a/unit_tests/tests/test_idle_controller.cpp b/unit_tests/tests/test_idle_controller.cpp index d62c906342..3faa89305e 100644 --- a/unit_tests/tests/test_idle_controller.cpp +++ b/unit_tests/tests/test_idle_controller.cpp @@ -24,11 +24,10 @@ TEST(idle_v2, timingPid) { engineConfiguration->useIdleTimingPidControl = true; - pid_s pidCfg{}; - pidCfg.pFactor = 0.1; - pidCfg.minValue = -10; - pidCfg.maxValue = 10; - dut.init(&pidCfg); + engineConfiguration->idleTimingPid.pFactor = 0.1; + engineConfiguration->idleTimingPid.minValue = -10; + engineConfiguration->idleTimingPid.maxValue = 10; + dut.init(); // Check that out of idle mode it doesn't do anything EXPECT_EQ(0, dut.getIdleTimingAdjustment(1050, 1000, ICP::Cranking)); @@ -300,6 +299,7 @@ extern int timeNowUs; TEST(idle_v2, closedLoopBasic) { EngineTestHelper eth(TEST_ENGINE); IdleController dut; + dut.init(); // Not testing PID here, so we can set very simple PID gains engineConfiguration->idleRpmPid.pFactor = 0.5; // 0.5 output per 1 RPM error = 50% per 100 rpm @@ -327,6 +327,8 @@ TEST(idle_v2, closedLoopBasic) { TEST(idle_v2, closedLoopDeadzone) { EngineTestHelper eth(TEST_ENGINE); IdleController dut; + dut.init(); + // Not testing PID here, so we can set very simple PID gains engineConfiguration->idleRpmPid.pFactor = 0.5; // 0.5 output per 1 RPM error = 50% per 100 rpm