I am having a strange character reading from this function !

Hello,
I am trying to smooth readings from a pot but when I upload the sketch to arduino nano, I get a strange character reading, can you please help me do this because I never used an array as an index for a function. So I need to know where is my mistake.
my sketch is based on this example from the arduino tutorials but I am trying to build a function that can be usable again

here is my sketch:

const int numReadings = 10;

int readings[numReadings];      // the readings from the analog input
int average = 0;                // the average

int inputPin = A0;

void setup()
{
  // initialize serial communication with computer:
  Serial.begin(9600);                  
  // initialize all the readings to 0:
  for (int thisReading = 0; thisReading < numReadings; thisReading++)
    readings[thisReading] = 0;          
}

void loop() {
  average = potAverage(inputPin , readings[numReadings], numReadings);
  Serial.println(average);  
  delay(1);        // delay in between reads for stability            
}

int potAverage(int potPin, int readings[], int numReadings){
  int index = 0;
  int value = analogRead(potPin);
  // subtract the last reading:
  int total = total - readings[index];        
  // read from the sensor:  
  readings[index] = value;
  // add the reading to the total:
  total = total + readings[index];      
  // advance to the next position in the array:  
  index = index + 1;                    

  // if we're at the end of the array...
  if (index >= numReadings){             
    // ...wrap around to the beginning:
    index = 0;  
  }                        

  // calculate the average:
  int average = total / numReadings;
  
  return average;
}

Your variables index and total are new each time potAverage is called, but potAverage looks very much like it expects them to retain their values across calls.

Declare them as "static" in the routine after the opening brace.

static int index = 0;
static int total = 0;

then drop the int from "int total" and "int index" when you use them throughout the rest of potAverage.

The value variable is new each time, but that's just as it should be, as it is read each time.

HTH

a7

Hello firashelou,
I don't wish to be rude or anything but the mistakes in your code are the kind that beginners make. This is probably not everything but you define some variables at the top with global scope, for example

int inputPin = A0;

(There are others)
Then you define new variables with the same name with local scope, for example

  average = potAverage(int inputPin , int readings[], int numReadings)

I feel I should not have to explain this to someone with 1663 posts.

PerryBebbington:
Hello firashelou,
I don't wish to be rude or anything but the mistakes in your code are the kind that beginners make. This is probably not everything but you define some variables at the top with global scope, for example

int inputPin = A0;

(There are others)
Then you define new variables with the same name with local scope, for example

  average = potAverage(int inputPin , int readings[], int numReadings)

I feel I should not have to explain this to someone with 1663 posts.

you are right i did not mean to do that, but i copied the lined and it slipped :slight_smile:
in my full sketch i did not do that mistake

alto777:
Your variables index and total are new each time potAverage is called, but potAverage looks very much like it expects them to retain their values across calls.

Declare them as "static" in the routine after the opening brace.

static int index = 0;
static int total = 0;

then drop the int from "int total" and "int index" when you use them throughout the rest of potAverage.

The value variable is new each time, but that's just as it should be, as it is read each time.

HTH

a7

it did not work, i got a value that keeps incrementing and it is not reading the pot

this is my actual function at this moment, after the modification you suggested

int potAverage(int potPin, int readings[], int numReadings){
  static int index = 0;
  static int total = 0;
  int value = analogRead(potPin);
  // subtract the last reading:
  total = total - readings[index];        
  // read from the sensor:  
  readings[index] = value;
  // add the reading to the total:
  total = total + readings[index];      
  // advance to the next position in the array:  
  index = index + 1;                    

  // if we're at the end of the array...
  if (index >= numReadings){             
    // ...wrap around to the beginning:
    index = 0;  
  }                        

  // calculate the average:
  int average = total / numReadings;
  
  return average;
}

firashelou:
you are right i did not mean to do that, but i copied the lined and it slipped :slight_smile:
in my full sketch i did not do that mistake

Can you post a version that compiles?

wildbill:
Can you post a version that compiles?

ok corrected it, it was a ; and i added the index in the array but i am not sure if it is right

firashelou:
ok corrected it, it was a ; and i added the index in the array but i am not sure if it is right

Me neither.

Just post you entire program that compiles. It’s hard to know where you are going wrong…

a7

alto777:
Just post you entire program that compiles. It’s hard to know where you are going wrong…

a7

ok this is the one that compiles

const int numReadings = 10;

int readings[numReadings];      // the readings from the analog input
int average = 0;                // the average

int inputPin = A0;

void setup()
{
  // initialize serial communication with computer:
  Serial.begin(9600);                  
  // initialize all the readings to 0:
  for (int thisReading = 0; thisReading < numReadings; thisReading++)
    readings[thisReading] = 0;          
}

void loop() {
  average = potAverage(inputPin , readings[numReadings], numReadings);
  Serial.println(average);  
  delay(1);        // delay in between reads for stability            
}

int potAverage(int potPin, int readings[], int numReadings){
  int index = 0;
  int value = analogRead(potPin);
  // subtract the last reading:
  int total = total - readings[index];        
  // read from the sensor:  
  readings[index] = value;
  // add the reading to the total:
  total = total + readings[index];      
  // advance to the next position in the array:  
  index = index + 1;                    

  // if we're at the end of the array...
  if (index >= numReadings){            
    // ...wrap around to the beginning:
    index = 0;  
  }                        

  // calculate the average:
  int average = total / numReadings;
  
  return average;
}

firashelou:
ok this is the one that compiles

I am surprised that it does.
Try changing this:

  average = potAverage(inputPin , readings[numReadings], numReadings);

to this:

  average = potAverage(inputPin , readings, numReadings);

You did it again!

Read my first reply about static variables. And/or read some educational material about C/C++ variables and declaring them &c.

