From 988aacdd915709a42e034da19152ddcb7bf50b89 Mon Sep 17 00:00:00 2001 From: Scott Smith Date: Wed, 17 Nov 2021 09:13:19 -0800 Subject: [PATCH] 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 --- .../controllers/actuators/idle_thread.cpp | 27 +-- firmware/controllers/actuators/idle_thread.h | 16 +- firmware/controllers/algo/advance_map.cpp | 5 +- firmware/controllers/algo/airmass/airmass.cpp | 3 +- firmware/controllers/algo/engine.cpp | 4 +- firmware/controllers/algo/engine.h | 14 +- firmware/controllers/algo/engine2.cpp | 2 +- .../controllers/algo/engine_configuration.cpp | 8 +- .../controllers/algo/fuel/injector_model.h | 4 +- firmware/controllers/algo/fuel_math.cpp | 6 +- firmware/controllers/core/engine_module.h | 13 ++ .../engine_cycle/main_trigger_callback.cpp | 2 +- firmware/util/containers/type_list.h | 194 ++++++++++++++++++ .../trigger/test_injection_scheduling.cpp | 2 +- .../tests/trigger/test_trigger_decoder.cpp | 8 +- 15 files changed, 255 insertions(+), 53 deletions(-) create mode 100644 firmware/controllers/core/engine_module.h create mode 100644 firmware/util/containers/type_list.h diff --git a/firmware/controllers/actuators/idle_thread.cpp b/firmware/controllers/actuators/idle_thread.cpp index 90e51f7de5..ccc03da940 100644 --- a/firmware/controllers/actuators/idle_thread.cpp +++ b/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().init(&engineConfiguration->idleTimingPid); getIdlePid()->initPidClass(&engineConfiguration->idleRpmPid); diff --git a/firmware/controllers/actuators/idle_thread.h b/firmware/controllers/actuators/idle_thread.h index 8079ecfdef..19bc8eda43 100644 --- a/firmware/controllers/actuators/idle_thread.h +++ b/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(); - diff --git a/firmware/controllers/algo/advance_map.cpp b/firmware/controllers/algo/advance_map.cpp index 3b3ff1ca4c..326d824599 100644 --- a/firmware/controllers/algo/advance_map.cpp +++ b/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().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().getIdleTimingAdjustment(rpm); if (engineConfiguration->debugMode == DBG_IGNITION_TIMING) { #if EFI_TUNER_STUDIO diff --git a/firmware/controllers/algo/airmass/airmass.cpp b/firmware/controllers/algo/airmass/airmass.cpp index ccd6812fc5..238afa6b77 100644 --- a/firmware/controllers/algo/airmass/airmass.cpp +++ b/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().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); diff --git a/firmware/controllers/algo/engine.cpp b/firmware/controllers/algo/engine.cpp index f088da8fdc..601a31089a 100644 --- a/firmware/controllers/algo/engine.cpp +++ b/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() { diff --git a/firmware/controllers/algo/engine.h b/firmware/controllers/algo/engine.h index ef4821e61e..d75c26b72d 100644 --- a/firmware/controllers/algo/engine.h +++ b/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, +#if EFI_IDLE_CONTROL + IdleController, +#endif + EngineModule // dummy placeholder so the previous entries can all have commas + > engineModules; cyclic_buffer triggerErrorDetection; diff --git a/firmware/controllers/algo/engine2.cpp b/firmware/controllers/algo/engine2.cpp index 3b52e65ddf..72adaff273 100644 --- a/firmware/controllers/algo/engine2.cpp +++ b/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().getInjectionDuration(injectionMass); float fuelLoad = getFuelingLoad(); injectionOffset = getInjectionOffset(rpm, fuelLoad); diff --git a/firmware/controllers/algo/engine_configuration.cpp b/firmware/controllers/algo/engine_configuration.cpp index 079f9fed53..6106a8c555 100644 --- a/firmware/controllers/algo/engine_configuration.cpp +++ b/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(); } diff --git a/firmware/controllers/algo/fuel/injector_model.h b/firmware/controllers/algo/fuel/injector_model.h index b0979bf2d5..c873601168 100644 --- a/firmware/controllers/algo/fuel/injector_model.h +++ b/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 }; diff --git a/firmware/controllers/algo/fuel_math.cpp b/firmware/controllers/algo/fuel_math.cpp index 51155023be..7181d705c0 100644 --- a/firmware/controllers/algo/fuel_math.cpp +++ b/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().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().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); diff --git a/firmware/controllers/core/engine_module.h b/firmware/controllers/core/engine_module.h new file mode 100644 index 0000000000..16d5073fc4 --- /dev/null +++ b/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() { } +}; diff --git a/firmware/controllers/engine_cycle/main_trigger_callback.cpp b/firmware/controllers/engine_cycle/main_trigger_callback.cpp index 160db0886f..9bd6015505 100644 --- a/firmware/controllers/engine_cycle/main_trigger_callback.cpp +++ b/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().getInjectionDuration(injectionMassGrams); #if EFI_PRINTF_FUEL_DETAILS if (printFuelDebug) { diff --git a/firmware/util/containers/type_list.h b/firmware/util/containers/type_list.h new file mode 100644 index 0000000000..cd07130150 --- /dev/null +++ b/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 +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 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 & unmock(); + * + * For Mockable, + * T::interface_t & get(); + * T & unmock(); + */ +template +struct type_list { + type_list first; + type_list others; + + static_assert(!decltype(others)::template has(), + "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 + 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 + auto get() -> std::enable_if_t(), + decltype(first.template get())> { + return first.template get(); + } + + template + auto get() -> std::enable_if_t(), + decltype(others.template get())> { + return others.template get(); + } + + /* + * Return the regular, unmocked object unmock_t. + * + * unmock_t should not be Mockable, it should be the actual type. + */ + template + auto unmock() -> std::enable_if_t(), unmock_t &> { + return first.template unmock(); + } + + template + auto unmock() -> std::enable_if_t(), unmock_t &> { + return others.template unmock(); + } + +#if EFI_UNIT_TEST + /* + * Change the object returned by get(). + * + * param_t should be a pointer or nullptr. If you pass nullptr, it will be reset to the + * unmock version. + */ + template + auto set(param_t ptr) -> std::enable_if_t()> { + first.template set(ptr); + } + + template + auto set(param_t ptr) -> std::enable_if_t()> { + others.template set(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 + static constexpr bool has() { + return decltype(first)::template has() || + decltype(others)::template has(); + } +}; + +/* + * Specialization of type_list for a single base_t (that is not Mockable<>). + */ +template +struct type_list { +private: + base_t me; + +public: + template + void apply_all(func_t const & f) { + f(me); + } + + template + static constexpr bool has() { + return std::is_same_v; + } + + template()>> + auto & get() { + return me; + } + + template()>> + auto & unmock() { + return me; + } + +}; + +#if !EFI_UNIT_TEST + +/* + * Production specialization of type_list for a single Mockable. + * + * Performs exactly as base_t. + */ +template +struct type_list> : public type_list { +}; + +#else // if EFI_UNIT_TEST: + +/* + * Unit test specialization of type_list for a single Mockable. + */ +template +struct type_list> { +private: + base_t me; + typename base_t::interface_t * cur = &me; + +public: + template + void apply_all(func_t const & f) { + f(me); + } + + template + static constexpr bool has() { + return std::is_same_v; + } + + template()>> + auto & get() { + return *cur; + } + + template()>> + auto & unmock() { + return me; + } + + template + auto set(param_t ptr) -> std::enable_if_t()> { + if (ptr) { + cur = ptr; + } else { + cur = &me; + } + } +}; + +#endif // EFI_UNIT_TEST diff --git a/unit_tests/tests/trigger/test_injection_scheduling.cpp b/unit_tests/tests/trigger/test_injection_scheduling.cpp index 362762d425..e633d01a58 100644 --- a/unit_tests/tests/trigger/test_injection_scheduling.cpp +++ b/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(&im); { InSequence is; diff --git a/unit_tests/tests/trigger/test_trigger_decoder.cpp b/unit_tests/tests/trigger/test_trigger_decoder.cpp index 8960d43d82..4685463bf1 100644 --- a/unit_tests/tests/trigger/test_trigger_decoder.cpp +++ b/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(&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(&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(&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(&im2); ASSERT_EQ( 10, getInjectorDutyCycle(GET_RPM())) << "Lduty for maf=3";