require ignition on for firing (#4628)

* require ignition on for firing

* too much havoc for a unit test

* patch up references

* last one

* fix changelog mangling

* fix the changelog template too

* bad merge

* fix the template again

* s

* fake battery voltage on hw ci

* ugh merge

* ugh merge

firmware/CHANGELOG.md

@@ -11,10 +11,16 @@ Release template (copy/paste this for new release):
 ## Month 202x Release - "Release Name"
 
 ### Breaking Changes
+ - 
 
 ### Added
+ -
 
 ### Fixed
+ - 
+
+### Removed
+ - 
 
  -->
 
@@ -26,10 +32,11 @@ Release template (copy/paste this for new release):
  - VVT minimum RPM setting #4545
  - Flexible ignition adder/trim tables #4586
  - Enforce board configuration overrides more strictly #4614
- - Fuel Priming reset fix #4627
  - Startup Frame should scan for available hardware #4633
+ - Don't fire the engine without the ignition on (avoids USB keeping engine alive after ignition off) #4474
 
 ### Fixed
+ - Fuel Priming reset fix #4627
  - Slower than expected RPM information was slowing engine start #4629
 
 ## September 2022 Release - "Day 203"

firmware/controllers/actuators/electronic_throttle.cpp

@@ -529,7 +529,7 @@ expected<percent_t> EtbController::getClosedLoop(percent_t target, percent_t obs
 		// Allow up to 10 percent-seconds of error
 		if (etbIntegralError > 10.0f) {
 			// TODO: figure out how to handle uncalibrated ETB 
-			//engine->limpManager.etbProblem();
+			//getLimpManager()->etbProblem();
 		}
 
 		// Normal case - use PID to compute closed loop part
@@ -548,7 +548,7 @@ void EtbController::setOutput(expected<percent_t> outputValue) {
 	if (!m_motor) return;
 
 	// If ETB is allowed, output is valid, and we aren't paused, output to motor.
-	if (engine->limpManager.allowElectronicThrottle()
+	if (getLimpManager()->allowElectronicThrottle()
 		&& outputValue
 		&& !engineConfiguration->pauseEtbControl) {
 		m_motor->enable();

firmware/controllers/algo/engine.cpp

@@ -544,7 +544,7 @@ TriggerCentral * getTriggerCentral() {
 }
 
 LimpManager * getLimpManager() {
-	return &engine->limpManager;
+	return &engine->module<LimpManager>().unmock();
 }
 
 FuelSchedule *getFuelSchedule() {

firmware/controllers/algo/engine.h

@@ -145,6 +145,7 @@ public:
 #endif // EFI_VEHICLE_SPEED
 		KnockController,
 		SensorChecker,
+		LimpManager,
 		EngineModule // dummy placeholder so the previous entries can all have commas
 		> engineModules;
 
@@ -310,8 +311,6 @@ public:
 	AirmassModelBase* mockAirmassModel = nullptr;
 #endif
 
-	LimpManager limpManager;
-
 private:
 	void reset();
 

firmware/controllers/algo/engine2.cpp

@@ -192,9 +192,6 @@ void EngineState::periodicFastCallback() {
 #if EFI_LAUNCH_CONTROL
 	engine->launchController.update();
 #endif //EFI_LAUNCH_CONTROL
-
-	engine->limpManager.updateState(rpm, nowNt);
-
 #endif // EFI_ENGINE_CONTROL
 }
 

firmware/controllers/core/error_handling.cpp

@@ -235,7 +235,7 @@ void firmwareError(obd_code_e code, const char *fmt, ...) {
 #if EFI_PROD_CODE
 	if (hasFirmwareErrorFlag)
 		return;
-	engine->limpManager.fatalError();
+	getLimpManager()->fatalError();
 	engine->engineState.warnings.addWarningCode(code);
 #ifdef EFI_PRINT_ERRORS_AS_WARNINGS
 	va_list ap;

firmware/controllers/engine_cycle/rpm_calculator.cpp

@@ -82,7 +82,7 @@ bool RpmCalculator::isRunning() const {
  * @return true if engine is spinning (cranking or running)
  */
 bool RpmCalculator::checkIfSpinning(efitick_t nowNt) const {
-	if (engine->limpManager.shutdownController.isEngineStop(nowNt)) {
+	if (getLimpManager()->shutdownController.isEngineStop(nowNt)) {
 		return false;
 	}
 

firmware/controllers/engine_cycle/spark_logic.cpp

@@ -461,7 +461,7 @@ void onTriggerEventSparkLogic(uint32_t trgEventIndex, int rpm, efitick_t edgeTim
 		return;
 	}
 
-	LimpState limitedSparkState = engine->limpManager.allowIgnition();
+	LimpState limitedSparkState = getLimpManager()->allowIgnition();
 
 	// todo: eliminate state copy logic by giving limpManager it's owm limp_manager.txt and leveraging LiveData
 	engine->outputChannels.sparkCutReason = (int8_t)limitedSparkState.reason;

firmware/controllers/limp_manager.cpp

@@ -34,10 +34,21 @@ static bool noFiringUntilVvtSync(vvt_mode_e vvtMode) {
 		operationMode == FOUR_STROKE_TWELVE_TIMES_CRANK_SENSOR;
 }
 
+void LimpManager::onFastCallback() {
+	updateState(Sensor::getOrZero(SensorType::Rpm), getTimeNowNt());
+}
+
 void LimpManager::updateState(int rpm, efitick_t nowNt) {
 	Clearable allowFuel = engineConfiguration->isInjectionEnabled;
 	Clearable allowSpark = engineConfiguration->isIgnitionEnabled;
 
+#if !EFI_UNIT_TEST
+	if (!m_ignitionOn) {
+		allowFuel.clear(ClearReason::IgnitionOff);
+		allowSpark.clear(ClearReason::IgnitionOff);
+	}
+#endif
+
 	{
 		// User-configured hard RPM limit, either constant or CLT-lookup
 		// todo: migrate to engineState->desiredRpmLimit to get this variable logged
@@ -159,6 +170,10 @@ todo AndreiKA this change breaks 22 unit tests?
 	m_transientAllowIgnition = allowSpark;
 }
 
+void LimpManager::onIgnitionStateChanged(bool ignitionOn) {
+	m_ignitionOn = ignitionOn;
+}
+
 void LimpManager::etbProblem() {
 	m_allowEtb.clear(ClearReason::EtbProblem);
 	setFaultRevLimit(1500);

firmware/controllers/limp_manager.h

@@ -20,6 +20,7 @@ enum class ClearReason : uint8_t {
 	FloodClear, // 11
 	EnginePhase, // 12
 	KickStart, // 13
+	IgnitionOff, // 14
 
 	// Keep this list in sync with fuelIgnCutCodeList in rusefi.input!
 	// todo: add a code generator between ClearReason and fuelIgnCutCodeList in rusefi.input
@@ -78,13 +79,16 @@ private:
 	bool m_state = false;
 };
 
-class LimpManager {
+class LimpManager : public EngineModule {
 public:
 	ShutdownController shutdownController;
 
 	// This is called from periodicFastCallback to update internal state
 	void updateState(int rpm, efitick_t nowNt);
 
+	void onFastCallback() override;
+	void onIgnitionStateChanged(bool ignitionOn) override;
+
 	// Other subsystems call these APIs to determine their behavior
 	bool allowElectronicThrottle() const;
 
@@ -116,6 +120,9 @@ private:
 	Clearable m_transientAllowIgnition = true;
 
 	bool m_hadOilPressureAfterStart = false;
+
+	// Ignition switch state
+	bool m_ignitionOn = false;
 };
 
 LimpManager * getLimpManager();

firmware/controllers/shutdown_controller.cpp

@@ -3,9 +3,7 @@
  *
  */
 
-#include "shutdown_controller.h"
-#include "loggingcentral.h"
-#include "limp_manager.h"
+#include "pch.h"
 
 void doScheduleStopEngine() {
 	efiPrintf("Starting doScheduleStopEngine");

firmware/controllers/trigger/trigger_central.cpp

@@ -629,9 +629,7 @@ void TriggerCentral::decodeMapCam(efitick_t timestamp, float currentPhase) {
 #if EFI_UNIT_TEST
 				// hack? feature? existing unit test relies on VVT phase available right away
 				// but current implementation which is based on periodicFastCallback would only make result available on NEXT tooth
-				int rpm = Sensor::getOrZero(SensorType::Rpm);
-				efitick_t nowNt = getTimeNowNt();
-				getLimpManager()->updateState(rpm, nowNt);
+				getLimpManager()->onFastCallback();
 #endif // EFI_UNIT_TEST
 			}
 

firmware/init/sensor/init_sensors.cpp

@@ -73,6 +73,16 @@ void initNewSensors() {
 
 	// Init CLI functionality for sensors (mocking)
 	initSensorCli();
+
+#ifdef HARDWARE_CI
+
+	chThdSleepMilliseconds(10);
+
+	if (Sensor::getOrZero(SensorType::BatteryVoltage) < 8) {
+		// Fake that we have battery voltage, some tests rely on it
+		Sensor::setMockValue(SensorType::BatteryVoltage, 10);
+	}
+#endif
 }
 
 void stopSensors() {

firmware/tunerstudio/rusefi.input

@@ -234,7 +234,7 @@ enable2ndByteCanID = false
 
 	wboFaultCodeList = bits, U08, [0:3], "OK", "Unk", "Unk", "failed to heat", "overheat", "underheat", "no supply"
 
-	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"
+	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"
 
 [ConstantsExtensions]
      ; defaultValue is used to provide TunerStudio with a value to use in the case of

unit_tests/tests/actuators/test_etb.cpp

@@ -732,7 +732,7 @@ TEST(etb, setOutputLimpHome) {
 	EXPECT_CALL(motor, disable());
 
 	// Trip a fatal error
-	engine->limpManager.fatalError();
+	getLimpManager()->fatalError();
 
 	etb.setOutput(25.0f);
 }

unit_tests/tests/trigger/test_map_cam.cpp

@@ -12,10 +12,10 @@ TEST(trigger, map_cam_by_magic_point) {
 	engineConfiguration->camInputs[0] = Gpio::A0;
 	engineConfiguration->vvtMode[0] = VVT_MAP_V_TWIN;
 	eth.engine.periodicFastCallback(); // trigger limp mode
-	ASSERT_FALSE(eth.engine.limpManager.allowIgnition());
-	ASSERT_FALSE(eth.engine.limpManager.allowInjection());
-	ASSERT_EQ(ClearReason::EnginePhase, eth.engine.limpManager.allowIgnition().reason);
-	ASSERT_EQ(ClearReason::EnginePhase, eth.engine.limpManager.allowInjection().reason);
+	ASSERT_FALSE(getLimpManager()->allowIgnition());
+	ASSERT_FALSE(getLimpManager()->allowInjection());
+	ASSERT_EQ(ClearReason::EnginePhase, getLimpManager()->allowIgnition().reason);
+	ASSERT_EQ(ClearReason::EnginePhase, getLimpManager()->allowInjection().reason);
 
 	engine->outputChannels.instantMAPValue = 100;
 
@@ -39,8 +39,8 @@ TEST(trigger, map_cam_by_magic_point) {
 	ASSERT_EQ(1, engine->triggerCentral.triggerState.camResyncCounter);
 	ASSERT_EQ(10, engine->triggerCentral.mapVvt_MAP_AT_CYCLE_COUNT);
 
-	ASSERT_EQ(ClearReason::None, eth.engine.limpManager.allowIgnition().reason);
-	ASSERT_EQ(ClearReason::None, eth.engine.limpManager.allowInjection().reason);
+	ASSERT_EQ(ClearReason::None, getLimpManager()->allowIgnition().reason);
+	ASSERT_EQ(ClearReason::None, getLimpManager()->allowInjection().reason);
 
 	// We have "VVT" sync, things should be scheduled!
 	ASSERT_EQ(2, engine->executor.size());