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

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<percent_t> 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<SensorChecker>()->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();
 }
 

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
 

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; }
 

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!
 };
 

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);

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;
 };

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

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<SensorChecker>()->onIgnitionStateChanged(true);
+	engine->module<SensorChecker>()->onSlowCallback();
+	timeNowUs += 10e6;
+	engine->module<SensorChecker>()->onSlowCallback();
+
+	ASSERT_TRUE(engine->module<SensorChecker>()->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<SensorChecker>()->onIgnitionStateChanged(true);
+	engine->module<SensorChecker>()->onSlowCallback();
+	timeNowUs += 10e6;
+	engine->module<SensorChecker>()->onSlowCallback();
+
+	ASSERT_TRUE(engine->module<SensorChecker>()->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);
 }