Unexpected behaviour simple C code

[colomer]

Hello,

I'm using an ESP32 to control a freezer. The ON/OFF of the freezer depends on two sensors, a digital Flow sensor and a pressure sensor. If Flow sensor is high OR the pressure sensor is higher than a value, the freezer must switch ON. If both conditions are to OFF, the device should remain OFF. Once it is switched ON, there is a delay to switch OFF (actually two delays because switching OFF depends on two relays one after the other).

I log the switching ON and OFF in an array.

The weird point to me is that the log contains more OFF switchings than ON switchings. Maybe it's something obvious that I don't see, but to me the code is OK.

I reboot the device every 5 hours so the millis() don't return to 0.

What do you think the issue is to get more OFFs than ONs in the log?

void Operations::HandleAutomaticCompressor()
{
  Flowsensor = digitalAverageRead(FLOW);

  if ((Flowsensor==HIGH)||(Pressure>_pressureToOn))
  {
    _triggerCooling = millis() + (_delay_off_cooling_seconds*1000);    
    
    if (IsCompressorON==false)
    {      
      FifoEnqueue("Compressor ON. Flow "+ String(Flowsensor)+" Pressure "+ String(Pressure));
    }  

    IsCompressorON = true;    
    
  }

  if (millis()<_triggerCooling )
  {    
    digitalWrite(RELAY_COMPRESSOR, HIGH);  
    digitalWrite(RELAY_COOLING,HIGH);
  }
  else
  { 
    digitalWrite(RELAY_COOLING,LOW);  
  } 

  if (millis()>(_triggerCooling + _delay_compressor_secs*1000))
  {
    if (IsCompressorON == true) 
    {
      FifoEnqueue("Compressor OFF");  
    }

    IsCompressorON = false;    
    digitalWrite(RELAY_COMPRESSOR, LOW); 
       
  }

}

[xfpd]

@colomer

_triggerCooling

Ensure this is type unsigned long

@colomer

1000

Postfix UL to every literal in an equation with an unsigned long so it looks like this: 1000UL so the math is done as an unsigned long (and not int or an overflow might occur).

[colomer]

Thanks xfpd

In this line, _triggerCooling was long int ( I changed to unsigned long int) and _delay_compressor_secs is int.

if (millis()>(_triggerCooling + _delay_compressor_secs*1000))

I'll try this

if (millis()>(_triggerCooling + (unsigned long)_delay_compressor_secs*1000UL))

[PaulRB]

@colomer

What do you think the issue is to get more OFFs than ONs in the log?

50% chance that it's something in the code you didn't post.

Please read the forum guide in the sticky post at the top of most forum categories.

[colomer]

@PaulRB

50% chance that it's something in the code you didn't post.

Probably yes. All that variables are only modified in that function so I can't imagine what other parts of the code should affect to this. But yes, I'll review the rest of the code.

[sterretje]

@colomer

so I can't imagine what other parts of the code should affect to this.

Writing outside bounds of array and stack overflow to name a few.

[colomer]

@sterretje

Writing outside bounds of array and stack overflow to name a few.

That wouldn't result in the output I get. And yes, I checked that.

[PerryBebbington]

@colomer

I reboot the device every 5 hours so the millis() don't return to 0.

It takes about 49 days for millis to return to zero and if you use it correctly it doesn't matter, see:

Using millis() for timing. A beginners guide

Part 1 It is not usually long before new Arduino users discover that although the delay() function is easy to use it has side effects, the main one of which is that its stops all activity on the Arduino until the delay is finished (not quite true, I know, but that is usually how the problem presents itself). When this occurs the new user is usually directed to the BlinkWithoutDelay example sketch (BWoD) and/or the excellent thread entitled Several things at the same time. Several things at the…

[alto777]

@colomer

[Writing outside bounds of array and stack overflow] wouldn't result in the output I get.

You may have checked everything and there may be no such activity, but the general sad truth is that once you write outside the bounds of array or overflow the stack, there is no saying what will or won't happen.

You cannot reason logically about what the effects are. What happens is what happens.

a7

[alto777]

I worked with chatGPT to reformat and refactor your code, we went around quite a few cycles and I can't say we fixed anything for sure, but one thing I noticed right away and told it was

I can't tell if it is because things that are already off are being told to go off again or what.

The timing stuff like

    _triggerCooling = millis() + (_delay_off_cooling_seconds*1000); 

is not idiomatic. Further more throwing timestamps into the future using addition will possibly be an issue when millis() rolls over.

