From 6d8eedf9cafed46ca7f71def1a3d44bf181c1739 Mon Sep 17 00:00:00 2001 From: Matthew Kennedy Date: Tue, 3 Mar 2020 14:56:50 -0800 Subject: [PATCH] Split out EtbHardware (#1168) * start tear out motor * fix * nuke reconfig * guard for test * test fixing * format, condense headers * fix todos * undo todo * format --- firmware/controllers/actuators/dc_motors.cpp | 129 +++++++++++++ firmware/controllers/actuators/dc_motors.h | 24 +++ .../actuators/electronic_throttle.cpp | 171 +++--------------- .../actuators/electronic_throttle.h | 3 - firmware/controllers/controllers.mk | 1 + firmware/hw_layer/hardware.cpp | 14 -- unit_tests/tests/test_etb.cpp | 25 ++- unit_tests/tests/test_idle_controller.cpp | 21 --- 8 files changed, 199 insertions(+), 189 deletions(-) create mode 100644 firmware/controllers/actuators/dc_motors.cpp create mode 100644 firmware/controllers/actuators/dc_motors.h diff --git a/firmware/controllers/actuators/dc_motors.cpp b/firmware/controllers/actuators/dc_motors.cpp new file mode 100644 index 0000000000..e5edd33863 --- /dev/null +++ b/firmware/controllers/actuators/dc_motors.cpp @@ -0,0 +1,129 @@ +/** + * @file dc_motors.cpp + * + * @date March 3, 2020 + * @author Matthew Kennedy (c) 2020 + */ + +#include "engine.h" +#include "io_pins.h" +#include "engine_configuration.h" +#include "engine_controller.h" +#include "periodic_task.h" + +#include "dc_motors.h" +#include "dc_motor.h" + +#include "efi_gpio.h" +#include "pwm_generator.h" + +EXTERN_ENGINE; + +class EtbHardware { +private: + OutputPin m_pinEnable; + OutputPin m_pinDir1; + OutputPin m_pinDir2; + + SimplePwm m_pwmEnable; + SimplePwm m_pwmDir1; + SimplePwm m_pwmDir2; + + SimplePwm etbPwmUp; + +public: + EtbHardware() : etbPwmUp("etbUp"), dcMotor(&m_pwmEnable, &m_pwmDir1, &m_pwmDir2) {} + + TwoPinDcMotor dcMotor; + + void setFrequency(int frequency) { + m_pwmEnable.setFrequency(frequency); + m_pwmDir1.setFrequency(frequency); + m_pwmDir2.setFrequency(frequency); + } + + void start(bool useTwoWires, + brain_pin_e pinEnable, + // since we have pointer magic here we cannot simply have value parameter + const pin_output_mode_e *pinEnableMode, + brain_pin_e pinDir1, + brain_pin_e pinDir2, + ExecutorInterface* executor, + int frequency) { + dcMotor.setType(useTwoWires ? TwoPinDcMotor::ControlType::PwmDirectionPins : TwoPinDcMotor::ControlType::PwmEnablePin); + + m_pinEnable.initPin("ETB Enable", pinEnable, pinEnableMode); + m_pinDir1.initPin("ETB Dir 1", pinDir1); + m_pinDir2.initPin("ETB Dir 2", pinDir2); + + // Clamp to >100hz + int clampedFrequency = maxI(100, frequency); + +// no need to complicate event queue with ETB PWM in unit tests +#if ! EFI_UNIT_TEST + startSimplePwm(&m_pwmEnable, "ETB Enable", + executor, + &m_pinEnable, + clampedFrequency, + 0, + (pwm_gen_callback*)applyPinState + ); + + startSimplePwm(&m_pwmDir1, "ETB Dir 1", + executor, + &m_pinDir1, + clampedFrequency, + 0, + (pwm_gen_callback*)applyPinState + ); + + startSimplePwm(&m_pwmDir2, "ETB Dir 2", + executor, + &m_pinDir2, + clampedFrequency, + 0, + (pwm_gen_callback*)applyPinState + ); +#endif /* EFI_UNIT_TEST */ + } +}; + +static EtbHardware etbHardware[ETB_COUNT]; + +DcMotor* initDcMotor(size_t index DECLARE_ENGINE_PARAMETER_SUFFIX) +{ + const auto& io = engineConfiguration->etbIo[index]; + auto& hw = etbHardware[index]; + + // controlPinMode is a strange feature - it's simply because I am short on 5v I/O on Frankenso with Miata NB2 test mule + hw.start( + CONFIG(etb_use_two_wires), + io.controlPin1, + &io.controlPinMode, + io.directionPin1, + io.directionPin2, + &ENGINE(executor), + CONFIG(etbFreq) + ); + + return &hw.dcMotor; +} + +void setDcMotorFrequency(size_t index, int hz) { + etbHardware[index].setFrequency(hz); +} + +void setDcMotorDuty(size_t index, float duty) { + etbHardware[index].dcMotor.set(duty); +} + +#if EFI_PROD_CODE +void showDcMotorInfo(Logging* logger) { + for (int i = 0 ; i < engine->etbActualCount; i++) { + EtbHardware *etb = &etbHardware[i]; + + scheduleMsg(logger, "ETB %d", i); + scheduleMsg(logger, "Motor: dir=%d DC=%f", etb->dcMotor.isOpenDirection(), etb->dcMotor.get()); + } +} +#endif \ No newline at end of file diff --git a/firmware/controllers/actuators/dc_motors.h b/firmware/controllers/actuators/dc_motors.h new file mode 100644 index 0000000000..0d9cfd4100 --- /dev/null +++ b/firmware/controllers/actuators/dc_motors.h @@ -0,0 +1,24 @@ +/** + * @file dc_motors.h + * + * @date March 3, 2020 + * @author Matthew Kennedy (c) 2020 + */ + +#pragma once + +#include +#include "global.h" + +class DcMotor; +class Logger; + +DcMotor* initDcMotor(size_t index DECLARE_ENGINE_PARAMETER_SUFFIX); + +// Manual control of motors for use by console commands +void setDcMotorFrequency(size_t index, int hz); +void setDcMotorDuty(size_t index, float duty); + +#if EFI_PROD_CODE +void showDcMotorInfo(Logging* logger); +#endif diff --git a/firmware/controllers/actuators/electronic_throttle.cpp b/firmware/controllers/actuators/electronic_throttle.cpp index 81e4e426e2..09384a4a14 100644 --- a/firmware/controllers/actuators/electronic_throttle.cpp +++ b/firmware/controllers/actuators/electronic_throttle.cpp @@ -83,8 +83,8 @@ #include "engine_controller.h" #include "periodic_task.h" #include "pin_repository.h" -#include "pwm_generator.h" #include "dc_motor.h" +#include "dc_motors.h" #include "pid_auto_tune.h" #if defined(HAS_OS_ACCESS) @@ -108,74 +108,6 @@ static bool startupPositionError = false; #define STARTUP_NEUTRAL_POSITION_ERROR_THRESHOLD 5 -class EtbHardware { -private: - OutputPin m_pinEnable; - OutputPin m_pinDir1; - OutputPin m_pinDir2; - - SimplePwm m_pwmEnable; - SimplePwm m_pwmDir1; - SimplePwm m_pwmDir2; - - SimplePwm etbPwmUp; - -public: - EtbHardware() : etbPwmUp("etbUp"), dcMotor(&m_pwmEnable, &m_pwmDir1, &m_pwmDir2) {} - - TwoPinDcMotor dcMotor; - - void setFrequency(int frequency) { - m_pwmEnable.setFrequency(frequency); - m_pwmDir1.setFrequency(frequency); - m_pwmDir2.setFrequency(frequency); - } - - void start(bool useTwoWires, - brain_pin_e pinEnable, - // since we have pointer magic here we cannot simply have value parameter - pin_output_mode_e *pinEnableMode, - brain_pin_e pinDir1, - brain_pin_e pinDir2, - ExecutorInterface* executor, - int frequency) { - dcMotor.setType(useTwoWires ? TwoPinDcMotor::ControlType::PwmDirectionPins : TwoPinDcMotor::ControlType::PwmEnablePin); - - m_pinEnable.initPin("ETB Enable", pinEnable, pinEnableMode); - m_pinDir1.initPin("ETB Dir 1", pinDir1); - m_pinDir2.initPin("ETB Dir 2", pinDir2); - - // Clamp to >100hz - int clampedFrequency = maxI(100, frequency); - - -// no need to complicate event queue with ETB PWM in unit tests -#if ! EFI_UNIT_TEST - startSimplePwm(&m_pwmEnable, "ETB Enable", - executor, - &m_pinEnable, - clampedFrequency, - 0, - (pwm_gen_callback*)applyPinState); - - startSimplePwm(&m_pwmDir1, "ETB Dir 1", - executor, - &m_pinDir1, - clampedFrequency, - 0, - (pwm_gen_callback*)applyPinState); - - startSimplePwm(&m_pwmDir2, "ETB Dir 2", - executor, - &m_pinDir2, - clampedFrequency, - 0, - (pwm_gen_callback*)applyPinState); -#endif /* EFI_UNIT_TEST */ - } -}; - - extern percent_t mockPedalPosition; static percent_t directPwmValue = NAN; @@ -196,7 +128,7 @@ void EtbController::reset() { } void EtbController::onConfigurationChange(pid_s* previousConfiguration) { - if (m_pid.isSame(previousConfiguration)) { + if (m_motor && m_pid.isSame(previousConfiguration)) { m_shouldResetPid = true; } } @@ -359,7 +291,6 @@ DISPLAY(DISPLAY_IF(hasEtbPedalPositionSensor)) } } -static EtbHardware etbHardware[ETB_COUNT]; // real implementation (we mock for some unit tests) EtbController etbControllers[ETB_COUNT]; @@ -390,13 +321,7 @@ static void showEthInfo(void) { scheduleMsg(&logger, "dir1=%s", hwPortname(CONFIG(etbIo[0].directionPin1))); scheduleMsg(&logger, "dir2=%s", hwPortname(CONFIG(etbIo[0].directionPin2))); - for (int i = 0 ; i < engine->etbActualCount; i++) { - EtbHardware *etb = &etbHardware[i]; - - scheduleMsg(&logger, "ETB %d", i); - scheduleMsg(&logger, "Motor: dir=%d DC=%f", etb->dcMotor.isOpenDirection(), etb->dcMotor.get()); - etbControllers[i].showStatus(&logger); - } + showDcMotorInfo(&logger); #endif /* EFI_PROD_CODE */ } @@ -425,7 +350,7 @@ void setThrottleDutyCycle(percent_t level) { float dc = ETB_PERCENT_TO_DUTY(level); directPwmValue = dc; for (int i = 0 ; i < engine->etbActualCount; i++) { - etbHardware[i].dcMotor.set(dc); + setDcMotorDuty(i, dc); } scheduleMsg(&logger, "duty ETB duty=%f", dc); } @@ -434,7 +359,7 @@ static void setEtbFrequency(int frequency) { engineConfiguration->etbFreq = frequency; for (int i = 0 ; i < engine->etbActualCount; i++) { - etbHardware[i].setFrequency(frequency); + setDcMotorFrequency(i, frequency); } } @@ -442,7 +367,7 @@ static void etbReset() { scheduleMsg(&logger, "etbReset"); for (int i = 0 ; i < engine->etbActualCount; i++) { - etbHardware[i].dcMotor.set(0); + setDcMotorDuty(i, 0); } etbPidReset(); @@ -549,63 +474,12 @@ void setDefaultEtbParameters(DECLARE_CONFIG_PARAMETER_SIGNATURE) { engineConfiguration->etb.maxValue = 200; } -static bool isEtbPinsChanged(etb_io *current, etb_io *active) { - return current->controlPin1 != active->controlPin1 || - current->controlPinMode != active->controlPinMode || - current->directionPin1 != active->directionPin1 || - current->directionPin2 != active->directionPin2; -} - -#if EFI_PROD_CODE -bool isETBRestartNeeded(void) { - for (int i = 0 ; i < ETB_COUNT; i++) { - /** - * We do not want any interruption in HW pin while adjusting other properties - */ - bool changed = isEtbPinsChanged(&engineConfiguration->etbIo[i], &activeConfiguration.etbIo[i]); - if (changed) { - return changed; - } - } - return false; -} - -void stopETBPins(void) { - for (int i = 0 ; i < ETB_COUNT; i++) { - etb_io *activeIo = &activeConfiguration.etbIo[i]; - brain_pin_markUnused(activeIo->controlPin1); - brain_pin_markUnused(activeIo->directionPin1); - brain_pin_markUnused(activeIo->directionPin2); - } -} -#endif /* EFI_PROD_CODE */ - void onConfigurationChangeElectronicThrottleCallback(engine_configuration_s *previousConfiguration) { for (int i = 0; i < ETB_COUNT; i++) { etbControllers[i].onConfigurationChange(&previousConfiguration->etb); } } -void startETBPins(DECLARE_ENGINE_PARAMETER_SIGNATURE) { - - /** - * safer to start 2nd ETB even if 2nd TPS is not configured by mistake - */ - for (int i = 0 ; i < ETB_COUNT; i++) { - etb_io *io = &engineConfiguration->etbIo[i]; - // controlPinMode is a strange feature - it's simply because I am short on 5v I/O on Frankenso with Miata NB2 test mule - etbHardware[i].start( - CONFIG(etb_use_two_wires), - io->controlPin1, - &io->controlPinMode, - io->directionPin1, - io->directionPin2, - &ENGINE(executor), - CONFIG(etbFreq) - ); - } -} - #if EFI_PROD_CODE && 0 static void setTempOutput(float value) { autoTune.output = value; @@ -679,20 +553,28 @@ void doInitElectronicThrottle(DECLARE_ENGINE_PARAMETER_SIGNATURE) { addConsoleActionI("etb_freq", setEtbFrequency); #endif /* EFI_PROD_CODE */ - for (int i = 0 ; i < ETB_COUNT; i++) { - engine->etbControllers[i]->init(&etbHardware[i].dcMotor, i, &engineConfiguration->etb); - INJECT_ENGINE_REFERENCE(engine->etbControllers[i]); - } - - - pedal2tpsMap.init(config->pedalToTpsTable, config->pedalToTpsPedalBins, config->pedalToTpsRpmBins); - engine->engineState.hasEtbPedalPositionSensor = hasPedalPositionSensor(PASS_ENGINE_PARAMETER_SIGNATURE); if (!engine->engineState.hasEtbPedalPositionSensor) { +#if EFI_PROD_CODE + // TODO: Once switched to new sensor model for pedal, we don't need this to be test-guarded. return; +#endif } engine->etbActualCount = hasSecondThrottleBody(PASS_ENGINE_PARAMETER_SIGNATURE) ? 2 : 1; + for (int i = 0 ; i < engine->etbActualCount; i++) { + auto motor = initDcMotor(i PASS_ENGINE_PARAMETER_SUFFIX); + + // If this motor is actually set up, init the etb + if (motor) + { + engine->etbControllers[i]->init(motor, i, &engineConfiguration->etb); + INJECT_ENGINE_REFERENCE(engine->etbControllers[i]); + } + } + + pedal2tpsMap.init(config->pedalToTpsTable, config->pedalToTpsPedalBins, config->pedalToTpsRpmBins); + #if 0 // not alive code autoTune.SetOutputStep(0.1); @@ -711,24 +593,19 @@ void doInitElectronicThrottle(DECLARE_ENGINE_PARAMETER_SIGNATURE) { } #endif /* EFI_UNIT_TEST */ - startETBPins(PASS_ENGINE_PARAMETER_SIGNATURE); - #if EFI_PROD_CODE if (engineConfiguration->etbCalibrationOnStart) { for (int i = 0 ; i < engine->etbActualCount; i++) { - EtbHardware *etb = &etbHardware[i]; - - etb->dcMotor.set(70); + setDcMotorDuty(i, 70); chThdSleep(600); // todo: grab with proper index grabTPSIsWideOpen(); - etb->dcMotor.set(-70); + setDcMotorDuty(i, -70); chThdSleep(600); // todo: grab with proper index grabTPSIsClosed(); } - } // manual duty cycle control without PID. Percent value from 0 to 100 diff --git a/firmware/controllers/actuators/electronic_throttle.h b/firmware/controllers/actuators/electronic_throttle.h index b5cb156a50..6957c2d297 100644 --- a/firmware/controllers/actuators/electronic_throttle.h +++ b/firmware/controllers/actuators/electronic_throttle.h @@ -61,8 +61,5 @@ void setEtbIFactor(float value); void setEtbDFactor(float value); void setEtbOffset(int value); void setThrottleDutyCycle(percent_t level); -bool isETBRestartNeeded(void); -void stopETBPins(void); -void startETBPins(DECLARE_ENGINE_PARAMETER_SIGNATURE); void onConfigurationChangeElectronicThrottleCallback(engine_configuration_s *previousConfiguration); void unregisterEtbPins(); diff --git a/firmware/controllers/controllers.mk b/firmware/controllers/controllers.mk index a2f6683766..ff0bc0b449 100644 --- a/firmware/controllers/controllers.mk +++ b/firmware/controllers/controllers.mk @@ -14,6 +14,7 @@ CONTROLLERS_SRC_CPP = \ $(CONTROLLERS_DIR)/actuators/electronic_throttle.cpp \ $(CONTROLLERS_DIR)/actuators/alternator_controller.cpp \ $(CONTROLLERS_DIR)/actuators/boost_control.cpp \ + $(CONTROLLERS_DIR)/actuators/dc_motors.cpp \ $(CONTROLLERS_DIR)/actuators/idle_thread.cpp \ $(CONTROLLERS_DIR)/actuators/pwm_tester.cpp \ $(CONTROLLERS_DIR)/actuators/algo/aux_pid.cpp \ diff --git a/firmware/hw_layer/hardware.cpp b/firmware/hw_layer/hardware.cpp index d8b019f3a6..035a71ed41 100644 --- a/firmware/hw_layer/hardware.cpp +++ b/firmware/hw_layer/hardware.cpp @@ -33,7 +33,6 @@ //#include "usb_msd.h" #include "AdcConfiguration.h" -#include "electronic_throttle.h" #include "idle_thread.h" #include "mcp3208.h" #include "hip9011.h" @@ -310,13 +309,6 @@ void applyNewHardwareSettings(void) { } #endif -#if EFI_ELECTRONIC_THROTTLE_BODY - bool etbRestartNeeded = isETBRestartNeeded(); - if (etbRestartNeeded) { - stopETBPins(); - } -#endif /* EFI_ELECTRONIC_THROTTLE_BODY */ - #if (BOARD_TLE6240_COUNT > 0) stopSmartCsPins(); #endif /* (BOARD_MC33972_COUNT > 0) */ @@ -383,12 +375,6 @@ void applyNewHardwareSettings(void) { } #endif -#if EFI_ELECTRONIC_THROTTLE_BODY - if (etbRestartNeeded) { - startETBPins(PASS_ENGINE_PARAMETER_SIGNATURE); - } -#endif /* EFI_ELECTRONIC_THROTTLE_BODY */ - #if EFI_VEHICLE_SPEED startVSSPins(); #endif /* EFI_VEHICLE_SPEED */ diff --git a/unit_tests/tests/test_etb.cpp b/unit_tests/tests/test_etb.cpp index 5e8dcf7df4..e781ab636a 100644 --- a/unit_tests/tests/test_etb.cpp +++ b/unit_tests/tests/test_etb.cpp @@ -8,6 +8,8 @@ #include "engine_test_helper.h" #include "electronic_throttle.h" #include "dc_motor.h" +#include "engine_controller.h" +#include "tps.h" class MockEtb : public IEtbController { public: @@ -48,7 +50,7 @@ TEST(etb, singleEtbInitialization) { engine->etbControllers[i] = &mocks[i]; } - engineConfiguration->throttlePedalPositionAdcChannel = EFI_ADC_9; + engineConfiguration->throttlePedalPositionAdcChannel = EFI_ADC_9; doInitElectronicThrottle(PASS_ENGINE_PARAMETER_SIGNATURE); @@ -56,16 +58,31 @@ TEST(etb, singleEtbInitialization) { ASSERT_EQ(1, mocks[0].initCount) << "1st init"; ASSERT_EQ(1, mocks[0].startCount); - // assert that 2nd ETB is initialized but not started - ASSERT_EQ(1, mocks[1].initCount) << "2nd init"; + // assert that 2nd ETB is neither initialized nor started + ASSERT_EQ(0, mocks[1].initCount) << "2nd init"; ASSERT_EQ(0, mocks[1].startCount) << "2nd start"; // todo: set mock pedal position // todo: set mock ETB throttle position // todo: invoke EtbController#PeriodicTask a few times and assert that duty cycle changes - } +TEST(idle, testTargetTpsIsFloatBug945) { + WITH_ENGINE_TEST_HELPER(TEST_ENGINE); + doInitElectronicThrottle(PASS_ENGINE_PARAMETER_SIGNATURE); + setMockThrottlePedalSensorVoltage(3 PASS_ENGINE_PARAMETER_SUFFIX); + engine->etbControllers[0]->PeriodicTask(); + ASSERT_NEAR(50, engine->engineState.targetFromTable, EPS4D); + + setMockThrottlePedalSensorVoltage(3.05 PASS_ENGINE_PARAMETER_SUFFIX); + ASSERT_NEAR(50.8302, getPedalPosition(PASS_ENGINE_PARAMETER_SIGNATURE), EPS4D); + engine->etbControllers[0]->PeriodicTask(); + ASSERT_NEAR(50.8302, engine->engineState.targetFromTable, EPS4D); + + setMockThrottlePedalSensorVoltage(3.1 PASS_ENGINE_PARAMETER_SUFFIX); + engine->etbControllers[0]->PeriodicTask(); + ASSERT_NEAR(51.6605, engine->engineState.targetFromTable, EPS4D); +} diff --git a/unit_tests/tests/test_idle_controller.cpp b/unit_tests/tests/test_idle_controller.cpp index 573b8665c3..4e1ee6c778 100644 --- a/unit_tests/tests/test_idle_controller.cpp +++ b/unit_tests/tests/test_idle_controller.cpp @@ -127,24 +127,3 @@ TEST(idle, timingPid) { ASSERT_FLOAT_EQ(-5.0f, corr) << "getAdvanceCorrections#7"; } - -// not great that we are reusing shared instance. todo: move EtbController to Engine? - -TEST(idle, testTargetTpsIsFloatBug945) { - - WITH_ENGINE_TEST_HELPER(TEST_ENGINE); - - setMockThrottlePedalSensorVoltage(3 PASS_ENGINE_PARAMETER_SUFFIX); - engine->etbControllers[0]->PeriodicTask(); - ASSERT_NEAR(50, engine->engineState.targetFromTable, EPS4D); - - setMockThrottlePedalSensorVoltage(3.05 PASS_ENGINE_PARAMETER_SUFFIX); - ASSERT_NEAR(50.8302, getPedalPosition(PASS_ENGINE_PARAMETER_SIGNATURE), EPS4D); - engine->etbControllers[0]->PeriodicTask(); - ASSERT_NEAR(50.8302, engine->engineState.targetFromTable, EPS4D); - - setMockThrottlePedalSensorVoltage(3.1 PASS_ENGINE_PARAMETER_SUFFIX); - engine->etbControllers[0]->PeriodicTask(); - ASSERT_NEAR(51.6605, engine->engineState.targetFromTable, EPS4D); - -}