From 4f16a4ca06a8bd8c59ecb675a28470b5243b681b Mon Sep 17 00:00:00 2001 From: Neil McKechnie Date: Fri, 15 Oct 2021 18:34:47 +0100 Subject: [PATCH] Fix GPIO Expander initial output state. Previously, pullups were enabled on GPIO Expander digital pins by default, even if the pin was only ever used as an output. This could lead to a spurious HIGH state being seen by external equipment before the output is initialised to LOW. To avoid this, the pin pullup is now not enabled until a configure or read operation is issued for the pin. --- IODevice.h | 4 ++-- IO_GPIOBase.h | 25 +++++++++++++++++++------ IO_MCP23008.h | 14 +++++++++----- IO_MCP23017.h | 15 ++++++++++----- IO_PCF8574.h | 20 +++++++++++++++++++- 5 files changed, 59 insertions(+), 19 deletions(-) diff --git a/IODevice.h b/IODevice.h index fe1d3e6..5d6abc3 100644 --- a/IODevice.h +++ b/IODevice.h @@ -120,7 +120,7 @@ public: } // User-friendly function for configuring a servo pin. - inline static bool configureServo(VPIN vpin, uint16_t activePosition, uint16_t inactivePosition, uint8_t profile, uint16_t duration, uint8_t initialState=0) { + inline static bool configureServo(VPIN vpin, uint16_t activePosition, uint16_t inactivePosition, uint8_t profile=0, uint16_t duration=0, uint8_t initialState=0) { int params[] = {(int)activePosition, (int)inactivePosition, profile, (int)duration, initialState}; return IODevice::configure(vpin, CONFIGURE_SERVO, 5, params); } @@ -203,7 +203,7 @@ protected: // Method to perform updates on an ongoing basis (optionally implemented within device class) virtual void _loop(unsigned long currentMicros) { - (void)currentMicros; // Suppress compiler warning. + delayUntil(currentMicros + 0x7fffffff); // Largest time in the future! Effectively disable _loop calls. }; // Method for displaying info on DIAG output (optionally implemented within device class) diff --git a/IO_GPIOBase.h b/IO_GPIOBase.h index 9b1bee5..4269a70 100644 --- a/IO_GPIOBase.h +++ b/IO_GPIOBase.h @@ -53,6 +53,7 @@ protected: T _portOutputState; T _portMode; T _portPullup; + T _portInUse; // Interval between refreshes of each input port static const int _portTickTime = 4000; @@ -100,7 +101,8 @@ void GPIOBase::_begin() { #endif _portMode = 0; // default to input mode _portPullup = -1; // default to pullup enabled - _portInputState = -1; + _portInputState = -1; + _portInUse = 0; _setupDevice(); _deviceState = DEVSTATE_NORMAL; } else { @@ -126,11 +128,15 @@ bool GPIOBase::_configure(VPIN vpin, ConfigTypeEnum configType, int paramCoun _portPullup |= mask; else _portPullup &= ~mask; + // Mark that port has been accessed + _portInUse |= mask; + // Set input mode + _portMode &= ~mask; // Call subclass's virtual function to write to device + _writePortModes(); _writePullups(); - // Re-read port following change - _readGpioPort(); + // Port change will be notified on next loop entry. return true; } @@ -149,6 +155,8 @@ void GPIOBase::_loop(unsigned long currentMicros) { I2CManager.getErrorMessage(status)); } _processCompletion(status); + // Set unused pin and write mode pin value to 1 + _portInputState |= ~_portInUse | _portMode; // Scan for changes in input states and invoke callback (if present) T differences = lastPortStates ^ _portInputState; @@ -199,8 +207,9 @@ void GPIOBase::_write(VPIN vpin, int value) { DIAG(F("%S I2C:x%x Write Pin:%d Val:%d"), _deviceName, _I2CAddress, pin, value); #endif - // Set port mode output + // Set port mode output if currently not output mode if (!(_portMode & mask)) { + _portInUse |= mask; _portMode |= mask; _writePortModes(); } @@ -220,12 +229,16 @@ int GPIOBase::_read(VPIN vpin) { int pin = vpin - _firstVpin; T mask = 1 << pin; - // Set port mode to input - if (_portMode & mask) { + // Set port mode to input if currently output or first use + if ((_portMode | ~_portInUse) & mask) { _portMode &= ~mask; + _portInUse |= mask; + _writePullups(); _writePortModes(); // Port won't have been read yet, so read it now. _readGpioPort(); + // Set unused pin and write mode pin value to 1 + _portInputState |= ~_portInUse | _portMode; #ifdef DIAG_IO DIAG(F("%S I2C:x%x PortStates:%x"), _deviceName, _I2CAddress, _portInputState); #endif diff --git a/IO_MCP23008.h b/IO_MCP23008.h index 3557b49..18ff12f 100644 --- a/IO_MCP23008.h +++ b/IO_MCP23008.h @@ -42,14 +42,18 @@ private: I2CManager.write(_I2CAddress, 2, REG_GPIO, _portOutputState); } void _writePullups() override { - I2CManager.write(_I2CAddress, 2, REG_GPPU, _portPullup); + // Set pullups only for in-use pins. This prevents pullup being set for a pin that + // is intended for use as an output but hasn't been written to yet. + I2CManager.write(_I2CAddress, 2, REG_GPPU, _portPullup & _portInUse); } void _writePortModes() override { - // Each bit is 1 for an input, 0 for an output, i.e. inverted. - I2CManager.write(_I2CAddress, 2, REG_IODIR, ~_portMode); - // Enable interrupt-on-change for pins that are inputs (_portMode=0) + // Write 0 to IODIR for in-use pins that are outputs, 1 for others. + uint8_t temp = ~(_portMode & _portInUse); + I2CManager.write(_I2CAddress, 2, REG_IODIR, temp); + // Enable interrupt-on-change for in-use pins that are inputs (_portMode=0) + temp = ~_portMode & _portInUse; I2CManager.write(_I2CAddress, 2, REG_INTCON, 0x00); - I2CManager.write(_I2CAddress, 2, REG_GPINTEN, ~_portMode); + I2CManager.write(_I2CAddress, 2, REG_GPINTEN, temp); } void _readGpioPort(bool immediate) override { if (immediate) { diff --git a/IO_MCP23017.h b/IO_MCP23017.h index d7c27ce..930b051 100644 --- a/IO_MCP23017.h +++ b/IO_MCP23017.h @@ -48,14 +48,19 @@ private: I2CManager.write(_I2CAddress, 3, REG_GPIOA, _portOutputState, _portOutputState>>8); } void _writePullups() override { - I2CManager.write(_I2CAddress, 3, REG_GPPUA, _portPullup, _portPullup>>8); + // Set pullups only for in-use pins. This prevents pullup being set for a pin that + // is intended for use as an output but hasn't been written to yet. + uint16_t temp = _portPullup & _portInUse; + I2CManager.write(_I2CAddress, 3, REG_GPPUA, temp, temp>>8); } void _writePortModes() override { - // Write 1 to IODIR for pins that are inputs, 0 for outputs (i.e. _portMode inverted) - I2CManager.write(_I2CAddress, 3, REG_IODIRA, ~_portMode, (~_portMode)>>8); - // Enable interrupt for those pins which are inputs (_portMode=0) + // Write 0 to IODIR for in-use pins that are outputs, 1 for others. + uint16_t temp = ~(_portMode & _portInUse); + I2CManager.write(_I2CAddress, 3, REG_IODIRA, temp, temp>>8); + // Enable interrupt for in-use pins which are inputs (_portMode=0) + temp = ~_portMode & _portInUse; I2CManager.write(_I2CAddress, 3, REG_INTCONA, 0x00, 0x00); - I2CManager.write(_I2CAddress, 3, REG_GPINTENA, ~_portMode, (~_portMode)>>8); + I2CManager.write(_I2CAddress, 3, REG_GPINTENA, temp, temp>>8); } void _readGpioPort(bool immediate) override { if (immediate) { diff --git a/IO_PCF8574.h b/IO_PCF8574.h index 2a8d363..dea5e2c 100644 --- a/IO_PCF8574.h +++ b/IO_PCF8574.h @@ -17,6 +17,24 @@ * along with CommandStation. If not, see . */ +/* + * The PCF8574 is a simple device; it only has one register. The device + * input/output mode and pullup are configured through this, and the + * output state is written and the input state read through it too. + * + * This is accomplished by having a weak resistor in series with the output, + * and a read-back of the other end of the resistor. As an output, the + * pin state is set to 1 or 0, and the output voltage goes to +5V or 0V + * (through the weak resistor). + * + * In order to use the pin as an input, the output is written as + * a '1' in order to pull up the resistor. Therefore the input will be + * 1 unless the pin is pulled down externally, in which case it will be 0. + * + * As a consequence of this approach, it is not possible to use the device for + * inputs without pullups. + */ + #ifndef IO_PCF8574_H #define IO_PCF8574_H @@ -70,7 +88,7 @@ private: if (status == I2C_STATUS_OK) _portInputState = ((uint16_t)inputBuffer[0]) & 0xff; else - _portInputState = 0xff; + _portInputState = 0xff; } // Set up device ports