Skip to content

Reduce code duplication for timeout handling in twi.c #1

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.

Sign up for GitHub

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

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Reduce code duplication for timeout handling in twi.c #1

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
c87bc7b
Add USBDevice.isSuspended()
algernon Dec 10, 2018
a901b08
Revert "Updated Arduino as ISP setting"
per1234 Dec 19, 2018
98c8a13
Add ATmega32U4-compatible Arduino as ISP programmer
per1234 Dec 19, 2018
4efd686
Merge pull request #56 from per1234/arduino-as-isp-atmega32u4-programmer
facchinm Dec 19, 2018
6ba4fd3
Merge pull request #54 from algernon/pull/wakeup-suspend-hooks
facchinm Dec 19, 2018
c27aef0
Remove commented out code for I2C interrupts on WInterrupts.c
paulo-raca Jan 4, 2019
5755dde
Merge pull request #59 from paulo-raca/wiring_private_i2c
facchinm Jan 7, 2019
a8ef5c0
#1476 Extend API by adding a possibility to set a global timeout in m…
wmarkow Feb 28, 2018
1e7fc4d
#1476. Introduce timeout when checking twi_state variable.
wmarkow Mar 1, 2018
14230ea
#1476 Add timout in twi_stop(void) method.
wmarkow Mar 3, 2018
7fcb4d4
#1476 Disable TWI before init.
wmarkow Mar 5, 2018
282faf0
Reduce code duplication with BUSYWAIT_WITH_TIMEOUT_UNTIL macro
hlovdal Feb 17, 2019
4040473
Remove trailing whitespace
hlovdal Feb 17, 2019
dd34343
Fix indentation
hlovdal Feb 17, 2019
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
2 changes: 2 additions & 0 deletions cores/arduino/USBAPI.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,8 @@ class USBDevice_
void detach(); // Serial port goes down too...
void poll();
bool wakeupHost(); // returns false, when wakeup cannot be processed

bool isSuspended();
};
extern USBDevice_ USBDevice;

Expand Down
6 changes: 6 additions & 0 deletions cores/arduino/USBCore.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -855,4 +855,10 @@ bool USBDevice_::wakeupHost()
return false;
}

bool USBDevice_::isSuspended()
{
return (_usbSuspendState & (1 << SUSPI));
}


#endif /* if defined(USBCON) */
14 changes: 0 additions & 14 deletions cores/arduino/WInterrupts.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,6 @@ static volatile voidFuncPtr intFunc[EXTERNAL_NUM_INTERRUPTS] = {
nothing,
#endif
};
// volatile static voidFuncPtr twiIntFunc;

