rusefi
/
custom-board-bundle-sample-fork-of-rusefi
mirror of https://github.com/rusefi/custom-board-bundle-sample-fork-of-rusefi.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Fix gpio reinit race condition (#2098) 

* fix unregister api for gpio

* consumers

* guard with a critical section

* index

* guard less

* unregister under lock

* fix reinit

* dead flag

* fix most tests

* initialize properly

* initialize properly

* assertions in tests

* fix message

* we must lock earlier to be truly safe

* this was using huge memory

* devirtualize
This commit is contained in:
Matthew Kennedy 2020-12-18 14:18:12 -08:00 committed by GitHub
parent 74d11e2957
commit 30fd52d82d
7 changed files with 70 additions and 78 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
firmware/controllers/actuators/aux_pid.cpp
View File

@ -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) {

6
firmware/controllers/algo/engine.h
View File

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

108
firmware/controllers/system/efi_gpio.cpp
View File

@ -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<int>(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

14
firmware/controllers/system/efi_gpio.h
View File

@ -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;
};
/**

7
firmware/controllers/trigger/trigger_emulator_algo.cpp
View File

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

6
firmware/hw_layer/sensors/hip9011.cpp
View File

@ -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() {

3
unit_tests/engine_test_helper.cpp
View File

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