Void vs return with arguments issue with function

[trilerian]

I was going to post this in my other thread, but since I marked that as solved, the forum requested that I open a new topic. And since this topic is technically different than the other topic, here it goes...

Having trouble with passing arguments to a function that works if I use it with no return.

IE

void  myfunction()
vs
int myfunction(int x, int y)

So here are the actual functions that I had created.

void getRpmData() {
  if (rpmNewIsrMicros == true) {
    prevRpmMicros = rpmMicros;  // save the previous value
    noInterrupts();
    rpmMicros = rpmIsrMicros;
    rpmNewIsrMicros = false;
    interrupts();
    rpmDuration = (rpmMicros - prevRpmMicros);
  }
}

Simply call the function from somewhere else by

getRpmData()

Use the variables inside the function, great. It works...

But I wanted to transform the function into a return and return rpmMicros - prevRpmMicros.

So I created this monstrosity.

uint32_t getRpmData(uint32_t time1, uint32_t time2, uint32_t time3, bool flag) {
   if (flag == true) { // if newRpmIsrMicros == true
    uint32_t rD = 0;
    time2 = time1;  // prevRpmMicros = rpmMicros   ---  Save the last rpm micros
    noInterrupts();
    time1 = time3; // rpmMicros = rpmIsrMicros --- Set rpm micros to the value from the ISR, ie current time
    flag = false;
    interrupts();
    rD = time1 - time2; // rpmMicros - prevRpmMicros
    return rD; // return the above statement
  }
}

I called it from the loop with:

rpmDuration = getRpmData(rpmMicros, prevRpmMicros, rpmIsrMicros, rpmNewIsrMicros);

But after some debugging I figured out that all the arguments pass as 0. So the return ends up being an incrementing of time, and that is it. No usable data.

My assumption is it has something to do with how fast the variables change, but I'm lost...

[alto777]

@trilerian

monstrosity.

I didn't damage myself looking at it.

Your only return from the function is inside the if statement. When that block is not executed, the function still returns something, but that something is probably random garbage.