And when you fix it all up, post it again as well as explaining just how it isn't working!

a7

Please do not ever go back and edit previous posts without necessity, especially code. It's clear that you did that to the original post. It destroys the logic of the replies.

wildbill:
I am surprised that it does.
Try changing this:

  average = potAverage(inputPin , readings[numReadings], numReadings);

to this:

  average = potAverage(inputPin , readings, numReadings);

it gives a value of 0, 1 and -1 only

aarg:
Please do not ever go back and edit previous posts without necessity, especially code. It's clear that you did that to the original post. It destroys the logic of the replies.

ah ok I am sorry I won't do it again

alto777:
You did it again!

Read my first reply about static variables. And/or read some educational material about C/C++ variables and declaring them &c.

And when you fix it all up, post it again as well as explaining just how it isn't working!

a7

you said in the routine, I assume you mean in the main loop ?
so if I remove the index declaration from the function it is throwing an error

Using a class-based approach is usually more appropriate for these kinds of filters.

For example, see this discussion of a simple moving average implementation:

template <uint8_t N, class input_t = uint16_t, class sum_t = uint32_t>
class SMA {
  public:
    input_t operator()(input_t input) {
        sum -= previousInputs[index];
        sum += input;
        previousInputs[index] = input;
        if (++index == N)
            index = 0;
        return (sum + (N / 2)) / N;
    }

  private:
    uint8_t index             = 0;
    input_t previousInputs[N] = {};
    sum_t sum                 = 0;
};

void setup() {
  Serial.begin(115200);
  while (!Serial);
}

const unsigned long interval = 10000; // 100 Hz

void loop() {
  static SMA<20> filter;
  static unsigned long prevMicros = micros();
  if (micros() - prevMicros >= interval) {
    uint16_t rawValue = analogRead(A0);
    uint16_t filteredValue = filter(rawValue);
    Serial.print(rawValue);
    Serial.print('\t');
    Serial.println(filteredValue);
    prevMicros += interval;
  }
}

You don't have to reinvent the wheel, though, you can find an implementation in the Arduino Filters library I maintain, have a look at the SimpleMovingAverage.ino example.

That being said, the simple moving average is a poor choice for filtering potentiometers. It's very wasteful with RAM, doesn't filter that well, introduces latency, and doesn't prevent flickering between two adjacent values.
Instead, I usually use an exponential moving average filter in combination with hysteresis. The Arduino Filters library also includes this, see the FilteredAnalog.ino example.

Pieter

PieterP:
Using a class-based approach is usually more appropriate for these kinds of filters.

For example, see this discussion of a simple moving average implementation:

template <uint8_t N, class input_t = uint16_t, class sum_t = uint32_t>

class SMA {
  public:
    input_t operator()(input_t input) {
        sum -= previousInputs[index];
        sum += input;
        previousInputs[index] = input;
        if (++index == N)
            index = 0;
        return (sum + (N / 2)) / N;
    }

private:
    uint8_t index            = 0;
    input_t previousInputs[N] = {};
    sum_t sum                = 0;
};

void setup() {
  Serial.begin(115200);
  while (!Serial);
}

const unsigned long interval = 10000; // 100 Hz

void loop() {
  static SMA<20> filter;
  static unsigned long prevMicros = micros();
  if (micros() - prevMicros >= interval) {
    uint16_t rawValue = analogRead(A0);
    uint16_t filteredValue = filter(rawValue);
    Serial.print(rawValue);
    Serial.print('\t');
    Serial.println(filteredValue);
    prevMicros += interval;
  }
}



You don't have to reinvent the wheel, though, you can find an implementation in the [Arduino Filters library](https://github.com/tttapa/Arduino-Filters) I maintain, have a look at the [SimpleMovingAverage.ino](https://tttapa.github.io/Arduino-Filters/Doxygen/d3/d8d/SimpleMovingAverage_8ino-example.html) example.

That being said, the simple moving average is a poor choice for filtering potentiometers. It's very wasteful with RAM, doesn't filter that well, introduces latency, and doesn't prevent flickering between two adjacent values.
Instead, I usually use an exponential moving average filter in combination with hysteresis. The Arduino Filters library also includes this, see the [FilteredAnalog.ino](https://tttapa.github.io/Arduino-Filters/Doxygen/d3/dbe/1_8FilteredAnalog_8ino-example.html) example.

Pieter

thank you for your suggestion but I came up with an easier way which is the below:

const int numReadings = 10;

int readings[numReadings];      // the readings from the analog input
int average = 0;                // the average
int total;
int inputPin = A0;

void setup()
{
  // initialize serial communication with computer:
  Serial.begin(9600);                  
  // initialize all the readings to 0:
  for (int thisReading = 0; thisReading < numReadings; thisReading++)
    readings[thisReading] = 0;          
}

void loop() {
  average = potAverage(inputPin);
  Serial.println(average);  
  delay(1);        // delay in between reads for stability            
}

int potAverage(int potPin){
  // subtract the last reading:
  total= total - readings[index];        
  // read from the sensor:  
  readings[index] = analogRead(inputPin);
  // add the reading to the total:
  total = total + readings[index];      
  // advance to the next position in the array:  
  index = index + 1;                    

  // if we're at the end of the array...
  if (index >= numReadings){            
    // ...wrap around to the beginning:
    index = 0;  
  }                        

  // calculate the average:
  int average = total / numReadings;
  
  return average;
}
  delay(1);        // delay in between reads for stability

Is it actually unstable without this?

  // initialize all the readings to 0:
  for (int thisReading = 0; thisReading < numReadings; thisReading++)
    readings[thisReading] = 0;

They're already zero.

The code you posted doesn't compile.

I wish you'd stop wasting time.