Go Down

Topic: While loop preventing ISR from working (Read 10249 times) previous topic - next topic

nickgammon

Just to be clear, this could be bad:

Code: [Select]
WDT_POOL_COUNT++;

That is because that both reads and writes.

And BTW @op - please don't name variables in all caps. Reserve that for constants.
Please post technical questions on the forum, not by personal message. Thanks!

More info: http://www.gammon.com.au/electronics

JChristensen


Yes, Jack, but reading one byte is an atomic operation. You don't need to put a critical section around that. If the interrupt occurs before the read you get the new value, if it happens after the read, well that would have happened anyway with a critical section. It can't happen during the read.


Can't disagree, but I might do it anyway just on the off chance that I'd come back some day and make it something other than a single byte data type.

As for the overhead involved in establishing an atomic block, I'd have to first be convinced that the overhead (which should be pretty small in most cases) was indeed likely to cause problems. And if the answer was yes, that'd indicate some pretty busy ISRs, and then I'd think the whole works would be on shaky ground as a result. But that's just me. Normally I go for efficiency but this might be an exception, I'd have to have a reason not to.

wanderson


wanderson, if you haven't read the links provided above, you really owe it to yourself to do that. I'll add one more:
http://www.nongnu.org/avr-libc/user-manual/group__util__atomic.html

But here's the deal.

When sharing a variable between the main code and an ISR, if any operation being done on the variable is interrupted in the middle, the ISR could update the variable in the middle of that operation, and then the main would continue its update after the ISR completes. This will result in a partial update, and the variable's value will be corrupted.

Many operations take multiple machine instructions and many variables are multi-byte, so even a seemingly simple operation like an increment or assignment can require multiple instructions that, if not executed "atomically" (i.e. with interrupts inhibited), risk data corruption.

Even worse, it's timing related. So any given code XD could run for a long time and "seem" to be OK. Sooner or later though, as sure as the sun rises, Mr. Murphy will get in there and cause trouble. And when that happens, as CB said, it is exceedingly difficult to debug, and not just because it'll be very difficult to reproduce the symptom.

So this is quite a dogmatic thing. Always declare variables shared with an ISR as volatile, and always access them atomically. Period. No exceptions. It's The Right Thing To Do.


Thank you for the link, it was useful information. 

I think I understand now WHY the data would normally need to be protected; however, I do not believe that this is a case where that is true--at least until I examine the assembly code produced by my code.

In my case there are four variables associated with a circular buffer that has data added to it by the ISR and then has data retrieved from it by an external class method.  There are only two of the four variables that are accessed by both the class method and the ISR.  The first of these is a counter that is implemented as a 8 bit unsigned integer, and hence inherently "atomic" if I understand the process correctly.  Th other variable is a 32-bit unsigned integer.  Now I understand that it impossible that an operation on a 32-bit integer can not be accomplished atomically on an 8-bit cpu; however, in my case this value is a true random sequence of four bytes.  So if one or more of the bytes are replaced by a different set of four random bytes by the ISR in the middle of the retrieving the original value, the only effect will be that the class method will return a different random value than it would have originally (or if it had been called after the ISR updated the values).

Now the other two variables are also unsigned 8-bit integers, but that also doesn't matter in this case, since the class method only uses and changes the pointer to the start of the buffer, and the ISR only uses and changes the pointer to the end of the buffer.  So as I was saying, with these specific conditions, I do not believe there is any possibility of any of the buggy behavior CB described.  The only problem that I can see from the class perspective is that the mixing of the one 32-bit integer could possibly introduce a bias into the production of random numbers.  And that is something that is best determined empirically.  Now as I said, after examining the asm produced, I may see something that alters my current belief, or if I see some unexpected behavior I would then add the suggested protection routines and see if the unexpected behavior goes away.

Also if I were producing commercial code and the resources were not too tight, I would have taken CB's suggestion to protect the data just as a typicall "best practice"; however, for me this is part of learning exercise, and the real goal (since I really don't need this library) is to UNDERSTAND the process in detail.

