How can i improve this code?

Hi
I just started learning a bit of arduino. I had a year of coding class a few years back but it wasn’t super great. Other than that its the first code i have ever written.

I’m doing an guide by elegoo (the most complete arduino starter kit) and decided to try and fiddle around a bit. I want a servo motor to turn on when i press a button and turn off when i press it again. The state should also be indicated by an LED.
Later i plan to add the function of reading out the servo motor angle and having another LED light up when its between “50 and 70 degree” or whatever.

How can i improve on this code? I read everywhere that goto statements are shit and you should avoid using them, but i don’t have the knowledge and understanding to find a different solution yet. Any other things you want to point out are appreciated, but my main interest is how to fix the goto situation.

#include <Servo.h>  //servo library

//define pins
#define red 8
#define butt 7
#define serv 9

//define initial states
int ledstate = 0;
boolean buttstate = false;
int i = 20;
boolean check = false;
int servspeed = 10;

//create servo object
Servo myservo;

void setup() {
  pinMode (red, OUTPUT);
  pinMode (butt, INPUT_PULLUP);
  analogWrite (red, ledstate);

  myservo.attach (serv);  //attach servo object to servo pin
  
}

void loop() {

  checkforbutt();
  onoff();

}

boolean checkforbutt () {

  boolean checked = false;  //has it been checked?

  if (digitalRead(butt) == LOW) {
    if (buttstate == true) {
      buttstate = false;
    }

    else {
      buttstate = true;
    }

    //LED shows if button is true or false
    if (buttstate == true) {
      ledstate = 255;
      analogWrite (red, ledstate);
    }

    else if (buttstate == false) {
      ledstate = 0;
      analogWrite (red, ledstate);
    }
    boolean checked = true; //if button was pressed, its true to get out of the loops in onoff
    delay (200);  //wait so the button isn't pressed twice by pressing it too long
    return checked;
  }
}

void onoff () {
here: //label to get out of loops
  //Servo is on or off
  if (buttstate == true) {

    //turn the servo from left to right
    for (i; i <= 180; i++) {
      myservo.write(i);
      delay (servspeed);
      check = checkforbutt();
      if (check == true) {  //if the button has been pressed, its supposed to stop moving, haven't found a way to do it differently. It would always complete the if statement first
        goto here;
      }
    }
    for (i; i > 19; i--) {
      myservo.write(i);
      delay (servspeed);
      check = checkforbutt();
      if (check == true) {
        goto here;
      }
    }

  }

}

I went through some iterations to get here. First i had everything in the loop function but I couldn’t check for input while the servo motor ran. I tried different things to end the loops, including trying to call the onoff function again if the buttoncheck was true. But it kept completing the if statement of the motor running before actually stopping. So in the end i did that stupid boolean checked and goto thing. Now it works but i know it isn’t optimal.

PS: i starts with 20 because the servo is doing weird things if lower, even though its supposed to go from 0 to 180
i don’t have a button that works like a switch, thats why i tried to code my own from a normal button. I know thats not optimal either, but im learning :smiley: so i think its not too bad of an exercise

I appreciate any help! I feel like being corrected on what i thought might be a good idea would really teach me a lot

How to eliminate goto's:

  1. Put your code aside. You can cut and paste sections from it if you like, but put it away.
  2. Look at the control structures that you have available to you.
  3. Formulate your problem or task
  4. Break the problem into sub tasks
  5. Fit each sub task with some control structure that you learned in 2)
  6. Connect all the sub tasks
  7. Done!

It's easier to re-think the entire problem without considering any goto's, than it is to "translate" the existing goto's into a conventional control structure.

A servo by itself does not tell you what angle it is at. The servo just tries to go to whatever angle you command, and then you should know what you commanded.

I don't like your variable names. They only go half way. For example, 'check'... check what? or 'red'... red what (it happens to represent a pin, so something like redLED or redPin would be clearer). Or 'checked'.. has what been checked... and so on...

Your use of 'ledstate' is strange, since you're passing it to analogWrite. Normally you would think of that as a value not a state.

