diff --git a/firmware/controllers/actuators/aux_pid.cpp b/firmware/controllers/actuators/aux_pid.cpp index 58144d4786..96d3b7e165 100644 --- a/firmware/controllers/actuators/aux_pid.cpp +++ b/firmware/controllers/actuators/aux_pid.cpp @@ -132,11 +132,9 @@ void startAuxPins() { } void stopAuxPins() { -#if EFI_PROD_CODE for (int i = 0;i < AUX_PID_COUNT;i++) { - efiSetPadUnused(activeConfiguration.auxPidPins[i]); + instances[i].auxOutputPin.deInit(); } -#endif /* EFI_PROD_CODE */ } void initAuxPid(Logging *sharedLogger) { diff --git a/firmware/controllers/algo/engine.h b/firmware/controllers/algo/engine.h index cd34b4ed9e..33f87ebabe 100644 --- a/firmware/controllers/algo/engine.h +++ b/firmware/controllers/algo/engine.h @@ -195,12 +195,6 @@ public: */ efitimems64_t callFromPitStopEndTime = 0; - /** - * This flag indicated a big enough problem that engine control would be - * prohibited if this flag is set to true. - */ - bool withError = false; - RpmCalculator rpmCalculator; persistent_config_s *config = nullptr; /** diff --git a/firmware/controllers/system/efi_gpio.cpp b/firmware/controllers/system/efi_gpio.cpp index 5529090792..9b3fdc6b09 100644 --- a/firmware/controllers/system/efi_gpio.cpp +++ b/firmware/controllers/system/efi_gpio.cpp @@ -9,6 +9,7 @@ #include "global.h" #include "engine.h" #include "efi_gpio.h" +#include "os_access.h" #include "drivers/gpio/gpio_ext.h" #include "perf_trace.h" #include "engine_controller.h" @@ -90,12 +91,9 @@ void RegisteredOutputPin::init(DECLARE_ENGINE_PARAMETER_SIGNATURE) { } void RegisteredOutputPin::unregister() { -#if EFI_PROD_CODE - brain_pin_e curPin = *(brain_pin_e *) ((void *) (&((char*)&activeConfiguration)[pinOffset])); - if (isPinConfigurationChanged()) { - unregisterOutput(curPin); - } -#endif // EFI_PROD_CODE + if (isPinConfigurationChanged()) { + OutputPin::deInit(); + } } #define CONFIG_OFFSET(x) x##_offset @@ -150,13 +148,13 @@ EnginePins::EnginePins() : #if EFI_PROD_CODE #define unregisterOutputIfPinChanged(output, pin) { \ if (isConfigurationChanged(pin)) { \ - (output).unregisterOutput(activeConfiguration.pin); \ + (output).deInit(); \ } \ } #define unregisterOutputIfPinOrModeChanged(output, pin, mode) { \ if (isPinOrModeChanged(pin, mode)) { \ - (output).unregisterOutput(activeConfiguration.pin); \ + (output).deInit(); \ } \ } @@ -191,15 +189,13 @@ void EnginePins::unregisterPins() { for (int i = 0;i < FSIO_COMMAND_COUNT;i++) { unregisterOutputIfPinChanged(fsioOutputs[i], fsioOutputPins[i]); } - +#endif /* EFI_PROD_CODE */ RegisteredOutputPin * pin = registeredOutputHead; while (pin != nullptr) { pin->unregister(); pin = pin->next; } - -#endif /* EFI_PROD_CODE */ } void EnginePins::debug() { @@ -387,7 +383,6 @@ bool OutputPin::getAndSet(int logicValue) { #if EFI_PROD_CODE void OutputPin::setOnchipValue(int electricalValue) { palWritePad(port, pin, electricalValue); - } #endif // EFI_PROD_CODE @@ -397,12 +392,21 @@ void OutputPin::setValue(int logicValue) { // ScopePerf perf(PE::OutputPinSetValue); #endif // ENABLE_PERF_TRACE -#if EFI_PROD_CODE + // Always store the current logical value of the pin (so it can be + // used internally even if not connected to a real hardware pin) + currentLogicValue = logicValue; + + // Nothing else to do if not configured + if (brainPin == GPIO_UNASSIGNED) { + return; + } + efiAssertVoid(CUSTOM_ERR_6621, modePtr!=NULL, "pin mode not initialized"); pin_output_mode_e mode = *modePtr; efiAssertVoid(CUSTOM_ERR_6622, mode <= OM_OPENDRAIN_INVERTED, "invalid pin_output_mode_e"); int electricalValue = getElectricalValue(logicValue, mode); +#if EFI_PROD_CODE #if (BOARD_EXT_GPIOCHIPS > 0) if (!this->ext) { setOnchipValue(electricalValue); @@ -414,13 +418,9 @@ void OutputPin::setValue(int logicValue) { #else setOnchipValue(electricalValue); #endif - #else /* EFI_PROD_CODE */ - setMockState(brainPin, logicValue); + setMockState(brainPin, electricalValue); #endif /* EFI_PROD_CODE */ - - // Lastly store the current logical value of the pin - currentLogicValue = logicValue; } bool OutputPin::getLogicValue() const { @@ -464,13 +464,19 @@ void OutputPin::initPin(const char *msg, brain_pin_e brainPin) { } void OutputPin::initPin(const char *msg, brain_pin_e brainPin, const pin_output_mode_e *outputMode) { -#if EFI_UNIT_TEST - this->brainPin = brainPin; -#endif - -#if EFI_GPIO_HARDWARE && EFI_PROD_CODE - if (brainPin == GPIO_UNASSIGNED) + if (brainPin == GPIO_UNASSIGNED) { return; + } + + // Enter a critical section so that other threads can't change the pin state out from underneath us + chibios_rt::CriticalSectionLocker csl; + + // Check that this OutputPin isn't already assigned to another pin (reinit is allowed to change mode) + // To avoid this error, call deInit() first + if (this->brainPin != GPIO_UNASSIGNED && this->brainPin != brainPin) { + firmwareError(CUSTOM_OBD_PIN_CONFLICT, "outputPin [%s] already assigned, cannot reassign without unregister first", msg); + return; + } if (*outputMode > OM_OPENDRAIN_INVERTED) { firmwareError(CUSTOM_INVALID_MODE_SETTING, "%s invalid pin_output_mode_e %d %s", @@ -480,6 +486,8 @@ void OutputPin::initPin(const char *msg, brain_pin_e brainPin, const pin_output_ ); return; } + +#if EFI_GPIO_HARDWARE && EFI_PROD_CODE iomode_t mode = (*outputMode == OM_DEFAULT || *outputMode == OM_INVERTED) ? PAL_MODE_OUTPUT_PUSHPULL : PAL_MODE_OUTPUT_OPENDRAIN; @@ -490,39 +498,24 @@ void OutputPin::initPin(const char *msg, brain_pin_e brainPin, const pin_output_ ioportid_t port = getHwPort(msg, brainPin); int pin = getHwPin(msg, brainPin); - /** - * This method is used for digital GPIO pins only, for peripheral pins see mySetPadMode - */ + // Validate port if (port == GPIO_NULL) { - // that's for GRIO_NONE - this->port = port; + firmwareError(OBD_PCM_Processor_Fault, "OutputPin::initPin got invalid port for pin idx %d", static_cast(brainPin)); return; } - /** - * @brief Initialize the hardware output pin while also assigning it a logical name - */ - if (this->port != NULL && (this->port != port || this->pin != pin)) { - /** - * here we check if another physical pin is already assigned to this logical output - */ - // todo: need to clear '&outputs' in io_pins.c - warning(CUSTOM_OBD_PIN_CONFLICT, "outputPin [%s] already assigned to %x%d", msg, this->port, this->pin); - engine->withError = true; - return; - } this->port = port; this->pin = pin; } #if (BOARD_EXT_GPIOCHIPS > 0) else { this->ext = true; - this->brainPin = brainPin; } #endif - #endif // briefly leave the include guard because we need to set default state in tests + this->brainPin = brainPin; + // The order of the next two calls may look strange, which is a good observation. // We call them in this order so that the pin is set to a known state BEFORE // it's enabled. Enabling the pin then setting it could result in a (brief) @@ -544,22 +537,35 @@ void OutputPin::initPin(const char *msg, brain_pin_e brainPin, const pin_output_ // if the pin was set to logical 1, then set an error and disable the pin so that things don't catch fire if (logicalValue) { - efiSetPadUnused(brainPin); firmwareError(OBD_PCM_Processor_Fault, "%s: startup pin state %s actual value=%d logical value=%d mode=%s", msg, hwPortname(brainPin), actualValue, logicalValue, getPin_output_mode_e(*outputMode)); + OutputPin::deInit(); } } } #endif /* EFI_GPIO_HARDWARE */ } -void OutputPin::unregisterOutput(brain_pin_e oldPin) { - if (oldPin != GPIO_UNASSIGNED) { - scheduleMsg(logger, "unregistering %s", hwPortname(oldPin)); -#if EFI_GPIO_HARDWARE && EFI_PROD_CODE - efiSetPadUnused(oldPin); - port = nullptr; -#endif /* EFI_GPIO_HARDWARE */ +void OutputPin::deInit() { + // Unregister under lock - we don't want other threads mucking with the pin while we're trying to turn it off + chibios_rt::CriticalSectionLocker csl; + + // nothing to do if not registered in the first place + if (brainPin == GPIO_UNASSIGNED) { + return; } + +#if (BOARD_EXT_GPIOCHIPS > 0) + ext = false; +#endif // (BOARD_EXT_GPIOCHIPS > 0) + + scheduleMsg(logger, "unregistering %s", hwPortname(brainPin)); + +#if EFI_GPIO_HARDWARE && EFI_PROD_CODE + efiSetPadUnused(brainPin); +#endif /* EFI_GPIO_HARDWARE */ + + // Clear the pin so that it won't get set any more + brainPin = GPIO_UNASSIGNED; } #if EFI_GPIO_HARDWARE diff --git a/firmware/controllers/system/efi_gpio.h b/firmware/controllers/system/efi_gpio.h index 5152519132..6288bd8f6f 100644 --- a/firmware/controllers/system/efi_gpio.h +++ b/firmware/controllers/system/efi_gpio.h @@ -47,10 +47,11 @@ public: * same as above, with DEFAULT_OUTPUT mode */ void initPin(const char *msg, brain_pin_e brainPin); + /** * dissociates pin from this output and un-registers it in pin repository */ - void unregisterOutput(brain_pin_e oldPin); + void deInit(); bool isInitialized(); @@ -59,19 +60,16 @@ public: void toggle(); bool getLogicValue() const; - #if EFI_GPIO_HARDWARE ioportid_t port = 0; uint8_t pin = 0; #endif /* EFI_GPIO_HARDWARE */ + brain_pin_e brainPin = GPIO_UNASSIGNED; + #if (EFI_GPIO_HARDWARE && (BOARD_EXT_GPIOCHIPS > 0)) /* used for external pins */ - brain_pin_e brainPin; - bool ext; -#elif EFI_SIMULATOR || EFI_UNIT_TEST - // used for setMockState - brain_pin_e brainPin; + bool ext = false; #endif /* EFI_GPIO_HARDWARE */ int8_t currentLogicValue = INITIAL_PIN_STATE; @@ -85,7 +83,7 @@ private: void setOnchipValue(int electricalValue); // 4 byte pointer is a bit of a memory waste here - const pin_output_mode_e *modePtr; + const pin_output_mode_e *modePtr = nullptr; }; /** diff --git a/firmware/controllers/trigger/trigger_emulator_algo.cpp b/firmware/controllers/trigger/trigger_emulator_algo.cpp index 8ca0277a19..1e22685ed4 100644 --- a/firmware/controllers/trigger/trigger_emulator_algo.cpp +++ b/firmware/controllers/trigger/trigger_emulator_algo.cpp @@ -222,12 +222,7 @@ void startTriggerEmulatorPins() { void stopTriggerEmulatorPins() { for (size_t i = 0; i < efi::size(emulatorOutputs); i++) { - brain_pin_e brainPin = activeConfiguration.triggerSimulatorPins[i]; - if (brainPin != GPIO_UNASSIGNED) { -#if EFI_PROD_CODE - efiSetPadUnused(brainPin); -#endif // EFI_PROD_CODE - } + triggerSignal.outputPins[i]->deInit(); } } diff --git a/firmware/hw_layer/sensors/hip9011.cpp b/firmware/hw_layer/sensors/hip9011.cpp index 80aaa90fc1..8c47428206 100644 --- a/firmware/hw_layer/sensors/hip9011.cpp +++ b/firmware/hw_layer/sensors/hip9011.cpp @@ -385,10 +385,8 @@ static msg_t hipThread(void *arg) { } void stopHip9001_pins() { -#if EFI_PROD_CODE - efiSetPadUnused(activeConfiguration.hip9011IntHoldPin); - efiSetPadUnused(activeConfiguration.hip9011CsPin); -#endif /* EFI_PROD_CODE */ + intHold.deInit(); + enginePins.hipCs.deInit(); } void startHip9001_pins() { diff --git a/unit_tests/engine_test_helper.cpp b/unit_tests/engine_test_helper.cpp index 308b7bd019..3c4231080a 100644 --- a/unit_tests/engine_test_helper.cpp +++ b/unit_tests/engine_test_helper.cpp @@ -51,6 +51,7 @@ EngineTestHelper::EngineTestHelper(engine_type_e engineType, configuration_callb memset(&activeConfiguration, 0, sizeof(activeConfiguration)); enginePins.reset(); + enginePins.unregisterPins(); persistent_config_s *config = &persistentConfig; Engine *engine = &this->engine; @@ -99,6 +100,8 @@ EngineTestHelper::~EngineTestHelper() { writeEvents(filePath.str().c_str()); // Cleanup + enginePins.reset(); + enginePins.unregisterPins(); Sensor::resetRegistry(); memset(mockPinStates, 0, sizeof(mockPinStates)); }