As a rule that you should break only very carefully ( like don't, for now),

  return whatever;

should be the last line in the function, such that it is guaranteed to be executed every time the function is called.

a7

[trilerian]

Not returning when the if statement wasn't executed was my approach to begin with.
EDIT: I guess this is a by product of being new and not knowing all the gotchas... My goal was to only have the function return if the if statement was true because I was going to fill an array with the return. I've managed to get that worked out, but during my experimentation I tried this approach and I'm stumped as to why it doesn't work. /EDIT

Adding return 0 on the last line kills the program.
Adding int something; to the beginning of the function and return something; doesn't change anything.

uint32_t getRpmData(uint32_t time1, uint32_t time2, uint32_t time3, bool flag) {
  int something;
   if (flag == true) { // if newRpmIsrMicros == true
    uint32_t rD = 0;
    time2 = time1;  // prevRpmMicros = rpmMicros   ---  Save the last rpm micros
    noInterrupts();
    time1 = time3; // rpmMicros = rpmIsrMicros --- Set rpm micros to the value from the ISR, ie current time
    flag = false;
    interrupts();
    rD = time1 - time2; // rpmMicros - prevRpmMicros
    return rD; // return the above statement
  }
  return something; 
}

This however works, which is a return without passing any arguments. Note that adding a return when the if doesn't run messes up the data.

uint32_t getRpmData() {
  if (rpmNewIsrMicros == true) {  // if newRpmIsrMicros == true
    prevRpmMicros = rpmMicros;    // prevRpmMicros = rpmMicros   ---  Save the last rpm micros
    noInterrupts();
    rpmMicros = rpmIsrMicros;  // rpmMicros = rpmIsrMicros --- Set rpm micros to the value from the ISR, ie current time
    rpmNewIsrMicros = false;
    interrupts();
    return rpmMicros - prevRpmMicros;
  }
}

[alto777]

Ok, I guess I cannot help.

Function always return. You can't keep a function from returning eventually, unless you write it to never return, in which case the rest of your code would no longer run.

Functions with void as the return type do not return a value, and to expect one would raise an error.

Functions with other types as the return type always return a value; to fail to do anything with that value would be flagged as a warning, as it would mean you might be forgetting to do something with that returned value.

The value returned in that case is either from any

    return whatever;

that gets executed in the body of the function, or if none gets executed before the function runs off the end garbage is returned - which is why a good place for a return statement is as the last line of code.

In the case where you defined

   int something;

you failed to assign a value. As a local variable, using it without initialising is opening the door to undefined behaviour, which literally means anything can happen. So don't do that, either.

There may yet be some other issue in your code, but functions work in all the ways you have tried to use them, so it will be interesting to discover what that might be, maybe some other case of invoking undefined behavoiur.

When I can take a look oops! now you must post the entire sketch, or a minimum sketch that exhibits the same problem. If you are not with the problems I have pointed out, there is something in the code you did not post messing you up.

a7

[Delta_G]

Show the code where you are calling the function. Maybe there is some problem with how it is being used? Maybe there is some scope issue or something? Post an example that uses this function and has the problem.

[trilerian]

Here is the code I was working with. I am in the process of making changes by adding a class and trying to decide on the member functions. Following your nice tutorial @Delta_G.

The below has a function generator currently hooked up to pin 2 and pin 3. Running at 500Hz and a 40* phase offset.

The below is also not complete as I gave up on the function when it didn't work for my needs. While it does display the issue at hand, I didn't finish it with the array.

// Test for reading zero cross and Timing


#define REVS 2
#define PH 3
#define LPIN 13


uint32_t rpmMicros;
uint32_t phMicros;
uint32_t prevRpmMicros;
uint32_t prevPhMicros;
uint32_t rpmDuration;
uint32_t phDuration;
uint32_t prevDisplayMillis;
uint32_t displayMillis;
uint32_t freq = 0;
uint32_t rpm = 0;
uint16_t refresh = 1000;  //refresh in mseconds
float timing = 0;
float degree = 0;
float adegree = 0;
const uint8_t N = 10;
uint32_t arrayRpmDuration[N] = {};
uint32_t sumRpmDuration = 0;
uint32_t arrayPhDuration[N] = {};
uint32_t sumPhDuration = 0;
float avgRpmDuration = 0;
float avgPhDuration = 0;
float avgTiming = 0;
float avgRpm = 0;
float afreq = 0;
uint8_t rsamples = 0;
uint8_t psamples = 0;
uint8_t led = LOW;

// variables for the RPM ISR
volatile unsigned long rpmIsrMicros = 0;
volatile bool rpmNewIsrMicros = false;

// variables for the PH ISR
volatile unsigned long phIsrMicros = 0;
volatile bool phNewIsrMicros = false;

void setup() {
  Serial.begin(115200);
  pinMode(REVS, INPUT);
  pinMode(PH, INPUT);
  pinMode(LPIN, OUTPUT);
  attachInterrupt(digitalPinToInterrupt(REVS), rpmSensorISR, FALLING);
  attachInterrupt(digitalPinToInterrupt(PH), phSensorISR, FALLING);
  prevDisplayMillis = millis();
}

void loop() {
  getPhData();
  //getRpmData();
  rpmDuration = getRpmData(rpmMicros, prevRpmMicros, rpmIsrMicros, rpmNewIsrMicros);
  getCalculations();
  displayMillis = millis();
  if (displayMillis >= prevDisplayMillis + refresh) {
    prevDisplayMillis = millis();
    if (led == LOW) {
      led = HIGH;
    } else {
      led = LOW;
    }
    digitalWrite(LPIN, led);
    showData();
  }
}

uint32_t getRpmData(uint32_t time, uint32_t time0, uint32_t time1, bool flag) {
  if (flag == true) {
    time0 = time;  // save the previous value
    noInterrupts();
    time = time1;
    flag = false;
    interrupts();
    return (time0 - time);  // (rpmMicros - prevRpmMicros)
  }
}

/*
void getRpmData() {
  if (rpmNewIsrMicros == true) {                // ISR flag is true, let's do something
    prevRpmMicros = rpmMicros;                  // save the previous value
    noInterrupts();                             // disables interrups while we save variables
    rpmMicros = rpmIsrMicros;                   // set the global variable for the falling edge to the ISR event time
    rpmNewIsrMicros = false;                    // ISR flag is false, I guess we wait until it is true again.
    interrupts();                               // enables interrupts again
    rpmDuration = (rpmMicros - prevRpmMicros);  // New falling edge - previous falling edge
    arrayRpmDuration[rsamples] = rpmDuration;   // Add reading to the array
    rsamples++;                                 // Increment the array, we are starting at 0
    if (rsamples >= N) {                        // if we have reached the sample size N, array is full, go to the average function
      getArpms();                               //
      rsamples = 0;                             // set samples back to 0 and start over
    }
  }
}*/

void getPhData() {
  if (phNewIsrMicros == true) {              // ISR flag is true, let's do something
    prevPhMicros = phMicros;                 // save the previous value
    noInterrupts();                          // disables interrups while we save variables
    phMicros = phIsrMicros;                  // set the global variable for the falling edge to the ISR event time
    phNewIsrMicros = false;                  // ISR flag is false, I guess we wait until it is true again.
    interrupts();                            // enables interrupts again
    phDuration = phMicros - rpmMicros;       // New falling edge - previous falling edge
    arrayPhDuration[psamples] = phDuration;  // Add reading to the array
    psamples++;                              // Increment the array, we are starting at 0
    if (psamples >= N) {                     // if we have reached the sample size N, array is full, go to the average function
      getAph();                              //
      psamples = 0;                          // set samples back to 0 and start over
    }
  }
}

void getCalculations() {
  degree = ((rpmDuration * 1000) / 360);                // Calculate how many degrees for timing
  adegree = ((avgRpmDuration * 1000) / 360);            // Average of the above
  timing = ((phDuration * 1000) / degree) - 30;         // timing - phase offset
  avgTiming = ((avgPhDuration * 1000) / adegree) - 30;  // average of the above
  freq = 1000000 / rpmDuration;                         // Problem child with ESP32
  afreq = 1000000 / avgRpmDuration;                     // Problem child with ESP32
  rpm = freq * 60;
  avgRpm = afreq * 60;
}

void getAph() {
  int k{};
  sumPhDuration = 0;
  for (k = 0; k < N; k++) {
    sumPhDuration += arrayPhDuration[k];
  }
  avgPhDuration = (sumPhDuration / N);
}

void getArpms() {
  int m{};
  sumRpmDuration = 0;
  for (m = 0; m < N; m++) {
    sumRpmDuration += arrayRpmDuration[m];  // Sum all the array elements
  }
  avgRpmDuration = (sumRpmDuration / N);  // Divide by the array sample size
}

void showData() {
  Serial.println();
  Serial.println("===============");
  Serial.print("  RPM Duration ");
  Serial.print(rpmDuration);
  Serial.print("  FREQ  ");
  Serial.print(freq);
  Serial.print(" Hz");
  Serial.print("  RPM  ");
  Serial.print(rpm);
  Serial.print("   Timing  ");
  Serial.print(timing);
  Serial.print("*");
  Serial.print("  PHDuration  ");
  Serial.print(phDuration);
  Serial.print(" Avg Timing ");
  Serial.print(avgTiming);
  Serial.print("*");
  Serial.println();

  Serial.println();
}

void rpmSensorISR() {
  rpmIsrMicros = micros();
  rpmNewIsrMicros = true;
}

void phSensorISR() {
  phIsrMicros = micros();
  phNewIsrMicros = true;
}

[trilerian]

@alto777

you failed to assign a value. As a local variable, using it without initialising is opening the door to undefined behaviour, which literally means anything can happen. So don't do that, either.

I did try to assign

uint32_t getRpmData(uint32_t time, uint32_t time0, uint32_t time1, bool flag) {
int something = 0;
  if (flag == true) {
    time0 = time;  // save the previous value
    noInterrupts();
    time = time1;
    flag = false;
    interrupts();
    return (time0 - time);  // (rpmMicros - prevRpmMicros)
  }
return something;
}

That broke the program just as much as

uint32_t getRpmData(uint32_t time, uint32_t time0, uint32_t time1, bool flag) {
  if (flag == true) {
    time0 = time;  // save the previous value
    noInterrupts();
    time = time1;
    flag = false;
    interrupts();
    return (time0 - time);  // (rpmMicros - prevRpmMicros)
  }
return 0;
}

I also tried

uint32_t getRpmData(uint32_t time, uint32_t time0, uint32_t time1, bool flag) {
int something = 1;
  if (flag == true) {
    time0 = time;  // save the previous value
    noInterrupts();
    time = time1;
    flag = false;
    interrupts();
    return (time0 - time);  // (rpmMicros - prevRpmMicros)
  }
return something;
}

And it gives the same results as the original issue.

[Delta_G]

@trilerian

time0 = time; // save the previous value

One major thing that sticks out is right here. This won't change the prevRpmMicros variable. What gets passed to the function is a copy. Just the value. So nothing is actually saved here. Search the term "pass by reference".

@trilerian

    time = time1;
    flag = false;

Same with these lines. They're just setting the local copies of those variables. They're not changing the variables the values came from when you called the function.

[trilerian]

Well then that is the issue. I wasn't sure if the arguments passed the actual variables or not. Sorry the C++ videos I am watching make it hard to hold my attention with the pointers and references sections. Seriously, I've watched that 2 hour block of videos multiple times and it I can't focus my attention that well. Makes it hard that it is mainly about cstrings. But, I was wondering if that was the issue, well more like I was thinking that the function was passing the address information of the variable (pointers?). But references makes sense too. But I thought, and this gets so confusing, but I thought when you modify a reference it modifies the actual variable...

EDIT:
Well, I guess that is that. I will leave the functions as void, and I learned something. I can't modify arguments passed into a function with the function I am passing them into.

[Delta_G]

@trilerian

but I thought when you modify a reference it modifies the actual variable...

That's when you do pass by reference. For instance:

int number = 3;

void myFun( int &x) {
   x = 5;
}

void setup() {
   myFun(number);
}

Note the & on the parameter name in the function definition. That tells the function to take the argument by reference instead of by value. If it takes it by reference then changing it inside the function will change the variable you called with.

[trilerian]

@Delta_G

Note the & on the parameter name in the function definition. That tells the function to take the argument by reference instead of by value. If it takes it by reference then changing it inside the function will change the variable you called with.

Are you saying to change it to

uint32_t getRpmData(uint32_t &time1, uint32_t &time2, volatile uint32_t &time3, volatile bool &flag) {
  if (flag == true) {
    time1 = time2;  // save the previous value
    noInterrupts();
    time1 = time3;
    flag = false;
    interrupts();
    return (time1 - time2);  // (rpmMicros - prevRpmMicros)
  }
}

I swapped the variable names really quick to 1, 2, 3 and had to change 2 of them to volatile as they are volatile globals. But... This didn't change the output at all.

EDIT: Oops, in my adjustment I mixed up one of the statements.
Fixed it to

uint32_t getRpmData(uint32_t &time1, uint32_t &time2, volatile uint32_t &time3, volatile bool &flag) {
  if (flag == true) {
    time2 = time1;  // save the previous value
    noInterrupts();
    time1 = time3;
    flag = false;
    interrupts();
    return (time1 - time2);  // (rpmMicros - prevRpmMicros)
  }
}

And that is working.

[trilerian]

Let me get this straight. So now I am passing by reference, and that works. How was it passing beforehand? You said it was a "copy" but not a pointer or a reference itself.

EDIT: And good lord, now I using a reference. Watching those videos all I can think is "what use are these?".

Last EDIT, maybe: Back to @alto777 point about the function not having a return. Should I instead use the if statement to call the function, that way it always has a return? My guess it that would be the way to go.

[Delta_G]

Before you were passing by value. That means only the value gets passed to a local copy of the variable. This is normally what we want from functions.

Pass by reference uses the &. This passes an actual reference to the variable like you have discovered.

Pointers are a whole different ball of wax. Passing a pointer means that you are passing a different type of variable that holds the address of the thing you actually want to use. In that case you can change the value at that address, or any address you can index to from it. This is useful with arrays.

[Delta_G]

@trilerian

Last EDIT, maybe: Back to @alto777 point about the function not having a return. Should I instead use the if statement to call the function, that way it always has a return? My guess it that would be the way to go.

Is there going to be a case where there is more than one of these things you are getting an rpm from? Unless that is the case then it makes more sense to do it the way you were doing without passing anything. Since all the variables are global, there's really no reason to use arguments for the function unless you want to be able to use the same function with a different set of variables.

[trilerian]

@Delta_G

Is there going to be a case where there is more than one of these things you are getting an rpm from? Unless that is the case then it makes more sense to do it the way you were doing without passing anything. Since all the variables are global, there's really no reason to use arguments for the function unless you want to be able to use the same function with a different set of variables.

There will be a total of 6 functions like this one. I plan to make this as a member function to a class I am making for the hall sensors and phases of a bldc motor. Mainly at this point I am using it as an exercise to better familiarize myself with classes. The 3 phases will actually end up being polymorphed? There are more to those specific functions. I guess I could create 2 separate classes, but hey, learning and all.

[paulpaulson]

@trilerian

learning and all.

Hello trilerian

Take a view here to get a reference:

https://www.learncpp.com/

Enjoy the day and have fun.

[Delta_G]

@trilerian

plan to make this as a member function to a class

Then you will probably have a function that takes no arguments. Those global variables will be the member variables. If you have more motors, then you'll create more instances of the same class.

Just like how the Servo class only defines the stuff for one servo and then to make a project with 6 servos you create 6 instances of the Servo class.

[trilerian]

I'm trying to think of a good way to describe this. The motor has 6 individual components, each component will be a class member with its own set variables. 3 hall sensors, 3, we'll call them the phases. Measuring the bemf of the phases and turning them into square waves with external comparators. Then comparing the rpm pulse to the bemf pulse of the respective phases.

I could be wrong in my coding approach, I know that happens a lot, lol. But it makes sense to me to make that a class.

[Delta_G]

@trilerian

with its own set variables

Whenever you say its own set of variables, then that's where you want a class.

I'll admit to not fully understanding the idea because I don't know the brushless motors that well. But it sounds like the class should represent a component and there should be six instances of that class. Perhaps the motor is another class which has six of those component classes as members.

The place to create the class is at the point where you have multiple "things" of the same type that would need the same code.

[trilerian]

This is what I have so far. The day job is coming quick so it is off to bed for the night. I'll work more on this, maybe tomorrow. Got other obligations that need attending as well.

class Pulse {
public:
  uint8_t pin;
  uint32_t pulseMicros;
  uint32_t prevPulseMicros;
  uint32_t pulseDuration;
  volatile uint32_t pulseIsrMicros;
  volatile bool pulseNewIsrMicros;
  const uint8_t N = 10;
  uint32_t arrayPulseDuration[N] = {};
  uint32_t sumPulseDuration;
  float avgPulseDuration;
  uint8_t samples;

  void begin();
  void edges();
  void avg();
  void isr();
};

void Pulse::begin() {
  pinMode(pin, OUTPUT);
}

void Pulse::edges() {
  if (pulseNewIsrMicros == true) {                    // ISR flag is true, let's do something
    prevPulseMicros = pulseMicros;                    // save the previous value
    noInterrupts();                                   // disables interrups while we save variables
    pulseMicros = pulseIsrMicros;                     // set the global variable for the falling edge to the ISR event time
    pulseNewIsrMicros = false;                        // ISR flag is false, I guess we wait until it is true again.
    interrupts();                                     // enables interrupts again
    pulseDuration = (pulseMicros - prevPulseMicros);  // New falling edge - previous falling edge
    arrayPulseDuration[samples] = pulseDuration;      // Add reading to the array
    samples++;                                        // Increment the array, we are starting at 0
    if (samples >= N) {                               // if we have reached the sample size N, array is full, go to the average function
      avg();                                                 //
      samples = 0;                                    // set samples back to 0 and start over
    }
  }
}

void Pulse::avg() {
  int i{};
  sumPulseDuration = 0;
  for (i = 0; i < N; i++) {
    sumPulseDuration += arrayPulseDuration[i];
  }
  avgPulseDuration = (sumPulseDuration / N);
}

void Pulse::isr)() {
  pulseIsrMicros = micros();
  pulseNewIsrMicros = true;
}