Interrupt routine ignores variable state

I am working on a program to control a stepper motor which drives a 16mm film camera but I am having an issue where the state of the variable “recording” is being partially ignored. When that variable is 0, the motor should be stopped.

Looking at the output from the serial monitor the switch is returning the expected values but when power is applied to the whole thing, the motor runs slowly when the switch is off and speeds up when the switch is on. This isn’t the desired behavior, what should happen is the motor is stopped when the switch is off and speeds up to full speed when the switch is on. The code worked perfectly before I added in the state checking of the switch and the motor would ramp up to full speed as soon as power was applied to the system.

The switch is attached to pin 8 on an arduino pro mini.

unsigned long previousMillis = 0; // will store last time LED was updated
#define OCR1A_START_INTERVAL 8000 // 1 Khz starting frequency calculated from your original sketch
#define OCR1A_RUN_INTERVAL 400
#define SOFTSTART_UPDATE_RATE 3 //ramp up function update rate
#define ACCEL 20 // acceleration rate

int interval = OCR1A_START_INTERVAL;
int toggle=0;
int recording=0;

void setup()
{
pinMode(3, OUTPUT);
pinMode(4, OUTPUT);
pinMode(5, OUTPUT);
pinMode(6, OUTPUT);
pinMode(7, OUTPUT);
pinMode(8,INPUT);
Serial.begin(9600);
digitalWrite(4,HIGH);
digitalWrite(5,LOW);
digitalWrite(6,HIGH);
noInterrupts(); 
TCCR1A = 0;
TCCR1B = 0;
TCNT1 = 0;
OCR1A = OCR1A_START_INTERVAL;  // compare match register
TCCR1B |= (1 << WGM12); // CTC mode
TCCR1B = (TCCR1B & 0b11111000) | 0x01;
TIMSK1 |= (1 << OCIE1A); // enable timer compare interrupt
interrupts(); // enable all interrupts
}

ISR(TIMER1_COMPA_vect)

{

  if(recording==1);
  {
    if(toggle==1)
    {
    PORTD=PORTD & B11110111;
    toggle=0;
    }
     else
    {
    PORTD=PORTD | B00001000;
    toggle=1;
    }
  }
 }


void loop()
{
  int recording = digitalRead(8);
  {
  Serial.println(recording);
  
  if (recording ==1)
  {
  unsigned long currentMillis = millis();

  if(currentMillis - previousMillis > SOFTSTART_UPDATE_RATE)
  {
    previousMillis = currentMillis;
    if(interval > OCR1A_RUN_INTERVAL)
    {
      interval = interval - ACCEL;
      OCR1A = interval;
    }
  }
  }
  }
}

you change toggle inside the ISR, but use it outside of the ISR.

Hence it must be declared volatile - otherwise the compiler will engage in optimizations that rely on the assumption that if the routine being compiled hasn't changed the value of a variable, it's value has not changed, an assumption which is not valid when it can be changed in an ISR.

Hi, thanks for your suggestion. I gave it a try and declared the variable volatile but I get exactly the same result. I assume

volatile int toggle=0;

is correct way to do it? I also tried declaring recording as volatile but it made no difference.

That should be fine. But because you declared it a int (which is 2 byte and thus 2 words on a 8-bit micro) reading (or writing) to it isn't atomic which can give you weird errors.

Why not make it a bool as it only needs to hold two states? Which is effectively a byte/uint_8 aka a single word and thus atomic.

septillion: That should be fine. But because you declared it a int (which is 2 byte and thus 2 words on a 8-bit micro) reading (or writing) to it isn't atomic which can give you weird errors.

Why not make it a bool as it only needs to hold two states? Which is effectively a byte/uint_8 aka a single word and thus atomic.

Does it matter on a single core processor? Because every instruction is finished before the next one anyway.

I have tried it as a bool but then the variable recording just stays at 0 no matter what state the record button is in.

 if(recording==1);

Do you see anything wrong with the above code?

Always ensure that code code is correctly formatted AT ALL TIMES

Had you none so you would not now look like a complete idiot!

Mark

