Function Not Working as Intended

Hi everyone, I am working on a project using a Mega2560, a W5100 ethernet shield, a 4-relay module and two ZMPT101B voltage sensors. The ZMPT101B's are connected to the Mega on 5v, GND, with the sensor outputs on A2 and A3. The relay module is connected to 5V, GND, and digital pins 22 through 25. The end goal is to utilize the ZMPT101B to monitor incoming line voltage and if voltage drops (i.e. power loss), start my generator and transfer the load via two magnetic contactors using the relays, and send an MQTT message to my Home Assistant server that will handle some automations.

I am working on one part of this project at a time and then combining the working sections of code into the final product. Currently I'm trying to get the voltage sensing and the generator start to work. I have been able to get this code to work to read the sensors on both pins and get the sensors calibrated.
Voltage Sensor Test Code

I attempted to take the above code and make it into a function with a parameter for an analog pin that I can pass the designated sensor input pin and measure line voltage and generator output voltage (to determine when the generator starts). My current code is attached as Generator_ATS

The problem is that the getVoltage() function isn't working the same way the calibration code worked. It is returning 0.00 Volts for both line and generator even when I have a plug hooked up and plugged into the wall.

At first I thought the analog input pin wasn't being passed correctly into the function, so I added a Serial.print to print sensorPin and got back out "56" which would correspond to A2 on the Mega. I've tried going through and adding some debugging Serial.prints in the function and it seems to never get past the first if() statement in the function.

I know I'm probably missing something simple that's causing this to not work, but I can't seem to figure it out and any help would be appreciated.

Generator_ATS.ino (6.85 KB)

I could be wrong, but after a quick look your defining

voltageSampleCount = 0

then

voltageSampleCount = voltageSampleCount + 1;

It will never reach 1000, I would make voltageSampleCount a global variable or if you dont mind waiting, do something like:

while voltageSampleCount !=1000

to get your readings then it will return a value.

That's along the lines of what I was thinking, but if I define

voltageSampleCount = 0

at the beginning of the function, wouldn't this section

if(micros() >= voltageLastSample + 1000 )
{
voltageSampleRead = (analogRead(sensorPin) - 512) + voltageOffset1;
voltageSampleSum = voltageSampleSum + sq(voltageSampleRead);

voltageSampleCount = voltageSampleCount + 1;
voltageLastSample = micros(); 
}

then increment voltageSampleCount until it reaches 1000? Or does it have to be a global variable to work like that?
Thanks for the help.

If you do that then voltageSampleCount gets set to 0 each time the function is called. Either make it global or define it as static.

You need to set it back to zero once you have your 1000 samples.

Note that I have not looked at your full code because you attached it rather than posting in code tags here; I am using a mobile phone.

All that makes sense. Was unable to upload entire code due to character limits on the post so I thought best option was to attach it so that the entire code was available. Sorry about that.

If I'm understanding the original code (which works when uploaded straight to the board and not placed in a function),

if (micros() >= voltageLastSample + 1000 )                /*every 0.2 milli second taking 1 reading */
  {
    voltageSampleRead = (analogRead(sensorPin) - 512) + voltageOffset1;    /* read the sample value including offset value*/
    voltageSampleSum = voltageSampleSum + sq(voltageSampleRead) ;         /* accumulate total analog values for each sample readings*/

    voltageSampleCount = voltageSampleCount + 1;                     /* to move on to the next following count */
    voltageLastSample = micros() ;                /* to reset the time again so that next cycle can start again*/
  }

  if (voltageSampleCount == 1000)                /* after 1000 count or 800 milli seconds (0.8 second), do the calculation and display value*/
  {
    voltageMean = voltageSampleSum / voltageSampleCount;         /* calculate average value of all sample readings taken*/
    RMSVoltageMean = (sqrt(voltageMean)) * 1.5;                 /* The value X 1.5 means the ratio towards the module amplification. */
    adjustRMSVoltageMean = RMSVoltageMean + voltageOffset2;    /* square root of the average value including offset value */    
    FinalRMSVoltage = RMSVoltageMean + voltageOffset2;        /* this is the final RMS voltage*/
    if (FinalRMSVoltage <= 2.5)                               /* to eliminate any possible ghost value*/
    {
      FinalRMSVoltage = 0;
    }
    return FinalRMSVoltage;
    voltageSampleSum = 0;                             /* to reset accumulate sample values for the next cycle */
    voltageSampleCount = 0;                                   /* to reset number of sample for the next cycle */
  }

