0

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).

Improve this question
6
  • Did you declare i2c1Millis1 as volatile?
    – Gerben
    Jun 5 '19 at 14:15
  • nope,never used that, when you mention that i read about that, looks like it not gonna help since millis is long.
    – user3503519
    Jun 6 '19 at 6:59
  • You should still use it!!! I think that's more likely to cause the issues you have. Both are problems, but one is more likely to occur than the other.
    – Gerben
    Jun 6 '19 at 10:13
  • After I moved millis update out of interrupt function, all works stable now.
    – user3503519
    Jun 6 '19 at 13:47
  • You then might want to add the volatile to the flag set in the interrupt.
    – Gerben
    Jun 6 '19 at 17:52
1

Yes. And it's even worse. Because your variables are longs, 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;
}
Improve this answer
5
  • 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 stable
    – user3503519
    Jun 5 '19 at 12:30
  • Depending on how large i2cset1 (<250) you could use single byte variable, instead of 4-byte longs. That way there is no need to disable interrupts.
    – Gerben
    Jun 5 '19 at 14:14
  • @user3503519, so you you used the flag recommendation from chrisl's answer and accepted Gerbens answer
    – Juraj ♦
    Jun 5 '19 at 16:53
  • 1st i read this answer and got idea to use a flag and do it. after that i read other anwers
    – user3503519
    Jun 6 '19 at 6:54
  • You can change the accepted answer to chrisl's, if I'm not mistaken.
    – Gerben
    Jun 6 '19 at 10:08
1

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.

Improve this answer
0

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.

Improve this answer
1
  • thank you for your answer, mentioning assembler I remind old Apple ][ days where I used to write in assembler and basic :)
    – user3503519
    Jun 5 '19 at 12:33

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.