How can 256 be greater than 500?

Forum 2005-2010 (read only) Software Syntax & Programs
1

I have a variable that I'm incrementing in an interrupt function. The variable is declared as a volatile int.

In my loop() I check the value of the incremented variable. When it passes a threshold, I take further action.

In essence, I'm doing this:

void interruptFunction()
{
sampleCount++;
}

void loop()
{
if (sampleCount > 500)
{
Serial.println(sampleCount);
sampleCount = 0;
}
}

Most of the time, sampleCount will be 500 or very occasionally 501. But maybe one in 20 to 30 times, sampleCount will be 256!

The only clue I have is that the incorrect value of sampleCount is always the greatest multiple of 256 that is less than the test amount. That is:

test for 500 sampleCount may be 256
test for 520 sampleCount may be 512
test for 1000 sampleCount may be 768

It may be worth noting that the interrupt is being called using the TimerOne library.

Any explanation for this mystery?

Thanks a lot for your help,
John

2

http://arduino.cc/en/Reference/Volatile

maybe

3

The way I read that document, "volatile" is the way to avoid inaccurate values:

"Under certain conditions, the value for a variable stored in registers can be inaccurate. [The volatile qualifier] directs the compiler to load the variable from RAM and not from a storage register."

Is there a better qualifier to use than volatile?

4

Any explanation for this mystery?

Tell us what "sampleCount" is, post your code.

5

What I'm doing "for real" is storing values in an array. So my interrupt looks more like this:

void interruptFunction()
{
records[sampleCount++] = analogRead(0);
}

Testing suggests that even when "if (sampleCount > 500)" is true, and sampleCount == 256, I actually have 500 valid samples.

Does that help?

6

Does this bring any clarity...
http://www.arduino.cc/cgi-bin/yabb2/YaBB.pl?num=1261124850

7

Does that help?

Not really. As long as you don't show the relevant code where you define your variables, we can't tell you where you messed up.

Korman

8

Did you use the 'volatile' qualiifier?
Do you understand the use of the 'volatile' qualifier?

9

Re: Coding Badly - Thanks, I'm going over the reference now.

Re: Korman - The interrupt function posted here is shown in its entirety.

void readToneData()
{
records[sampleCount++] = analogRead(0);
}

Re: Groove - As I mentioned in my first post, sampleCount is declared volatile. Specifically:

volatile int sampleCount;

10

Yet you're still not posting the code. :o

11

Let me reduce the code to the simplest state that still creates an error. Then I'll post that.

12

THE EXPLANATION:

Points that may be relevant. I'm sampling an analog input using a prescaler that speeds up the sample rate. (Based on - http://www.arduino.cc/cgi-bin/yabb2/YaBB.pl?num=1208715493/11 )

I switch between two interrupt functions, one that sits and waits for relevant signals on the analog pin and another that samples the relevant signals.

It's the sampling interrupt that shows the problem. The code for the other interrupt is not posted here.

THE CODE:

#include "TimerOne.h"

// defines for setting and clearing register bits
#ifndef cbi
#define cbi(sfr, bit) (_SFR_BYTE(sfr) &= ~_BV(bit))
#endif
#ifndef sbi
#define sbi(sfr, bit) (_SFR_BYTE(sfr) |= _BV(bit))
#endif

#define SAMPLES 500

enum {
Start = 1,
Sample = 2,
Done = 3,
};

volatile int sampleCount; // max SAMPLES
volatile byte records[SAMPLES + 10]; // 10 spare records for overreading interrupt

const unsigned int kWaitFrequency = 2000;
const unsigned int kWaitPeriod = 1000000 / kWaitFrequency;

const unsigned int kSampleFrequency = 10000;
const unsigned int kSamplePeriod = 1000000 / kSampleFrequency;

byte toneSampleState;

void readToneData() // interrupt function in question
{
records[sampleCount++] = analogRead(0);
}

void waitForTone() // another interrupt function
{
// REMOVED TO REDUCE THE SIZE OF THE POST

// sets the variable toneSampleState to Start when a tone is detected
}

void setup()
{
// set A/D prescale to 16
sbi(ADCSRA,ADPS2) ;
cbi(ADCSRA,ADPS1) ;
cbi(ADCSRA,ADPS0) ;

sampleCount = 0;

Serial.begin(115200);

toneSampleState = Done;

Timer1.initialize(kWaitPeriod);
Timer1.attachInterrupt(waitForTone);
}

void loop()
{
switch (toneSampleState)
{
case Start:

Timer1.detachInterrupt(); // detach waitForTone
Timer1.attachInterrupt(readToneData, kSamplePeriod);
toneSampleState = Sample;

break;

case Sample:

if (sampleCount >= SAMPLES)
{

// PROBLEM APPEARS HERE

Serial.print((int) sampleCount);
Serial.print(" samples\n");
Timer1.detachInterrupt(); // detach readToneData

// OTHER STUFF HAPPENS HERE

sampleCount = 0;
Timer1.attachInterrupt(waitForTone, kWaitPeriod);
toneSampleState = Done;
}
break;

default: // i.e. Done
break;
}
}

13

THE CODE:

At this point, you're supposed to hit the "#" button on the editor''s toolbar, between the printer icon and the "insert quote" icon-thingy.

Nothing calls "readToneData"

// PROBLEM APPEARS TO HAPPEN HERE

14

Take 2:

THE CODE:

#include "TimerOne.h"

// defines for setting and clearing register bits
#ifndef cbi
#define cbi(sfr, bit) (_SFR_BYTE(sfr) &= ~_BV(bit))
#endif
#ifndef sbi
#define sbi(sfr, bit) (_SFR_BYTE(sfr) |= _BV(bit))
#endif

