Create a base class EngineModule for that contains various useful callbacks. (#3548)

* Create a base class EngineModule for that contains various useful callbacks.

This cleans up the API by not requiring the notifiers to know about who wants callbacks.  The only
place you need to update to add a module is in one place.

* Add mockability.

* Convert InjectorModel to a Mockable EngineModule

firmware/controllers/actuators/idle_thread.cpp

@@ -472,26 +472,11 @@ float IdleController::getClosedLoop(IIdleController::Phase phase, float tpsPos,
 		return iacPosition;
 }
 
-void IdleController::update() {
+void IdleController::onSlowCallback() {
 	float position = getIdlePosition();
 	applyIACposition(position);
 }
 
-IdleController idleControllerInstance;
-
-void updateIdleControl()
-{
-	idleControllerInstance.update();
-}
-
-float getIdleTimingAdjustment(int rpm) {
-	return idleControllerInstance.getIdleTimingAdjustment(rpm);
-}
-
-bool isIdlingOrTaper() {
-	return idleControllerInstance.isIdlingOrTaper();
-}
-
 static void applyPidSettings() {
 	getIdlePid()->updateFactors(engineConfiguration->idleRpmPid.pFactor, engineConfiguration->idleRpmPid.iFactor, engineConfiguration->idleRpmPid.dFactor);
 	iacPidMultMap.init(engineConfiguration->iacPidMultTable, engineConfiguration->iacPidMultLoadBins, engineConfiguration->iacPidMultRpmBins);
@@ -518,13 +503,15 @@ void setDefaultIdleParameters() {
 	engineConfiguration->idlePidRpmUpperLimit = 100;
 }
 
+void IdleController::onConfigurationChange(engine_configuration_s const * previousConfiguration) {
 #if ! EFI_UNIT_TEST
-
-void onConfigurationChangeIdleCallback(engine_configuration_s *previousConfiguration) {
 	engine->idle.shouldResetPid = !getIdlePid()->isSame(&previousConfiguration->idleRpmPid);
 	engine->idle.mustResetPid = engine->idle.shouldResetPid;
+#endif
 }
 
+#if ! EFI_UNIT_TEST
+
 void setTargetIdleRpm(int value) {
 	setTargetRpmCurve(value);
 	efiPrintf("target idle RPM %d", value);
@@ -566,9 +553,7 @@ void startIdleBench(void) {
 #endif /* EFI_UNIT_TEST */
 
 void startIdleThread() {
-	idleControllerInstance.init(&engineConfiguration->idleTimingPid);
-
-	engine->idleController = &idleControllerInstance;
+	engine->engineModules.unmock<IdleController>().init(&engineConfiguration->idleTimingPid);
 
 	getIdlePid()->initPidClass(&engineConfiguration->idleRpmPid);
 

firmware/controllers/actuators/idle_thread.h

@@ -8,6 +8,7 @@
 
 #pragma once
 
+#include "engine_module.h"
 #include "engine_ptr.h"
 #include "rusefi_types.h"
 #include "periodic_task.h"
@@ -31,12 +32,13 @@ struct IIdleController {
 	virtual float getCrankingTaperFraction() const = 0;
 };
 
-class IdleController : public IIdleController {
+class IdleController : public IIdleController, public EngineModule {
 public:
+	typedef IIdleController interface_t;
+
 	void init(pid_s* idlePidConfig);
 
 	float getIdlePosition();
-	void update();
 
 	// TARGET DETERMINATION
 	int getTargetRpm(float clt) const override;
@@ -56,6 +58,9 @@ public:
 	// CLOSED LOOP CORRECTION
 	float getClosedLoop(IIdleController::Phase phase, float tpsPos, int rpm, int targetRpm) override;
 
+	void onConfigurationChange(engine_configuration_s const * previousConfig) final;
+	void onSlowCallback() final;
+
 	// Allow querying state from outside
 	bool isIdlingOrTaper() {
 		return m_lastPhase == Phase::Idling || (engineConfiguration->useSeparateIdleTablesForCrankingTaper && m_lastPhase == Phase::CrankToIdleTaper);
@@ -72,13 +77,8 @@ private:
 	Pid m_timingPid;
 };
 
-void updateIdleControl();
 percent_t getIdlePosition();
 
-float getIdleTimingAdjustment(int rpm);
-
-bool isIdlingOrTaper();
-
 void applyIACposition(percent_t position);
 void setManualIdleValvePosition(int positionPercent);
 
@@ -91,8 +91,6 @@ void setIdleIFactor(float value);
 void setIdleDFactor(float value);
 void setIdleMode(idle_mode_e value);
 void setTargetIdleRpm(int value);
-void onConfigurationChangeIdleCallback(engine_configuration_s *previousConfiguration);
 Pid * getIdlePid();
 void startPedalPins();
 void stopPedalPins();
-

firmware/controllers/algo/advance_map.cpp

@@ -50,7 +50,8 @@ static angle_t getRunningAdvance(int rpm, float engineLoad) {
 	float advanceAngle = advanceMap.getValue((float) rpm, engineLoad);
 
 	// get advance from the separate table for Idle
-	if (engineConfiguration->useSeparateAdvanceForIdle && isIdlingOrTaper()) {
+	if (engineConfiguration->useSeparateAdvanceForIdle &&
+	    engine->engineModules.unmock<IdleController>().isIdlingOrTaper()) {
 		float idleAdvance = interpolate2d(rpm, config->idleAdvanceBins, config->idleAdvance);
 
 		auto [valid, tps] = Sensor::get(SensorType::DriverThrottleIntent);
@@ -88,7 +89,7 @@ angle_t getAdvanceCorrections(int rpm) {
 		iatCorrection = iatAdvanceCorrectionMap.getValue(rpm, iat);
 	}
 
-	float pidTimingCorrection = getIdleTimingAdjustment(rpm);
+	float pidTimingCorrection = ENGINE(engineModules).unmock<IdleController>().getIdleTimingAdjustment(rpm);
 
 	if (engineConfiguration->debugMode == DBG_IGNITION_TIMING) {
 #if EFI_TUNER_STUDIO

firmware/controllers/algo/airmass/airmass.cpp

@@ -24,7 +24,8 @@ float AirmassVeModelBase::getVe(int rpm, float load) const {
 
 	auto tps = Sensor::get(SensorType::Tps1);
 	// get VE from the separate table for Idle if idling
-	if (isIdlingOrTaper() && tps && engineConfiguration->useSeparateVeForIdle) {
+	if (engine->engineModules.unmock<IdleController>().isIdlingOrTaper() &&
+	    tps && engineConfiguration->useSeparateVeForIdle) {
 		float idleVe = interpolate2d(rpm, config->idleVeBins, config->idleVe);
 		// interpolate between idle table and normal (running) table using TPS threshold
 		ve = interpolateClamped(0.0f, idleVe, engineConfiguration->idlePidDeactivationTpsThreshold, ve, tps.Value);

firmware/controllers/algo/engine.cpp

@@ -235,7 +235,7 @@ void Engine::periodicSlowCallback() {
 
 	updateGppwm();
 
-	updateIdleControl();
+	ENGINE(engineModules).apply_all([](auto & m) { m.onSlowCallback(); });
 
 #if EFI_BOOST_CONTROL
 	updateBoostControl();
@@ -631,6 +631,8 @@ void Engine::periodicFastCallback() {
 	knockController.periodicFastCallback();
 
 	tachSignalCallback();
+
+	ENGINE(engineModules).apply_all([](auto & m) { m.onFastCallback(); });
 }
 
 void doScheduleStopEngine() {

firmware/controllers/algo/engine.h

@@ -23,7 +23,10 @@
 #include "ac_control.h"
 #include "knock_logic.h"
 #include "idle_state_generated.h"
+#include "idle_thread.h"
+#include "injector_model.h"
 #include "launch_control.h"
+#include "type_list.h"
 
 #if EFI_SIGNAL_EXECUTOR_ONE_TIMER
 // PROD real firmware uses this implementation
@@ -59,7 +62,6 @@ struct AirmassModelBase;
 
 class IEtbController;
 struct IFuelComputer;
-struct IInjectorModel;
 struct IIdleController;
 
 class PrimaryTriggerConfiguration final : public TriggerConfiguration {
@@ -108,8 +110,14 @@ public:
 
 	IEtbController *etbControllers[ETB_COUNT] = {nullptr};
 	IFuelComputer *fuelComputer = nullptr;
-	IInjectorModel *injectorModel = nullptr;
-	IIdleController* idleController = nullptr;
+
+	type_list<
+		Mockable<InjectorModel>,
+#if EFI_IDLE_CONTROL
+		IdleController,
+#endif
+		EngineModule // dummy placeholder so the previous entries can all have commas
+		> engineModules;
 
 	cyclic_buffer<int> triggerErrorDetection;
 

firmware/controllers/algo/engine2.cpp

@@ -155,7 +155,7 @@ void EngineState::periodicFastCallback() {
 	}
 
 	// Store the pre-wall wetting injection duration for scheduling purposes only, not the actual injection duration
-	engine->injectionDuration = engine->injectorModel->getInjectionDuration(injectionMass);
+	engine->injectionDuration = engine->engineModules.get<InjectorModel>().getInjectionDuration(injectionMass);
 
 	float fuelLoad = getFuelingLoad();
 	injectionOffset = getInjectionOffset(rpm, fuelLoad);

firmware/controllers/algo/engine_configuration.cpp

@@ -186,10 +186,6 @@ void incrementGlobalConfigurationVersion() {
 	onConfigurationChangeElectronicThrottleCallback(&activeConfiguration);
 #endif /* EFI_ELECTRONIC_THROTTLE_BODY */
 
-#if EFI_IDLE_CONTROL && ! EFI_UNIT_TEST
-	onConfigurationChangeIdleCallback(&activeConfiguration);
-#endif /* EFI_IDLE_CONTROL */
-
 #if EFI_SHAFT_POSITION_INPUT
 	onConfigurationChangeTriggerCallback();
 #endif /* EFI_SHAFT_POSITION_INPUT */
@@ -200,6 +196,10 @@ void incrementGlobalConfigurationVersion() {
 #if EFI_FSIO
 	onConfigurationChangeFsioCallback(&activeConfiguration);
 #endif /* EFI_FSIO */
+
+	ENGINE(engineModules).apply_all([](auto & m) {
+			m.onConfigurationChange(&activeConfiguration);
+		});
 	rememberCurrentConfiguration();
 }
 

firmware/controllers/algo/fuel/injector_model.h

@@ -2,7 +2,7 @@
 
 #include "expected.h"
 
-struct IInjectorModel {
+struct IInjectorModel : public EngineModule {
 	virtual void prepare() = 0;
 	virtual floatms_t getInjectionDuration(float fuelMassGram) const = 0;
 	virtual float getFuelMassForDuration(floatms_t duration) const = 0;
@@ -38,4 +38,6 @@ public:
 	// Small pulse correction logic
 	float correctShortPulse(float baseDuration) const override;
 	virtual float correctInjectionPolynomial(float baseDuration) const;
+
+	using interface_t = IInjectorModel; // Mock interface
 };

firmware/controllers/algo/fuel_math.cpp

@@ -279,7 +279,7 @@ float getInjectionMass(int rpm) {
 	float injectionFuelMass = cycleFuelMass * durationMultiplier;
 
 	// Prepare injector flow rate & deadtime
-	engine->injectorModel->prepare();
+	engine->engineModules.get<InjectorModel>().prepare();
 
 	floatms_t tpsAccelEnrich = ENGINE(tpsAccelEnrichment.getTpsEnrichment());
 	efiAssert(CUSTOM_ERR_ASSERT, !cisnan(tpsAccelEnrich), "NaN tpsAccelEnrich", 0);
@@ -288,7 +288,7 @@ float getInjectionMass(int rpm) {
 	// For legacy reasons, the TPS accel table is in units of milliseconds, so we have to convert BACK to mass
 	float tpsAccelPerInjection = durationMultiplier * tpsAccelEnrich;
 
-	float tpsFuelMass = engine->injectorModel->getFuelMassForDuration(tpsAccelPerInjection);
+	float tpsFuelMass = engine->engineModules.get<InjectorModel>().getFuelMassForDuration(tpsAccelPerInjection);
 
 	return injectionFuelMass + tpsFuelMass;
 #else
@@ -297,7 +297,6 @@ float getInjectionMass(int rpm) {
 }
 
 static FuelComputer fuelComputer(lambdaMap);
-static InjectorModel injectorModel;
 
 /**
  * @brief	Initialize fuel map data structure
@@ -308,7 +307,6 @@ void initFuelMap() {
 
 
 	engine->fuelComputer = &fuelComputer;
-	engine->injectorModel = &injectorModel;
 
 	mapEstimationTable.init(config->mapEstimateTable, config->mapEstimateTpsBins, config->mapEstimateRpmBins);
 

firmware/controllers/core/engine_module.h

@@ -0,0 +1,13 @@
+#pragma once
+
+class EngineModule {
+public:
+	// Called when the engine_configuration_s part of the tune has changed.
+	virtual void onConfigurationChange(engine_configuration_s const * /*previousConfig*/) { }
+
+	// Called approx 20Hz
+	virtual void onSlowCallback() { }
+
+	// Called approx 200Hz
+	virtual void onFastCallback() { }
+};

firmware/controllers/engine_cycle/main_trigger_callback.cpp

@@ -172,7 +172,7 @@ void InjectionEvent::onTriggerTooth(size_t trgEventIndex, int rpm, efitick_t now
 	// Perform wall wetting adjustment on fuel mass, not duration, so that
 	// it's correct during fuel pressure (injector flow) or battery voltage (deadtime) transients
 	injectionMassGrams = wallFuel.adjust(injectionMassGrams);
-	const floatms_t injectionDuration = engine->injectorModel->getInjectionDuration(injectionMassGrams);
+	const floatms_t injectionDuration = engine->engineModules.get<InjectorModel>().getInjectionDuration(injectionMassGrams);
 
 #if EFI_PRINTF_FUEL_DETAILS
 	if (printFuelDebug) {

firmware/util/containers/type_list.h

@@ -0,0 +1,194 @@
+#pragma once
+
+/*
+ * Indicates that a member of type_list should be able to be replaced in a unit test.
+ */
+template<typename base_t>
+struct Mockable;
+
+/*
+ * Instantiates each type, allowing you to fetch by type.
+ *
+ * This is basically std::tuple, std::get, and std::apply, but with the API oriented more toward
+ * our needs.
+ *
+ * The primary use of this is to provide a list of engine modules in the giant Engine structure.
+ *
+ * The main feature is that objects are mockable.  If you pass in Mockable<T> instead of T, and
+ * typedef T::interface_t, then you can call set to change what get returns.
+ *
+ * For a normal type T,
+ * T & get<T>();
+ * T & unmock<T>();
+ *
+ * For Mockable<T>,
+ * T::interface_t & get<T>();
+ * T & unmock<T>();
+ */
+template<typename base_t, typename... tail_t>
+struct type_list {
+	type_list<base_t> first;
+	type_list<tail_t...> others;
+
+	static_assert(!decltype(others)::template has<base_t>(),
+		      "Each type can only be listed once.");
+
+	/*
+	 * Call the given function on the unmocked version of each type.
+	 *
+	 * It is probably best (and maybe only possible?) to call this with a generic lambda, as:
+	 * tl.apply_all([](auto & m) { m.WhateverFuncIWant(); });
+	 */
+	template<typename func_t>
+	void apply_all(func_t const & f) {
+		first.apply_all(f);
+		others.apply_all(f);
+	}
+
+	/*
+	 * Return the object representing the type get_t.  May return a mocked get_t::interface_t instead.
+	 *
+	 * get_t should not be Mockable, it should be the actual type.
+	 */
+	template<typename get_t>
+	auto get() -> std::enable_if_t<decltype(first)::template has<get_t>(),
+				       decltype(first.template get<get_t>())> {
+		return first.template get<get_t>();
+	}
+
+	template<typename get_t>
+	auto get() -> std::enable_if_t<!decltype(first)::template has<get_t>(),
+				       decltype(others.template get<get_t>())> {
+		return others.template get<get_t>();
+	}
+
+	/*
+	 * Return the regular, unmocked object unmock_t.
+	 *
+	 * unmock_t should not be Mockable, it should be the actual type.
+	 */
+	template<typename unmock_t>
+	auto unmock() -> std::enable_if_t<decltype(first)::template has<unmock_t>(), unmock_t &> {
+		return first.template unmock<unmock_t>();
+	}
+
+	template<typename unmock_t>
+	auto unmock() -> std::enable_if_t<!decltype(first)::template has<unmock_t>(), unmock_t &> {
+		return others.template unmock<unmock_t>();
+	}
+
+#if EFI_UNIT_TEST
+	/*
+	 * Change the object returned by get<set_t>().
+	 *
+	 * param_t should be a pointer or nullptr.  If you pass nullptr, it will be reset to the
+	 * unmock version.
+	 */
+	template<typename set_t, typename param_t>
+	auto set(param_t ptr) -> std::enable_if_t<decltype(first)::template has<set_t>()> {
+		first.template set<set_t>(ptr);
+	}
+
+	template<typename set_t, typename param_t>
+	auto set(param_t ptr) -> std::enable_if_t<!decltype(first)::template has<set_t>()> {
+		others.template set<set_t>(ptr);
+	}
+#endif // EFI_UNIT_TEST
+
+	/*
+	 * Returns whether has_t exists in the type list
+	 *
+	 * has_t should not be Mockable, it should be the actual type.
+	 */
+	template<typename has_t>
+	static constexpr bool has() {
+		return decltype(first)::template has<has_t>() ||
+			decltype(others)::template has<has_t>();
+	}
+};
+
+/*
+ * Specialization of type_list for a single base_t (that is not Mockable<>).
+ */
+template<typename base_t>
+struct type_list<base_t> {
+private:
+	base_t me;
+
+public:
+	template<typename func_t>
+	void apply_all(func_t const & f) {
+		f(me);
+	}
+
+	template<typename has_t>
+	static constexpr bool has() {
+		return std::is_same_v<has_t, base_t>;
+	}
+
+	template<typename get_t, typename = std::enable_if_t<has<get_t>()>>
+	auto & get() {
+		return me;
+	}
+
+	template<typename unmock_t, typename = std::enable_if_t<has<unmock_t>()>>
+	auto & unmock() {
+		return me;
+	}
+
+};
+
+#if !EFI_UNIT_TEST
+
+/*
+ * Production specialization of type_list for a single Mockable<base_t>.
+ *
+ * Performs exactly as base_t.
+ */
+template<typename base_t>
+struct type_list<Mockable<base_t>> : public type_list<base_t> {
+};
+
+#else // if EFI_UNIT_TEST:
+
+/*
+ * Unit test specialization of type_list for a single Mockable<base_t>.
+ */
+template<typename base_t>
+struct type_list<Mockable<base_t>> {
+private:
+	base_t me;
+	typename base_t::interface_t * cur = &me;
+
+public:
+	template<typename func_t>
+	void apply_all(func_t const & f) {
+		f(me);
+	}
+
+	template<typename has_t>
+	static constexpr bool has() {
+		return std::is_same_v<has_t, base_t>;
+	}
+
+	template<typename get_t, typename = std::enable_if_t<has<get_t>()>>
+	auto & get() {
+		return *cur;
+	}
+
+	template<typename unmock_t, typename = std::enable_if_t<has<unmock_t>()>>
+	auto & unmock() {
+		return me;
+	}
+
+	template<typename set_t, typename param_t>
+	auto set(param_t ptr) -> std::enable_if_t<has<set_t>()> {
+		if (ptr) {
+			cur = ptr;
+		} else {
+			cur = &me;
+		}
+	}
+};
+
+#endif // EFI_UNIT_TEST

unit_tests/tests/trigger/test_injection_scheduling.cpp

@@ -22,7 +22,7 @@ TEST(injectionScheduling, NormalDutyCycle) {
 	// Injection duration of 20ms
 	MockInjectorModel2 im;
 	EXPECT_CALL(im, getInjectionDuration(_)).WillOnce(Return(20.0f));
-	engine->injectorModel = &im;
+	engine->engineModules.set<InjectorModel>(&im);
 
 	{
 		InSequence is;

unit_tests/tests/trigger/test_trigger_decoder.cpp

@@ -697,7 +697,7 @@ void doTestFuelSchedulerBug299smallAndMedium(int startUpDelayMs) {
 	// Injection duration of 12.5ms
 	MockInjectorModel2 im;
 	EXPECT_CALL(im, getInjectionDuration(_)).WillRepeatedly(Return(12.5f));
-	engine->injectorModel = &im;
+	engine->engineModules.set<InjectorModel>(&im);
 
 	assertEqualsM("duty for maf=3", 62.5, getInjectorDutyCycle(GET_RPM()));
 
@@ -857,7 +857,7 @@ void doTestFuelSchedulerBug299smallAndMedium(int startUpDelayMs) {
 	// Injection duration of 17.5ms
 	MockInjectorModel2 im2;
 	EXPECT_CALL(im2, getInjectionDuration(_)).WillRepeatedly(Return(17.5f));
-	engine->injectorModel = &im2;
+	engine->engineModules.set<InjectorModel>(&im2);
 
 	// duty cycle above 75% is a special use-case because 'special' fuel event overlappes the next normal event in batch mode
 	assertEqualsM("duty for maf=3", 87.5, getInjectorDutyCycle(GET_RPM()));
@@ -994,7 +994,7 @@ TEST(big, testFuelSchedulerBug299smallAndLarge) {
 	// Injection duration of 17.5ms
 	MockInjectorModel2 im;
 	EXPECT_CALL(im, getInjectionDuration(_)).WillRepeatedly(Return(17.5f));
-	engine->injectorModel = &im;
+	engine->engineModules.set<InjectorModel>(&im);
 
 	assertEqualsM("Lduty for maf=3", 87.5, getInjectorDutyCycle(GET_RPM()));
 
@@ -1061,7 +1061,7 @@ TEST(big, testFuelSchedulerBug299smallAndLarge) {
 	engine->injectionDuration = 2.0f;
 	MockInjectorModel2 im2;
 	EXPECT_CALL(im2, getInjectionDuration(_)).WillRepeatedly(Return(2.0f));
-	engine->injectorModel = &im2;
+	engine->engineModules.set<InjectorModel>(&im2);
 
 	ASSERT_EQ( 10,  getInjectorDutyCycle(GET_RPM())) << "Lduty for maf=3";