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

[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.

[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.

[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.

[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.

[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.

[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;
}

[alto777]

@trilerian

The 3 phases will actually end up being polymorphed?

I have been doing this stuff for some time, and would calll myself a C programmer if I was to call myself a programmer and someone needed to know what language.

I have a vague idea about polymorphism in this context. Less than vague.

You are painting a giant rectangle, but you are doing it from left to right, a vertical strip at a time.

I'm painting bottom to top, a horizontal strip at a time.

It's a huge wall, taller than wide.

Consider that the first few horizontal strips are C. By the time you are tinkering with C++, you shouldn't be wondering about this kind of stuff.

I could write another analogy using a swimming pool. You in the very deep water and you do not know that water, when inhaled, can be a bad thing. Let alone how to swim.

C doesn't even have the pass by reference @Delta_G is helping you with! So when I got to it, I took it in having already learned the "whole different ball of wax" which is pointers.

I find the same with so many things in C++: knowing C helps. Which is why I will always tell ppl to master C.

Another point is about OOP is it can be the philosophy you follow when designing code in any language.

Quite a bit of C++ has leaked back into modern C. Enough to make OOP easier and more universally readable - hand made OOP can be an idiosyncratic impenetrable nightmare when written in a language with zero build in support for doing.

So I have had no good reason to develop anything beyond an ability to read most pedestrian C++. But when it is an expert writing it, and it doesn't even look like C, I can't make a bit of sense of it. Polymorphic lambda templates with reinterpret castings? Time to go to the beach.

I place the blame on the course you are trying to follow. I consider arriving you at wanting to do this the way you are without knowing how to use a function an indictment.

I rarely use functions that modify arguments. Which is why I didn't even see the "major thing that sticks out" when a more expensive (haha, spell check experienced) programmer takes a peek.

Find an article detailing OOP using C. Avoid features of C++ that, TBH, seem to be obscure even to ppl who claim to code in C++.

For now. And perhaps like me, forever. These are tiny machines and C is a very good match to the resources they provide.

I can believe someone who says she knows C backwards and forwards, top to bottom, wading area to the deep end.

I am impressed while skeptical of anyone claiming the same mastery of C++.

HTH and I wish I had deduced the difficulty you were having for what it turns out to be! So obvious when someone points it out.

a7

[trilerian]

Thank you for your thoughts. I did a little research on which language to start with, C vs C++. The internet seems to be divided on the subject with veterans of C saying you should learn C while others versed in C++ saying there is enough C in C++ that you can pick up what you don't learn from C++ easily. In all honesty, I probably should have just started with the Arduino IDE built in examples. The only real starter sketch I explored after jumping into the deep end to start with was a sketch posted in these forums called "SeveralThingsAtTheSameTime" found https://forum.arduino.cc/t/demonstration-code-for-several-things-at-the-same-time/217158

I just read that and tried to get the basic idea.

I would liken this to when I learned to play the guitar. My father tried to teach me some basic chords and scales to start with, but I found that all quite boring. I instead found this "cheating" method that was called Tab music, and off I went. To this day I still can't read music, but I can play over 100 songs and have made up (I would say written, except they live in my mind, lol) a couple as well.

Anyway, back to the point here. I decided to go down the C++ path, now that may not be the wisest approach, but to be fair I couldn't exactly make a "wise" decision as my knowledge of both languages was nil to begin with. The best I could do was a little research on which to learn. I considered posting the question of which to learn, but let's face that, there are hundreds of those posts already.

So what really led me to start with C++? Well, in the end it was the availability of a 31 hour C++ video on Youtube https://www.youtube.com/watch?v=8jLOx1hD3_o. At the current point in time I am half way through that video, and I have watched multiple parts multiple times. I figure in another month or so I might get it finished. This is not to say that I have boarded the C++ train, just where I have decided to start my journey. I intend to pick up a good book on both C and C++ to have as references and try to read them, lol. There are also some other programming languages I need to learn, Java, Ansible, Ruby, Chef, Puppet. These I need to learn for my day job and certifications. So as soon as I get through C++ and C, I am on to those...

All that said, the best way I have found to learn is to try and code something, make a mistake and then figure out how to correct that mistake. That can be through a lot of research, asking questions on this site or even by accident sometimes. What is really great is when you get to learn about that mistake you made later and the details really sink in. For example, and by coincidence, after I finally drug myself away from my laptop last this morning I did my nightly routine of watching some more of the video. So funny really, but the three quick topics that were up for me last night were passing values to a function, pass by pointers, and finally pass by reference. Yes, I found those 3 sections of great interest considering my struggles the past couple of days. And even better, they served the purpose of an "aha" moment to cement what @Delta_G was helping me with.

And incidentally I learned that can also be done with a pass by pointer.

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


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)
  }
}

This compiles and produces the same result as the pass by reference I learned about last night. Although the syntax in this is a bit more clunky because you have to dereference the pointers to modify what they are pointing to. And now I've got a vague idea of what that means, lol.

[alto777]

@trilerian

a good book on both C

I am not the only one who would think of The C Programming Language by Kernighan and Ritchie, which is very thin and is still an exemplary programming language text. Perhaps not so much as a learning to program text, but as a Here's C and how to use its features.

C so small! Nice thin book, referred to sometimes as the Bible. The entire language laid out step by step. Their coverage of arrays, pointers and structs (chapter 5 and 6) alone are worth whatever the book costs.

The examples are dated, and the context very much not embedded systems. Nevertheless I think it is worth any time spent.

I found a free online PDF and have it in my Books on the tablet. I don't feel bad, as I have purchased it for realz more than once. There's the copy my office mate took away with her, I hadda buy the ANSI version and several times it was a gift for ppl whose questions were good, but I had my own work to do.

The one physical copy I can see now was, in fact, a gift from a friend who thought my questions were good, but had his own work to do…

Anyway, sounds like you will get where you are headed and beyond.

a7

[trilerian]

Thank you, and I pulled the trigger. I've been meaning to get the "C Programming Language". I just bought both that and "C++ Primer" 5th edition.

[bitherder_57]

@alto777

C so small!

Do you know the author(s)? I cannot find it on Amazon and all I get are references to the C drive on Google.

[kenb4]

Type k&r c and programming should be suggested for completion. Click it.

[bitherder_57]

The one I cannot find is “C So Small!”, having the author(s) name(S) would help.