From 705617239fa69d7110077b9053d16d0e48fd25bf Mon Sep 17 00:00:00 2001 From: Neil McKechnie Date: Sun, 22 Jan 2023 13:13:20 +0000 Subject: [PATCH] Sort out I2C timeout handling, and further I2C diagnostics. Timeout handling and recovery in loop() function now operative. Start-up check for I2C signals short to ground added. Initial I2C device probe speed up. Possible infinite loops in I2C AVR native driver during fault conditions removed. --- I2CManager.cpp | 66 +++++++++++++++++++++++----------------- I2CManager.h | 22 ++++++++------ I2CManager_AVR.h | 15 +++++---- I2CManager_NonBlocking.h | 35 +++++++++++++-------- I2CManager_Wire.h | 43 +++++++++++++++++++++----- 5 files changed, 114 insertions(+), 67 deletions(-) diff --git a/I2CManager.cpp b/I2CManager.cpp index d7be935..eafdb70 100644 --- a/I2CManager.cpp +++ b/I2CManager.cpp @@ -1,6 +1,6 @@ /* + * © 2023, Neil McKechnie * © 2022 Paul M Antoine - * © 2021, Neil McKechnie * All rights reserved. * * This file is part of CommandStation-EX @@ -43,12 +43,21 @@ // If not already initialised, initialise I2C void I2CManagerClass::begin(void) { - //setTimeout(25000); // 25 millisecond timeout if (!_beginCompleted) { _beginCompleted = true; _initialise(); - // Probe and list devices. + // Check for short-circuits on I2C + if (!digitalRead(SDA)) + DIAG(F("WARNING: Possible short-circuit on I2C SDA line")); + if (!digitalRead(SCL)) + DIAG(F("WARNING: Possible short-circuit on I2C SCL line")); + + // Probe and list devices. Use standard mode + // (clock speed 100kHz) for best device compatibility. + _setClock(100000); + unsigned long originalTimeout = timeout; + setTimeout(1000); // use 1ms timeout for probes bool found = false; for (byte addr=1; addr<127; addr++) { if (exists(addr)) { @@ -57,6 +66,8 @@ void I2CManagerClass::begin(void) { } } if (!found) DIAG(F("No I2C Devices found")); + _setClock(_clockSpeed); + setTimeout(originalTimeout); // set timeout back to original } } @@ -65,18 +76,17 @@ void I2CManagerClass::begin(void) { void I2CManagerClass::setClock(uint32_t speed) { if (speed < _clockSpeed && !_clockSpeedFixed) { _clockSpeed = speed; + DIAG(F("I2C clock speed set to %l Hz"), _clockSpeed); } _setClock(_clockSpeed); } -// Force clock speed to that specified. It can then only -// be overridden by calling Wire.setClock directly. +// Force clock speed to that specified. void I2CManagerClass::forceClock(uint32_t speed) { - if (!_clockSpeedFixed) { - _clockSpeed = speed; - _clockSpeedFixed = true; - _setClock(_clockSpeed); - } + _clockSpeed = speed; + _clockSpeedFixed = true; + _setClock(_clockSpeed); + DIAG(F("I2C clock speed forced to %l Hz"), _clockSpeed); } // Check if specified I2C address is responding (blocking operation) @@ -181,40 +191,40 @@ const FSH *I2CManagerClass::getErrorMessage(uint8_t status) { ***************************************************************************/ I2CManagerClass I2CManager = I2CManagerClass(); +// Default timeout 100ms on I2C request block completion. +// A full 32-byte transmission takes about 8ms at 100kHz, +// so this value allows lots of headroom. +// It can be modified by calling I2CManager.setTimeout() function. +// When retries are enabled, the timeout applies to each +// try, and failure from timeout does not get retried. +unsigned long I2CManagerClass::timeout = 100000UL; + ///////////////////////////////////////////////////////////////////////////// // Helper functions associated with I2C Request Block ///////////////////////////////////////////////////////////////////////////// /*************************************************************************** - * 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 (1sec, 1000ms) 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. + * Block waiting for request to complete, and return completion status. + * Timeout monitoring is performed in the I2CManager.loop() function. ***************************************************************************/ uint8_t I2CRB::wait() { - unsigned long waitStart = millis(); - do { + while (status==I2C_STATUS_PENDING) { I2CManager.loop(); - // Rather than looping indefinitely, let's set a very high timeout (1s). - if ((millis() - waitStart) > 1000UL) { - 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; } /*************************************************************************** * Check whether request is still in progress. + * Timeout monitoring is performed in the I2CManager.loop() function. ***************************************************************************/ bool I2CRB::isBusy() { - I2CManager.loop(); - return (status==I2C_STATUS_PENDING); + if (status==I2C_STATUS_PENDING) { + I2CManager.loop(); + return true; + } else + return false; } /*************************************************************************** diff --git a/I2CManager.h b/I2CManager.h index bcb7092..677c5f9 100644 --- a/I2CManager.h +++ b/I2CManager.h @@ -1,6 +1,6 @@ /* + * © 2023, Neil McKechnie. All rights reserved. * © 2022 Paul M Antoine - * © 2021, Neil McKechnie. All rights reserved. * * This file is part of CommandStation-EX * @@ -29,7 +29,7 @@ * of the Wire class, but also has a native implementation for AVR * which supports non-blocking queued I/O requests. * - * Helps to avoid calling Wire.begin() multiple times (which is not) + * Helps to avoid calling Wire.begin() multiple times (which is not * entirely benign as it reinitialises). * * Also helps to avoid the Wire clock from being set, by another device @@ -76,6 +76,8 @@ * Timeout monitoring is possible, but requires that the following call is made * reasonably frequently in the program's loop() function: * I2CManager.loop(); + * So that the application doesn't need to do this explicitly, this call is performed + * from the I2CRB::isBusy() or I2CRB::wait() functions. * */ @@ -111,9 +113,11 @@ * */ -// Maximum number of retries on an I2C operation +// Maximum number of retries on an I2C operation. // A value of zero will disable retries. // Maximum value is 254 (unsigned byte counter) +// Note that timeout failures are not retried, but any timeout +// configured applies to each try separately. #define MAX_I2C_RETRIES 2 // Add following line to config.h to enable Wire library instead of native I2C drivers @@ -203,6 +207,8 @@ public: void setClock(uint32_t speed); // Force clock speed void forceClock(uint32_t speed); + // setTimeout sets the timout value for I2C transactions (milliseconds). + void setTimeout(unsigned long); // Check if specified I2C address is responding. uint8_t checkAddress(uint8_t address); inline bool exists(uint8_t address) { @@ -239,11 +245,14 @@ public: private: bool _beginCompleted = false; bool _clockSpeedFixed = false; + static uint8_t retryCounter; // Count of retries #if defined(__arm__) uint32_t _clockSpeed = 32000000L; // 3.2MHz max on SAMD and STM32 #else uint32_t _clockSpeed = 400000L; // 400kHz max on Arduino. #endif + static unsigned long timeout; // Transaction timeout in microseconds. 0=disabled. + // Finish off request block by waiting for completion and posting status. uint8_t finishRB(I2CRB *rb, uint8_t status); @@ -272,9 +281,6 @@ private: static volatile uint8_t operation; static volatile unsigned long startTime; - static unsigned long timeout; // Transaction timeout in microseconds. 0=disabled. - static uint8_t retryCounter; // Count of retries - void startTransaction(); // Low-level hardware manipulation functions. @@ -286,10 +292,6 @@ private: static void I2C_close(); public: - // 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(); #endif diff --git a/I2CManager_AVR.h b/I2CManager_AVR.h index 6492e00..267a921 100644 --- a/I2CManager_AVR.h +++ b/I2CManager_AVR.h @@ -1,5 +1,5 @@ /* - * © 2021, Neil McKechnie. All rights reserved. + * © 2023, Neil McKechnie. All rights reserved. * * This file is part of CommandStation-EX * @@ -96,9 +96,8 @@ void I2CManagerClass::I2C_init() void I2CManagerClass::I2C_sendStart() { bytesToSend = currentRequest->writeLen; bytesToReceive = currentRequest->readLen; - // We may have initiated a stop bit before this without waiting for it. - // Wait for stop bit to be sent before sending start. - while (TWCR & (1<operation & OPERATION_MASK; // Start the I2C process going. I2C_sendStart(); - startTime = micros(); } } } @@ -167,21 +168,30 @@ uint8_t I2CManagerClass::read(uint8_t i2cAddress, uint8_t *readBuffer, uint8_t r return I2C_STATUS_OK; } +/*************************************************************************** + * Set I2C timeout value in microseconds. The timeout applies to the entire + * I2CRB request, e.g. where a write+read is performed, the timer is not + * reset before the read. + ***************************************************************************/ +void I2CManagerClass::setTimeout(unsigned long value) { + timeout = value; +}; + /*************************************************************************** * checkForTimeout() function, called from isBusy() and wait() to cancel - * requests that are taking too long to complete. - * This function doesn't fully work as intended so is not currently called. - * Instead we check for an I2C hang-up and report an error from - * I2CRB::wait(), but we aren't able to recover from the hang-up. Such faults + * requests that are taking too long to complete. Such faults * may be caused by an I2C wire short for example. ***************************************************************************/ void I2CManagerClass::checkForTimeout() { - unsigned long currentMicros = micros(); ATOMIC_BLOCK(ATOMIC_RESTORESTATE) { I2CRB *t = queueHead; if (state==I2C_STATE_ACTIVE && t!=0 && t==currentRequest && timeout > 0) { // Check for timeout - if (currentMicros - startTime > timeout) { + unsigned long elapsed = micros() - startTime; + if (elapsed > timeout) { +#ifdef DIAG_IO + //DIAG(F("I2CManager Timeout on x%x, I2CRB=x%x"), t->i2cAddress, currentRequest); +#endif // Excessive time. Dequeue request queueHead = t->nextRequest; if (!queueHead) queueTail = NULL; @@ -192,7 +202,9 @@ void I2CManagerClass::checkForTimeout() { // Try close and init, not entirely satisfactory but sort of works... I2C_close(); // Shutdown and restart twi interface I2C_init(); + _setClock(_clockSpeed); state = I2C_STATE_FREE; +// I2C_sendStop(); // in case device is waiting for a stop condition // Initiate next queued request if any. startTransaction(); @@ -208,10 +220,8 @@ void I2CManagerClass::loop() { #if !defined(I2C_USE_INTERRUPTS) handleInterrupt(); #endif - // Timeout is now reported in I2CRB::wait(), not here. - // I've left the code, commented out, as a reminder to look at this again - // in the future. - //checkForTimeout(); + // Call function to monitor for stuch I2C operations. + checkForTimeout(); } /*************************************************************************** @@ -270,7 +280,6 @@ volatile uint8_t I2CManagerClass::operation; volatile uint8_t I2CManagerClass::bytesToSend; volatile uint8_t I2CManagerClass::bytesToReceive; volatile unsigned long I2CManagerClass::startTime; -unsigned long I2CManagerClass::timeout = 0; uint8_t I2CManagerClass::retryCounter = 0; #endif \ No newline at end of file diff --git a/I2CManager_Wire.h b/I2CManager_Wire.h index aea91aa..9749565 100644 --- a/I2CManager_Wire.h +++ b/I2CManager_Wire.h @@ -1,5 +1,5 @@ /* - * © 2021, Neil McKechnie. All rights reserved. + * © 2023, Neil McKechnie. All rights reserved. * * This file is part of CommandStation-EX * @@ -30,11 +30,19 @@ #define I2C_USE_WIRE #endif +// Older versions of Wire don't have setWireTimeout function. AVR does. +#ifdef ARDUINO_ARCH_AVR +#define WIRE_HAS_TIMEOUT +#endif + /*************************************************************************** * Initialise I2C interface software ***************************************************************************/ void I2CManagerClass::_initialise() { Wire.begin(); +#if defined(WIRE_HAS_TIMEOUT) + Wire.setWireTimeout(timeout, true); +#endif } /*************************************************************************** @@ -45,6 +53,18 @@ void I2CManagerClass::_setClock(unsigned long i2cClockSpeed) { Wire.setClock(i2cClockSpeed); } +/*************************************************************************** + * Set I2C timeout value in microseconds. The timeout applies to each + * Wire call separately, i.e. in a write+read, the timer is reset before the + * read is started. + ***************************************************************************/ +void I2CManagerClass::setTimeout(unsigned long value) { + timeout = value; +#if defined(WIRE_HAS_TIMEOUT) + Wire.setWireTimeout(value, true); +#endif +} + /*************************************************************************** * Initiate a write to an I2C device (blocking operation on Wire) ***************************************************************************/ @@ -93,10 +113,21 @@ uint8_t I2CManagerClass::read(uint8_t address, uint8_t readBuffer[], uint8_t rea status = Wire.endTransmission(false); // Don't free bus yet } if (status == I2C_STATUS_OK) { +#ifdef WIRE_HAS_TIMEOUT + Wire.clearWireTimeoutFlag(); +#endif Wire.requestFrom(address, (size_t)readSize); - while (Wire.available() && nBytes < readSize) - readBuffer[nBytes++] = Wire.read(); - if (nBytes < readSize) status = I2C_STATUS_TRUNCATED; +#ifdef WIRE_HAS_TIMEOUT + if (!Wire.getWireTimeoutFlag()) { +#endif + while (Wire.available() && nBytes < readSize) + readBuffer[nBytes++] = Wire.read(); + if (nBytes < readSize) status = I2C_STATUS_TRUNCATED; +#ifdef WIRE_HAS_TIMEOUT + } else { + status = I2C_STATUS_TIMEOUT; + } +#endif } } while (!(status == I2C_STATUS_OK || ++retryCount > MAX_I2C_RETRIES || rb->operation & OPERATION_NORETRY)); @@ -136,8 +167,4 @@ void I2CManagerClass::queueRequest(I2CRB *req) { ***************************************************************************/ void I2CManagerClass::loop() {} -// Loop function -void I2CManagerClass::checkForTimeout() {} - - #endif \ No newline at end of file