voltageSampleCount is supposed to get reset to zero each time the function is called. My understanding is that the first if() statement is taking measurements every .2 milliseconds and summing them until 1000 samples are taken and then the second if() statement calculates RMS voltage using the measurements collected in the first if() statement and should return that value.

I agree that voltageSampleCount needs to reset to zero each time the function is called. Only problem is that the function is returning 0.00 and if I'm debugging correctly it doesn't appear to ever reach the second if() statement.

Going to try making the variables all globals and then throw this:

Serial.println(voltageSampleCount);

inside the first if() statement in the function and see what I get.
Thanks

So my problem is apparently in the first if() statement with regards to the sample timing.

I got the following from the serial monitor after I uploaded this code:

/* 0- General */

int decimalPrecision = 2;                   // decimal places for all values shown in LED Display & Serial Monitor
int lineVolts;
int genVolts;

/* 1- AC Voltage Measurement */
int lineVoltage = A2;             // Which pin to measure voltage Value (Pin A0 is reserved for button function)
int genVoltage = A3;
float voltageSampleRead  = 0;               /* to read the value of a sample in analog including voltageOffset1 */
float voltageLastSample  = 0;               /* to count time for each sample. Technically 1 milli second 1 sample is taken */
float voltageSampleSum   = 0;               /* accumulation of sample readings */
float voltageSampleCount = 0;               /* to count number of sample. */
float voltageMean ;                         /* to calculate the average value from all samples, in analog values*/
float RMSVoltageMean ;                      /* square roof of voltageMean without offset value, in analog value*/
float adjustRMSVoltageMean;
float FinalRMSVoltage;                      /* final voltage value with offset value*/


/* 1.1- AC Voltage Offset */

float voltageOffset1 = 0.00 ;         // to Offset deviation and accuracy.
float voltageOffset2 = 0.00;          // too offset value due to calculation error from squared and square root



void setup() {

  /* 0- General */

  Serial.begin(9600);                             /* In order to see value in serial monitor */

}

void loop()

{

  lineVolts = getVoltage(lineVoltage);
  delay(5000);

}

float getVoltage(int pin)
{
  /* 1- AC Voltage Measurement */
  int sensorPin = pin;
  Serial.println(sensorPin);
  if (millis() >= voltageLastSample + 1 )                                                   /* every 1 milli second taking 1 reading */
  {
    voltageSampleRead = 2 * (analogRead(sensorPin) - 512) + voltageOffset1;   /* read the sample value */
    voltageSampleSum = voltageSampleSum + sq(voltageSampleRead) ;                         /* accumulate value with older sample readings*/
    voltageSampleCount = voltageSampleCount + 1;                                          /* to move on to the next following count */
    Serial.println(voltageSampleCount);
    voltageLastSample = millis() ;                                                        /* to reset the time again so that next cycle can start again*/
  }
  
   if (voltageSampleCount == 1000)                                                                               /* after 4000 count or 800 milli seconds (0.8 second), do the calculation and display value*/
  {
    Serial.print("Voltage Sum: ");
    Serial.println(voltageSampleSum);
    voltageMean = voltageSampleSum / voltageSampleCount;                                                      /* calculate average value of all sample readings taken*/
    RMSVoltageMean = (sqrt(voltageMean)) * 1.5;                                                               // The value X 1.5 means the ratio towards the module amplification.
    adjustRMSVoltageMean = RMSVoltageMean + voltageOffset2;                                                   /* square root of the average value including offset value */                                                                                                                                                       /* square root of the average value*/
    FinalRMSVoltage = RMSVoltageMean + voltageOffset2;                                                        /* this is the final RMS voltage*/
    if (FinalRMSVoltage <= 2.5)                                                                               /* to eliminate any possible ghost value*/
    {
      FinalRMSVoltage = 0;
    }
    Serial.print(" The Voltage RMS value is: ");
    Serial.print(FinalRMSVoltage, decimalPrecision);
    Serial.println(" V ");
    voltageSampleSum = 0;                                                                                     /* to reset accumulate sample values for the next cycle */
    voltageSampleCount = 0;                                                                                   /* to reset number of sample for the next cycle */
  }
}

