From b906d12268c918a25b2f71f44c61f7d58392a930 Mon Sep 17 00:00:00 2001 From: Matthew Kennedy Date: Thu, 11 Jun 2020 17:43:26 -0700 Subject: [PATCH] Fix gppwm properly (#1487) * directly control output in onoff mode * test fixing --- .../controllers/actuators/gppwm/gppwm.cpp | 2 +- .../actuators/gppwm/gppwm_channel.cpp | 17 +++++++------ .../actuators/gppwm/gppwm_channel.h | 3 ++- firmware/controllers/system/efi_gpio.h | 9 ++++++- unit_tests/mocks.h | 5 ++++ unit_tests/tests/test_gppwm.cpp | 24 +++++++++++-------- 6 files changed, 40 insertions(+), 20 deletions(-) diff --git a/firmware/controllers/actuators/gppwm/gppwm.cpp b/firmware/controllers/actuators/gppwm/gppwm.cpp index 5547992008..acd83386c7 100644 --- a/firmware/controllers/actuators/gppwm/gppwm.cpp +++ b/firmware/controllers/actuators/gppwm/gppwm.cpp @@ -43,7 +43,7 @@ void initGpPwm(DECLARE_ENGINE_PARAMETER_SIGNATURE) { // Finally configure the channel INJECT_ENGINE_REFERENCE(&channels[i]); - channels[i].init(usePwm, &outputs[i], tables[i], &cfg); + channels[i].init(usePwm, &outputs[i], &pins[i], tables[i], &cfg); } } diff --git a/firmware/controllers/actuators/gppwm/gppwm_channel.cpp b/firmware/controllers/actuators/gppwm/gppwm_channel.cpp index 1f67a02f4b..b643092249 100644 --- a/firmware/controllers/actuators/gppwm/gppwm_channel.cpp +++ b/firmware/controllers/actuators/gppwm/gppwm_channel.cpp @@ -34,11 +34,15 @@ expected readGppwmChannel(gppwm_channel_e channel DECLARE_ENGINE_PARAMETE void GppwmChannel::setOutput(float result) { // Not init yet, nothing to do. - if (!m_pwm || !m_config) { + if (!m_config) { return; } - - if (!m_usePwm) { + + if (m_usePwm) { + efiAssertVoid(OBD_PCM_Processor_Fault, m_usePwm, "m_usePwm null"); + m_pwm->setSimplePwmDutyCycle(clampF(0, result / 100.0f, 1)); + } else { + efiAssertVoid(OBD_PCM_Processor_Fault, m_output, "m_output null"); // Apply hysteresis with provided values if (m_state && result < m_config->offBelowDuty) { m_state = false; @@ -46,15 +50,14 @@ void GppwmChannel::setOutput(float result) { m_state = true; } - result = m_state ? 100 : 0; + m_output->setValue(m_state); } - - m_pwm->setSimplePwmDutyCycle(clampF(0, result / 100.0f, 1)); } -void GppwmChannel::init(bool usePwm, SimplePwm* pwm, const ValueProvider3D* table, const gppwm_channel* config) { +void GppwmChannel::init(bool usePwm, SimplePwm* pwm, OutputPin* outputPin, const ValueProvider3D* table, const gppwm_channel* config) { m_usePwm = usePwm; m_pwm = pwm; + m_output = outputPin; m_table = table; m_config = config; } diff --git a/firmware/controllers/actuators/gppwm/gppwm_channel.h b/firmware/controllers/actuators/gppwm/gppwm_channel.h index 1045435157..19ee7a62be 100644 --- a/firmware/controllers/actuators/gppwm/gppwm_channel.h +++ b/firmware/controllers/actuators/gppwm/gppwm_channel.h @@ -10,7 +10,7 @@ class GppwmChannel { public: DECLARE_ENGINE_PTR; - void init(bool usePwm, SimplePwm* pwm, const ValueProvider3D* table, const gppwm_channel* config); + void init(bool usePwm, SimplePwm* pwm, OutputPin* outputPin, const ValueProvider3D* table, const gppwm_channel* config); void update(); private: @@ -24,5 +24,6 @@ private: const gppwm_channel* m_config = nullptr; bool m_usePwm = false; SimplePwm* m_pwm = nullptr; + OutputPin* m_output = nullptr; const ValueProvider3D* m_table = nullptr; }; diff --git a/firmware/controllers/system/efi_gpio.h b/firmware/controllers/system/efi_gpio.h index b1250d7c42..3113c91e80 100644 --- a/firmware/controllers/system/efi_gpio.h +++ b/firmware/controllers/system/efi_gpio.h @@ -23,6 +23,13 @@ void turnAllPinsOff(void); #define turnAllPinsOff() {} #endif /* EFI_GPIO_HARDWARE */ +// Used if you want a function to be virtual only for unit testing purposes +#if EFI_UNIT_TEST +#define TEST_VIRTUAL virtual +#else +#define TEST_VIRTUAL +#endif + #ifdef __cplusplus /** * @brief Single output pin reference and state @@ -48,7 +55,7 @@ public: bool isInitialized(); bool getAndSet(int logicValue); - void setValue(int logicValue); + TEST_VIRTUAL void setValue(int logicValue); void toggle(); bool getLogicValue() const; diff --git a/unit_tests/mocks.h b/unit_tests/mocks.h index 0aa45f47bf..2a093f05f1 100644 --- a/unit_tests/mocks.h +++ b/unit_tests/mocks.h @@ -41,6 +41,11 @@ public: MOCK_METHOD(void, setSimplePwmDutyCycle, (float dutyCycle), (override)); }; +class MockOutputPin : public OutputPin { +public: + MOCK_METHOD(void, setValue, (int value), (override)); +}; + class MockExecutor : public TestExecutor { public: MOCK_METHOD(void, scheduleByTimestamp, (scheduling_s *scheduling, efitimeus_t timeUs, action_s action), (override)); diff --git a/unit_tests/tests/test_gppwm.cpp b/unit_tests/tests/test_gppwm.cpp index 0ab1cc8925..f337a1bb5d 100644 --- a/unit_tests/tests/test_gppwm.cpp +++ b/unit_tests/tests/test_gppwm.cpp @@ -26,7 +26,7 @@ TEST(GpPwm, OutputWithPwm) { EXPECT_CALL(pwm, setSimplePwmDutyCycle(1.0f)); } - ch.init(true, &pwm, nullptr, &cfg); + ch.init(true, &pwm, nullptr, nullptr, &cfg); // Set the output - should set directly to PWM ch.setOutput(25.0f); @@ -44,19 +44,23 @@ TEST(GpPwm, OutputOnOff) { cfg.onAboveDuty = 50; cfg.offBelowDuty = 40; - MockPwm pwm; + MockOutputPin pin; { 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)); + + // Rising edge test + EXPECT_CALL(pin, setValue(0)); + EXPECT_CALL(pin, setValue(1)); + EXPECT_CALL(pin, setValue(1)); + + // Falling edge test + EXPECT_CALL(pin, setValue(1)); + EXPECT_CALL(pin, setValue(0)); + EXPECT_CALL(pin, setValue(0)); } - ch.init(false, &pwm, nullptr, &cfg); + ch.init(false, nullptr, &pin, nullptr, &cfg); // Test rising edge - these should output 0, 1, 1 ch.setOutput(49.0f); @@ -86,7 +90,7 @@ TEST(GpPwm, GetOutput) { return tps; }); - ch.init(false, nullptr, &table, &cfg); + ch.init(false, nullptr, nullptr, &table, &cfg); Sensor::resetAllMocks();