BTW, This library is designed to provide a source of true hardware generated random numbers using the timer jitter associated with the Watch Dog Timer and Timer 1

wanderson


Just to be clear, this could be bad:

Code: [Select]
WDT_POOL_COUNT++;

That is because that both reads and writes.

And BTW @op - please don't name variables in all caps. Reserve that for constants.


Thanks, but this variable is an uint8_t which I believe would be handled atomically, correct?

The caps is the style I always used when dealing with global variables/constants the last time I used C++ (since I used the const keyword on constants), what is the accepted practice for the arduino community to distinguish a global variable and a local variable?

nickgammon


Thanks, but this variable is an uint8_t which I believe would be handled atomically, correct?


No.

Take this sketch:

Code: [Select]
volatile byte foo;

void setup ()
{
 foo++;
}
void loop () {}


Generated code for setup:

Code: [Select]
void setup ()
{
 foo++;
 a6: 80 91 00 01 lds r24, 0x0100
 aa: 8f 5f       subi r24, 0xFF ; 255
 ac: 80 93 00 01 sts 0x0100, r24
}
 b0: 08 95       ret


If, after the lds, but before the sts, the ISR changed foo, then the wrong value is incremented, and the change made by the ISR is discarded.

Quote
what is the accepted practice for the arduino community to distinguish a global variable and a local variable?


I don't know of one for the Arduino, but very generally all caps refers to constants, so it's confusing to use that.

If you like, put a "g" in front, eg.

Code: [Select]
volatile byte gWDTpoolCount;
Please post technical questions on the forum, not by personal message. Thanks!

More info: http://www.gammon.com.au/electronics

nickgammon

Quote
So if one or more of the bytes are replaced by a different set of four random bytes by the ISR in the middle of the retrieving the original value, the only effect will be that the class method will return a different random value than it would have originally (or if it had been called after the ISR updated the values).


I'm not so sure I like this reasoning. You don't mind data corruption "because it is a random number anyway" seems to me to be a bit weak.
Please post technical questions on the forum, not by personal message. Thanks!

More info: http://www.gammon.com.au/electronics

wanderson



Thanks, but this variable is an uint8_t which I believe would be handled atomically, correct?


No.

Take this sketch:

Code: [Select]
volatile byte foo;

void setup ()
{
 foo++;
}
void loop () {}


Generated code for setup:

Code: [Select]
void setup ()
{
 foo++;
 a6: 80 91 00 01 lds r24, 0x0100
 aa: 8f 5f       subi r24, 0xFF ; 255
 ac: 80 93 00 01 sts 0x0100, r24
}
 b0: 08 95       ret


If, after the lds, but before the sts, the ISR changed foo, then the wrong value is incremented, and the change made by the ISR is discarded.

Quote
what is the accepted practice for the arduino community to distinguish a global variable and a local variable?


I don't know of one for the Arduino, but very generally all caps refers to constants, so it's confusing to use that.

If you like, put a "g" in front, eg.

Code: [Select]
volatile byte gWDTpoolCount;


I was making the assumption that the AVR's had an instruction to increment a memory location, I just searched and saw that they do not...  The way I would have handled this on other microprocessors/micro-controllers would have been to declare that variable as a register.  Did a quick search of the web and found that it seems possible with AVR-GCC,  What are the potential drawbacks to that approach?

wanderson


Quote
So if one or more of the bytes are replaced by a different set of four random bytes by the ISR in the middle of the retrieving the original value, the only effect will be that the class method will return a different random value than it would have originally (or if it had been called after the ISR updated the values).


I'm not so sure I like this reasoning. You don't mind data corruption "because it is a random number anyway" seems to me to be a bit weak.


I don't really consider it corruption, if it produces an equally valid result.  This particular algorithm is producing bytes with equal probabilities on all values, therefore swapping parts of two sequences to make a new sequence doesn't change the answer being generated-a long integer comprised of four bytes who are uniformly and randomly distributed between the values of 0x00 and 0xff

