Global variable doesn't update prior to next loop

Question

I'm trying to build a tachometer in C++ for my ESP32. When I uncomment Serial.printf("outside rev: %d \n", rev); outside of the conditional it works, but when I comment it I get values that are orders of magnitude greater than they should be (700 revolutions without, vs 7 revolutions with). My best guess is that the print statement is slowing the loop() down just enough for incrementRevolutions() to toggle the global variable passedMagnet from true to false before the next loop. That would make sense, since a delay in updating passedMagnet would allow newRevCount++; to be triggered multiple times. But this is obviously something I can't debug with either print statements or step-through debugging given the time-sensitive nature of the race condition.

bool passedMagnet = true;
int incrementRevolutions(int runningRevCount, bool passingMagnet)
{
//    Serial.printf("passedMagnet: %d ,  passingMagnet %d , runningRevCount: %d \n", passedMagnet, passingMagnet, runningRevCount);
    int newRevCount = runningRevCount;
    if (passedMagnet && passingMagnet)
    { //Started a new pass of the magnet
        passedMagnet = false;
        newRevCount++;
    }
    else if (!passedMagnet && !passingMagnet)
    { //The new pass of the magnet is complete
        passedMagnet = true;
    }
    return newRevCount;
}

unsigned long elapsedTime = 0;
unsigned long intervalTime = 0;
int rev = 0;
void loop()
{
    intervalTime = millis() - elapsedTime;
    rev = incrementRevolutions(rev, digitalRead(digitalPin));

//    Serial.printf("outside rev: %d \n", rev);
    if (intervalTime > 1000)
    {
        Serial.printf("rev: %d \n", rev);
        rev = 0;
        elapsedTime = millis();
    }
}

Is this a known gotcha with Arduino or C++ programming? What should I do to fix it?

Answer 1

I think the test is to blame. I had to rename and move things a bit to visualize the logic, sorry about that.

bool magStateOld = false;  // initialize to digitalRead(digitalPin) in setup()

int incrementRevolutions(int runningRevCount, bool magState)
{
    int newRevCount = runningRevCount;

    // detect positive edge.
    if (magState && !magStateOld)      // <- was eq. to if (magState && magStateOld)
                                       // the large counts came from here.  
    { 
        newRevCount++;
    }
    magStateOld = magState; // record last state unconditionally

    return newRevCount;
}

You could also write it as...

int incrementRevolutions(int n, bool magState)
{
    n += (magState && !magStateOld);
    magStateOld = magState;
    return n;
}

But the most economical (and fastest) way of doing what you want would be:

bool magStateOld;

inline bool positiveEdge(bool state, bool& oldState)
{
    bool result = (state && !oldState);
    oldState = state;
    return result;
}  

void setup()
{
  // ...

  magStateOld = digitalRead(digitalPin);
}

void loop()
{
    // ...

    rev += (int)positiveEdge(digitalRead(digitalPin), magStateOld);

    // ...
}

It's reusable, and saves both stack space and unnecessary assignments.

If you cannot get clean transitions from your sensor (noise on positive and negative edges, you'll need to debounce the signal a bit, using a timer.

Example:

constexpr byte debounce_delay = 50; // ms, you may want to play with
                                    // this value, smaller is better.
                                    // but must be high enough to 
                                    // avoid issues on expected
                                    // RPM range. 
                                    // 50 ms is on the high side.
    
byte debounce_timestamp;            // byte is large enough for delays 
                                    // up to 255ms. 

// ...

void loop()
{
    // ...

    byte now = (byte)millis();

    if (now - debounce_timestamp >= debounce_delay)
    {
        debounce_timestamp = now;
        rev += (int)positiveEdge(digitalRead(digitalPin), magStateOld);
    }

    // ...
}