From 15d172ab083d269b406b2651efd3292ae8788743 Mon Sep 17 00:00:00 2001 From: Matthew Kennedy Date: Sat, 12 Jun 2021 11:21:11 -0700 Subject: [PATCH] apply open loop idle when in automatic mode (#2745) * always apply open loop in closed loop * simplify flags * comment * move * comment * missed one * config for fan bump * adjust miata defaults * test fan/ac open loop * idle CL tests * test coasting * test integration case * fix merge * put back reset logic for now * s * don't need that part * I guess use OL as baseIdlePosition * reset condition --- firmware/config/engines/mazda_miata_1_6.cpp | 15 +-- .../controllers/actuators/idle_thread.cpp | 90 ++++---------- firmware/controllers/actuators/idle_thread.h | 3 + unit_tests/tests/test_idle_controller.cpp | 113 ++++++++++++++++-- 4 files changed, 131 insertions(+), 90 deletions(-) diff --git a/firmware/config/engines/mazda_miata_1_6.cpp b/firmware/config/engines/mazda_miata_1_6.cpp index 68a7725eec..ee15ad9def 100644 --- a/firmware/config/engines/mazda_miata_1_6.cpp +++ b/firmware/config/engines/mazda_miata_1_6.cpp @@ -391,11 +391,10 @@ void setMiataNA6_MAP_MRE(DECLARE_CONFIG_PARAMETER_SIGNATURE) { engineConfiguration->idle_antiwindupFreq = 0.1; engineConfiguration->idle_derivativeFilterLoss = 0.1; engineConfiguration->idleRpmPid.dFactor = 0.002; - engineConfiguration->idleRpmPid.offset = 37; + engineConfiguration->idleRpmPid.offset = 0; engineConfiguration->acIdleExtraOffset = 14; - engineConfiguration->idleRpmPid.minValue = 30; - engineConfiguration->acIdleExtraMin = 14; - engineConfiguration->idleRpmPid.maxValue = 70; + engineConfiguration->idleRpmPid.minValue = -7; + engineConfiguration->idleRpmPid.maxValue = 35; engineConfiguration->idleRpmPid.periodMs = 40; engineConfiguration->idlerpmpid_iTermMin = -6; engineConfiguration->idlerpmpid_iTermMax = 30; @@ -403,14 +402,6 @@ void setMiataNA6_MAP_MRE(DECLARE_CONFIG_PARAMETER_SIGNATURE) { engineConfiguration->idlePidRpmDeadZone = 25; engineConfiguration->idlePidRpmUpperLimit = 1000; - - engineConfiguration->useFSIO12ForIdleOffset = true; - setFsioExpression(QUOTE(MAGIC_OFFSET_FOR_IDLE_OFFSET), "ac_on_switch 0 cfg_acIdleExtraOffset if" PASS_CONFIG_PARAMETER_SUFFIX); - - engineConfiguration->useFSIO13ForIdleMinValue = true; - setFsioExpression(QUOTE(MAGIC_OFFSET_FOR_IDLE_MIN_VALUE), "ac_on_switch 0 cfg_acIdleExtraMin if" PASS_CONFIG_PARAMETER_SUFFIX); - - engineConfiguration->useIdleTimingPidControl = true; engineConfiguration->idleTimingPid.pFactor = 0.05; engineConfiguration->idleTimingPid.iFactor = 0.0; diff --git a/firmware/controllers/actuators/idle_thread.cpp b/firmware/controllers/actuators/idle_thread.cpp index 5668743439..b42c03f42d 100644 --- a/firmware/controllers/actuators/idle_thread.cpp +++ b/firmware/controllers/actuators/idle_thread.cpp @@ -312,13 +312,6 @@ float IdleController::getIdleTimingAdjustment(int rpm, int targetRpm, Phase phas return m_timingPid.getOutput(targetRpm, rpm, FAST_CALLBACK_PERIOD_MS / 1000.0f); } -static percent_t manualIdleController(float cltCorrection DECLARE_ENGINE_PARAMETER_SUFFIX) { - - percent_t correctedPosition = cltCorrection * CONFIG(manIdlePosition); - - return correctedPosition; -} - /** * idle blip is a development tool: alternator PID research for instance have benefited from a repetitive change of RPM */ @@ -380,8 +373,10 @@ float IdleController::getClosedLoop(IIdleController::Phase phase, float tpsPos, } engine->engineState.idle.idleState = TPS_THRESHOLD; - // just leave IAC position as is (but don't return currentIdlePosition - it may already contain additionalAir) - return engine->engineState.idle.baseIdlePosition; + + // We aren't idling, so don't apply any correction. A positive correction could inhibit a return to idle. + m_lastAutomaticPosition = 0; + return 0; } // #1553 we need to give FSIO variable offset or minValue a chance @@ -390,15 +385,16 @@ float IdleController::getClosedLoop(IIdleController::Phase phase, float tpsPos, if (!acToggleJustTouched && absI(rpm - targetRpm) <= CONFIG(idlePidRpmDeadZone)) { engine->engineState.idle.idleState = RPM_DEAD_ZONE; // current RPM is close enough, no need to change anything - return engine->engineState.idle.baseIdlePosition; + return m_lastAutomaticPosition; } // When rpm < targetRpm, there's a risk of dropping RPM too low - and the engine dies out. // So PID reaction should be increased by adding extra percent to PID-error: percent_t errorAmpCoef = 1.0f; - if (rpm < targetRpm) + if (rpm < targetRpm) { errorAmpCoef += (float)CONFIG(pidExtraForLowRpm) / PERCENT_MULT; - + } + // if PID was previously reset, we store the time when it turned on back (see errorAmpCoef correction below) if (wasResetPid) { restoreAfterPidResetTimeUs = nowUs; @@ -423,31 +419,16 @@ float IdleController::getClosedLoop(IIdleController::Phase phase, float tpsPos, float engineLoad = getFuelingLoad(PASS_ENGINE_PARAMETER_SIGNATURE); float multCoef = iacPidMultMap.getValue(rpm / RPM_1_BYTE_PACKING_MULT, engineLoad); // PID can be completely disabled of multCoef==0, or it just works as usual if multCoef==1 - newValue = interpolateClamped(0, engine->engineState.idle.baseIdlePosition, 1.0f, newValue, multCoef); + newValue = interpolateClamped(0, 0, 1, newValue, multCoef); } // Apply PID Deactivation Threshold as a smooth taper for TPS transients. // if tps==0 then PID just works as usual, or we completely disable it if tps>=threshold - newValue = interpolateClamped(0.0f, newValue, CONFIG(idlePidDeactivationTpsThreshold), engine->engineState.idle.baseIdlePosition, tpsPos); - - // Interpolate to the manual position when RPM is close to the upper RPM limit (if idlePidRpmUpperLimit is set). - // If RPM increases and the throttle is closed, then we're in coasting mode, and we should smoothly disable auto-pid. - // If we just leave IAC at baseIdlePosition (as in case of TPS deactivation threshold), RPM would get stuck. - // That's why there's 'useIacTableForCoasting' setting which involves a separate IAC position table for coasting (iacCoasting). - // Currently it's user-defined. But eventually we'll use a real calculated and stored IAC position instead. - int idlePidLowerRpm = targetRpm + CONFIG(idlePidRpmDeadZone); - if (CONFIG(idlePidRpmUpperLimit) > 0) { - engine->engineState.idle.idleState = PID_UPPER; - const auto [cltValid, clt] = Sensor::get(SensorType::Clt); - if (CONFIG(useIacTableForCoasting) && cltValid) { - percent_t iacPosForCoasting = interpolate2d(clt, CONFIG(iacCoastingBins), CONFIG(iacCoasting)); - newValue = interpolateClamped(idlePidLowerRpm, newValue, idlePidLowerRpm + CONFIG(idlePidRpmUpperLimit), iacPosForCoasting, rpm); - } else { - // Well, just leave it as is, without PID regulation... - newValue = engine->engineState.idle.baseIdlePosition; - } - } + // TODO: should we just remove this? It reduces the gain if your zero throttle stop isn't perfect, + // which could give unstable results. + newValue = interpolateClamped(0, newValue, CONFIG(idlePidDeactivationTpsThreshold), 0, tpsPos); + m_lastAutomaticPosition = newValue; return newValue; } @@ -497,56 +478,25 @@ float IdleController::getClosedLoop(IIdleController::Phase phase, float tpsPos, finishIdleTestIfNeeded(); undoIdleBlipIfNeeded(); - // cltCorrection is used only for cranking or running in manual mode - float cltCorrection; - // Use separate CLT correction table for cranking - if (engineConfiguration->overrideCrankingIacSetting && phase == IIdleController::Phase::Cranking) { - cltCorrection = interpolate2d(clt, config->cltCrankingCorrBins, config->cltCrankingCorr); - } else { - // this value would be ignored if running in AUTO mode - // but we need it while cranking in AUTO mode - cltCorrection = interpolate2d(clt, config->cltIdleCorrBins, config->cltIdleCorr); - } - percent_t iacPosition; if (timeToStopBlip != 0) { iacPosition = blipIdlePosition; - engine->engineState.idle.baseIdlePosition = iacPosition; engine->engineState.idle.idleState = BLIP; - } else if (phase == IIdleController::Phase::Cranking) { - // during cranking it's always manual mode, PID would make no sense during cranking - iacPosition = clampPercentValue(cltCorrection * engineConfiguration->crankingIACposition); - // save cranking position & cycles counter for taper transition - lastCrankingIacPosition = iacPosition; - lastCrankingCyclesCounter = engine->rpmCalculator.getRevolutionCounterSinceStart(); - engine->engineState.idle.baseIdlePosition = iacPosition; } else { - if (!tps.Valid || engineConfiguration->idleMode == IM_MANUAL) { - // let's re-apply CLT correction - iacPosition = manualIdleController(cltCorrection PASS_ENGINE_PARAMETER_SUFFIX); - } else { - iacPosition = getClosedLoop(phase, tps.Value, rpm, targetRpm); - } - - iacPosition = clampPercentValue(iacPosition); - - // store 'base' iacPosition without adjustments + // Always apply closed loop correction + iacPosition = getOpenLoop(phase, clt, tps); engine->engineState.idle.baseIdlePosition = iacPosition; - float additionalAir = (float)engineConfiguration->iacByTpsTaper; - - if (tps.Valid) { - iacPosition += interpolateClamped(0.0f, 0.0f, CONFIG(idlePidDeactivationTpsThreshold), additionalAir, tps.Value); + // If TPS is working and automatic mode enabled, add any automatic correction + if (tps.Valid && engineConfiguration->idleMode == IM_AUTO) { + iacPosition += getClosedLoop(phase, tps.Value, rpm, targetRpm); } - // taper transition from cranking to running (uint32_t to float conversion is safe here) - if (engineConfiguration->afterCrankingIACtaperDuration > 0) - iacPosition = interpolateClamped(lastCrankingCyclesCounter, lastCrankingIacPosition, - lastCrankingCyclesCounter + engineConfiguration->afterCrankingIACtaperDuration, iacPosition, - engine->rpmCalculator.getRevolutionCounterSinceStart()); + iacPosition = clampPercentValue(iacPosition); } + #if EFI_TUNER_STUDIO tsOutputChannels.isIdleClosedLoop = phase == Phase::Idling; tsOutputChannels.isIdleCoasting = phase == Phase::Coasting; diff --git a/firmware/controllers/actuators/idle_thread.h b/firmware/controllers/actuators/idle_thread.h index a69635ce41..58ed7407e8 100644 --- a/firmware/controllers/actuators/idle_thread.h +++ b/firmware/controllers/actuators/idle_thread.h @@ -65,6 +65,9 @@ private: Phase m_lastPhase = Phase::Cranking; int m_lastTargetRpm = 0; + // This is stored by getClosedLoop and used in case we want to "do nothing" + float m_lastAutomaticPosition = 0; + Pid m_timingPid; }; diff --git a/unit_tests/tests/test_idle_controller.cpp b/unit_tests/tests/test_idle_controller.cpp index 60dfcaba63..eb1dfee648 100644 --- a/unit_tests/tests/test_idle_controller.cpp +++ b/unit_tests/tests/test_idle_controller.cpp @@ -347,12 +347,10 @@ TEST(idle_v2, closedLoopBasic) { timeNowUs += 5'000'000; // Test above target, should return negative - // TODO: this fails, returns 55, why? - // EXPECT_FLOAT_EQ(-25, dut.getClosedLoop(ICP::Idling, 0, /*rpm*/ 950, /*tgt*/ 900)); + EXPECT_FLOAT_EQ(-25, dut.getClosedLoop(ICP::Idling, 0, /*rpm*/ 950, /*tgt*/ 900)); // Below target, should return positive - // TODO: this fails, returns 55, why? - // EXPECT_FLOAT_EQ(25, dut.getClosedLoop(ICP::Idling, 0, /*rpm*/ 850, /*tgt*/ 900)); + EXPECT_FLOAT_EQ(25, dut.getClosedLoop(ICP::Idling, 0, /*rpm*/ 850, /*tgt*/ 900)); } TEST(idle_v2, closedLoopDeadzone) { @@ -377,10 +375,109 @@ TEST(idle_v2, closedLoopDeadzone) { timeNowUs += 5'000'000; // Test above target, should return negative - // TODO: this fails, returns 55, why? - // EXPECT_FLOAT_EQ(-25, dut.getClosedLoop(ICP::Idling, 0, /*rpm*/ 950, /*tgt*/ 900)); + EXPECT_FLOAT_EQ(-25, dut.getClosedLoop(ICP::Idling, 0, /*rpm*/ 950, /*tgt*/ 900)); // Inside deadzone, should return same as last time - // TODO: this fails, returns 55, why? - // EXPECT_FLOAT_EQ(-25, dut.getClosedLoop(ICP::Idling, 0, /*rpm*/ 900, /*tgt*/ 900)); + EXPECT_FLOAT_EQ(-25, dut.getClosedLoop(ICP::Idling, 0, /*rpm*/ 900, /*tgt*/ 900)); +} + +struct IntegrationIdleMock : public IdleController { + MOCK_METHOD(int, getTargetRpm, (float clt), (const, override)); + MOCK_METHOD(ICP, determinePhase, (int rpm, int targetRpm, SensorResult tps), (const, override)); + MOCK_METHOD(float, getOpenLoop, (ICP phase, float clt, SensorResult tps), (const, override)); + MOCK_METHOD(float, getClosedLoop, (ICP phase, float tps, int rpm, int target), (override)); +}; + +TEST(idle_v2, IntegrationManual) { + WITH_ENGINE_TEST_HELPER(TEST_ENGINE); + StrictMock dut; + INJECT_ENGINE_REFERENCE(&dut); + + SensorResult expectedTps = 1; + float expectedClt = 37; + Sensor::setMockValue(SensorType::DriverThrottleIntent, expectedTps.Value); + Sensor::setMockValue(SensorType::Clt, expectedClt); + ENGINE(rpmCalculator.mockRpm) = 950; + + // Target of 1000 rpm + EXPECT_CALL(dut, getTargetRpm(expectedClt)) + .WillOnce(Return(1000)); + + // Determine phase will claim we're idling + EXPECT_CALL(dut, determinePhase(950, 1000, expectedTps)) + .WillOnce(Return(ICP::Idling)); + + // Open loop should be asked for an open loop position + EXPECT_CALL(dut, getOpenLoop(ICP::Idling, expectedClt, expectedTps)) + .WillOnce(Return(13)); + + // getClosedLoop() should not be called! + + EXPECT_EQ(13, dut.getIdlePosition()); +} + +TEST(idle_v2, IntegrationAutomatic) { + WITH_ENGINE_TEST_HELPER(TEST_ENGINE); + StrictMock dut; + INJECT_ENGINE_REFERENCE(&dut); + + CONFIG(idleMode) = IM_AUTO; + + SensorResult expectedTps = 1; + float expectedClt = 37; + Sensor::setMockValue(SensorType::DriverThrottleIntent, expectedTps.Value); + Sensor::setMockValue(SensorType::Clt, expectedClt); + ENGINE(rpmCalculator.mockRpm) = 950; + + // Target of 1000 rpm + EXPECT_CALL(dut, getTargetRpm(expectedClt)) + .WillOnce(Return(1000)); + + // Determine phase will claim we're idling + EXPECT_CALL(dut, determinePhase(950, 1000, expectedTps)) + .WillOnce(Return(ICP::Idling)); + + // Open loop should be asked for an open loop position + EXPECT_CALL(dut, getOpenLoop(ICP::Idling, expectedClt, expectedTps)) + .WillOnce(Return(13)); + + // Closed loop should get called + EXPECT_CALL(dut, getClosedLoop(ICP::Idling, expectedTps.Value, 950, 1000)) + .WillOnce(Return(7)); + + // Result should be open + closed + EXPECT_EQ(13 + 7, dut.getIdlePosition()); +} + +TEST(idle_v2, IntegrationClamping) { + WITH_ENGINE_TEST_HELPER(TEST_ENGINE); + StrictMock dut; + INJECT_ENGINE_REFERENCE(&dut); + + CONFIG(idleMode) = IM_AUTO; + + SensorResult expectedTps = 1; + float expectedClt = 37; + Sensor::setMockValue(SensorType::DriverThrottleIntent, expectedTps.Value); + Sensor::setMockValue(SensorType::Clt, expectedClt); + ENGINE(rpmCalculator.mockRpm) = 950; + + // Target of 1000 rpm + EXPECT_CALL(dut, getTargetRpm(expectedClt)) + .WillOnce(Return(1000)); + + // Determine phase will claim we're idling + EXPECT_CALL(dut, determinePhase(950, 1000, expectedTps)) + .WillOnce(Return(ICP::Idling)); + + // Open loop should be asked for an open loop position + EXPECT_CALL(dut, getOpenLoop(ICP::Idling, expectedClt, expectedTps)) + .WillOnce(Return(75)); + + // Closed loop should get called + EXPECT_CALL(dut, getClosedLoop(ICP::Idling, expectedTps.Value, 950, 1000)) + .WillOnce(Return(75)); + + // Result would be 75 + 75 = 150, but it should clamp to 100 + EXPECT_EQ(100, dut.getIdlePosition()); }