Have a look at this version. It’s not complete and I have not tested it but it should give you a sense of how to approach the problem. Also, I’m not sure if I fully understood what you want to happen.

bool buttonPressed = false;


void loop() {
    checkButton();
    updateLed();
    updateServo();
}

void checkButton() {
    static byte prevButtonState = HIGH; // static means the value will be remembered between calls to this function
    byte buttonState = digitalRead(buttonPin);
    if (buttonState == LOW and prevButtonState == HIGH) { // assumes LOW when pressed
        buttonPressed = true;
    }
    else {
        buttonPressed = false;
    }
    prevButtonState = buttonState;
}

void updateLed() {
        // turn on led when button is pressed and turn it off after 200 millisecs
    static unsigned long ledStartTime;
    if (buttonPressed == true) {
        digitalWrite(ledPin, HIGH);
        ledStartTime = millis();
    }
    if (millis() - ledStartTime > = 200) {
        digitalWrite(ledPin, LOW);
    }
}

void updateServo() {
    static unsigned long previousServoMillis;
    unsigned long servoInterval =  10;
    static byte servoDegrees = 5; // move 5 degrees per step
    static bool servoMoving = false;
    if (buttonPressed == true and servoMoving == false) {
        servoMoving = true;
    }
    if (servoMoving == true) {
        if (currentMillis - previousServoMillis >= servoInterval) {
            previousServoMillis += servoInterval;
            servoPosition = servoPosition + servoDegrees; // servoDegrees might be negative

            if ((servoPosition >= servoMaxDegrees) || (servoPosition <= servoMinDegrees))  {
                        // if the servo is at either extreme 
                        //    change the sign of the degrees to make it move the other way
                servoDegrees = - servoDegrees; // reverse direction
                        // and update the position to ensure it is within range
                servoPosition = servoPosition + servoDegrees; 
                servoMoving = false; // so it won't move again until button is pressed
            }
            myservo.write(servoPosition);
        }
}

And have a look at Planning and Implementing a Program

…R

Your function boolean checkforbutt () returns true or false but when you call it in loop(); you don't do anything with the returned value, so what is the point of it returning a value if you don't use what it returns?

Robin2:
Also, I'm not sure if I fully understood what you want to happen.

Yes, I suspect that

I want a servo motor to turn on when i press a button and turn off when i press it again. The state should also be indicated by an LED.

hides some additional requirements that haven't been fully explained.

Also, 'butt' has certain connotations in English... not the best name for a switch.

i think it would help you, as well as others, if you added comments at the top of the file that succinctly describes what the program does, and before each sub-function that succinctly describes what the function does.

it looks like you checkforbutt() does more than check for buttons and you can certainly come up with a better name or onoff().

camelBack is a common way to capitalize symbol names instead of using underscores "_": checkForButt(), buttState

constants should at least begin with a capital: Red, Butt, Serv.

replacing goto with comefrom seldom works.

I don't know what is meant but neither goto nor comefrom are required. break is not required but helps.

consider

// swing servo back and forth controlled enabled by button press

#include <Servo.h>  //servo library

#define ButPin  A1
#define LedPin  10
#define ServPin 11

Servo myservo;

byte servoAng  = 90;
byte servoDang = 1;

// -----------------------------------------------------------------------------
// return true if button pressed
bool getButPress (void)
{
    static byte butLst = HIGH;
    byte but = digitalRead (ButPin);

    if (butLst != but)  {
        butLst = but;
        if (LOW == but)  {
            return true;
        }
    }

    return false;
}

// -----------------------------------------------------------------------------
void loop() {
    static bool enable = false;

    // toggle enable if button pressed and set LED to enable state
    if (getButPress ())  {
        enable = !enable;

        if (enable)
            digitalWrite (LedPin, LOW);  // on
        else
            digitalWrite (LedPin, HIGH); // off

        Serial.println (enable);
        delay (10);     // debounce
    }

    // optionally swing servo back and forth 0-180 deg
    if (enable)  {
        servoAng += servoDang;
        myservo.write (servoAng);
        delay (20);

        if (180 <= servoAng)  {
            servoDang = -1;
            Serial.println (" set Dang neg");
        }
        else if (0 >= servoAng)  {
            servoDang = 1;
            Serial.println (" set Dang pos");
        }
    }
}

// -----------------------------------------------------------------------------
void setup() {
    pinMode      (ButPin, INPUT_PULLUP);
    pinMode      (LedPin, OUTPUT);
    digitalWrite (LedPin, HIGH);    // off

    myservo.attach (ServPin);  //attach servo object to servo pin
    myservo.write  (servoAng);

    Serial.begin (115200);
}

"Also, 'butt' has certain connotations in English... not the best name for a switch."

In American maybe, but in English a butt can be a barrel for the storage of rainwater or the filter end of a coffin nail.

gcjr:
consider

// swing servo back and forth controlled enabled by button press

#include <Servo.h>  //servo library

#define ButPin  A1
#define LedPin  10
#define ServPin 11

Servo myservo;

byte servoAng  = 90;
byte servoDang = 1;

// -----------------------------------------------------------------------------
// return true if button pressed
bool getButPress (void)
{
    static byte butLst = HIGH;
    byte but = digitalRead (ButPin);

if (butLst != but)  {
        butLst = but;
        if (LOW == but)  {
            return true;
        }
    }

return false;
}

// -----------------------------------------------------------------------------
void loop() {
    static bool enable = false;

// toggle enable if button pressed and set LED to enable state
    if (getButPress ())  {
        enable = !enable;

if (enable)
            digitalWrite (LedPin, LOW);  // on
        else
            digitalWrite (LedPin, HIGH); // off

Serial.println (enable);
        delay (10);    // debounce
    }

// optionally swing servo back and forth 0-180 deg
    if (enable)  {
        servoAng += servoDang;
        myservo.write (servoAng);
        delay (20);

if (180 <= servoAng)  {
            servoDang = -1;
            Serial.println (" set Dang neg");
        }
        else if (0 >= servoAng)  {
            servoDang = 1;
            Serial.println (" set Dang pos");
        }
    }
}

// -----------------------------------------------------------------------------
void setup() {
    pinMode      (ButPin, INPUT_PULLUP);
    pinMode      (LedPin, OUTPUT);
    digitalWrite (LedPin, HIGH);    // off

myservo.attach (ServPin);  //attach servo object to servo pin
    myservo.write  (servoAng);

Serial.begin (115200);
}

Thanks that helped a ton! It of course makes a lot of sense to just increment the servo once per loop() loop, not in a for() loop. That way it always checks for button input and i dont have to get out of the for loops if the button is pressed. Also the way you increment it and change direction is infinetly smarter than the way i did it.
If i may I do have a few questions:

  1. Why are you using the analog pin for button? I didn’t understand yet what the exact difference is between analog and digital pins. And why are you using a digitalread on the analog pin? If someone could explain the difference between that in one or two sentences or link to a post where it is explained, that would be super cool.
  2. Could you put in words how the getButPress if statement works? I think i understood it, but im not quite sure.
  3. why are you putting in the serial.println commands? Is that just for control, so i can manually check if its doing what i want? should one get into the habit of doing that? And where do i read it out? The serial monitor of the arduino IDE just shows question marks.
  4. I’ve seen a lot of byte instead of int. Is there any rule when to use what? Back in school i just learned to use int for everything, thats why i did it.

To everyone else: I’ve noted that my names aren’t great and will make an effort to find better names in the future. But i also want you to know that this is an experiment, not something i need for a project. It will literally be “code it, try it, test it and move on” so its not like i need to go through this again and relearn what the code does. I was aware of what butt means but it’s not like my boss will see it and fire me :smiley:

  1. Why are you using the analog pin for button? I didn’t understand yet what the exact difference is between analog and digital pins. And why are you using a digitalread on the analog pin? If someone could explain the difference between that in one or two sentences or link to a post where it is explained, that would be super cool.

All the pins are digital pins, including the analogue pins. The difference is that the digital pins can only be digital pins whereas the analogue pins can be digital pins or analogue pins. Unless you need the analogue pins for analogue inputs they can be used exactly the same as any other digital pin. I guess it would be clearer to call them analogue/digital pins, but that would take up more space to print.

since you made the effort to try doing it yourself, i think it's worth seeing how simple the program can be

felix100m:

  1. Why are you using the analog pin for button?.

i test the code with an Arduino MultiFunction board that has 3 buttons wired to A1, A2, A3.

felix100m:
2. Could you put in words how the getButPress if statement works?

if the button state has changed capture new state. return true if new state is low (pressed), otherwise return false

bool getButPress (void)
{
    static byte butLst = HIGH;
    byte but = digitalRead (ButPin);

    if (butLst != but)  {
        butLst = but;
        if (LOW == but)  {
            return true;
        }
    }

    return false;
}

felix100m:
3. why are you putting in the serial.println commands? And where do i read it out? The serial monitor of the arduino IDE just shows question marks.

when i first wrote the code, it didn't work because i forgot to capture the new state so i added the prints to debug.

felix100m:
And where do i read it out? The serial monitor of the arduino IDE just shows question marks.

the bit rate in the Serial.begin() and on the serial monitor (bottom right) need to match

felix100m:
4. I've seen a lot of byte instead of int. Is there any rule when to use what? Back in school i just learned to use int for everything, thats why i did it.

it's good practice to use variables of the right size. this can save space (byte array vs int array). and may save hours of debugging when you need something bigger than an int (unsigned long msec = millis())

felix100m:
To everyone else: I've noted But i also want you to know that this is an experiment, not something i need for a project. It will literally be "code it, try it, test it and move on" so its not like i need to go through this again and relearn what the code does.

the first customer of any code are the developers. following good practices allow you write, test and debug quicker. developing good programming habits and skills with small programs is needed when writing more complicated code

I now added the function i mentioned yesterday to your code. It wasn’t clear in my last post, but i essentially wanted to make it a reaction minigame, where you have to stop the servo at an angle/when an LED is lit up. The first LED was just so i would see wether the servo should be running right now, because i didn’t know about the serial monitor yet.

I tried to choose better names for variables and also explain everything i was doing in comments. Also i tried to think first of what i wanted, how that could be accomplished and where the parts of code would make sense. Since the setNewReactionPoint () function ended up beeing just one line, i probably should’ve just put the line instead of calling a function, right? Again for storage space reasons?
But in my head that was gonna be a bigger function than it turned out to be. I didn’t know random was such an easy function. Thats why i made it seperate.

I feel like that went a lot neater than yesterday. Yesterday i knew it wasn’t great code, today i feel okay about it. But i still appreciate input, if there’s something i could’ve done better.

// reaction game: stop the servo when the LED is lit up

#include <Servo.h>  //servo library

#define ButPin  A1
#define LedPin  10
#define LedPin2 6
#define ServPin 11

Servo myservo;

byte servoAng  = 90;
byte servoDang = 1;
byte servoSpeed = 20;

byte reactionPoint;
byte reactionTime = 10; //you have 10 degrees to react and stop the servo

// -----------------------------------------------------------------------------
// return true if button pressed
bool getButPress (void)
{
  static byte butLst = HIGH;
  byte but = digitalRead (ButPin);

  if (butLst != but)  {
    butLst = but;
    if (LOW == but)  {
      return true;
    }
  }

  return false;
}

// -----------------------------------------------------------------------------
void loop() {
  static bool enable = false;

  // toggle enable if button pressed and set LED to enable state, control for wether the servo is running or not. Doesn't have a use other than me wanting to try it out
  if (getButPress ())  {
    enable = !enable;

    if (enable)
      digitalWrite (LedPin, HIGH);  // off
    else
      digitalWrite (LedPin, LOW); // on

    Serial.println (enable);
    delay (100);     // debounce
  }

  // optionally swing servo back and forth 0-180 deg
  if (enable)  {
    servoAng += servoDang;
    myservo.write (servoAng);
    delay (servoSpeed);

    if (180 <= servoAng)  {
      servoDang = -1;
      Serial.println (" set Dang neg");
      setNewReactionPoint (); //choose a new reactionPoint for the next cycle
    }
    else if (20 >= servoAng)  {
      servoDang = 1;
      Serial.println (" set Dang pos");
      setNewReactionPoint (); //choose a new reactionPoint for the next cycle
    }
  }


  //gamemode 1: reaction game, stop the servo when the LED lights up
  //read the current angle of the servo and light an LED when its in a specified range
  byte currentAngle = myservo.read ();
  //  Serial.println (currentAngle);
  if (currentAngle > reactionPoint && currentAngle < reactionPoint + reactionTime) {
    digitalWrite (LedPin2, HIGH);
    //    Serial.println ("is in range");
  }
  else {
    digitalWrite (LedPin2, LOW);
  }

  //gamemode 2 could be: It tells you an angle, you try to stop there, if right a green LED lights up, if wrong a red one
  
}

// -----------------------------------------------------------------------------
void setup() {
  pinMode      (ButPin, INPUT_PULLUP);
  pinMode      (LedPin, OUTPUT);
  pinMode      (LedPin2, OUTPUT);
  digitalWrite (LedPin, HIGH);    // off
  digitalWrite (LedPin2, HIGH);    // off

  myservo.attach (ServPin);  //attach servo object to servo pin
  myservo.write  (servoAng);

  Serial.begin (9600);
}


//--------------------------------------------------------------------------------
//function to set a new random value for reactionPoint, so the LED lights up at a different point every cycle.
//Makes it a reaction game instead of timing when it is at an fixed angle x
void setNewReactionPoint () {
  reactionPoint = random (20, 181 - reactionTime); //random new point between 20 (servo behaves weirdly below 20) and 180 minus the value of reactionTime, so it doesn't overshoot
}

There's some minor stuff you can improve. Not that important in a little Arduino sketch, but worth doing anyway.

You're using C++, so you can use const instead of #define and you should.

Your names could still be better - there's no need to abbreviate: ButtonPin, not ButPin. One of the things you're trying to do with your code is make it easy to follow, both for yourself and others. This is a trivial example, but I suspect you would find six months later that the longer name is trivial to understand, but the abbreviation will take a tiny amount of brain power. Multiply that across a 3000 line program and you've made life rather harder for yourself just to save a few keystrokes. Copy & paste can be handy here.

LedPin is presumably attached to a Led. Would be better if the name reflected what it is used for. Then LedPin2 would have a different name too. Any time you find yourself putting a numeric suffix on a variable name, it's a bad sign.

What does ServoDang mean? I can see what it does, but the name doesn't help me at all.

Functions should be short ideally, or at least simple. What those things mean in practice can be holy war territory. Personally I like to be able to view the whole function at one time and if there are more than three loops or ifs or switches, it's too complicated.

Your setNewReactionPoint function is debatable. Good name - yay! A single line of code isn't ideal for putting in a function but it can be sensible. If it's complex, that can make sense. Just the bonus of getting a descriptive name can be worth it. It makes it easier to put more comments in without disturbing the flow in the code that calls it. If you call it from multiple places in the code, it means when you do want to change it, it's a single change - the DRY principle (Don't repeat yourself).

For me, your loop function is too long because I have to scroll to view it.

You have magic numbers in your code (20, 180). Define them in one place and give them good names. Make them consts.

That's a lot of critique. Your code is fine as it is for the purpose. You can make it better though, as discussed.

is there a need to read() the servo angle? isn't it servoAng?

do your change do what you expected?

calling the setNewReactionPoint() twice is not so bad. there's only one change to make if you decide to change how the point is selected. calling the function twice is better than having 2 lines of code determining a reaction point. if efficiency were an issue, it could be made an inline function.

is it a "reactionPt" or reactionAngle? is it a reactionTime, or angle?

wildBill would "dAng" make sense to you?

gcjr:
wildBill would "dAng" make sense to you?

Looking at what it does, I assume it's deltaAngle or something like that. But I had to read the code to figure that out.

Obviously in such a little program it doesn't really matter, but enough of those in a sketch I'm being asked to help with and I'm going to shrug, mutter "life's too short" and move on.