arduino_new:
Does it matter on a single core processor? Because every instruction is finished before the next one anyway.

That is true, but applies to machine instruction, not C statements.

Had you none so

Who looks like the idiot now?

arduino_new: Does it matter on a single core processor? Because every instruction is finished before the next one anyway.

Yes! Because you need at least 2 instructions to read/write a full 16-bit (although it's a single C/C++ command). And guess where the interrupt will pop up once in a while ;) Right in the middle!

I have changed it to a boolean but still the same result, it just appeared to be zero all the time because serial print doesn't print boolean unless you put DEC after it.

Who looks like the idiot now?

Until you fix the serious problem pointed out by Holmes4, you do.

I have changed it to a boolean but still the same result

Post the revised code, using code tags.

unsigned long previousMillis = 0; // will store last time LED was updated
#define OCR1A_START_INTERVAL 8000 // 1 Khz starting frequency calculated from your original sketch
#define OCR1A_RUN_INTERVAL 400
#define SOFTSTART_UPDATE_RATE 3 //ramp up function update rate
#define ACCEL 20 // acceleration rate

int interval = OCR1A_START_INTERVAL;
bool toggle=0;
bool recording=0;

void setup()
{
pinMode(3, OUTPUT);
pinMode(4, OUTPUT);
pinMode(5, OUTPUT);
pinMode(6, OUTPUT);
pinMode(7, OUTPUT);
pinMode(8,INPUT);
Serial.begin(9600);
digitalWrite(4,HIGH);
digitalWrite(5,LOW);
digitalWrite(6,HIGH);
noInterrupts(); 
TCCR1A = 0;
TCCR1B = 0;
TCNT1 = 0;
OCR1A = OCR1A_START_INTERVAL;  // compare match register
TCCR1B |= (1 << WGM12); // CTC mode
TCCR1B = (TCCR1B & 0b11111000) | 0x01;
TIMSK1 |= (1 << OCIE1A); // enable timer compare interrupt
interrupts(); // enable all interrupts
}

ISR(TIMER1_COMPA_vect)

{

  if(recording==1);
  {
    if(toggle==1)
    {
    PORTD=PORTD & B11110111;
    toggle=0;
    }
     else
    {
    PORTD=PORTD | B00001000;
    toggle=1;
    }
  }
 }


void loop()
{
  bool recording = digitalRead(8);
  {
  Serial.println(recording,DEC);
  
  if (recording ==1)
  {
  unsigned long currentMillis = millis();

  if(currentMillis - previousMillis > SOFTSTART_UPDATE_RATE)
  {
    previousMillis = currentMillis;
    if(interval > OCR1A_RUN_INTERVAL)
    {
      interval = interval - ACCEL;
      OCR1A = interval;
    }
  }
  }
  }
}
bool toggle=0;

Try again, after re-reading reply #1.

  if(recording==1);Fail!

RB358: Who looks like the idiot now?

Grow up. There's a big difference between typos and syntax.

septillion: Yes! Because you need at least 2 instructions to read/write a full 16-bit (although it's a single C/C++ command). And guess where the interrupt will pop up once in a while ;) Right in the middle!

Okay, that makes sense.

RB358: ``` ... ISR(TIMER1_COMPA_vect)

{

 if(recording==1);  {    if(toggle==1)    {    PORTD=PORTD & B11110111;    toggle=0;    }     else    {    PORTD=PORTD | B00001000;    toggle=1;    }  } } ...

Reply #6

Holmes4 called me an idiot without actually saying what the problem was. By his reasoning, anyone who isn't a programming expert is an idiot. Well I think he is an idiot because he doesn't know how design, leak test and charge a refrigeration system.

Both of you stop the childish name calling.

....
bool recording=0;
....
ISR(TIMER1_COMPA_vect)
{
  if(recording==1)
  {
....
  }
 }
...
void loop()
{
  bool recording = digitalRead(8);
....
}

Scope trouble.

...
TCCR1B |= (1 << WGM12); // CTC mode
...
      OCR1A = interval;
...

Is OCR double-buffered in CTC mode?