Skip to content

I2C/Wire library: changes for issue #42 #107

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 17 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions libraries/Wire/src/Wire.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

Modified 2012 by Todd Krein ([email protected]) to implement repeated starts
Modified 2017 by Chuck Todd ([email protected]) to correct Unconfigured Slave Mode reboot
Modified 2020 by Greyson Christoforo ([email protected]) to implement timeouts
*/

extern "C" {
Expand Down Expand Up @@ -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
Comment on lines +91 to +95
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* 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
* 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::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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/***
* 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) {
Expand Down
6 changes: 5 additions & 1 deletion libraries/Wire/src/Wire.h
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA

Modified 2012 by Todd Krein ([email protected]) to implement repeated starts
Modified 2020 by Greyson Christoforo ([email protected]) to implement timeouts
*/

#ifndef TwoWire_h
Expand Down Expand Up @@ -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);
Expand Down
171 changes: 136 additions & 35 deletions libraries/Wire/src/utility/twi.c
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,17 @@
Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA

Modified 2012 by Todd Krein ([email protected]) to implement repeated starts
Modified 2020 by Greyson Christoforo ([email protected]) to implement timeouts
*/

#include <math.h>
#include <stdlib.h>
#include <inttypes.h>
#include <avr/io.h>
#include <avr/interrupt.h>
#include <util/delay.h>
#include <compat/twi.h>
#include "Arduino.h" // for digitalWrite
#include "Arduino.h" // for digitalWrite and micros

#ifndef cbi
#define cbi(sfr, bit) (_SFR_BYTE(sfr) &= ~_BV(bit))
Expand All @@ -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);

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}

Expand All @@ -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)
{
Expand All @@ -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;
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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);
}
}

Expand All @@ -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
Copy link
Collaborator

@matthijskooijman matthijskooijman May 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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

No need to make this volatile AFAICS, there will not be an ISR interrupting this code and writing to counter. Also, round up, to make sure the timeout is never shorter than configured. And also use a constant, to prevent duplicating the 10.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to use 8 as a value for us_per_loop variable? Is division by 8 simple to perform as it is mostly like shifting the register by three bits?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, seems like a good suggestion to me. Suggestion updated.

while(TWCR & _BV(TWSTO)){
continue;
if(twi_timeout_us > 0ul){
if (counter > 0ul){
_delay_us(10);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
_delay_us(10);
_delay_us(us_per_loop);

counter--;
} else {
twi_handleTimeout(twi_do_reset_on_timeout);
return;
}
}
}

// update twi state
Expand All @@ -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){
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why have a reset parameter here? It seems it is always passed twi_do_reset_on_timeout, so might as well use that global variable directly here?

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){
Expand All @@ -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
Expand Down Expand Up @@ -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){
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be a space before the { I think.

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;
Expand Down Expand Up @@ -560,4 +662,3 @@ ISR(TWI_vect)
break;
}
}

6 changes: 5 additions & 1 deletion libraries/Wire/src/utility/twi.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 ([email protected]) to implement timeouts
*/

#ifndef twi_h
Expand Down Expand Up @@ -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