Reduce code duplication for timeout handling in twi.c #1
Conversation
Based on code originally by Rob van der Veer <[email protected]>, this adds USBDevice.isSuspended(), so user sketches can run custom code in their `loop` methods after checking if the device is suspended or not. Signed-off-by: Gergely Nagy <[email protected]>
This reverts commit b084848.
Use of the stk500v1 protocol for Arduino as ISP does not work with native USB boards on Windows. The arduino protocol does. However, the arduino protocol makes it more likely that boards with an external USB interface chip will require the auto-reset circuitry to be disabled to allow them to be used as Arduino as ISP. That adds extra complexity to a process already difficult for the average Arduino user. For this reason, a new programmer using the arduino protocol is added specifically for using native USB boards as Arduino as ISP and the previous Arduino as ISP configuration is retained for use with all other boards.
…programmer Add ATmega32U4-compatible Arduino as ISP programmer
Add USBDevice.isSuspended()
These are currently implemented by the Wire library, on twi.c
Remove commented out code for I2C interrupts on WInterrupts.c
|
Hi @hlovdal, thanks for your input in this case. Indeed - my solution introduces some code duplication. I will take a look at this soon. In your application is the issue (hang in endless while loops) easy to reproduce? |
|
|
||
| // update twi state | ||
| twi_state = TWI_READY; | ||
|
|
There was a problem hiding this comment.
@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?
| goto timedout_label; \ | ||
| } \ | ||
| } \ | ||
| } while (0) |
There was a problem hiding this comment.
@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?
| // 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. |
There was a problem hiding this comment.
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.
| // 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. | ||
| // TODO: Make the upper counter limit reflect the twi_timeout_ms value. |
There was a problem hiding this comment.
That seems to be - more or less - to be defined by a similar macro as commented above.
|
@wmarkow Could you delete the post at mathertel/Radio#27 ? The Wire library is part of the Stream family. The Wire library needs a low level timeout for the bus, but when using the higher level Stream functions (for example The name The 'goto' is not needed in my opinion. The SDA or SCL could be shortcut to GND or to each other. That is a situation that might happen, and it can be handled with normal if-statements. Nevertheless, an embedded system that can be stopped by an event from the outside is in my opinion a nightmare. So any timeout in the Wire library is better than no timeout. |
|
@Koepel, @mathertel, sorry for inconvenience - I have already deleted my post in mathertel's issue#27. @Koepel, thanks for your input in the case. I will take a look at this Anyway thank, I will take a look. |
Hi. I have some improvements to your
issue_#42branch. First I fetched the original commits from the originalissue_#1476branch in the Arduino repository to keep the history intact and rebased to the latest upstream commit. Then I introduced a macro for timeout handling to reduce code duplication and increase readability. There are also some code formatting cleanup, let me know if you want this in a different pull request.Do a pull of upstream master branch before completing this pull request or let me know if you want me to rebase down to your latest master.