vvt is engine module

* vvt refactoring

* vvt constexpr livedata

* format

* fix null deref

firmware/console/binary/live_data.cpp

@@ -134,6 +134,21 @@ const trigger_state_s* getLiveData(size_t idx) {
 #endif
 }
 
+template<>
+const vvt_s* getLiveData(size_t idx) {
+#if EFI_VVT_PID
+	switch (idx) {
+		case 0: return &engine->module<VvtController1>().unmock();
+		case 1: return &engine->module<VvtController2>().unmock();
+		case 2: return &engine->module<VvtController3>().unmock();
+		case 3: return &engine->module<VvtController4>().unmock();
+		default: return nullptr;
+	}
+#else
+	return nullptr;
+#endif
+}
+
 template<>
 const trigger_state_primary_s* getLiveData(size_t) {
 #if EFI_SHAFT_POSITION_INPUT

firmware/controllers/actuators/vvt.cpp

@@ -18,23 +18,27 @@ using vvt_map_t = Map3D<SCRIPT_TABLE_8, SCRIPT_TABLE_8, int8_t, uint16_t, uint16
 static vvt_map_t vvtTable1;
 static vvt_map_t vvtTable2;
 
-void VvtController::init(int index, int bankIndex, int camIndex, const ValueProvider3D* targetMap) {
-	this->index = index;
-	m_bank = bankIndex;
-	m_cam = camIndex;
+VvtController::VvtController(int index, int bankIndex, int camIndex)
+	: index(index)
+	, m_bank(bankIndex)
+	, m_cam(camIndex)
+{
+}
 
+void VvtController::init(const ValueProvider3D* targetMap, IPwm* pwm) {
 	// Use the same settings for the Nth cam in every bank (ie, all exhaust cams use the same PID)
-	m_pid.initPidClass(&engineConfiguration->auxPid[camIndex]);
+	m_pid.initPidClass(&engineConfiguration->auxPid[index]);
 
 	m_targetMap = targetMap;
+	m_pwm = pwm;
 }
 
-int VvtController::getPeriodMs() {
-	return isBrainPinValid(engineConfiguration->vvtPins[index]) ?
-		GET_PERIOD_LIMITED(&engineConfiguration->auxPid[index]) : NO_PIN_PERIOD;
-}
+void VvtController::onFastCallback() {
+	if (!m_pwm || !m_targetMap) {
+		// not init yet
+		return;
+	}
 
-void VvtController::PeriodicTask() {
 	if (engine->vvtParametersVersion.isOld(engine->getGlobalConfigurationVersion())) {
 		m_pid.reset();
 	}
@@ -106,9 +110,9 @@ void VvtController::setOutput(expected<percent_t> outputValue) {
 	vvtOutput = outputValue.value_or(0);
 
 	if (outputValue && enabled) {
-		m_pwm.setSimplePwmDutyCycle(PERCENT_TO_DUTY(outputValue.Value));
+		m_pwm->setSimplePwmDutyCycle(PERCENT_TO_DUTY(outputValue.Value));
 	} else {
-		m_pwm.setSimplePwmDutyCycle(0);
+		m_pwm->setSimplePwmDutyCycle(0);
 
 		// we need to avoid accumulating iTerm while engine is not running
 		m_pid.reset();
@@ -131,11 +135,11 @@ static const char *vvtOutputNames[CAM_INPUTS_COUNT] = {
 #endif
  };
 
-
-static VvtController instances[CAM_INPUTS_COUNT];
+static OutputPin vvtPins[CAM_INPUTS_COUNT];
+static SimplePwm vvtPwms[CAM_INPUTS_COUNT];
 
 OutputPin* getVvtOutputPin(int index) {
-    return &instances[index].m_pin;
+    return &vvtPins[index];
 }
 
 static void applyVvtPinState(int stateIndex, PwmConfig *state) /* pwm_gen_callback */ {
@@ -151,7 +155,7 @@ static void turnVvtPidOn(int index) {
 		return;
 	}
 
-	startSimplePwmExt(&instances[index].m_pwm, vvtOutputNames[index],
+	startSimplePwmExt(&vvtPwms[index], vvtOutputNames[index],
 			&engine->executor,
 			engineConfiguration->vvtPins[index],
 			getVvtOutputPin(index),
@@ -181,32 +185,13 @@ void initVvtActuators() {
 	vvtTable2.init(config->vvtTable2, config->vvtTable2LoadBins,
 			config->vvtTable2RpmBins);
 
-	for (int i = 0;i < CAM_INPUTS_COUNT;i++) {
 
-		int camIndex = i % CAMS_PER_BANK;
-		int bankIndex = i / CAMS_PER_BANK;
-		auto targetMap = camIndex == 0 ? &vvtTable1 : &vvtTable2;
-		instances[i].init(i, bankIndex, camIndex, targetMap);
-	}
+	engine->module<VvtController1>()->init(&vvtTable1, &vvtPwms[0]);
+	engine->module<VvtController2>()->init(&vvtTable2, &vvtPwms[1]);
+	engine->module<VvtController3>()->init(&vvtTable1, &vvtPwms[2]);
+	engine->module<VvtController4>()->init(&vvtTable2, &vvtPwms[3]);
 
 	startVvtControlPins();
-
-	for (int i = 0;i < CAM_INPUTS_COUNT;i++) {
-		instances[i].start();
-	}
 }
 
 #endif
-
-template<>
-const vvt_s* getLiveData(size_t idx) {
-#if EFI_AUX_PID
-	if (idx >= efi::size(instances)) {
-		return nullptr;
-	}
-
-	return &instances[idx];
-#else
-	return nullptr;
-#endif
-}

firmware/controllers/actuators/vvt.h

@@ -20,13 +20,14 @@ void startVvtControlPins();
 void stopVvtControlPins();
 OutputPin* getVvtOutputPin(int index);
 
-class VvtController : public PeriodicTimerController, public ClosedLoopController<angle_t, percent_t>, public vvt_s {
+class VvtController : public EngineModule, public ClosedLoopController<angle_t, percent_t>, public vvt_s {
 public:
-	void init(int index, int bankIndex, int camIndex, const ValueProvider3D* targetMap);
+	VvtController(int index, int bankIndex, int camIndex);
 
-	// PeriodicTimerController implementation
-	int getPeriodMs() override;
-	void PeriodicTask() override;
+	void init(const ValueProvider3D* targetMap, IPwm* pwm);
+
+	// EngineModule implementation
+	void onFastCallback() override;
 
 	// ClosedLoopController implementation
 	expected<angle_t> observePlant() const override;
@@ -37,18 +38,31 @@ public:
 	void setOutput(expected<percent_t> outputValue) override;
 
 private:
-	Pid m_pid;
-	const ValueProvider3D* m_targetMap = nullptr;
-	int index = 0;
-
+	const int index = 0;
 	// Bank index, 0 or 1
-	uint8_t m_bank = 0;
-
+	const uint8_t m_bank = 0;
 	// Cam index, 0 = intake, 1 = exhaust
-	uint8_t m_cam = 0;
+	const uint8_t m_cam = 0;
 
-public:
-	// todo: encapsulate or inject these
-	SimplePwm m_pwm;
-	OutputPin m_pin;
+	Pid m_pid;
+
+	const ValueProvider3D* m_targetMap = nullptr;
+	IPwm* m_pwm = nullptr;
+};
+
+// Unique types for each VVT so they can be engine modules
+struct VvtController1 : public VvtController {
+	VvtController1() : VvtController(0, 0, 0) { }
+};
+
+struct VvtController2 : public VvtController {
+	VvtController2() : VvtController(1, 0, 1) { }
+};
+
+struct VvtController3 : public VvtController {
+	VvtController3() : VvtController(2, 1, 0) { }
+};
+
+struct VvtController4 : public VvtController {
+	VvtController4() : VvtController(3, 1, 1) { }
 };

firmware/controllers/algo/engine.h

@@ -51,6 +51,7 @@
 #include "gc_generic.h"
 #include "lambda_monitor.h"
 #include "efi_output.h"
+#include "vvt.h"
 
 #ifndef EFI_UNIT_TEST
 #error EFI_UNIT_TEST must be defined!
@@ -157,6 +158,12 @@ public:
 		KnockController,
 		SensorChecker,
 		LimpManager,
+#if EFI_VVT_PID
+		VvtController1,
+		VvtController2,
+		VvtController3,
+		VvtController4,
+#endif // EFI_VVT_PID
 		EngineModule // dummy placeholder so the previous entries can all have commas
 		> engineModules;
 

firmware/integration/LiveData.yaml

@@ -181,6 +181,9 @@ Usages:
   - name: vvt
     java: VvtState.java
     folder: controllers/actuators
+    constexpr: "___engine.module<VvtController1>()"
+    isPtr: true
+    conditional_compilation: "EFI_VVT_PID"
 
   - name: lambda_monitor
     java: LambdaMonitor.java

unit_tests/tests/actuators/test_vvt.cpp

@@ -17,8 +17,8 @@ TEST(Vvt, TestSetPoint) {
 	engine->engineState.fuelingLoad = 55;
 	Sensor::setMockValue(SensorType::Rpm,  4321);
 
-	VvtController dut;
-	dut.init(0, 0, 0, &targetMap);
+	VvtController dut(0, 0, 0);
+	dut.init(&targetMap, nullptr);
 
 	// Test dut
 	EXPECT_EQ(20, dut.getSetpoint().value_or(0));
@@ -29,14 +29,14 @@ TEST(Vvt, observePlant) {
 
 	engine->triggerCentral.vvtPosition[0][0] = 23;
 
-	VvtController dut;
-	dut.init(0, 0, 0, nullptr);
+	VvtController dut(0, 0, 0);
+	dut.init(nullptr, nullptr);
 
 	EXPECT_EQ(23, dut.observePlant().value_or(0));
 }
 
 TEST(Vvt, openLoop) {
-	VvtController dut;
+	VvtController dut(0, 0, 0);
 
 	// No open loop for now
 	EXPECT_EQ(dut.getOpenLoop(10), 0);
@@ -45,8 +45,8 @@ TEST(Vvt, openLoop) {
 TEST(Vvt, ClosedLoopNotInverted) {
 	EngineTestHelper eth(engine_type_e::TEST_ENGINE);
 
-	VvtController dut;
-	dut.init(0, 0, 0, nullptr);
+	VvtController dut(0, 0, 0);
+	dut.init(nullptr, nullptr);
 
 	engineConfiguration->auxPid[0].pFactor = 1.5f;
 	engineConfiguration->auxPid[0].iFactor = 0;
@@ -60,8 +60,8 @@ TEST(Vvt, ClosedLoopNotInverted) {
 TEST(Vvt, ClosedLoopInverted) {
 	EngineTestHelper eth(engine_type_e::TEST_ENGINE);
 
-	VvtController dut;
-	dut.init(0, 0, 0, nullptr);
+	VvtController dut(0, 0, 0);
+	dut.init(nullptr, nullptr);
 
 	engineConfiguration->invertVvtControlIntake = true;
 	engineConfiguration->auxPid[0].pFactor = 1.5f;