From ecee1dd02d7017ac4f9f0ddd4951363dda480b04 Mon Sep 17 00:00:00 2001 From: Matthew Kennedy Date: Wed, 30 Nov 2022 19:20:09 -0800 Subject: [PATCH] ETB error counter logic (#4854) * simplify ETB error counter logic * dropped this: { * happy test * reorder logic, test etbErrorCode * test that fails * independent TPS and PPS counters * missed a file * happy test --- .../actuators/electronic_throttle.cpp | 68 ++++++++------ .../actuators/electronic_throttle.txt | 3 +- .../actuators/electronic_throttle_impl.h | 4 +- firmware/controllers/limp_manager.h | 5 +- .../controllers/sensors/sensor_checker.cpp | 11 ++- firmware/controllers/sensors/sensor_checker.h | 6 ++ firmware/tunerstudio/rusefi.input | 2 +- .../tests/actuators/test_etb_integrated.cpp | 88 +++++++++++++++---- 8 files changed, 132 insertions(+), 55 deletions(-) diff --git a/firmware/controllers/actuators/electronic_throttle.cpp b/firmware/controllers/actuators/electronic_throttle.cpp index fbb1bdab0b..1e64462937 100644 --- a/firmware/controllers/actuators/electronic_throttle.cpp +++ b/firmware/controllers/actuators/electronic_throttle.cpp @@ -223,6 +223,8 @@ void EtbController::reset() { etbDutyRateOfChange = etbDutyAverage = 0; m_dutyRocAverage.reset(); m_dutyAverage.reset(); + etbTpsErrorCounter = 0; + etbPpsErrorCounter = 0; } void EtbController::onConfigurationChange(pid_s* previousConfiguration) { @@ -510,7 +512,6 @@ expected EtbController::getClosedLoop(percent_t target, percent_t obs } if (m_isAutotune) { - etbInputErrorCounter = 0; return getClosedLoopAutotune(target, observation); } else { // Check that we're not over the error limit @@ -572,17 +573,52 @@ void EtbController::update() { return; } + // Only allow autotune with stopped engine, and on the first throttle + // Update local state about autotune + m_isAutotune = Sensor::getOrZero(SensorType::Rpm) == 0 + && engine->etbAutoTune + && m_function == ETB_Throttle1; + + bool shouldCheckSensorFunction = engine->module()->analogSensorsShouldWork(); + + if (!m_isAutotune && shouldCheckSensorFunction) { + bool isTpsError = !Sensor::get(m_positionSensor).Valid; + + // If we have an error that's new, increment the counter + if (isTpsError && !hadTpsError) { + etbTpsErrorCounter++; + } + + hadTpsError = isTpsError; + + bool isPpsError = !Sensor::get(SensorType::AcceleratorPedal).Valid; + + // If we have an error that's new, increment the counter + if (isPpsError && !hadPpsError) { + etbPpsErrorCounter++; + } + + hadPpsError = isPpsError; + } else { + // Either sensors are expected to not work, or autotune is running, so reset the error counter + etbTpsErrorCounter = 0; + etbPpsErrorCounter = 0; + } + TpsState localReason = TpsState::None; if (engineConfiguration->disableEtbWhenEngineStopped && !engine->triggerCentral.engineMovedRecently()) { - localReason = TpsState::Setting; - } else if (etbInputErrorCounter > 50) { - localReason = TpsState::InputJitter; + localReason = TpsState::EngineStopped; + } else if (etbTpsErrorCounter > 50) { + localReason = TpsState::IntermittentTps; + } else if (etbPpsErrorCounter > 50) { + localReason = TpsState::IntermittentPps; } else if (engine->engineState.lua.luaDisableEtb) { localReason = TpsState::Lua; } + etbErrorCode = (int8_t)localReason; + if (localReason != TpsState::None) { - etbErrorCode = (int8_t)localReason; // If engine is stopped and so configured, skip the ETB update entirely // This is quieter and pulls less power than leaving it on all the time m_motor->disable(); @@ -593,28 +629,6 @@ void EtbController::update() { m_pid.iTermMin = engineConfiguration->etb_iTermMin; m_pid.iTermMax = engineConfiguration->etb_iTermMax; - // Only allow autotune with stopped engine, and on the first throttle - // Update local state about autotune - m_isAutotune = Sensor::getOrZero(SensorType::Rpm) == 0 - && engine->etbAutoTune - && m_function == ETB_Throttle1; - - if (!m_isAutotune) { - // note that ClosedLoopController has it's own setpoint error validation so ours with the counter has to be here - // seems good enough to simply check for both TPS sensors - int errorState = (isTps1Error() ? 1 : 0) + (isTps2Error() ? 2 : 0) - + (isPedalError() ? 4 : 0); - - // current basic implementation is to check for input error counter only while engine is not running - // we can make this logic smarter one day later - if (Sensor::getOrZero(SensorType::Rpm) == 0 - && prevErrorState != errorState) { - prevErrorState = errorState; - etbInputErrorCounter++; - } - } - - ClosedLoopController::update(); } diff --git a/firmware/controllers/actuators/electronic_throttle.txt b/firmware/controllers/actuators/electronic_throttle.txt index 46d175dab6..009980d2c5 100644 --- a/firmware/controllers/actuators/electronic_throttle.txt +++ b/firmware/controllers/actuators/electronic_throttle.txt @@ -14,7 +14,8 @@ float luaAdjustment;"ETB: luaAdjustment" bit etbRevLimitActive float etbDutyRateOfChange float etbDutyAverage - uint16_t etbInputErrorCounter;"ETB inputs error counter" + uint16_t etbTpsErrorCounter;"ETB TPS error counter" + uint16_t etbPpsErrorCounter;"ETB pedal error counter" int8_t etbErrorCode diff --git a/firmware/controllers/actuators/electronic_throttle_impl.h b/firmware/controllers/actuators/electronic_throttle_impl.h index be2ac73ad2..a0613b29f1 100644 --- a/firmware/controllers/actuators/electronic_throttle_impl.h +++ b/firmware/controllers/actuators/electronic_throttle_impl.h @@ -73,12 +73,14 @@ public: float getLuaAdjustment() const; float prevOutput = 0; - int prevErrorState = false; protected: // This is set if an automatic TPS calibration should be run bool m_isAutocal = false; + bool hadTpsError = false; + bool hadPpsError = false; + etb_function_e getFunction() const { return m_function; } DcMotor* getMotor() { return m_motor; } diff --git a/firmware/controllers/limp_manager.h b/firmware/controllers/limp_manager.h index 524b5932bc..df44e50275 100644 --- a/firmware/controllers/limp_manager.h +++ b/firmware/controllers/limp_manager.h @@ -28,15 +28,16 @@ enum class ClearReason : uint8_t { enum class TpsState : uint8_t { None, // 0 - Setting, + EngineStopped, TpsError, PpsError, // 3 - InputJitter, + IntermittentTps, PidJitter, Lua, // 6 Manual, NotConfigured, Redundancy, // 9 + IntermittentPps, // keep this list in sync with etbCutCodeList in rusefi.input! }; diff --git a/firmware/controllers/sensors/sensor_checker.cpp b/firmware/controllers/sensors/sensor_checker.cpp index a5cd521536..a41c1d1822 100644 --- a/firmware/controllers/sensors/sensor_checker.cpp +++ b/firmware/controllers/sensors/sensor_checker.cpp @@ -146,7 +146,9 @@ static obd_code_e getCodeForIgnition(int idx, brain_pin_diag_e diag) { void SensorChecker::onSlowCallback() { // Don't check when the ignition is off, or when it was just turned on (let things stabilize) // TODO: also inhibit checking if we just did a flash burn, since that blocks the ECU for a few seconds. - if (!m_ignitionIsOn || !m_timeSinceIgnOff.hasElapsedSec(5)) { + bool shouldCheck = m_ignitionIsOn && m_timeSinceIgnOff.hasElapsedSec(5); + m_analogSensorsShouldWork = shouldCheck; + if (shouldCheck) { return; } @@ -158,11 +160,12 @@ void SensorChecker::onSlowCallback() { check(SensorType::Tps2Secondary); check(SensorType::Tps2); - check(SensorType::Tps2); - check(SensorType::Tps2Primary); - check(SensorType::Tps2Secondary); + check(SensorType::AcceleratorPedalPrimary); + check(SensorType::AcceleratorPedalSecondary); + check(SensorType::AcceleratorPedal); check(SensorType::Map); + check(SensorType::Map2); check(SensorType::Clt); check(SensorType::Iat); diff --git a/firmware/controllers/sensors/sensor_checker.h b/firmware/controllers/sensors/sensor_checker.h index 1d93502658..2d164d561f 100644 --- a/firmware/controllers/sensors/sensor_checker.h +++ b/firmware/controllers/sensors/sensor_checker.h @@ -6,7 +6,13 @@ public: void onSlowCallback() override; void onIgnitionStateChanged(bool ignitionOn) override; + bool analogSensorsShouldWork() const { + return m_analogSensorsShouldWork; + } + private: bool m_ignitionIsOn = false; Timer m_timeSinceIgnOff; + + bool m_analogSensorsShouldWork = false; }; diff --git a/firmware/tunerstudio/rusefi.input b/firmware/tunerstudio/rusefi.input index 3fd4064e52..26645f846d 100644 --- a/firmware/tunerstudio/rusefi.input +++ b/firmware/tunerstudio/rusefi.input @@ -239,7 +239,7 @@ enable2ndByteCanID = false fuelIgnCutCodeList = bits, U08, [0:7], "None", "fatal error", "setting disabled", "RPM limit", "fault RPM limit", "boost cut", "oil pressure", "stop requested", "ETB problem", "launch control", "max injector duty", "flood clear", "engine sync", "kickstart", "ign off" ; TpsState - etbCutCodeList = bits, U08, [0:7], "None", "engine off setting", "TPS error", "PPS error", "Input noise", "PID noise", "Lua", "Manual", "N/A", "Redundancy" + etbCutCodeList = bits, U08, [0:7], "None", "engine stopped", "TPS error", "PPS error", "TPS noise", "PID noise", "Lua", "Manual", "N/A", "Redundancy", "PPS noise" [ConstantsExtensions] ; defaultValue is used to provide TunerStudio with a value to use in the case of diff --git a/unit_tests/tests/actuators/test_etb_integrated.cpp b/unit_tests/tests/actuators/test_etb_integrated.cpp index 296379dd2a..e1ba581255 100644 --- a/unit_tests/tests/actuators/test_etb_integrated.cpp +++ b/unit_tests/tests/actuators/test_etb_integrated.cpp @@ -18,10 +18,7 @@ static EtbController * initEtbIntegratedTest() { initTps(); doInitElectronicThrottle(); - EtbController *etb = (EtbController*)engine->etbControllers[0]; - etb->etbInputErrorCounter = 0; // ETB controlles are global shared instances :( - etb->prevErrorState = false; - return etb; + return (EtbController*)engine->etbControllers[0]; } TEST(etb, integrated) { @@ -48,40 +45,93 @@ TEST(etb, integrated) { ASSERT_NEAR(destination, 130.2554, EPS3D); } -TEST(etb, integratedTpsJitter) { +extern int timeNowUs; + +TEST(etb, intermittentTps) { EngineTestHelper eth(TEST_ENGINE); // we have a destructor so cannot move EngineTestHelper into utility method EtbController *etb = initEtbIntegratedTest(); + // Tell the sensor checker that the ignition is on + engine->module()->onIgnitionStateChanged(true); + engine->module()->onSlowCallback(); + timeNowUs += 10e6; + engine->module()->onSlowCallback(); + + ASSERT_TRUE(engine->module()->analogSensorsShouldWork()); + ASSERT_FALSE(isTps1Error()); - ASSERT_FALSE(isTps2Error()); - ASSERT_TRUE(isPedalError()); etb->update(); - Sensor::setInvalidMockValue(SensorType::AcceleratorPedal); - ASSERT_TRUE(isPedalError()); - etb->update(); + EXPECT_EQ(0, etb->etbTpsErrorCounter); + EXPECT_EQ(0, etb->etbErrorCode); - ASSERT_EQ(1, etb->etbInputErrorCounter); + int badCount = 0; + // Do some bad/good/bad/good cycles, make sure count keeps up + for (size_t i = 0; i < 50; i++) { + Sensor::setInvalidMockValue(SensorType::Tps1); + ASSERT_TRUE(isTps1Error()); + etb->update(); + + badCount++; + EXPECT_EQ(badCount, etb->etbTpsErrorCounter); + EXPECT_EQ(0, etb->etbErrorCode); + + Sensor::setMockValue(SensorType::Tps1, 20); + ASSERT_FALSE(isTps1Error()); + etb->update(); + } + + // 51st bad TPS should set etbErrorCode Sensor::setInvalidMockValue(SensorType::Tps1); + ASSERT_TRUE(isTps1Error()); etb->update(); - ASSERT_EQ(2, etb->etbInputErrorCounter); + EXPECT_NE(0, etb->etbErrorCode); } TEST(etb, intermittentPps) { EngineTestHelper eth(TEST_ENGINE); // we have a destructor so cannot move EngineTestHelper into utility method + + Sensor::setMockValue(SensorType::AcceleratorPedal, 10, true); EtbController *etb = initEtbIntegratedTest(); - Sensor::setMockValue(SensorType::AcceleratorPedal, 25.0f, true); - ASSERT_FALSE(isTps1Error()); - ASSERT_FALSE(isTps2Error()); + // Tell the sensor checker that the ignition is on + engine->module()->onIgnitionStateChanged(true); + engine->module()->onSlowCallback(); + timeNowUs += 10e6; + engine->module()->onSlowCallback(); + + ASSERT_TRUE(engine->module()->analogSensorsShouldWork()); + ASSERT_FALSE(isPedalError()); - etb->update(); - ASSERT_EQ(0, etb->etbInputErrorCounter); - Sensor::setInvalidMockValue(SensorType::AcceleratorPedal); etb->update(); - ASSERT_EQ(1, etb->etbInputErrorCounter); + + EXPECT_EQ(0, etb->etbPpsErrorCounter); + EXPECT_EQ(0, etb->etbErrorCode); + + int badCount = 0; + + // Do some bad/good/bad/good cycles, make sure count keeps up + for (size_t i = 0; i < 50; i++) { + Sensor::setInvalidMockValue(SensorType::AcceleratorPedal); + ASSERT_TRUE(isPedalError()); + etb->update(); + + badCount++; + EXPECT_EQ(badCount, etb->etbPpsErrorCounter); + EXPECT_EQ(0, etb->etbErrorCode); + + Sensor::setMockValue(SensorType::AcceleratorPedal, 20); + ASSERT_FALSE(isPedalError()); + etb->update(); + } + + // 51st bad TPS should set etbErrorCode + Sensor::setInvalidMockValue(SensorType::AcceleratorPedal); + ASSERT_TRUE(isPedalError()); + etb->update(); + EXPECT_NE(0, etb->etbErrorCode); }