void attachInterrupt(uint8_t interruptNum, void (*userFunc)(void), int mode) {
if(interruptNum < EXTERNAL_NUM_INTERRUPTS) {
Expand Down Expand Up @@ -274,11 +273,6 @@ void detachInterrupt(uint8_t interruptNum) {
}
}

/*
void attachInterruptTwi(void (*userFunc)(void) ) {
twiIntFunc = userFunc;
}
*/

#define IMPLEMENT_ISR(vect, interrupt) \
ISR(vect) { \
Expand Down Expand Up @@ -314,11 +308,3 @@ IMPLEMENT_ISR(INT2_vect, EXTERNAL_INT_2)
#endif

#endif

/*
ISR(TWI_vect) {
if(twiIntFunc)
twiIntFunc();
}
*/

5 changes: 5 additions & 0 deletions libraries/Wire/src/Wire.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,11 @@ void TwoWire::setClock(uint32_t clock)
twi_setFrequency(clock);
}

void TwoWire::setTimeoutInMillis(uint8_t timeout)
{
twi_setTimeoutInMillis(timeout);
}

uint8_t TwoWire::requestFrom(uint8_t address, uint8_t quantity, uint32_t iaddress, uint8_t isize, uint8_t sendStop)
{
if (isize > 0) {
Expand Down
1 change: 1 addition & 0 deletions libraries/Wire/src/Wire.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ class TwoWire : public Stream
void begin(int);
void end();
void setClock(uint32_t);
void setTimeoutInMillis(uint8_t);
void beginTransmission(uint8_t);
void beginTransmission(int);
uint8_t endTransmission(void);
Expand Down
71 changes: 56 additions & 15 deletions libraries/Wire/src/utility/twi.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
#include <avr/io.h>
#include <avr/interrupt.h>
#include <compat/twi.h>
#include "Arduino.h" // for digitalWrite
#include "Arduino.h" // for digitalWrite and millis

#ifndef cbi
#define cbi(sfr, bit) (_SFR_BYTE(sfr) &= ~_BV(bit))
Expand All @@ -38,10 +38,38 @@
#include "pins_arduino.h"
#include "twi.h"

#define BUSYWAIT_WITH_TIMEOUT_UNTIL(exit_condition, timedout_label) \
do { \
uint32_t startMillis = millis(); \
while (!(exit_condition)) { \
if ((twi_timeout_ms > 0) && (millis() - startMillis > twi_timeout_ms)) { \
goto timedout_label; \
} \
} \
} while (0)
Copy link
Owner

Choose a reason for hiding this comment

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

@hlovdal, Is this do {} while(0) a way to put the macro around a simple code block, so the BUSYWAIT_WITH_TIMEOUT_UNTIL macro can be put multiple times in the same function and the compiler will not complain about multiple startMillis variable definition?


// Same as BUSYWAIT_WITH_TIMEOUT_UNTIL but intended to be used in code
// executed in interrupt service routines where millis() cannot be used.
// https://www.arduino.cc/reference/en/language/functions/external-interrupts/attachinterrupt/:
// "millis() relies on interrupts to count, so it will never increment inside an ISR."
// TODO: Document calculations behind the 25000 upper counter limit.
Copy link
Owner

Choose a reason for hiding this comment

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

In this pull request
arduino/Arduino@5e0fd77
similar solution has been done but they relied only on the max counter of 100000 iterations, not the exact time in milliseconds. The milliseconds aproach is independent from the cpu speed. However in the ISR we can not use millis().
I think I have also took a short look at the disassembled listing of twi_stop() method in one of my project. Roughly it contains around 20 instructions. I have assumed average 2 clock cycles per instruction which will give 40 clock cycles. With 16 MHz clock I did the math: 40 * 25000 / 16000000 = 0.0625s = 62.5 ms as the value of timeout. I'm not uite sure if I calculated this correctly :)
I think - if we goes this way - it would be possible to implement some macro that will calculate the upper counter limit based on current cpu clock (I think the clock value is defined as some variable somewhere - F_CPU?). I see those macros already defined in Arduino.h:

#define clockCyclesPerMicrosecond() ( F_CPU / 1000000L )
#define clockCyclesToMicroseconds(a) ( (a) / clockCyclesPerMicrosecond() )
#define microsecondsToClockCycles(a) ( (a) * clockCyclesPerMicrosecond() )

I will figure something out.

// TODO: Make the upper counter limit reflect the twi_timeout_ms value.
Copy link
Owner

Choose a reason for hiding this comment

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

That seems to be - more or less - to be defined by a similar macro as commented above.

#define IRS_BUSYWAIT_WITH_TIMEOUT_UNTIL(exit_condition, timedout_label) \
do { \
uint32_t counter = 0; \
while (!(exit_condition)) { \
counter++; \
if ((twi_timeout_ms > 0) && (counter >= 25000)) { \
goto timedout_label; \
} \
} \
} while (0)

static volatile uint8_t twi_state;
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
static volatile uint8_t twi_timeout_ms = 0;

static void (*twi_onSlaveTransmit)(void);
static void (*twi_onSlaveReceive)(uint8_t*, int);
Expand Down Expand Up @@ -154,9 +182,8 @@ uint8_t twi_readFrom(uint8_t address, uint8_t* data, uint8_t length, uint8_t sen
}

// wait until twi is ready, become master receiver
while(TWI_READY != twi_state){
continue;
}
BUSYWAIT_WITH_TIMEOUT_UNTIL(twi_state == TWI_READY, waiting_timedout);

twi_state = TWI_MRX;
twi_sendStop = sendStop;
// reset error state (0xFF.. no error occured)
Expand Down Expand Up @@ -193,9 +220,7 @@ uint8_t twi_readFrom(uint8_t address, uint8_t* data, uint8_t length, uint8_t sen
TWCR = _BV(TWEN) | _BV(TWIE) | _BV(TWEA) | _BV(TWINT) | _BV(TWSTA);

// wait for read operation to complete
while(TWI_MRX == twi_state){
continue;
}
BUSYWAIT_WITH_TIMEOUT_UNTIL(twi_state != TWI_MRX, waiting_timedout);

if (twi_masterBufferIndex < length)
length = twi_masterBufferIndex;
Expand All @@ -206,6 +231,11 @@ uint8_t twi_readFrom(uint8_t address, uint8_t* data, uint8_t length, uint8_t sen
}

return length;

waiting_timedout:
twi_disable();
twi_init();
return 0;
}

/*
Expand Down Expand Up @@ -233,9 +263,8 @@ uint8_t twi_writeTo(uint8_t address, uint8_t* data, uint8_t length, uint8_t wait
}

// wait until twi is ready, become master transmitter
while(TWI_READY != twi_state){
continue;
}
BUSYWAIT_WITH_TIMEOUT_UNTIL(twi_state == TWI_READY, waiting_timedout);

twi_state = TWI_MTX;
twi_sendStop = sendStop;
// reset error state (0xFF.. no error occured)
Expand Down Expand Up @@ -275,8 +304,8 @@ uint8_t twi_writeTo(uint8_t address, uint8_t* data, uint8_t length, uint8_t wait
TWCR = _BV(TWINT) | _BV(TWEA) | _BV(TWEN) | _BV(TWIE) | _BV(TWSTA); // enable INTs

// wait for write operation to complete
while(wait && (TWI_MTX == twi_state)){
continue;
if (wait) {
BUSYWAIT_WITH_TIMEOUT_UNTIL(twi_state != TWI_MTX, waiting_timedout);
}

if (twi_error == 0xFF)
Expand All @@ -287,6 +316,11 @@ uint8_t twi_writeTo(uint8_t address, uint8_t* data, uint8_t length, uint8_t wait
return 3; // error: data send, nack received
else
return 4; // other twi error

waiting_timedout:
twi_disable();
twi_init();
return 4;
}

/*
Expand Down Expand Up @@ -373,12 +407,14 @@ void twi_stop(void)

// wait for stop condition to be exectued on bus
// TWINT is not set after a stop condition!
while(TWCR & _BV(TWSTO)){
continue;
}
IRS_BUSYWAIT_WITH_TIMEOUT_UNTIL((TWCR & _BV(TWSTO)) == 0, waiting_timedout);

// update twi state
twi_state = TWI_READY;

Copy link
Owner

Choose a reason for hiding this comment

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

@hlovdal , is here (before waiting_timedout label) a return; statement missing? Currently the TWI is always disabled and initated again, no matter if it was timedout or not. Am i rght?

waiting_timedout:
twi_disable();
twi_init();
}

/*
Expand All @@ -396,6 +432,11 @@ void twi_releaseBus(void)
twi_state = TWI_READY;
}

void twi_setTimeoutInMillis(uint8_t timeout)
{
twi_timeout_ms = timeout;
}

ISR(TWI_vect)
{
switch(TW_STATUS){
Expand Down
1 change: 1 addition & 0 deletions libraries/Wire/src/utility/twi.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@
void twi_reply(uint8_t);
void twi_stop(void);
void twi_releaseBus(void);
void twi_setTimeoutInMillis(uint8_t);

#endif

13 changes: 11 additions & 2 deletions programmers.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,13 +43,22 @@ parallel.program.extra_params=-F

arduinoasisp.name=Arduino as ISP
arduinoasisp.communication=serial
arduinoasisp.protocol=arduino
arduinoasisp.protocol=stk500v1
arduinoasisp.speed=19200
arduinoasisp.program.protocol=arduino
arduinoasisp.program.protocol=stk500v1
arduinoasisp.program.speed=19200
arduinoasisp.program.tool=avrdude
arduinoasisp.program.extra_params=-P{serial.port} -b{program.speed}

arduinoasispatmega32u4.name=Arduino as ISP (ATmega32U4)
arduinoasispatmega32u4.communication=serial
arduinoasispatmega32u4.protocol=arduino
arduinoasispatmega32u4.speed=19200
arduinoasispatmega32u4.program.protocol=arduino
arduinoasispatmega32u4.program.speed=19200
arduinoasispatmega32u4.program.tool=avrdude
arduinoasispatmega32u4.program.extra_params=-P{serial.port} -b{program.speed}

usbGemma.name=Arduino Gemma
usbGemma.protocol=arduinogemma
usbGemma.program.tool=avrdude
Expand Down