From f128b336945d4413401ad31ef64497fcbafb46c9 Mon Sep 17 00:00:00 2001 From: Matthew Kennedy Date: Sun, 26 Apr 2020 11:06:28 -0700 Subject: [PATCH] General purpose PWM: implementation & tests (#1366) * config * significant digits * renumber enum, no need for a "none" * ui * impl base * error handle MAP * init & update * don't need arg * don't lie about sensor * fix test build * test stub * initialize * null check * fix clamping * test output * types & enums * don't need param * test getOutput * fix * output pin instead of brain pin * default config --- .../controllers/actuators/gppwm/gppwm.cpp | 54 ++++++++++ firmware/controllers/actuators/gppwm/gppwm.h | 6 ++ .../actuators/gppwm/gppwm_channel.cpp | 89 +++++++++++++++++ .../actuators/gppwm/gppwm_channel.h | 28 ++++++ firmware/controllers/algo/engine.cpp | 3 + .../controllers/algo/engine_configuration.cpp | 30 ++++++ firmware/controllers/algo/rusefi_types.h | 2 +- firmware/controllers/controllers.mk | 3 + firmware/controllers/engine_controller.cpp | 3 + .../system/timer/pwm_generator_logic.h | 2 +- firmware/integration/rusefi_config.txt | 2 +- firmware/util/containers/table_helper.h | 4 +- unit_tests/mocks.h | 6 ++ unit_tests/tests/test_gppwm.cpp | 99 +++++++++++++++++++ unit_tests/tests/tests.mk | 1 + 15 files changed, 327 insertions(+), 5 deletions(-) create mode 100644 firmware/controllers/actuators/gppwm/gppwm.cpp create mode 100644 firmware/controllers/actuators/gppwm/gppwm.h create mode 100644 firmware/controllers/actuators/gppwm/gppwm_channel.cpp create mode 100644 firmware/controllers/actuators/gppwm/gppwm_channel.h create mode 100644 unit_tests/tests/test_gppwm.cpp diff --git a/firmware/controllers/actuators/gppwm/gppwm.cpp b/firmware/controllers/actuators/gppwm/gppwm.cpp new file mode 100644 index 0000000000..e82b4617e2 --- /dev/null +++ b/firmware/controllers/actuators/gppwm/gppwm.cpp @@ -0,0 +1,54 @@ + +#include "global.h" +#include "engine.h" + +#include "gppwm_channel.h" +#include "pwm_generator_logic.h" + +EXTERN_ENGINE; + +static GppwmChannel channels[4]; +static OutputPin pins[4]; +static SimplePwm outputs[4]; + +static gppwm_Map3D_t table1("GPPWM 1"); +static gppwm_Map3D_t table2("GPPWM 2"); +static gppwm_Map3D_t table3("GPPWM 3"); +static gppwm_Map3D_t table4("GPPWM 4"); + +static gppwm_Map3D_t* tables[] = { + &table1, + &table2, + &table3, + &table4, +}; + +void initGpPwm(DECLARE_ENGINE_PARAMETER_SIGNATURE) { + for (size_t i = 0; i < efi::size(channels); i++) { + auto& cfg = CONFIG(gppwm)[i]; + + // If no pin, don't enable this channel. + if (cfg.pin == GPIO_UNASSIGNED) continue; + + // Determine frequency and whether PWM is enabled + float freq = cfg.pwmFrequency; + bool usePwm = freq > 0; + + // Setup pin & pwm + pins[i].initPin("gp pwm", cfg.pin); + startSimplePwm(&outputs[i], "gp pwm", &engine->executor, &pins[i], freq, 0); + + // Set up this channel's lookup table + tables[i]->init(cfg.table, cfg.loadBins, cfg.rpmBins); + + // Finally configure the channel + INJECT_ENGINE_REFERENCE(&channels[i]); + channels[i].init(usePwm, &outputs[i], tables[i], &cfg); + } +} + +void updateGppwm() { + for (size_t i = 0; i < efi::size(channels); i++) { + channels[i].update(); + } +} diff --git a/firmware/controllers/actuators/gppwm/gppwm.h b/firmware/controllers/actuators/gppwm/gppwm.h new file mode 100644 index 0000000000..1326f0e8c1 --- /dev/null +++ b/firmware/controllers/actuators/gppwm/gppwm.h @@ -0,0 +1,6 @@ +#pragma once + +#include "engine.h" + +void initGpPwm(DECLARE_ENGINE_PARAMETER_SIGNATURE); +void updateGppwm(); diff --git a/firmware/controllers/actuators/gppwm/gppwm_channel.cpp b/firmware/controllers/actuators/gppwm/gppwm_channel.cpp new file mode 100644 index 0000000000..1f67a02f4b --- /dev/null +++ b/firmware/controllers/actuators/gppwm/gppwm_channel.cpp @@ -0,0 +1,89 @@ + +#include "gppwm_channel.h" + +#include "engine.h" +#include "pwm_generator_logic.h" +#include "table_helper.h" +#include "expected.h" +#include "sensor.h" +#include "map.h" + +EXTERN_ENGINE; + +expected readGppwmChannel(gppwm_channel_e channel DECLARE_ENGINE_PARAMETER_SUFFIX) { + switch (channel) { + case GPPWM_Tps: + return Sensor::get(SensorType::Tps1); + case GPPWM_Map: { + float map = getMap(PASS_ENGINE_PARAMETER_SIGNATURE); + + if (cisnan(map)) { + return unexpected; + } + + return map; + } + case GPPWM_Clt: + return Sensor::get(SensorType::Clt); + case GPPWM_Iat: + return Sensor::get(SensorType::Iat); + default: + return unexpected; + } +} + +void GppwmChannel::setOutput(float result) { + // Not init yet, nothing to do. + if (!m_pwm || !m_config) { + return; + } + + if (!m_usePwm) { + // Apply hysteresis with provided values + if (m_state && result < m_config->offBelowDuty) { + m_state = false; + } else if (!m_state && result > m_config->onAboveDuty) { + m_state = true; + } + + result = m_state ? 100 : 0; + } + + m_pwm->setSimplePwmDutyCycle(clampF(0, result / 100.0f, 1)); +} + +void GppwmChannel::init(bool usePwm, SimplePwm* pwm, const ValueProvider3D* table, const gppwm_channel* config) { + m_usePwm = usePwm; + m_pwm = pwm; + m_table = table; + m_config = config; +} + +float GppwmChannel::getOutput() const { + expected loadAxisValue = readGppwmChannel(m_config->loadAxis PASS_ENGINE_PARAMETER_SUFFIX); + + // If we couldn't get load axis value, fall back on error value + if (!loadAxisValue) { + return m_config->dutyIfError; + } + + float rpm = GET_RPM(); + + float result = m_table->getValue(rpm / RPM_1_BYTE_PACKING_MULT, loadAxisValue.Value); + + if (cisnan(result)) { + return m_config->dutyIfError; + } + + return result; +} + +void GppwmChannel::update() { + // Without a config, nothing to do. + if (!m_config) { + return; + } + + float output = getOutput(); + setOutput(output); +} diff --git a/firmware/controllers/actuators/gppwm/gppwm_channel.h b/firmware/controllers/actuators/gppwm/gppwm_channel.h new file mode 100644 index 0000000000..1045435157 --- /dev/null +++ b/firmware/controllers/actuators/gppwm/gppwm_channel.h @@ -0,0 +1,28 @@ +#pragma once + +#include "gppwm.h" + +class OutputPin; +class SimplePwm; +class ValueProvider3D; + +class GppwmChannel { +public: + DECLARE_ENGINE_PTR; + + void init(bool usePwm, SimplePwm* pwm, const ValueProvider3D* table, const gppwm_channel* config); + void update(); + +private: + float getOutput() const; + void setOutput(float result); + + // Store the current state so we can apply hysteresis + bool m_state = false; + + // Configuration fields + const gppwm_channel* m_config = nullptr; + bool m_usePwm = false; + SimplePwm* m_pwm = nullptr; + const ValueProvider3D* m_table = nullptr; +}; diff --git a/firmware/controllers/algo/engine.cpp b/firmware/controllers/algo/engine.cpp index 8f39d9ec3a..03cad6a76c 100644 --- a/firmware/controllers/algo/engine.cpp +++ b/firmware/controllers/algo/engine.cpp @@ -25,6 +25,7 @@ #include "fsio_impl.h" #include "perf_trace.h" #include "sensor.h" +#include "gppwm.h" #if EFI_PROD_CODE #include "bench_test.h" @@ -140,6 +141,8 @@ void Engine::periodicSlowCallback(DECLARE_ENGINE_PARAMETER_SIGNATURE) { runHardcodedFsio(PASS_ENGINE_PARAMETER_SIGNATURE); #endif /* EFI_FSIO */ + updateGppwm(); + cylinderCleanupControl(PASS_ENGINE_PARAMETER_SIGNATURE); #if (BOARD_TLE8888_COUNT > 0) diff --git a/firmware/controllers/algo/engine_configuration.cpp b/firmware/controllers/algo/engine_configuration.cpp index 041da2186b..bff36419d5 100644 --- a/firmware/controllers/algo/engine_configuration.cpp +++ b/firmware/controllers/algo/engine_configuration.cpp @@ -629,6 +629,34 @@ void setDefaultMultisparkParameters(DECLARE_ENGINE_PARAMETER_SIGNATURE) { engineConfiguration->multisparkMaxSparkingAngle = 30; } +void setDefaultGppwmParameters(DECLARE_ENGINE_PARAMETER_SIGNATURE) { + // Same config for all channels + for (size_t i = 0; i < efi::size(CONFIG(gppwm)); i++) { + auto& cfg = CONFIG(gppwm)[i]; + + cfg.pin = GPIO_UNASSIGNED; + cfg.dutyIfError = 0; + cfg.onAboveDuty = 60; + cfg.offBelowDuty = 50; + cfg.pwmFrequency = 250; + + for (size_t j = 0; j < efi::size(cfg.loadBins); j++) { + uint8_t z = j * 100 / (efi::size(cfg.loadBins) - 1); + cfg.loadBins[j] = z; + + // Fill some values in the table + for (size_t k = 0; k < efi::size(cfg.rpmBins); k++) { + cfg.table[j][k] = z; + } + + } + + for (size_t j = 0; j < efi::size(cfg.rpmBins); j++) { + cfg.rpmBins[j] = 1000 * j / RPM_1_BYTE_PACKING_MULT; + } + } +} + /** * @brief Global default engine configuration * This method sets the global engine configuration defaults. These default values are then @@ -871,6 +899,8 @@ static void setDefaultEngineConfiguration(DECLARE_ENGINE_PARAMETER_SIGNATURE) { setDefaultMultisparkParameters(PASS_ENGINE_PARAMETER_SIGNATURE); + setDefaultGppwmParameters(PASS_ENGINE_PARAMETER_SIGNATURE); + #if !EFI_UNIT_TEST engineConfiguration->analogInputDividerCoefficient = 2; #endif diff --git a/firmware/controllers/algo/rusefi_types.h b/firmware/controllers/algo/rusefi_types.h index ddd660a1d9..a3b998363e 100644 --- a/firmware/controllers/algo/rusefi_types.h +++ b/firmware/controllers/algo/rusefi_types.h @@ -101,7 +101,7 @@ typedef float fsio_table_8x8_f32t[FSIO_TABLE_8][FSIO_TABLE_8]; typedef float tps_tps_table_t[TPS_TPS_ACCEL_TABLE][TPS_TPS_ACCEL_TABLE]; typedef uint8_t fsio_table_8x8_u8t[FSIO_TABLE_8][FSIO_TABLE_8]; typedef uint8_t boost_table_t[BOOST_LOAD_COUNT][BOOST_RPM_COUNT]; - typedef uint8_t gppwm_table_t[GPPWM_LOAD_COUNT][GPPWM_RPM_COUNT]; +typedef uint8_t gppwm_table_t[GPPWM_LOAD_COUNT][GPPWM_RPM_COUNT]; // this is different type simply to have different hi/low range in rusefi.ini diff --git a/firmware/controllers/controllers.mk b/firmware/controllers/controllers.mk index b4e322af7c..78dcadfa8f 100644 --- a/firmware/controllers/controllers.mk +++ b/firmware/controllers/controllers.mk @@ -18,6 +18,8 @@ CONTROLLERS_SRC_CPP = \ $(CONTROLLERS_DIR)/actuators/idle_thread.cpp \ $(CONTROLLERS_DIR)/actuators/pwm_tester.cpp \ $(CONTROLLERS_DIR)/actuators/algo/aux_pid.cpp \ + $(CONTROLLERS_DIR)/actuators/gppwm/gppwm_channel.cpp \ + $(CONTROLLERS_DIR)/actuators/gppwm/gppwm.cpp \ $(CONTROLLERS_DIR)/gauges/tachometer.cpp \ $(CONTROLLERS_DIR)/gauges/malfunction_indicator.cpp \ $(CONTROLLERS_DIR)/gauges/lcd_controller.cpp \ @@ -61,4 +63,5 @@ CONTROLLERS_INC=\ $(CONTROLLERS_DIR)/math \ $(CONTROLLERS_DIR)/generated \ $(CONTROLLERS_DIR)/actuators \ + $(CONTROLLERS_DIR)/actuators/gppwm \ diff --git a/firmware/controllers/engine_controller.cpp b/firmware/controllers/engine_controller.cpp index eac2ee330c..10c719eeeb 100644 --- a/firmware/controllers/engine_controller.cpp +++ b/firmware/controllers/engine_controller.cpp @@ -55,6 +55,7 @@ #include "boost_control.h" #include "launch_control.h" #include "tachometer.h" +#include "gppwm.h" #if EFI_SENSOR_CHART #include "sensor_chart.h" @@ -136,6 +137,8 @@ static void mostCommonInitEngineController(Logging *sharedLogger DECLARE_ENGINE_ initFsioImpl(sharedLogger PASS_ENGINE_PARAMETER_SUFFIX); #endif /* EFI_FSIO */ + initGpPwm(PASS_ENGINE_PARAMETER_SIGNATURE); + #if EFI_IDLE_CONTROL startIdleThread(sharedLogger PASS_ENGINE_PARAMETER_SUFFIX); #endif /* EFI_IDLE_CONTROL */ diff --git a/firmware/controllers/system/timer/pwm_generator_logic.h b/firmware/controllers/system/timer/pwm_generator_logic.h index 5d93944e3d..f30e1a81d3 100644 --- a/firmware/controllers/system/timer/pwm_generator_logic.h +++ b/firmware/controllers/system/timer/pwm_generator_logic.h @@ -117,7 +117,7 @@ class SimplePwm : public PwmConfig { public: SimplePwm(); explicit SimplePwm(const char *name); - void setSimplePwmDutyCycle(float dutyCycle); + virtual void setSimplePwmDutyCycle(float dutyCycle); pin_state_t pinStates[2]; SingleChannelStateSequence sr[1]; float _switchTimes[2]; diff --git a/firmware/integration/rusefi_config.txt b/firmware/integration/rusefi_config.txt index 80a1d63ae0..cbc654ba07 100644 --- a/firmware/integration/rusefi_config.txt +++ b/firmware/integration/rusefi_config.txt @@ -297,7 +297,7 @@ end_struct custom gppwm_channel_e 1 bits, U08, @OFFSET@, [0:1], @@gppwm_channel_e_enum@@ struct gppwm_channel - brain_pin_e pin;+Select a pin to use for PWM or on-off output.; + output_pin_e pin;+Select a pin to use for PWM or on-off output.; uint8_t dutyIfError;+If an error (with a sensor, etc) is detected, this value is used instead of reading from the table.\nThis should be a safe value for whatever hardware is connected to prevent damage.;"%", 1, 0, 0, 100, 0 uint16_t pwmFrequency;+Select a frequency to run PWM at.\nSet this to 0hz to enable on-off mode.;"hz", 1, 0, 0, 500, 0 diff --git a/firmware/util/containers/table_helper.h b/firmware/util/containers/table_helper.h index 37ad332c7c..bc2842f7ca 100644 --- a/firmware/util/containers/table_helper.h +++ b/firmware/util/containers/table_helper.h @@ -30,7 +30,7 @@ public: explicit Map3D(const char*name); Map3D(const char*name, float multiplier); void init(vType table[RPM_BIN_SIZE][LOAD_BIN_SIZE], const kType loadBins[LOAD_BIN_SIZE], const kType rpmBins[RPM_BIN_SIZE]); - float getValue(float xRpm, float y) const; + float getValue(float xRpm, float y) const override; void setAll(vType value); vType *pointers[LOAD_BIN_SIZE]; private: @@ -161,7 +161,7 @@ typedef Map3D pedal2tps_ typedef Map3D boostOpenLoop_Map3D_t; typedef Map3D boostClosedLoop_Map3D_t; typedef Map3D iacPidMultiplier_t; -typedef Map3D gppwm_Map3D_t; +typedef Map3D gppwm_Map3D_t; void setRpmBin(float array[], int size, float idleRpm, float topRpm); diff --git a/unit_tests/mocks.h b/unit_tests/mocks.h index 294f142dd4..65194ddd69 100644 --- a/unit_tests/mocks.h +++ b/unit_tests/mocks.h @@ -1,6 +1,7 @@ #include "electronic_throttle.h" #include "dc_motor.h" #include "table_helper.h" +#include "pwm_generator_logic.h" #include "gmock/gmock.h" @@ -37,3 +38,8 @@ class MockVp3d : public ValueProvider3D { public: MOCK_METHOD(float, getValue, (float xRpm, float y), (const, override)); }; + +class MockPwm : public SimplePwm { +public: + MOCK_METHOD(void, setSimplePwmDutyCycle, (float dutyCycle), (override)); +}; diff --git a/unit_tests/tests/test_gppwm.cpp b/unit_tests/tests/test_gppwm.cpp new file mode 100644 index 0000000000..0ab1cc8925 --- /dev/null +++ b/unit_tests/tests/test_gppwm.cpp @@ -0,0 +1,99 @@ + +#include "engine_test_helper.h" +#include "gppwm_channel.h" +#include "gppwm.h" +#include "sensor.h" + +#include "mocks.h" + +using ::testing::InSequence; + +TEST(GpPwm, OutputWithPwm) { + GppwmChannel ch; + + gppwm_channel cfg; + + MockPwm pwm; + + // Shouldn't throw with no config + EXPECT_NO_THROW(ch.setOutput(10)); + + { + InSequence i; + EXPECT_CALL(pwm, setSimplePwmDutyCycle(0.25f)); + EXPECT_CALL(pwm, setSimplePwmDutyCycle(0.75f)); + EXPECT_CALL(pwm, setSimplePwmDutyCycle(0.0f)); + EXPECT_CALL(pwm, setSimplePwmDutyCycle(1.0f)); + } + + ch.init(true, &pwm, nullptr, &cfg); + + // Set the output - should set directly to PWM + ch.setOutput(25.0f); + ch.setOutput(75.0f); + + // Test clamping behavior - should clamp to [0, 100] + ch.setOutput(-10.0f); + ch.setOutput(110.0f); +} + +TEST(GpPwm, OutputOnOff) { + GppwmChannel ch; + + gppwm_channel cfg; + cfg.onAboveDuty = 50; + cfg.offBelowDuty = 40; + + MockPwm pwm; + + { + InSequence i; + EXPECT_CALL(pwm, setSimplePwmDutyCycle(0.0f)); + EXPECT_CALL(pwm, setSimplePwmDutyCycle(1.0f)); + EXPECT_CALL(pwm, setSimplePwmDutyCycle(1.0f)); + EXPECT_CALL(pwm, setSimplePwmDutyCycle(1.0f)); + EXPECT_CALL(pwm, setSimplePwmDutyCycle(0.0f)); + EXPECT_CALL(pwm, setSimplePwmDutyCycle(0.0f)); + } + + ch.init(false, &pwm, nullptr, &cfg); + + // Test rising edge - these should output 0, 1, 1 + ch.setOutput(49.0f); + ch.setOutput(51.0f); + ch.setOutput(49.0f); + + // Test falling edge - these should output 1, 0, 0 + ch.setOutput(41.0f); + ch.setOutput(39.0f); + ch.setOutput(41.0f); +} + +TEST(GpPwm, GetOutput) { + WITH_ENGINE_TEST_HELPER(TEST_ENGINE); + GppwmChannel ch; + INJECT_ENGINE_REFERENCE(&ch); + + gppwm_channel cfg; + cfg.loadAxis = GPPWM_Tps; + cfg.dutyIfError = 21.0f; + + MockVp3d table; + + engine->rpmCalculator.mockRpm = 1200; + EXPECT_CALL(table, getValue(1200 / RPM_1_BYTE_PACKING_MULT, 35.0f)) + .WillRepeatedly([](float x, float tps) { + return tps; + }); + + ch.init(false, nullptr, &table, &cfg); + + Sensor::resetAllMocks(); + + // Should return dutyIfError + EXPECT_FLOAT_EQ(21.0f, ch.getOutput()); + + // Set TPS, should return tps value + Sensor::setMockValue(SensorType::Tps1, 35.0f); + EXPECT_FLOAT_EQ(35.0f, ch.getOutput()); +} diff --git a/unit_tests/tests/tests.mk b/unit_tests/tests/tests.mk index 2ae4d56354..b48dfb3882 100644 --- a/unit_tests/tests/tests.mk +++ b/unit_tests/tests/tests.mk @@ -49,4 +49,5 @@ TESTS_SRC_CPP = \ tests/sensor/redundant.cpp \ tests/sensor/test_sensor_init.cpp \ tests/test_closed_loop_controller.cpp \ + tests/test_gppwm.cpp \