From 0a9fcf6ebccf01269d6e98c3d101adb23e314708 Mon Sep 17 00:00:00 2001 From: Neil McKechnie <75813993+Neil-McK@users.noreply.github.com> Date: Thu, 26 Aug 2021 23:04:13 +0100 Subject: [PATCH] Neil bugfixes. (#186) * Re-enable native I2C driver. * Minor non-functional changes to native I2C Manager. * Minor changes to make variable types explicit in comparisons. * Fix IODevice::loop() to avoid null pointer dereference. Strange problems with LCD driver tracked down to being caused by a call to p->_loop() when p is NULL. * Correct sense of comparison in LCN support function Turnout::setClosedStateOnly() * Remove code (now unused) from LCD driver. * Add I2C textual error messages. * Add I2C textual error messages. * Fix compile error in 4809 I2C driver. * Remove init function call from SSD1306 driver. --- I2CManager.cpp | 38 +++++++++++++++++++++++++++++++++++--- I2CManager.h | 25 ++++++++++++++++++------- I2CManager_Mega4809.h | 10 +++++----- I2CManager_NonBlocking.h | 35 ++++++++++++++++++----------------- IODevice.cpp | 6 ++++-- IO_GPIOBase.h | 5 +++-- IO_HCSR04.h | 2 +- LiquidCrystal_I2C.cpp | 8 -------- LiquidCrystal_I2C.h | 5 +---- SSD1306Ascii.cpp | 3 --- Turnouts.cpp | 2 +- 11 files changed, 86 insertions(+), 53 deletions(-) diff --git a/I2CManager.cpp b/I2CManager.cpp index 82f5f46..94c4baf 100644 --- a/I2CManager.cpp +++ b/I2CManager.cpp @@ -147,6 +147,25 @@ uint8_t I2CManagerClass::finishRB(I2CRB *rb, uint8_t status) { return status; } +/*************************************************************************** + * Get a message corresponding to the error status + ***************************************************************************/ +const FSH *I2CManagerClass::getErrorMessage(uint8_t status) { + switch (status) { + case I2C_STATUS_OK: return F("OK"); + case I2C_STATUS_TRUNCATED: return F("Transmission truncated"); + case I2C_STATUS_NEGATIVE_ACKNOWLEDGE: return F("No response from device (address NAK)"); + case I2C_STATUS_TRANSMIT_ERROR: return F("Transmit error (data NAK)"); + case I2C_STATUS_OTHER_TWI_ERROR: return F("Other Wire/TWI error"); + case I2C_STATUS_TIMEOUT: return F("Timeout"); + case I2C_STATUS_ARBITRATION_LOST: return F("Arbitration lost"); + case I2C_STATUS_BUS_ERROR: return F("I2C bus error"); + case I2C_STATUS_UNEXPECTED_ERROR: return F("Unexpected error"); + case I2C_STATUS_PENDING: return F("Request pending"); + default: return F("Error code not recognised"); + } +} + /*************************************************************************** * Declare singleton class instance. ***************************************************************************/ @@ -158,12 +177,25 @@ I2CManagerClass I2CManager = I2CManagerClass(); ///////////////////////////////////////////////////////////////////////////// /*************************************************************************** - * Block waiting for request block to complete, and return completion status + * Block waiting for request block to complete, and return completion status. + * Since such a loop could potentially last for ever if the RB status doesn't + * change, we set a high limit (0.1sec, 100ms) on the wait time and, if it + * hasn't changed by that time we assume it's not going to, and just return + * a timeout status. This means that CS will not lock up. ***************************************************************************/ uint8_t I2CRB::wait() { - do + unsigned long waitStart = millis(); + do { I2CManager.loop(); - while (status==I2C_STATUS_PENDING); + // Rather than looping indefinitely, let's set a very high timeout (100ms). + if ((millis() - waitStart) > 100UL) { + DIAG(F("I2C TIMEOUT I2C:x%x I2CRB:x%x"), i2cAddress, this); + status = I2C_STATUS_TIMEOUT; + // Note that, although the timeout is posted, the request may yet complete. + // TODO: Ideally we would like to cancel the request. + return status; + } + } while (status==I2C_STATUS_PENDING); return status; } diff --git a/I2CManager.h b/I2CManager.h index 342d8d4..25ab004 100644 --- a/I2CManager.h +++ b/I2CManager.h @@ -111,19 +111,22 @@ * */ -#define I2C_USE_WIRE +//#define I2C_USE_WIRE #ifndef I2C_NO_INTERRUPTS #define I2C_USE_INTERRUPTS #endif // Status codes for I2CRB structures. enum : uint8_t { + // Codes used by Wire and by native drivers I2C_STATUS_OK=0, I2C_STATUS_TRUNCATED=1, - I2C_STATUS_DEVICE_NOT_PRESENT=2, + I2C_STATUS_NEGATIVE_ACKNOWLEDGE=2, I2C_STATUS_TRANSMIT_ERROR=3, - I2C_STATUS_NEGATIVE_ACKNOWLEDGE=4, I2C_STATUS_TIMEOUT=5, + // Code used by Wire only + I2C_STATUS_OTHER_TWI_ERROR=4, // catch-all error + // Codes used by native drivers only I2C_STATUS_ARBITRATION_LOST=6, I2C_STATUS_BUS_ERROR=7, I2C_STATUS_UNEXPECTED_ERROR=8, @@ -151,14 +154,16 @@ typedef enum : uint8_t #define I2C_FREQ 400000L #endif -// Struct defining a request context for an I2C operation. -struct I2CRB { +// Class defining a request context for an I2C operation. +class I2CRB { +public: volatile uint8_t status; // Completion status, or pending flag (updated from IRC) volatile uint8_t nBytes; // Number of bytes read (updated from IRC) + inline I2CRB() { status = I2C_STATUS_OK; }; uint8_t wait(); bool isBusy(); - inline void init() { status = I2C_STATUS_OK; }; + void setReadParams(uint8_t i2cAddress, uint8_t *readBuffer, uint8_t readLen); void setRequestParams(uint8_t i2cAddress, uint8_t *readBuffer, uint8_t readLen, const uint8_t *writeBuffer, uint8_t writeLen); void setWriteParams(uint8_t i2cAddress, const uint8_t *writeBuffer, uint8_t writeLen); @@ -213,6 +218,10 @@ public: // Loop method void loop(); + // Expand error codes into text. Note that they are in flash so + // need to be printed using FSH. + static const FSH *getErrorMessage(uint8_t status); + private: bool _beginCompleted = false; bool _clockSpeedFixed = false; @@ -258,7 +267,9 @@ private: static void I2C_close(); public: - void setTimeout(unsigned long value) { timeout = value;}; + // setTimeout sets the timout value for I2C transactions. + // TODO: Get I2C timeout working before uncommenting the code below. + void setTimeout(unsigned long value) { (void)value; /* timeout = value; */ }; // handleInterrupt needs to be public to be called from the ISR function! static void handleInterrupt(); diff --git a/I2CManager_Mega4809.h b/I2CManager_Mega4809.h index 18f33e5..551b4f9 100644 --- a/I2CManager_Mega4809.h +++ b/I2CManager_Mega4809.h @@ -105,14 +105,14 @@ void I2CManagerClass::I2C_handleInterrupt() { I2C_sendStart(); // Reinitiate request } else if (currentStatus & TWI_BUSERR_bm) { // Bus error - status = I2C_STATUS_BUS_ERROR; + state = I2C_STATUS_BUS_ERROR; TWI0.MSTATUS = currentStatus; // clear all flags } else if (currentStatus & TWI_WIF_bm) { // Master write completed if (currentStatus & TWI_RXACK_bm) { // Nacked, send stop. TWI0.MCTRLB = TWI_MCMD_STOP_gc; - status = I2C_STATUS_NEGATIVE_ACKNOWLEDGE; + state = I2C_STATUS_NEGATIVE_ACKNOWLEDGE; } else if (bytesToSend) { // Acked, so send next byte if (currentRequest->operation == OPERATION_SEND_P) @@ -126,7 +126,7 @@ void I2CManagerClass::I2C_handleInterrupt() { } else { // No more data to send/receive. Initiate a STOP condition. TWI0.MCTRLB = TWI_MCMD_STOP_gc; - status = I2C_STATUS_OK; // Done + state = I2C_STATUS_OK; // Done } } else if (currentStatus & TWI_RIF_bm) { // Master read completed without errors @@ -136,7 +136,7 @@ void I2CManagerClass::I2C_handleInterrupt() { } else { // Buffer full, issue nack/stop TWI0.MCTRLB = TWI_ACKACT_bm | TWI_MCMD_STOP_gc; - status = I2C_STATUS_OK; + state = I2C_STATUS_OK; } if (bytesToReceive) { // More bytes to receive, issue ack and start another read @@ -144,7 +144,7 @@ void I2CManagerClass::I2C_handleInterrupt() { } else { // Transaction finished, issue NACK and STOP. TWI0.MCTRLB = TWI_ACKACT_bm | TWI_MCMD_STOP_gc; - status = I2C_STATUS_OK; + state = I2C_STATUS_OK; } } } diff --git a/I2CManager_NonBlocking.h b/I2CManager_NonBlocking.h index 036504e..3bdfc38 100644 --- a/I2CManager_NonBlocking.h +++ b/I2CManager_NonBlocking.h @@ -59,10 +59,9 @@ void I2CManagerClass::_setClock(unsigned long i2cClockSpeed) { ***************************************************************************/ void I2CManagerClass::startTransaction() { ATOMIC_BLOCK(ATOMIC_RESTORESTATE) { - I2CRB *t = queueHead; - if ((state == I2C_STATE_FREE) && (t != NULL)) { + if ((state == I2C_STATE_FREE) && (queueHead != NULL)) { state = I2C_STATE_ACTIVE; - currentRequest = t; + currentRequest = queueHead; rxCount = txCount = 0; // Copy key fields to static data for speed. operation = currentRequest->operation; @@ -85,9 +84,9 @@ void I2CManagerClass::queueRequest(I2CRB *req) { queueHead = queueTail = req; // Only item on queue else queueTail = queueTail->nextRequest = req; // Add to end + startTransaction(); } - startTransaction(); } /*************************************************************************** @@ -135,7 +134,7 @@ void I2CManagerClass::checkForTimeout() { unsigned long currentMicros = micros(); ATOMIC_BLOCK(ATOMIC_RESTORESTATE) { I2CRB *t = queueHead; - if (state==I2C_STATE_ACTIVE && t!=0 && timeout > 0) { + if (state==I2C_STATE_ACTIVE && t!=0 && t==currentRequest && timeout > 0) { // Check for timeout if (currentMicros - startTime > timeout) { // Excessive time. Dequeue request @@ -150,7 +149,7 @@ void I2CManagerClass::checkForTimeout() { I2C_init(); state = I2C_STATE_FREE; - // Initiate next queued request + // Initiate next queued request if any. startTransaction(); } } @@ -173,27 +172,29 @@ void I2CManagerClass::loop() { ***************************************************************************/ void I2CManagerClass::handleInterrupt() { + // Update hardware state machine I2C_handleInterrupt(); - // Experimental -- perform the post processing with interrupts enabled. - //interrupts(); - - if (state!=I2C_STATE_ACTIVE && state != I2C_STATE_FREE) { + // Check if current request has completed. If there's a current request + // and state isn't active then state contains the completion status of the request. + if (state != I2C_STATE_ACTIVE && currentRequest != NULL) { // Remove completed request from head of queue - I2CRB * t; ATOMIC_BLOCK(ATOMIC_RESTORESTATE) { - t = queueHead; - if (t != NULL) { + I2CRB * t = queueHead; + if (t == queueHead) { queueHead = t->nextRequest; if (!queueHead) queueTail = queueHead; t->nBytes = rxCount; t->status = state; + + // I2C state machine is now free for next request + currentRequest = NULL; + state = I2C_STATE_FREE; + + // Start next request (if any) + I2CManager.startTransaction(); } - // I2C state machine is now free for next request - state = I2C_STATE_FREE; } - // Start next request (if any) - I2CManager.startTransaction(); } } diff --git a/IODevice.cpp b/IODevice.cpp index e7b9dbd..3269abe 100644 --- a/IODevice.cpp +++ b/IODevice.cpp @@ -69,8 +69,10 @@ void IODevice::loop() { unsigned long currentMicros = micros(); // Call every device's loop function in turn, one per entry. if (!_nextLoopDevice) _nextLoopDevice = _firstDevice; - _nextLoopDevice->_loop(currentMicros); - _nextLoopDevice = _nextLoopDevice->_nextDevice; + if (_nextLoopDevice) { + _nextLoopDevice->_loop(currentMicros); + _nextLoopDevice = _nextLoopDevice->_nextDevice; + } // Report loop time if diags enabled #if defined(DIAG_LOOPTIMES) diff --git a/IO_GPIOBase.h b/IO_GPIOBase.h index 7179f9f..6e4a032 100644 --- a/IO_GPIOBase.h +++ b/IO_GPIOBase.h @@ -145,7 +145,8 @@ void GPIOBase::_loop(unsigned long currentMicros) { _deviceState = DEVSTATE_NORMAL; } else { _deviceState = DEVSTATE_FAILED; - DIAG(F("%S I2C:x%x Error:%d"), _deviceName, _I2CAddress, status); + DIAG(F("%S I2C:x%x Error:%d %S"), _deviceName, _I2CAddress, status, + I2CManager.getErrorMessage(status)); } _processCompletion(status); @@ -174,7 +175,7 @@ void GPIOBase::_loop(unsigned long currentMicros) { if (digitalRead(_gpioInterruptPin)) return; } else // No interrupt pin. Check if tick has elapsed. If not, finish. - if (currentMicros - _lastLoopEntry < _portTickTime) return; + if (currentMicros - _lastLoopEntry < (unsigned long)_portTickTime) return; // TODO: Could suppress reads if there are no pins configured as inputs! diff --git a/IO_HCSR04.h b/IO_HCSR04.h index 5234fe1..e66c343 100644 --- a/IO_HCSR04.h +++ b/IO_HCSR04.h @@ -102,7 +102,7 @@ protected: // _loop function - read HC-SR04 once every 50 milliseconds. void _loop(unsigned long currentMicros) override { - if (currentMicros - _lastExecutionTime > 50000) { + if (currentMicros - _lastExecutionTime > 50000UL) { _lastExecutionTime = currentMicros; _value = read_HCSR04device(); diff --git a/LiquidCrystal_I2C.cpp b/LiquidCrystal_I2C.cpp index d7953f7..e036b98 100644 --- a/LiquidCrystal_I2C.cpp +++ b/LiquidCrystal_I2C.cpp @@ -59,8 +59,6 @@ LiquidCrystal_I2C::LiquidCrystal_I2C(uint8_t lcd_Addr, uint8_t lcd_cols, backlight(); lcdDisplay = this; } - // Initialise request block for comms. - requestBlock.setWriteParams(lcd_Addr, outputBuffer, sizeof(outputBuffer)); } void LiquidCrystal_I2C::begin() { @@ -192,8 +190,6 @@ void LiquidCrystal_I2C::send(uint8_t value, uint8_t mode) { mode |= _backlightval; uint8_t highnib = (((value >> 4) & 0x0f) << BACKPACK_DATA_BITS) | mode; uint8_t lownib = ((value & 0x0f) << BACKPACK_DATA_BITS) | mode; - // Wait for previous request to complete before writing to outputbuffer. - requestBlock.wait(); // Send both nibbles uint8_t len = 0; outputBuffer[len++] = highnib|En; @@ -209,8 +205,6 @@ void LiquidCrystal_I2C::write4bits(uint8_t value) { // Enable must be set/reset for at least 450ns. This is well within the // I2C clock cycle time of 2.5us at 400kHz. Data is clocked in to the // HD44780 on the trailing edge of the Enable pin. - // Wait for previous request to complete before writing to outputbuffer. - requestBlock.wait(); uint8_t len = 0; outputBuffer[len++] = _data|En; outputBuffer[len++] = _data; @@ -220,8 +214,6 @@ void LiquidCrystal_I2C::write4bits(uint8_t value) { // write a byte to the PCF8574 I2C interface. We don't need to set // the enable pin for this. void LiquidCrystal_I2C::expanderWrite(uint8_t value) { - // Wait for previous request to complete before writing to outputbuffer. - requestBlock.wait(); outputBuffer[0] = value | _backlightval; I2CManager.write(_Addr, outputBuffer, 1); // Write command synchronously } \ No newline at end of file diff --git a/LiquidCrystal_I2C.h b/LiquidCrystal_I2C.h index 6cd4384..6881a69 100644 --- a/LiquidCrystal_I2C.h +++ b/LiquidCrystal_I2C.h @@ -75,10 +75,8 @@ public: void backlight(); void command(uint8_t); - void init(); private: - void init_priv(); void send(uint8_t, uint8_t); void write4bits(uint8_t); void expanderWrite(uint8_t); @@ -88,10 +86,9 @@ private: uint8_t _displaymode; uint8_t _backlightval; - I2CRB requestBlock; uint8_t outputBuffer[4]; // I/O is synchronous, so if this is called we're not busy! - bool isBusy() { return false; } + bool isBusy() override { return false; } }; #endif diff --git a/SSD1306Ascii.cpp b/SSD1306Ascii.cpp index 07a7883..17fd5a2 100644 --- a/SSD1306Ascii.cpp +++ b/SSD1306Ascii.cpp @@ -99,9 +99,6 @@ SSD1306AsciiWire::SSD1306AsciiWire(int width, int height) { lcdRows = height / 8; lcdCols = width / 6; - // Initialise request block for I2C - requestBlock.init(); - I2CManager.begin(); I2CManager.setClock(400000L); // Set max supported I2C speed for (byte address = 0x3c; address <= 0x3d; address++) { diff --git a/Turnouts.cpp b/Turnouts.cpp index 552cd9d..e309604 100644 --- a/Turnouts.cpp +++ b/Turnouts.cpp @@ -107,7 +107,7 @@ bool Turnout::setClosedStateOnly(uint16_t id, bool close) { Turnout *tt = get(id); - if (tt) return false; + if (!tt) return false; tt->_turnoutData.closed = close; return true; }