I got an issues from few days, got some code which reset millis time on successful i2c communication, if not it restart an relay after specified period of time. Here is part of the code:
if ((unsigned long)(i2c1Millis1 - i2cpreviousMillis1) >= i2cset1 ) // check if not restarted soon
{
i2c1Millis1
are updated every run of the loop and on successful i2c communication.
i2cpreviousMillis1
is updated on successful i2c request (coming from interrupt)
I manage to debug when false restart occur and looks like i2c1Millis1
is lower than i2cpreviousMillis1
So question is: can an interrupt occur while those two millis are compared when value of i2c1Millis1
is taken for compare and then come interrupt where i2cpreviousMillis1
is updated to current millis and it become greater and trigger the condition?
(I already modified my code i2cpreviousMillis1
is updated with i2c1Millis1
value on successful communication to prevent that, but it is not working, from time to time it made false restarts).
3 Answers 3
Yes. And it's even worse. Because your variables are long
s, they are 4 bytes long, and most Arduinos are only 8-bit processors, the code only checks one byte at a time. So for example, after checking the first byte, an interrupt could occur, changing the 2nd, 3rd, and 4th byte. So you end up comparing the first byte of the old value, and other bytes of the new value.
To prevent this you could disable interrupt for a very short time, and make a copy the variable.
cli();//disable interrupts
unsigned long temp = i2c1Millis1;
sei();//enable interrupts
Or use the util/atomic.h macros
#include <util/atomic.h>
...
ATOMIC_BLOCK(ATOMIC_FORCEON)
{
unsigned long temp = i2c1Millis1;
}
-
I have too many disable interrupts at my code, if increase them too much gonna start to mess with i2c communication, I changed code,raising a flag (byte variable) to trigger updating millis in main loop, not inside interrupt routene, so far works stableuser3503519– user35035192019年06月05日 12:30:53 +00:00Commented Jun 5, 2019 at 12:30
-
Depending on how large
i2cset1
(<250) you could use singlebyte
variable, instead of 4-bytelong
s. That way there is no need to disable interrupts.Gerben– Gerben2019年06月05日 14:14:43 +00:00Commented Jun 5, 2019 at 14:14 -
@user3503519, so you you used the flag recommendation from chrisl's answer and accepted Gerbens answer2019年06月05日 16:53:40 +00:00Commented Jun 5, 2019 at 16:53
-
1st i read this answer and got idea to use a flag and do it. after that i read other anwersuser3503519– user35035192019年06月06日 06:54:06 +00:00Commented Jun 6, 2019 at 6:54
-
You can change the accepted answer to chrisl's, if I'm not mistaken.Gerben– Gerben2019年06月06日 10:08:40 +00:00Commented Jun 6, 2019 at 10:08
Yes, an interrupt can occur at any time and can change the values during a calculation. And since you work with unsigned long
variables (which are 4 bytes as I remember), it is also possible, that the calculation uses the old value for the first bytes and the new value for the other bytes, meaning that the used value for calcuation is totally garbage.
To prevent that your checks must either be atomic (meaning consisting only of 1 base instruction, which cannot be interrupted, e.g. handling only 1 byte) (that is not possible here), or you must prevent the calculation from being interrupted. For this you can turn off all or only the corresponding interrupt. To turn off all interrupts you can use noInterrupts()
and interrupts()
to turn them on again.
But it is questionable, why you need to update this time directly in the ISR. Instead you could only set an atomic flag (1 byte) and check this flag in the loop. If the flag was set, reset it and save the millis value. If you loop runs fast enough (which seems the case, since you already use it to set a millis timestamp) that should be sufficient
Both ways can be used and it depends on you, if it's ok to turn of interrupts for a short time, since you might miss an I2C interrupt this way.
Yes. An interrupt can occur pretty much anywhere.
Interrupts are processed between each assembly language instruction. That if
will be made up of many many ASM instructions, so there are many many places where an interrupt can occur.
To avoid it you use what are called critical sections where you disable interrupts before performing some operations, then enable them again afterwards.
It is recommended to keep critical sections as short as possible to avoid breaking your interrupts. For this reason most critical sections generally involve copying values to or from local variables before then doing operations on them.
noInterrupts();
uint32_t localPreviousMillis = i2cpreviousMillis1;
interrupts();
if ((unsigned long)(i2c1Millis1 - localPreviousMillis) >= i2cset1 ) // check if not restarted soon
On an 8-bit system even assigning a simple value like that can be interrupted. A 32-bit value is made up of four 8-bit values, and an interrupt can occur between the copying of any of those 4 bytes.
-
thank you for your answer, mentioning assembler I remind old Apple ][ days where I used to write in assembler and basic :)user3503519– user35035192019年06月05日 12:33:27 +00:00Commented Jun 5, 2019 at 12:33
i2c1Millis1
asvolatile
?volatile
to the flag set in the interrupt.