#define  SAMPLES 500    

enum {
    Start = 1,
    Sample = 2,
    Done = 3,
};

volatile int sampleCount;   // max SAMPLES
volatile byte records[SAMPLES + 10];    // 10 spare records for overreading interrupt

const    unsigned int  kWaitFrequency = 2000;
const    unsigned int  kWaitPeriod = 1000000 / kWaitFrequency;

const    unsigned int  kSampleFrequency = 10000;
const    unsigned int  kSamplePeriod = 1000000 / kSampleFrequency;

byte toneSampleState;


void readToneData()        // interrupt function in question
{
    records[sampleCount++] = analogRead(0);
}

void waitForTone()        // another interrupt function
{
    // REMOVED TO REDUCE SIZE OF POST
}


void setup() 
{
    // set A/D prescale to 16
    sbi(ADCSRA,ADPS2) ;
    cbi(ADCSRA,ADPS1) ;
    cbi(ADCSRA,ADPS0) ;

    sampleCount = 0; 

    Serial.begin(115200);

    toneSampleState = Done;   

    Timer1.initialize(kWaitPeriod);
    Timer1.attachInterrupt(waitForTone);
}

void loop() 
{
    switch (toneSampleState)
    {
    case Start:

        Timer1.detachInterrupt();    // detach waitForTone
        Timer1.attachInterrupt(readToneData, kSamplePeriod);
        toneSampleState = Sample;

        break;

    case Sample:

        if (sampleCount >= SAMPLES)
        {  
        
            // PROBLEM APPEARS HERE            

            Serial.print((int) sampleCount);
            Serial.print(" samples\n");
            Timer1.detachInterrupt();    // detach readToneData

            // OTHER STUFF HAPPENS HERE

            sampleCount = 0;
            Timer1.attachInterrupt(waitForTone, kWaitPeriod);
            toneSampleState = Done; 
        }
        break;

    default:    // i.e. Done
        break;
    }
}
15

You can get an interrupt between the highlighted lines, corrupting sampleCount.

[glow]if (sampleCount >= SAMPLES)[/glow]
{  
  // PROBLEM APPEARS HERE            

  [glow]Serial.print((int) sampleCount);[/glow]
  Serial.print(" samples\n");

Change to something like this:

   int savedSampleCount;

   if ((savedSampleCount=sampleCount) >= SAMPLES)
   {  
    
 // PROBLEM APPEARS HERE            

      Serial.print(savedSampleCount);
      Serial.print(" samples\n");
16

Nothing calls "readToneData"

readToneData gets called by the interrupt:

Timer1.attachInterrupt(readToneData, kSamplePeriod);
17 
void readToneData()        // interrupt function in question
{
   records[sampleCount++] = analogRead(0);
}

There is nothing here that prevents sampleCount from being incremented to a value beyond the end of the array. Other variables in your program's data space can be overwritten (it's undefined behavior).

Regards,

Dave

18

Re: Andy Brown - You're on to something with this suggestion:

Change to something like this:

   int savedSampleCount;

if ((savedSampleCount=sampleCount) >= SAMPLES)
  {  
   
  // PROBLEM APPEARS HERE

Serial.print(savedSampleCount);
     Serial.print(" samples\n");

This was first suggested in the reference provided in Reply #5 by Coding Badly - http://www.arduino.cc/cgi-bin/yabb2/YaBB.pl?num=1261124850

Data that cannot be accessed atomically and is shared with an ISR needs to be protected by disabling interrupts during the access. Access means reading or writing. Only byte-sized data can be accessed atomically. Anything larger than a byte needs to be protected.

Changing my code using his example solves the problem:

uint8_t SaveSREG = SREG;   // save interrupt flag
cli();                     // disable interrupts
int   currentSampleCount = sampleCount;
SREG = SaveSREG;           // restore the interrupt flag

if (currentSampleCount >= SAMPLES)
{  
        
 // PROBLEM SOLVED!!            

     Serial.print((currentSampleCount);
     Serial.print(" samples\n");

Coding Badly FTW!

Thanks to all the players. I think I learned something, at very least, about how to format posts.

19

Re: davekw7x

void readToneData()        // interrupt function in question

{
  records[sampleCount++] = analogRead(0);
}




There is nothing here that prevents sampleCount from being incremented to a value beyond the end of the array. Other variables in your program's data space can be overwritten (it's undefined behavior).

Regards,
Dave

I'm not happy with that possibility, but my memory allocation for the records array is always 10 more than the number of samples I want. The ISR would have to get called 10 more times before I'd have a problem.

In practice, I've never seen it go over by more than 1 record. I could add a safety buffer bigger than 10, but that appears to be more than enough.

20

Changing my code using his example solves the problem:

uint8_t SaveSREG = SREG;   // save interrupt flag
cli();                     // disable interrupts
int   currentSampleCount = sampleCount;
SREG = SaveSREG;           // restore the interrupt flag

if (currentSampleCount >= SAMPLES)
{  
        
 // PROBLEM SOLVED!!            

     Serial.print((currentSampleCount);
     Serial.print(" samples\n");

FYI, the Arduino core functions added the two following functions to perforn the same atomic protection method:

Changing my code using his example solves the problem:

uint8_t SaveSREG = SREG;   // save interrupt flag
 [glow]noInterrupts(); [/glow] //cli();   disable interrupts
int   currentSampleCount = sampleCount;
[glow]interrupts();[/glow]   //SREG = SaveSREG; restore the interrupt flag

if (currentSampleCount >= SAMPLES)
{  
        
 // PROBLEM SOLVED!!            

     Serial.print((currentSampleCount);
     Serial.print(" samples\n");

Lefty