How to move a servo with a function

Question

Im currently moving a servo from one side to another by using the typical for loop like this:

int lightON = 180;
int lightOFF = 90;

if (buttonState == HIGH) {

  digitalWrite(LED, HIGH);
  for (pos1 = lightOFF; pos1 <= lightON; pos1 += 1) {
    servo1.write(pos1);
    delay(15);
  }
  digitalWrite(LED, LOW);

}

That would be moving the servo one way and if I push the button again the servo would move another way.

if (buttonState == LOW) {

  digitalWrite(LED, HIGH);
  for (pos1 = lightON; pos1 >= lightOFF; pos1 -= 1) {
    servo1.write(pos1);
    delay(15);
  }
  digitalWrite(LED, LOW);

}

The thing is, I have this very same code in 3 other servos and I don't want to be repeating the same code with 2 different variables (pos and servo) over and over again, so I tried

void runServoLight(int pos, int light1, int light2, Servo servo, int state) { 

  if (state == 1) {

  digitalWrite(LED, HIGH);

  for (pos = light1; pos <= light2; pos += 1) {
     servo.write(pos);
     delay(15);
  }

  digitalWrite(LED, LOW);

  } else if (state == 0) {

  digitalWrite(LED, HIGH);

  for (pos = light2; pos >= light1; pos -= 1) {
    servo.write(pos);
    delay(15);
  }

  digitalWrite(LED, LOW);

  }

}

Now instead of running the whole for loop code I only run:

runServoLight(pos1, lightON, lightOFF, servo1, 1);

The problem is that it doesn't seem to work and I can understand why, I may be writing the function wrong but im nor really sure. I would really appreciate some help. Thank you!

Answer 1

You wrote:

void runServoLight(
        int pos, int light1, int light2, Servo servo, int state)

Here you are giving the list of parameters used by the function. In C++, parameters are by default passed by value. This means that, when the function is called, the arguments passed in the call are copied into the parameters listed here, which are local variables of the function. Changes to those parameters will not propagate to the arguments provided by the caller.

This can be an issue for the Servo object. This object keeps some internal state, which has the memory of the current configuration of the timers. Since you are working on a copy of the provided object, the original object is not updated when you act on the copy. I do not know whether this is making your program misbehave, but it would be safer to pass the Servo object by reference:

void runServoLight(
        int pos, int light1, int light2, Servo &servo, int state)

This way the object will not be copied: the function will instead work on the original object provided by the caller.

The same may apply to the pos parameter. The very first thing the function does with this parameter is to overwrite it. Then there is no point in receiving that value from the caller. You can instead remove the parameter and create a local variable with the same name:

void runServoLight(int light1, int light2, Servo &servo, int state) {
    int pos;  // local variable
    ...
}

If you want the caller's version of this variable to be updated, you could pass it by reference, just like the Servo object. It is, however, more efficient to keep the variable local and have the function return the final position. The call would then look like:

pos1 = runServoLight(lightON, lightOFF, servo1, 1);

One final point that is not visible in the snippets you showed: are you doing edge detection on the buttons? Edge detection means that you act on the changes of state of the buttons, not on the states themselves. For example, you move from lightOFF to lightON not when the button state is HIGH but rather when it turns HIGH.

Answer 2

This is not a solution, however, by rafactoring it makes your sketch more maintainable as duplicated code is removed:

void runServoLight(int pos, int light1, int light2, Servo servo, int state) 
{ 
  if (state == 1) {
    process(light1, light2, +1, servo);
  } 
  else if (state == 0) {
    process(light2, light1, -1, servo);
  }
}

def process(int this_light, int other_light, int delta, Servo servo)
{
  digitalWrite(LED, HIGH);
  for (pos = this_light; pos >= other_light; pos += delta) {
    servo.write(pos);
    delay(15);
  }
  digitalWrite(LED, LOW); 
}

Instead of the state integer, it's more clear to use an enumeration and use a switch statement.

#include "Servo.h"

const int LED = 13;

enum EServoState { LEFT, RIGHT };

void runServoLight(int light1, int light2, Servo servo, EServoState state);

void setup() 
{
}

void loop() 
{
  int light1 = 1;
  int light2 = 2;
  EServoState state;
  Servo servo;
  runServoLight(light1, light2, servo, state);
}

void runServoLight(int light1, int light2, Servo servo, EServoState state) 
{ 
  switch (state)
  {
  case LEFT:
    process(light1, light2, +1, servo);
    break;
    
  case RIGHT:
    process(light2, light1, -1, servo);
    break;
  
  //default:
   // Handle program error
  }
}

void process(int this_light, int other_light, int delta, Servo servo)
{
  digitalWrite(LED, HIGH);
  for (int pos = this_light; pos >= other_light; pos += delta) 
  {
    servo.write(pos);
    delay(15);
  }
  digitalWrite(LED, LOW); 
}