My head hurt too much to go further than this last advice from the AI:

The fix?

Only set IsCompressorON = true at the moment you actually commit to energizing the compressor relay — not just because the flow/pressure condition looks good.

Or better yet, don’t manage state with a loose flag like IsCompressorON. Use a clearer, singular state machine or at least track actual relay state, not just the sensor condition.

We went on to eliminate the odd timing logic and a few other things. I questioned many of its moves and reasoning but found chatGPT to be sticking to its story.

I cannot test this, but it illustrates a different approach:

enum CompressorState {
    COMPRESSOR_OFF,
    COMPRESSOR_ON
};

CompressorState _compressorState = COMPRESSOR_OFF;

void Operations::HandleAutomaticCompressor()
{
    Flowsensor = digitalAverageRead(FLOW);
    unsigned long now = millis();

    // Check if conditions require compressor ON
    bool shouldTurnOn = (Flowsensor == HIGH) || (Pressure > _pressureToOn);

    switch (_compressorState)
    {
        case COMPRESSOR_OFF:
            if (shouldTurnOn) {
                // Record the trigger time with no addition involved
                _triggerCooling = now;
                FifoEnqueue("Compressor ON. Flow " + String(Flowsensor) + " Pressure " + String(Pressure));
                IsCompressorON = true;
                _compressorState = COMPRESSOR_ON;
                digitalWrite(RELAY_COMPRESSOR, HIGH);
                digitalWrite(RELAY_COOLING, HIGH);
            }
            break;

        case COMPRESSOR_ON:
            // Check if enough time has passed for cooling relay
            if (now - _triggerCooling < _delay_off_cooling_seconds * 1000) {
                digitalWrite(RELAY_COOLING, HIGH);
            } else {
                digitalWrite(RELAY_COOLING, LOW);
            }

            // Check if enough time has passed to turn off compressor
            if (now - _triggerCooling > _delay_compressor_secs * 1000) {
                FifoEnqueue("Compressor OFF");
                IsCompressorON = false;
                _compressorState = COMPRESSOR_OFF;
                digitalWrite(RELAY_COMPRESSOR, LOW);
            }
            break;
    }
}

It still turns on RELAY_COOLING when is already, and off when it is off, but you aren't logging that and digitalWrite() does not care.

HTH

a7

[6v6gt]

From the code sample you have provided, there is no explanation of how this phenomenon of an unbalanced number of logged "on" and "off" switchings could occur, unless you are manipulating the value of global variables such as IsCompressorON outside that function Operations::HandleAutomaticCompressor() which, of course, we can't see.

Maybe you have a subtle typo somewhere like:

if ( IsCompressorON = true ) {
    // switch on compressor
}

[alto777]

It seems obvious (now), and was all along to the OP. I took a clean chatGPT and fed it a bit differently, here's the main fruit:

if (...) {
  ...
  if (IsCompressorON == false) {
    FifoEnqueue("Compressor ON. Flow "+ String(Flowsensor)+" Pressure "+ String(Pressure));
  }
  IsCompressorON = true;
}
...
if (...) {
  if (IsCompressorON == true) {
    FifoEnqueue("Compressor OFF");  
  }
  IsCompressorON = false;
}

Then for more fun I said

It may interest you to know that yesterday I talked a copy of you into "fixing" the function after telling it the function was broken because of unbalanced logging. Long chat. Serves to illustrate it's all in the prompts and so far humans have, at times, an edge in things like this.

And got this

That is interesting—and a great example of how powerful (and fragile) the prompt-context dynamic can be. When you frame something as broken, especially with just enough ambiguity, it can push even a solid function into being "repaired" unnecessarily. A good reminder: even the best tools reflect the clarity of their inputs.

And yes, in nuanced reasoning like this—tracing flags, guarding logic, interpreting absence of bugs—humans still often have the edge, especially when they trust the code more than the narrative.

So either that is right or it is wrong.

a7

[xfpd]

It is righter or wronger when piped to TTS with Batman's voice.

[colomer]

Finally it turns out that, as PaulRB wrote, the problem was outside the function and actually outside all the program.

The log is received by a internet server and stored in a database on it. The problem was there (I expected it to work OK, but it didn't)

Thanks so much for your help. Even if it is not a problem because I reboot the device frequently, I will follow your suggestions and change the way to calculate timestamps.

[xfpd]

@colomer

PaulRB wrote, the problem was outside the function

Always with words of wisdom.