From deea9293201dbab724b6b0519c35ddba3e6b92d9 Mon Sep 17 00:00:00 2001 From: Greyson Christoforo Date: Wed, 28 Feb 2018 20:59:45 +0000 Subject: Introduce non compulsory Wire timeout move timout handling into its own function change timeout from milliseconds to microseconds don't forget operating slave address or the bitrate when we reset because of a timeout Co-Authored-By: Witold Markowski fix delay datatype uint16_t --> uint32_t Update libraries/Wire/src/utility/twi.c fix mix up using TWBR instea of TWAR! Co-Authored-By: Matthijs Kooijman Update libraries/Wire/src/utility/twi.c fix 2nd TWBR/TWAR mixup Co-Authored-By: Matthijs Kooijman twi_stop() should use the same timeout as everywhere else all while loops are now protected by timeouts Revert "twi_stop() should use the same timeout as everywhere else" This reverts commit 68fe5f1dae1bb41183bb37eeda3fb453394a580c. make timeout counter volatile rename timeout function for improved clarity - resetting the twi interface on timeouts is now optional - timeouts in the ISR are no longer hardcoded and now obey the set timeout value - a user-readable flag is now set whenever a timeout occurs - the user can clear this flag whenever they like --- libraries/Wire/src/Wire.cpp | 28 +++++++ libraries/Wire/src/Wire.h | 6 +- libraries/Wire/src/utility/twi.c | 171 +++++++++++++++++++++++++++++++-------- libraries/Wire/src/utility/twi.h | 6 +- 4 files changed, 174 insertions(+), 37 deletions(-) (limited to 'libraries/Wire/src') diff --git a/libraries/Wire/src/Wire.cpp b/libraries/Wire/src/Wire.cpp index 58916ce..b446f9a 100644 --- a/libraries/Wire/src/Wire.cpp +++ b/libraries/Wire/src/Wire.cpp @@ -18,6 +18,7 @@ Modified 2012 by Todd Krein (todd@krein.org) to implement repeated starts Modified 2017 by Chuck Todd (ctodd@cableone.net) to correct Unconfigured Slave Mode reboot + Modified 2020 by Greyson Christoforo (grey@christoforo.net) to implement timeouts */ extern "C" { @@ -86,6 +87,33 @@ void TwoWire::setClock(uint32_t clock) twi_setFrequency(clock); } +/*** + * Sets the TWI timeout. + * + * @param timeout a timeout value in microseconds, if zero then timeout checking is disabled + * @param reset_with_timeout if true then TWI interface will be automatically reset on timeout + * if false then TWI interface will not be reset on timeout + */ +void TwoWire::setWireTimeoutUs(uint32_t timeout, bool reset_with_timeout){ + twi_setTimeoutInMicros(timeout, reset_with_timeout); +} + +/*** + * Returns the TWI timeout flag. + * + * @return true if timeout has occured + */ +bool TwoWire::getWireTimeoutFlag(void){ + return(twi_manageTimeoutFlag(false)); +} + +/*** + * Clears the TWI timeout flag. + */ +void TwoWire::clearWireTimeoutFlag(void){ + twi_manageTimeoutFlag(true); +} + uint8_t TwoWire::requestFrom(uint8_t address, uint8_t quantity, uint32_t iaddress, uint8_t isize, uint8_t sendStop) { if (isize > 0) { diff --git a/libraries/Wire/src/Wire.h b/libraries/Wire/src/Wire.h index 702f37d..c8b9a3e 100644 --- a/libraries/Wire/src/Wire.h +++ b/libraries/Wire/src/Wire.h @@ -17,6 +17,7 @@ Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA Modified 2012 by Todd Krein (todd@krein.org) to implement repeated starts + Modified 2020 by Greyson Christoforo (grey@christoforo.net) to implement timeouts */ #ifndef TwoWire_h @@ -54,13 +55,16 @@ class TwoWire : public Stream void begin(int); void end(); void setClock(uint32_t); + void setWireTimeoutUs(uint32_t, bool); + bool getWireTimeoutFlag(void); + void clearWireTimeoutFlag(void); void beginTransmission(uint8_t); void beginTransmission(int); uint8_t endTransmission(void); uint8_t endTransmission(uint8_t); uint8_t requestFrom(uint8_t, uint8_t); uint8_t requestFrom(uint8_t, uint8_t, uint8_t); - uint8_t requestFrom(uint8_t, uint8_t, uint32_t, uint8_t, uint8_t); + uint8_t requestFrom(uint8_t, uint8_t, uint32_t, uint8_t, uint8_t); uint8_t requestFrom(int, int); uint8_t requestFrom(int, int, int); virtual size_t write(uint8_t); diff --git a/libraries/Wire/src/utility/twi.c b/libraries/Wire/src/utility/twi.c index 1a35146..b3096c6 100644 --- a/libraries/Wire/src/utility/twi.c +++ b/libraries/Wire/src/utility/twi.c @@ -17,6 +17,7 @@ Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA Modified 2012 by Todd Krein (todd@krein.org) to implement repeated starts + Modified 2020 by Greyson Christoforo (grey@christoforo.net) to implement timeouts */ #include @@ -24,8 +25,9 @@ #include #include #include +#include #include -#include "Arduino.h" // for digitalWrite +#include "Arduino.h" // for digitalWrite and micros #ifndef cbi #define cbi(sfr, bit) (_SFR_BYTE(sfr) &= ~_BV(bit)) @@ -43,6 +45,16 @@ static volatile uint8_t twi_slarw; static volatile uint8_t twi_sendStop; // should the transaction end with a stop static volatile uint8_t twi_inRepStart; // in the middle of a repeated start +// twi_timeout_us > 0 prevents the code from getting stuck in various while loops here +// if twi_timeout_us == 0 then timeout checking is disabled (the previous Wire lib behavior) +// at some point in the future, the default twi_timeout_us value could become 25000 +// and twi_do_reset_on_timeout could become true +// to conform to the SMBus standard +// http://smbus.org/specs/SMBus_3_1_20180319.pdf +static volatile uint32_t twi_timeout_us = 0ul; +static volatile bool twi_timed_out_flag = false; // a timeout has been seen +static volatile bool twi_do_reset_on_timeout = false; // reset the TWI registers on timeout + static void (*twi_onSlaveTransmit)(void); static void (*twi_onSlaveReceive)(uint8_t*, int); @@ -154,8 +166,12 @@ uint8_t twi_readFrom(uint8_t address, uint8_t* data, uint8_t length, uint8_t sen } // wait until twi is ready, become master receiver + uint32_t startMicros = micros(); while(TWI_READY != twi_state){ - continue; + if((twi_timeout_us > 0ul) && ((micros() - startMicros) > twi_timeout_us)) { + twi_handleTimeout(twi_do_reset_on_timeout); + return 0; + } } twi_state = TWI_MRX; twi_sendStop = sendStop; @@ -183,28 +199,38 @@ uint8_t twi_readFrom(uint8_t address, uint8_t* data, uint8_t length, uint8_t sen // up. Also, don't enable the START interrupt. There may be one pending from the // repeated start that we sent ourselves, and that would really confuse things. twi_inRepStart = false; // remember, we're dealing with an ASYNC ISR + startMicros = micros(); do { TWDR = twi_slarw; + if((twi_timeout_us > 0ul) && ((micros() - startMicros) > twi_timeout_us)) { + twi_handleTimeout(twi_do_reset_on_timeout); + return 0; + } } while(TWCR & _BV(TWWC)); TWCR = _BV(TWINT) | _BV(TWEA) | _BV(TWEN) | _BV(TWIE); // enable INTs, but not START - } - else + } else { // send start condition TWCR = _BV(TWEN) | _BV(TWIE) | _BV(TWEA) | _BV(TWINT) | _BV(TWSTA); + } // wait for read operation to complete + startMicros = micros(); while(TWI_MRX == twi_state){ - continue; + if((twi_timeout_us > 0ul) && ((micros() - startMicros) > twi_timeout_us)) { + twi_handleTimeout(twi_do_reset_on_timeout); + return 0; + } } - if (twi_masterBufferIndex < length) + if (twi_masterBufferIndex < length) { length = twi_masterBufferIndex; + } // copy twi buffer to data for(i = 0; i < length; ++i){ data[i] = twi_masterBuffer[i]; } - + return length; } @@ -222,6 +248,7 @@ uint8_t twi_readFrom(uint8_t address, uint8_t* data, uint8_t length, uint8_t sen * 2 .. address send, NACK received * 3 .. data send, NACK received * 4 .. other twi error (lost bus arbitration, bus error, ..) + * 5 .. timeout */ uint8_t twi_writeTo(uint8_t address, uint8_t* data, uint8_t length, uint8_t wait, uint8_t sendStop) { @@ -233,8 +260,12 @@ uint8_t twi_writeTo(uint8_t address, uint8_t* data, uint8_t length, uint8_t wait } // wait until twi is ready, become master transmitter + uint32_t startMicros = micros(); while(TWI_READY != twi_state){ - continue; + if((twi_timeout_us > 0ul) && ((micros() - startMicros) > twi_timeout_us)) { + twi_handleTimeout(twi_do_reset_on_timeout); + return (5); + } } twi_state = TWI_MTX; twi_sendStop = sendStop; @@ -265,18 +296,27 @@ uint8_t twi_writeTo(uint8_t address, uint8_t* data, uint8_t length, uint8_t wait // up. Also, don't enable the START interrupt. There may be one pending from the // repeated start that we sent outselves, and that would really confuse things. twi_inRepStart = false; // remember, we're dealing with an ASYNC ISR + startMicros = micros(); do { - TWDR = twi_slarw; + TWDR = twi_slarw; + if((twi_timeout_us > 0ul) && ((micros() - startMicros) > twi_timeout_us)) { + twi_handleTimeout(twi_do_reset_on_timeout); + return (5); + } } while(TWCR & _BV(TWWC)); TWCR = _BV(TWINT) | _BV(TWEA) | _BV(TWEN) | _BV(TWIE); // enable INTs, but not START - } - else + } else { // send start condition TWCR = _BV(TWINT) | _BV(TWEA) | _BV(TWEN) | _BV(TWIE) | _BV(TWSTA); // enable INTs + } // wait for write operation to complete + startMicros = micros(); while(wait && (TWI_MTX == twi_state)){ - continue; + if((twi_timeout_us > 0ul) && ((micros() - startMicros) > twi_timeout_us)) { + twi_handleTimeout(twi_do_reset_on_timeout); + return (5); + } } if (twi_error == 0xFF) @@ -356,7 +396,7 @@ void twi_reply(uint8_t ack) if(ack){ TWCR = _BV(TWEN) | _BV(TWIE) | _BV(TWINT) | _BV(TWEA); }else{ - TWCR = _BV(TWEN) | _BV(TWIE) | _BV(TWINT); + TWCR = _BV(TWEN) | _BV(TWIE) | _BV(TWINT); } } @@ -373,8 +413,17 @@ void twi_stop(void) // wait for stop condition to be exectued on bus // TWINT is not set after a stop condition! + volatile uint32_t counter = twi_timeout_us/10ul; // approximate the timeout while(TWCR & _BV(TWSTO)){ - continue; + if(twi_timeout_us > 0ul){ + if (counter > 0ul){ + _delay_us(10); + counter--; + } else { + twi_handleTimeout(twi_do_reset_on_timeout); + return; + } + } } // update twi state @@ -396,6 +445,59 @@ void twi_releaseBus(void) twi_state = TWI_READY; } +/* + * Function twi_setTimeoutInMicros + * Desc set a timeout for while loops that twi might get stuck in + * Input timeout value in microseconds (0 means never time out) + * Input reset_with_timeout: true causes timeout events to reset twi + * Output none + */ +void twi_setTimeoutInMicros(uint32_t timeout, bool reset_with_timeout){ + twi_timed_out_flag = false; + twi_timeout_us = timeout; + twi_do_reset_on_timeout = reset_with_timeout; +} + +/* + * Function twi_handleTimeout + * Desc this gets called whenever a while loop here has lasted longer than + * twi_timeout_us microseconds. always sets twi_timed_out_flag + * Input reset: true causes this function to reset the twi hardware interface + * Output none + */ +void twi_handleTimeout(bool reset){ + twi_timed_out_flag = true; + + if (reset) { + // remember bitrate and address settings + uint8_t previous_TWBR = TWBR; + uint8_t previous_TWAR = TWAR; + + // reset the interface + twi_disable(); + twi_init(); + + // reapply the previous register values + TWAR = previous_TWAR; + TWBR = previous_TWBR; + } +} + +/* + * Function twi_manageTimeoutFlag + * Desc returns true if twi has seen a timeout + * optionally clears the timeout flag + * Input clear_flag: true if we should reset the hardware + * Output none + */ +bool twi_manageTimeoutFlag(bool clear_flag){ + bool flag = twi_timed_out_flag; + if (clear_flag){ + twi_timed_out_flag = false; + } + return(flag); +} + ISR(TWI_vect) { switch(TW_STATUS){ @@ -416,16 +518,16 @@ ISR(TWI_vect) TWDR = twi_masterBuffer[twi_masterBufferIndex++]; twi_reply(1); }else{ - if (twi_sendStop) + if (twi_sendStop){ twi_stop(); - else { - twi_inRepStart = true; // we're gonna send the START - // don't enable the interrupt. We'll generate the start, but we - // avoid handling the interrupt until we're in the next transaction, - // at the point where we would normally issue the start. - TWCR = _BV(TWINT) | _BV(TWSTA)| _BV(TWEN) ; - twi_state = TWI_READY; - } + } else { + twi_inRepStart = true; // we're gonna send the START + // don't enable the interrupt. We'll generate the start, but we + // avoid handling the interrupt until we're in the next transaction, + // at the point where we would normally issue the start. + TWCR = _BV(TWINT) | _BV(TWSTA)| _BV(TWEN) ; + twi_state = TWI_READY; + } } break; case TW_MT_SLA_NACK: // address sent, nack received @@ -457,17 +559,17 @@ ISR(TWI_vect) case TW_MR_DATA_NACK: // data received, nack sent // put final byte into buffer twi_masterBuffer[twi_masterBufferIndex++] = TWDR; - if (twi_sendStop) - twi_stop(); - else { - twi_inRepStart = true; // we're gonna send the START - // don't enable the interrupt. We'll generate the start, but we - // avoid handling the interrupt until we're in the next transaction, - // at the point where we would normally issue the start. - TWCR = _BV(TWINT) | _BV(TWSTA)| _BV(TWEN) ; - twi_state = TWI_READY; - } - break; + if (twi_sendStop){ + twi_stop(); + } else { + twi_inRepStart = true; // we're gonna send the START + // don't enable the interrupt. We'll generate the start, but we + // avoid handling the interrupt until we're in the next transaction, + // at the point where we would normally issue the start. + TWCR = _BV(TWINT) | _BV(TWSTA)| _BV(TWEN) ; + twi_state = TWI_READY; + } + break; case TW_MR_SLA_NACK: // address sent, nack received twi_stop(); break; @@ -560,4 +662,3 @@ ISR(TWI_vect) break; } } - diff --git a/libraries/Wire/src/utility/twi.h b/libraries/Wire/src/utility/twi.h index d27325e..85b9837 100644 --- a/libraries/Wire/src/utility/twi.h +++ b/libraries/Wire/src/utility/twi.h @@ -15,6 +15,8 @@ You should have received a copy of the GNU Lesser General Public License along with this library; if not, write to the Free Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA + + Modified 2020 by Greyson Christoforo (grey@christoforo.net) to implement timeouts */ #ifndef twi_h @@ -50,6 +52,8 @@ void twi_reply(uint8_t); void twi_stop(void); void twi_releaseBus(void); + void twi_setTimeoutInMicros(uint32_t, bool); + void twi_handleTimeout(bool); + bool twi_manageTimeoutFlag(bool); #endif - -- cgit v1.2.3-18-g5258 From 38ff552087efdbe0d98a88e9f74bfeb5e1504997 Mon Sep 17 00:00:00 2001 From: Martino Facchin Date: Thu, 11 Jun 2020 15:36:44 +0200 Subject: Wire: add sensible defaults to setWireTimeout --- libraries/Wire/src/Wire.cpp | 2 +- libraries/Wire/src/Wire.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) (limited to 'libraries/Wire/src') diff --git a/libraries/Wire/src/Wire.cpp b/libraries/Wire/src/Wire.cpp index b446f9a..81aca98 100644 --- a/libraries/Wire/src/Wire.cpp +++ b/libraries/Wire/src/Wire.cpp @@ -94,7 +94,7 @@ void TwoWire::setClock(uint32_t clock) * @param reset_with_timeout if true then TWI interface will be automatically reset on timeout * if false then TWI interface will not be reset on timeout */ -void TwoWire::setWireTimeoutUs(uint32_t timeout, bool reset_with_timeout){ +void TwoWire::setWireTimeout(uint32_t timeout, bool reset_with_timeout){ twi_setTimeoutInMicros(timeout, reset_with_timeout); } diff --git a/libraries/Wire/src/Wire.h b/libraries/Wire/src/Wire.h index c8b9a3e..e70d72e 100644 --- a/libraries/Wire/src/Wire.h +++ b/libraries/Wire/src/Wire.h @@ -55,7 +55,7 @@ class TwoWire : public Stream void begin(int); void end(); void setClock(uint32_t); - void setWireTimeoutUs(uint32_t, bool); + void setWireTimeout(uint32_t timeout = 25000, bool reset_with_timeout = false); bool getWireTimeoutFlag(void); void clearWireTimeoutFlag(void); void beginTransmission(uint8_t); -- cgit v1.2.3-18-g5258 From f1fe5e8f08a54c053e12d84810a62e677a69294e Mon Sep 17 00:00:00 2001 From: Matthijs Kooijman Date: Thu, 11 Jun 2020 15:39:45 +0200 Subject: Wire: improve comments on timeout --- libraries/Wire/src/Wire.cpp | 21 ++++++++++++++++++++- libraries/Wire/src/utility/twi.c | 4 +++- 2 files changed, 23 insertions(+), 2 deletions(-) (limited to 'libraries/Wire/src') diff --git a/libraries/Wire/src/Wire.cpp b/libraries/Wire/src/Wire.cpp index 81aca98..c407776 100644 --- a/libraries/Wire/src/Wire.cpp +++ b/libraries/Wire/src/Wire.cpp @@ -90,9 +90,28 @@ void TwoWire::setClock(uint32_t clock) /*** * Sets the TWI timeout. * + * This limits the maximum time to wait for the TWI hardware. If more time passes, the bus is assumed + * to have locked up (e.g. due to noise-induced glitches or faulty slaves) and the transaction is aborted. + * Optionally, the TWI hardware is also reset, which can be required to allow subsequent transactions to + * succeed in some cases (in particular when noise has made the TWI hardware think there is a second + * master that has claimed the bus). + * + * When a timeout is triggered, a flag is set that can be queried with `getWireTimeoutFlag()` and is cleared + * when `clearWireTimeoutFlag()` or `setWireTimeoutUs()` is called. + * + * Note that this timeout can also trigger while waiting for clock stretching or waiting for a second master + * to complete its transaction. So make sure to adapt the timeout to accomodate for those cases if needed. + * A typical timeout would be 25ms (which is the maximum clock stretching allowed by the SMBus protocol), + * but (much) shorter values will usually also work. + * + * In the future, a timeout will be enabled by default, so if you require the timeout to be disabled, it is + * recommended you disable it by default using `setWireTimeoutUs(0)`, even though that is currently + * the default. + * * @param timeout a timeout value in microseconds, if zero then timeout checking is disabled * @param reset_with_timeout if true then TWI interface will be automatically reset on timeout * if false then TWI interface will not be reset on timeout + */ void TwoWire::setWireTimeout(uint32_t timeout, bool reset_with_timeout){ twi_setTimeoutInMicros(timeout, reset_with_timeout); @@ -101,7 +120,7 @@ void TwoWire::setWireTimeout(uint32_t timeout, bool reset_with_timeout){ /*** * Returns the TWI timeout flag. * - * @return true if timeout has occured + * @return true if timeout has occured since the flag was last cleared. */ bool TwoWire::getWireTimeoutFlag(void){ return(twi_manageTimeoutFlag(false)); diff --git a/libraries/Wire/src/utility/twi.c b/libraries/Wire/src/utility/twi.c index b3096c6..5539633 100644 --- a/libraries/Wire/src/utility/twi.c +++ b/libraries/Wire/src/utility/twi.c @@ -413,7 +413,9 @@ void twi_stop(void) // wait for stop condition to be exectued on bus // TWINT is not set after a stop condition! - volatile uint32_t counter = twi_timeout_us/10ul; // approximate the timeout + // We cannot use micros() from an ISR, so approximate the timeout with cycle-counted delays + const uint8_t us_per_loop = 8; + uint32_t counter = (twi_timeout_us + us_per_loop - 1)/us_per_loop; // Round up while(TWCR & _BV(TWSTO)){ if(twi_timeout_us > 0ul){ if (counter > 0ul){ -- cgit v1.2.3-18-g5258 From 3055c1efa3c6980c864f661e6c8cc5d5ac773af4 Mon Sep 17 00:00:00 2001 From: Martino Facchin Date: Thu, 11 Jun 2020 17:12:12 +0200 Subject: Wire: apply last suggested comment from @matthijskooijman --- libraries/Wire/src/utility/twi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'libraries/Wire/src') diff --git a/libraries/Wire/src/utility/twi.c b/libraries/Wire/src/utility/twi.c index 5539633..e8a41a2 100644 --- a/libraries/Wire/src/utility/twi.c +++ b/libraries/Wire/src/utility/twi.c @@ -419,7 +419,7 @@ void twi_stop(void) while(TWCR & _BV(TWSTO)){ if(twi_timeout_us > 0ul){ if (counter > 0ul){ - _delay_us(10); + _delay_us(us_per_loop); counter--; } else { twi_handleTimeout(twi_do_reset_on_timeout); -- cgit v1.2.3-18-g5258