56
1.00
56
2.00
56
3.00
56
4.00
56
5.00
56
6.00
56
7.00
56
8.00
56
9.00
56
10.00

The 56 is from a serial.print of the sensorPin variable to make sure it got passed correctly. So it looks like the function is being called in loop() every 5 seconds correctly and the analog pin number is being passed correctly to the function, but the first if() statement to take a sample every .2 milliseconds isn't working as intended. It's only taking a sample and incrementing sample count every 5 seconds when the function is called.

Thinking I need to set a time variable to micros() at during setup() and then use that time variable inside the function. Thoughts?

but the first if() statement to take a sample every .2 milliseconds isn't working as intended.

Likely because you're using millis() for timing, which has a maximum resolution of 1.0 milliseconds. You even say, " /* every 1 milli second taking 1 reading */" so I'm now wondering even more what you mean. You mentioned micros(). Not sure what you meant about that either, but try using micros() instead of millis() in the same places. 0.2ms is 200us.

e.g.

  if (micros() >= voltageLastSample + 200 )                                                  
  {

except that you should not do it that way, as it will fail micros() roll over. You need to

  if (micros() - voltageLastSample >= 200 )                                                
  {

Hi,
Did you write your code in stages?
If your code is too big for the code window, when did you realize the problem you had?

Do you have code that just reads the voltage input and interprets it?
Forget at this moment about generator control.
Also bump your serial speed up from 9600 to 115200.

Just post that complete bit of code, that way we can also run it and check its performance.

Thanks.. Tom... :slight_smile:

My understanding is that the first if() statement is taking measurements every .2 milliseconds and summing them until 1000 samples are taken.

if (micros() >= voltageLastSample + 1000 )                /*every 0.2 milli second taking 1 reading */
  {
    voltageSampleRead = (analogRead(sensorPin) - 512) + voltageOffset1;    /* read the sample value including offset value*/
    voltageSampleSum = voltageSampleSum + sq(voltageSampleRead) ;         /* accumulate total analog values for each sample readings*/

    voltageSampleCount = voltageSampleCount + 1;                     /* to move on to the next following count */
    voltageLastSample = micros() ;                /* to reset the time again so that next cycle can start again*/
  }

I've not looked at your entire code. What I suggest you do is write a short sketch that just does this particular thing and sends the results to the serial monitor. When it works put the code into a function of its own so it is separate from everything else and clearly only does one thing. If you do that and get stuck it's a lot easier for us to help when we only have to study one bit of complete code. It also helps you because you will probably discover what the problem is without help. I have lots and lots of short sketches I wrote to test how to do one thing, once I am happy with how something works I then incorporate it into the thing I am really doing. I keep the test sketches for future reference.

Based on your description I think you fundamentally don't understand what if does. The code above runs once to to bottom, either through all the code if the condition is true or skipping it all if the condition is false. I think you think it goes round and round, it doesn't.

I found this web site useful when I was learning C C Tutorial

Switch to millis() in the above code snippet was a test I was running to see if millis() would work instead of micros().

I have written the code in stages. I first tested this sample code for voltage sampling:

/* 0- General */

        int decimalPrecision = 2;                   // decimal places for all values shown in LED Display & Serial Monitor

        /* 1- AC Voltage Measurement */
        
        int VoltageAnalogInputPin = A2;             // Which pin to measure voltage Value (Pin A0 is reserved for button function)
        float voltageSampleRead  = 0;               /* to read the value of a sample in analog including voltageOffset1 */
        float voltageLastSample  = 0;               /* to count time for each sample. Technically 1 milli second 1 sample is taken */
        float voltageSampleSum   = 0;               /* accumulation of sample readings */
        float voltageSampleCount = 0;               /* to count number of sample. */
        float voltageMean ;                         /* to calculate the average value from all samples, in analog values*/ 
        float RMSVoltageMean ;                      /* square roof of voltageMean without offset value, in analog value*/
        float adjustRMSVoltageMean;
        float FinalRMSVoltage;                      /* final voltage value with offset value*/
   
        
        /* 1.1- AC Voltage Offset */
        
              float voltageOffset1 =0.00 ;          // to Offset deviation and accuracy. Offset any fake current when no current operates. 
                                                    // Offset will automatically callibrate when SELECT Button on the LCD Display Shield is pressed.
                                                    // If you do not have LCD Display Shield, look into serial monitor to add or minus the value manually and key in here.
                                                    // 26 means add 26 to all analog value measured.
              float voltageOffset2 = 0.00;          // too offset value due to calculation error from squared and square root 
              


void setup() {
 
/* 0- General */

    Serial.begin(9600);                             /* In order to see value in serial monitor */

}
     
void loop() 

{
  
   
   /* 1- AC Voltage Measurement */
        if(micros() >= voltageLastSample + 1000 )                                                                      /* every 0.2 milli second taking 1 reading */
          {
            voltageSampleRead = (analogRead(VoltageAnalogInputPin)- 512)+ voltageOffset1;                             /* read the sample value including offset value*/
            voltageSampleSum = voltageSampleSum + sq(voltageSampleRead) ;                                             /* accumulate total analog values for each sample readings*/
            
            voltageSampleCount = voltageSampleCount + 1;                                                              /* to move on to the next following count */
            voltageLastSample = micros() ;                                                                            /* to reset the time again so that next cycle can start again*/ 
          }
        
        if(voltageSampleCount == 1000)                                                                                /* after 4000 count or 800 milli seconds (0.8 second), do the calculation and display value*/
          {
            voltageMean = voltageSampleSum/voltageSampleCount;                                                        /* calculate average value of all sample readings taken*/
            RMSVoltageMean = (sqrt(voltageMean))*1.5;                                                                 // The value X 1.5 means the ratio towards the module amplification.      
            adjustRMSVoltageMean = RMSVoltageMean + voltageOffset2;                                                   /* square root of the average value including offset value */                                                                                                                                                       /* square root of the average value*/                                                                                                             
            FinalRMSVoltage = RMSVoltageMean + voltageOffset2;                                                        /* this is the final RMS voltage*/
            if(FinalRMSVoltage <= 2.5)                                                                                /* to eliminate any possible ghost value*/
            {FinalRMSVoltage = 0;}
            Serial.print(" The Voltage RMS value is: ");
            Serial.print(FinalRMSVoltage,decimalPrecision);
            Serial.println(" V ");
            voltageSampleSum =0;                                                                                      /* to reset accumulate sample values for the next cycle */
            voltageSampleCount=0;                                                                                     /* to reset number of sample for the next cycle */
          }


}

and it works perfectly when uploaded to the board. I then continued on to place this code into a function and added the function into my other code for the generator control. Ran into problems with the voltage sampling code when I placed it into a function. I took that function code and created a sketch that simply has the function and then calls the function every 5 seconds (posted above).
PerryBebbington, I do understand how if() statements work (run once while condition is true and or skipping all together if false), but your statement made me re-think what was actually happening.
I didn't fully think through what changes needed to be made to place the code into a function...realized that the code works when the entire thing is placed in loop() because the if() statement is constantly being re-ran and incrementing sample counts. Once I placed the code in the function it only runs every time the function is called and for some reason that fact wasn't clicking last night after pounding my head on this for hours.
Need to switch over to use either a for() or while() loop and then test that.

Thanks for making me think differently about this and all the help. I'll post an update when I get it working.

For some reason that fact wasn't clicking last night after pounding my head on this for hours.

Been there done that! Usually the best cure is some sleep!

Need to switch over to use either a for() or while() loop and then test that.

No, you don't, that's bad. That's blocking code.

The approach is to maintain a variable that counts the number of times the measurement has been taken, take another one, increment the count then exit. When the count gets to 1000 (or whatever) you process the data, set the count to zero and start again. Don't take 1000 measurements in 1 go.

You also need to take note of what aarg told you in reply #6 and change the way you use millis or micros.

Switch to millis() in the above code snippet was a test I was running to see if millis() would work instead of micros().

You should have mentioned that before. Nobody can read your mind.

I have written the code in stages. I first tested this sample code for voltage sampling:

...

and it works perfectly when uploaded to the board. I then continued on to place this code into a function and added the function into my other code for the generator control.

Get out of the bad habit of putting code in loop. Always put code in a function, however simple, and call the function from loop. It's much tidier and it avoids problems like this.

aarg:
You should have mentioned that before. Nobody can read your mind.

It was late last night when I was testing/posting and I just got started again this morning. Agree with you 100%.

PerryBebbington:
I have written the code in stages. I first tested this sample code for voltage sampling:

...

and it works perfectly when uploaded to the board. I then continued on to place this code into a function and added the function into my other code for the generator control.

Get out of the bad habit of putting code in loop. Always put code in a function, however simple, and call the function from loop. It's much tidier and it avoids problems like this.

Tracking all of this. That's kind of what I was trying to do. I used the code I found (written that way) to get the sensors calibrated and then started modifying to clean it up and make it work for me.

PerryBebbington:
Been there done that! Usually the best cure is some sleep!
No, you don't, that's bad. That's blocking code.

The approach is to maintain a variable that counts the number of times the measurement has been taken, take another one, increment the count then exit. When the count gets to 1000 (or whatever) you process the data, set the count to zero and start again. Don't take 1000 measurements in 1 go.

You also need to take note of what aarg told you in reply #6 and change the way you use millis or micros.

I think I'm understanding this statement. Wouldn't a for() loop do exactly what you're saying (maintain a variable, take a reading, then increment the count)?
Couldn't I use the example from blink without delay inside a for() loop to take a reading every 2 milliseconds until 1000 readings are taken like this?

float getVoltage(int pin)
{
  float voltageSampleRead  = 0;               /* to read the value of a sample in analog including voltageOffset1 */
  const long sampleInterval = 2;
  unsigned long previousSampleMillis  = 0;       /* to count time for each sample. Technically 1 milli second 1 sample is taken */
  float voltageSampleSum   = 0;               /* accumulation of sample readings */
  float voltageSampleCount = 0;               /* to count number of sample. */
  float voltageMean ;                         /* to calculate the average value from all samples, in analog values*/
  float RMSVoltageMean ;                      /* square roof of voltageMean without offset value, in analog value*/
  float adjustRMSVoltageMean;
  float FinalRMSVoltage;                      /* final voltage value with offset value*/

  /* 1.1- AC Voltage Offset */
  float voltageOffset1 = 0.00 ;               /* to Offset deviation and accuracy */
  float voltageOffset2 = -9.00;               /* too offset value due to calculation error from squared and square root */
  
  /* 1- AC Voltage Measurement */
  int sensorPin = pin;
  Serial.println(sensorPin);
  
  for (i = 0; i < 1000; i++)
  {
    unsigned long currentSampleMillis = millis();
    if (currentSampleMillis - previousSampleMillis >= sampleInterval)                          /* every 0.2 milli second taking 1 reading */
    {
      previousSampleMillis = currentSampleMillis;                                              /* to reset the time again so that next cycle can start again */
      voltageSampleRead = (analogRead(VoltageAnalogInputPin) - 512) + voltageOffset1;          /* read the sample value including offset value */
      voltageSampleSum = voltageSampleSum + sq(voltageSampleRead);                             /* accumulate total analog values for each sample readings */
      //voltageSampleCount = voltageSampleCount + 1;                                             /* to move on to the next following count */      
    }
  }

I think I'm understanding this statement. Wouldn't a for() loop do exactly what you're saying (maintain a variable, take a reading, then increment the count)?
Couldn't I use the example from blink without delay inside a for() loop to take a reading every 2 milliseconds until 1000 readings are taken like this?

A for loop will do as you suggest, but 1000 loops at 2ms per loop is 2 seconds while nothing else can happen anywhere in your code; no buttons read, no display updated, no extra loud emergency warning siren, no flashing lights, nothing.

Let loop do the looping and keep track in your function of how many times it's been called.

FYI this is a really important thing to learn for programming, and is a step from beginner programming to experienced.

PerryBebbington:
A for loop will do as you suggest, but 1000 loops at 2ms per loop is 2 seconds while nothing else can happen anywhere in your code; no buttons read, no display updated, no extra loud emergency warning siren, no flashing lights, nothing.

Let loop do the looping and keep track in your function of how many times it's been called.

FYI this is a really important thing to learn for programming, and is a step from beginner programming to experienced.

Ok that makes more sense...I think.
So I should create a function that handles the sampling, stores that sampling data to a variable, and increments another variable tracking how many times it been sampled, and then create a function to do the calculations. Then call that new function in loop() to allow "loop to do the looping" along with an if() statement to determine when to call the calculation function. Is this what you mean?
Or did you mean to put sampling code in a function like above and the calculation code in another function along with the if() statement and then just call both functions in loop()?
Know enough about this right now to be dangerous, but by no means an experienced programmer so trying to learn the right way. Thanks again for the help.

It's simpler than that - all you need to ensure, is that loop() repeats at least once every millisecond or so. The details vary but not that.

Either a function that does the sampling and does the calculation, or 2 functions, one for sampling, one for calculation, that's not so important.

Loop should be like

loop {
  sampleFunction();
}

or

loop {
  sampleFunction();
  calculationFunction();
}

Something like this

void sampleFunction() {
  static uint16_t sampleCount;
  const uint16_t noOfSamples = 1000;
  if (sampleCount >= noOfSamples) {
  sampleCount = 0;
  // do the calculation thing
  } else {
  ++sampleCount;
  //Do the sample thing
  }
}

I'm not going to do any more otherwise I'll have done it for you.

PerryBebbington:
Either a function that does the sampling and does the calculation, or 2 functions, one for sampling, one for calculation, that's not so important.

Loop should be like

loop {

sampleFunction();
}



or


loop {
 sampleFunction();
 calculationFunction();
}




Something like this


void sampleFunction() {
 static uint16_t sampleCount;
 const uint16_t noOfSamples = 1000;
 if (sampleCount >= noOfSamples) {
 sampleCount = 0;
 // do the calculation thing
 } else {
 ++sampleCount;
 //Do the sample thing
 }
}



I'm not going to do any more otherwise I'll have done it for you.

Ok that makes sense. Didn't think it mattered which way it was done, but wanted to ask which one is the preferred way. Thanks for all the help and teaching me something new.

Didn't think it mattered which way it was done, but wanted to ask which one is the preferred way. Thanks for all the help and teaching me something new.

The important things are:

  • Don't block, let loop() loop freely
  • Split things up into functions that do one clearly defined thing

You can argue either way as to whether the calculation is part of the sample function or separate, the important points are above.