nickgammon


Did a quick search of the web and found that it seems possible with AVR-GCC,  What are the potential drawbacks to that approach?


That it won't work.

By making the variable volatile you are saying "don't keep it in a register". There is no atomic increment/decrement of a memory location, so the data has to go from variable, to register, back to variable. If you make it "register" that is completely incompatible with "volatile".
Please post technical questions on the forum, not by personal message. Thanks!

More info: http://www.gammon.com.au/electronics

GoForSmoke

So an interrupt library that doesn't safeguard data is okay because an application that will use it might not have problems with the data?

I saw both HOW and WHY in the links that CB provided. I don't remember seeing "you have to" so much as "you need to be careful" over and over.

Is the difference in speed or a very few less extra lines of code worth it?
Or is there Face to be saved?

Arguing on the internet is like running in the special olympics....


1) http://gammon.com.au/blink  <-- tasking Arduino 1-2-3
2) http://gammon.com.au/serial <-- techniques howto
3) http://gammon.com.au/interrupts
Your sketch can sense ongoing process events in time.
Your sketch can make events to control it over time.

Coding Badly

Quote
I don't remember seeing "you have to" so much as "you need to be careful" over and over.


The more important question is "why bother".  We're talking about...

Is the difference in speed or a very few less extra lines of code worth it?


One line of C or three assembly instructions compiled to three one cycle machine instructions.  In other words, six bytes of Flash that take 0.1875 microseconds to execute.

Which is why I am willing to answer your question but not participate in the futile discussion.

nickgammon


So an interrupt library that doesn't safeguard data is okay because an application that will use it might not have problems with the data?


What do you mean? What library?

An "interrupt library" (whatever that is exactly) can't safeguard data unless it provides "safe" hooks for getting the data out (eg. like HardwareSerial does1).




1. Or does it?

This is HardwareSerial::available ...

Code: [Select]
int HardwareSerial::available(void)
{
  return (unsigned int)(SERIAL_BUFFER_SIZE + _rx_buffer->head - _rx_buffer->tail) % SERIAL_BUFFER_SIZE;
}


Wouldn't it be possible for the head or tail pointers to be altered in the middle of that expression evaluation?
Please post technical questions on the forum, not by personal message. Thanks!

More info: http://www.gammon.com.au/electronics

Coding Badly

#27
Jun 01, 2012, 09:21 am Last Edit: Jun 01, 2012, 09:45 am by Coding Badly Reason: 1
Quote
Wouldn't it be possible for the head or tail pointers to be altered in the middle of that expression evaluation?


Excellent example.  head and tail are both a two byte data-type which requires the use of a critical section.  However, the value is always less than 256; the high-byte is always zero.  Essentially, they are single-byte variables with padding.  One side is write-only (e.g. only the ISR writes to head) and the other side is read-only (e.g. only loop reads head).  Because of those conditions, the expression is safe.

Make one small change, like increasing the buffer to 512 bytes, and the expression is no longer reliable.  With no comments and no critical section, a person increasing the buffer size has no idea that an ass-whooping is just over the horizon.  In my opinion, even that "safe" expression should be protected with a critical section.  Not having a critical section imposes an unknown risk and a large burden on the next person.

nickgammon

Yes I see where you are coming from. Without protection you are asking for trouble. Interestingly, the reference page for attachInterrupt does not mention this problem at all. It also says:

Quote
You should declare as volatile any variables that you modify within the attached function.


That isn't quite right. Any variables that are shared with the main code should be declared volatile. Otherwise you impose a needless overhead.
Please post technical questions on the forum, not by personal message. Thanks!

More info: http://www.gammon.com.au/electronics

wanderson

Gentleman,

Thank you very much for the discussion.  I now understand how and why such code is needed, and more importantly when.  I have modified the code appropriately.

Go Up