Updating an int variable in loop() which is also updated by an ISR

I have a program that counts the number of revolutions of each of 2 wheels by updating 2 int variables in ISRs triggered by a rotation sensor in each wheel. After a period of measurement done using millis() in loop() the 2 counts are compared, the speeds of the wheels are adjusted if they are different, the measurement start time is reset to millis(), the counts are reset to zero and the counting cycle starts again. Before anyone asks, the variables are ints because their value could exceed 255 and they are declared as global and volatile.

Am I right in thinking that it would be wise to disable interrupts whilst the counter variables are reset to zero because they are ints, the updating of which is a multi stage process, in case an interrupt should occur during the update process ?

Yea it is wise to do this? The other way to do it is to set a flag in The loop and have the ISR clear the counter and the flag.

Am I right in thinking that it would be wise to disable interrupts whilst the counter variables are reset to zero because they are ints, the updating of which is a multi stage process, in case an interrupt should occur during the update process ?

No! Updating ints is not a multi stage process.

If you check the datasheet you will find that int operations (plus, minus shift etc but not *,/) take a single instruction but 2 clock cycles. Interrupts "happen" only at the end of the instruction. You do however have a number of var's so you may want to find a way of updating them without the interrupts getting in the way.

Mark

holmes4: No! Updating ints is not a multi stage process.

Would you like to wager on that? I have $50 in my pocket that says you are wrong.

UKHeliBob: Am I right in thinking that it would be wise to disable interrupts whilst the counter variables are reset to zero because they are ints, the updating of which is a multi stage process, in case an interrupt should occur during the update process ?

No. You are not correct. You need to disable interrupts for [u]all[/u] access; not just resetting them to zero.

Updating an int variable in loop() which is also updated by an ISR

I think I would like to see code here. The question is too general. Do you set them both to zero at once? Does it matter if one is set to zero and the other not?

holmes4:
No! Updating ints is not a multi stage process.

Sorry, CB, if you were about to earn $50 …

Example code:

volatile int foo;
void setup () { }
void loop ()
  {
  foo = 0;
  }  // end of loop

Generated machine code:

000000a8 <loop>:
  a8:	10 92 01 01 	sts	0x0101, r1
  ac:	10 92 00 01 	sts	0x0100, r1
  b0:	08 95       	ret

Regardless of what the datasheet says, that’s two instructions.

Difficult to see how it could be otherwise, on an eight-bit processor.

holmes4: If you check the datasheet you will find that int operations (plus, minus shift etc but not *,/) take a single instruction but 2 clock cycles. Interrupts "happen" only at the end of the instruction. You do however have a number of var's so you may want to find a way of updating them without the interrupts getting in the way.

Yes, there are a few of them, but none of them store the result. (If I'm wrong please quote the instruction).

For example, ADIW (add immediate to word) adds a constant to a word, but that word is in two registers.

Code as requested. This code is not tested as the hardware is work in progress.

#include <HUBeeBMDWheel.h>

HUBeeBMDWheel leftWheel;
HUBeeBMDWheel rightWheel;

byte forward = 1;
byte reverse = 0;

int leftSpeed = 100;    //needs an int as values can be negative
int rightSpeed = 100;
int speedIncrement = 10;

volatile int leftCount =  0;
volatile int rightCount =  0;

long countStartTime = 0;
long countInterval = 1000;

void setup() 
{
  leftWheel.setupPins(8, 11, 9);
  rightWheel.setupPins(13, 12, 10);

  leftWheel.setBrakeMode(1);
  rightWheel.setBrakeMode(1);

  leftWheel.setDirectionMode(forward);
  rightWheel.setDirectionMode(reverse);
  
  attachInterrupt(0, updateLeftCounter, RISING);
  attachInterrupt(1, updateRightCounter, RISING);
}

void loop() 
{
  leftWheel.setMotorPower(leftSpeed);
  rightWheel.setMotorPower(rightSpeed); 

  if ((millis() - countStartTime) > countInterval)
  {
    // <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< I could turn off interrupts here
    if (leftCount > rightCount)
    {
      leftSpeed -= speedIncrement;
      rightSpeed += speedIncrement;
    }
    else if (leftCount < rightCount)
    {
      leftSpeed += speedIncrement;
      rightSpeed -= speedIncrement;    
    }
    countStartTime = millis();
    noInterrupts();   //prevent interrupts during multi byte update
    leftCount = 0;
    rightCount = 0;
    interrupts();     //turn interrupts on again
  }
  constrain(leftSpeed, 0, 255);
  constrain(rightSpeed, 0, 255);
}

void updateLeftCounter()
{
  leftCount++; 
}

void updateRightCounter()
{
  rightCount++; 
}

I'm reminded of the infamous words of Bill Clinton, "It depends upon what the meaning of the word 'is' is."

In holmes4's defense, it depends upon what the meaning of the word "update" is. If the int in question is an auto variable, then there are indeed atomic instructions available for updating the int. Of course, it would be difficult in the Arduino world to share such a variable with an interrupt service routine.

UKHeliBob: Code as requested.

Yup. You are going to need to wrap those rascals. Be back in a minute...

Assuming this compiles, it should work. You will have to merge it with your code…

static void GetAndClearCount( int & local, int volatile & shared )
{
  noInterrupts();
  local = shared;
  shared = 0;
  interrupts();
}

void loop() 
{
  int lc;
  int rc;

  GetAndClearCount( lc, leftCount );
  GetAndClearCount( rc, rightCount );

  leftWheel.setMotorPower(leftSpeed);
  rightWheel.setMotorPower(rightSpeed); 

  if ((millis() - countStartTime) > countInterval)
  {
    // <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< I could turn off interrupts here
    if (lc > rc)
    {
      leftSpeed -= speedIncrement;
      rightSpeed += speedIncrement;
    }
    else if (lc< rc)
    {
      leftSpeed += speedIncrement;
      rightSpeed -= speedIncrement;    
    }
    countStartTime = millis();
  }
  constrain(leftSpeed, 0, 255);
  constrain(rightSpeed, 0, 255);
}

CB - thanks for the suggested function but at the moment I think that I will stick to my simple, linear approach. As you may have guessed the wheels are on a 2 wheeled vehicle and all it is going to do is trundle around the floor so speed is not a problem. Initially there will only be a maximum of 32 counts per wheel revolution so dealing with the interrupts and checking 2 counters is hardly going to stretch the Arduino.

I am, however, interested in the function declaration

static void GetAndClearCount( int & local, int volatile & shared )

I am used to the idea of static variables in functions, but what is the significance of a function being declared static ? A search has not found the answer as to its effect. I would surmise that the variables, which are passed to the function by reference, are made static by the function declaration, but is that right ?

UKHeliBob: Initially there will only be a maximum of 32 counts per wheel revolution so dealing with the interrupts and checking 2 counters is hardly going to stretch the Arduino.

The good news is that, with the value always less than 256, you won't have to worry about "sheared" data.

The bad news it that your flawed code will occasionally miss a count.

I am used to the idea of static variables in functions, but what is the significance of a function being declared static ?

Changes the scope of the function to "module" (aka compilation unit). Nothing outside of the current module will have access to the function.

Making a function static also gives the optimizer a helping hand.

I would surmise that the variables, which are passed to the function by reference, are made static by the function declaration, but is that right ?

No. It has no affect on any data.

... what is the significance of a function being declared static ...

It means the function (name) is not exported for the linker's benefit. It doesn't make much difference in a single compilation unit.

TVM for the explanations