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

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

firmware/controllers/actuators/gppwm/gppwm.h

@@ -0,0 +1,6 @@
+#pragma once
+
+#include "engine.h"
+
+void initGpPwm(DECLARE_ENGINE_PARAMETER_SIGNATURE);
+void updateGppwm();

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<float> 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<float> 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);
+}

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

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)

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

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

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 \
 

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 */

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

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
 

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:

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

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

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 \