Testing code. Need some simplification recommendations

Hi guys, I want to play around with some motor speeds and adjust them in real time via the serial monitor.
This is what I came up with and it checks out fine, though for some reason I couldn't use 'else if' as the compiler gives an error saying that there can't be an 'else if' statement if there is no initial 'if' statement.
Clearly not the case when you look at the code.

Should I be using switch/case instead? Can anyone help me out with any other ways of simplifying the code so that it runs just that little bit quicker?

Thanks in advance :slight_smile:

#include <Servo.h>
#include <string.h>

int value = 0; // 750 - 2000
int in; //Serial read value

Servo ESCa, ESCb, ESCc, ESCd;

void setup() {

  ESCa.attach(6);
  ESCb.attach(9);
  ESCc.attach(10);
  ESCd.attach(11);
  Serial.begin(9600);


  ESCa.writeMicroseconds(value);
  ESCb.writeMicroseconds(value);
  ESCc.writeMicroseconds(value);
  ESCd.writeMicroseconds(value);

}

void currentstatus(){
  Serial.print("1: ");
  Serial.print(ESCa.readMicroseconds());
  Serial.print("2: ");
  Serial.print(ESCb.readMicroseconds());
  Serial.print("3: ");
  Serial.print(ESCc.readMicroseconds());
  Serial.print("4: ");
  Serial.println(ESCd.readMicroseconds());
}  


void loop() {
  
  // start by checking which motors to alter
  
  if(Serial.available()) 
    {
      in = Serial.parseInt();
      
      if(in == 0)
      {
        Serial.println("You got all da motors hynere");
        Serial.flush();
        while (!Serial.available());
        value = Serial.parseInt();
        ESCa.writeMicroseconds(value);
        ESCb.writeMicroseconds(value);
        ESCc.writeMicroseconds(value);
        ESCd.writeMicroseconds(value);
        Serial.print("All motors set to: ");
        Serial.println(value);
        currentstatus();
      }
  
  
    if(in == 1)     // once the appropriate motor(s) have been selected, write the following integer
      { 
        Serial.println("Motor Uno...");
        Serial.flush();
        while (!Serial.available());
        value = Serial.parseInt();
        ESCa.writeMicroseconds(value);
        Serial.print("Motor Uno is now running at a full ");
        Serial.println(value);
        currentstatus();
      }
    else if(in == 2) 
      {
        Serial.println("Nummber two...");
        Serial.flush();
        while (!Serial.available());
        value = Serial.parseInt();
        ESCb.writeMicroseconds(value);
        Serial.print("Second motor now spizzing at ");
        Serial.println(value);
        currentstatus();
      }
    else if(in == 3) 
      {
        Serial.println("Pull up on da terd won");
        Serial.flush();
        while (!Serial.available());
        value = Serial.parseInt();
        ESCc.writeMicroseconds(value);
        Serial.print("Twee no spinning at ");
        Serial.println(value);
        currentstatus();
      }
    else if(in == 4) 
      {
        Serial.println("Number Four to dutyyy!!!");
        Serial.flush();      
        while (!Serial.available());
        value = Serial.parseInt();
        ESCd.writeMicroseconds(value);
        Serial.print("Four now humming at ");
        Serial.println(value);
        currentstatus();
      }
    }
}

Edit: updated code

  if(Serial.available() == 'all')

Two problems here. First, single quotes are for single characters. Please post a picture of your keyboard with the all key circled.

Second, Serial.available() returns the NUMBER of characters in the buffer that have not been read. How is that going to equal 'all' or "all" or "some of them" or 'none of the above'?

if statements should ALWAYS have curly braces. You are obviously assuming that an if statement embodies everything that is indented some level, which is an erroneous assumption. Use curly braces and Tools + Auto Format.

You could try the attached sketch from the excellent Arduino Cookbook

BrushedMotor_Basic01.ino (851 Bytes)

You set value to 0, and use that for the initial value for Servo.writeMicroseconds().

Is that allowed ? I think the value should be between 1000 and 2000, and if you want to turn it off, you can try Servo.detach(), but it depends on the servo motor if that works.

The serial format/protocol is this ?
a1000 -> set servo 'a' to 1000 microseconds.
b2000 -> set servo 'b' to 2000 microseconds.
and so on.

You could use 'x' for all of them. It is possible to change your sketch and check for the keyword "all", but the first character of "all" is 'a', and that is the same character for servo 'a'.

I see too many things I would like to do: Put the servo pins in a table; make a function that sets a servo motor; make a if-else-if-else or case.
So I just show you a global use of if-else-if-else and a case.

char c = Serial.read();
if ( c == 'a')
{
  value = Serial.parseInt();
} 
else if ( c == 'b')
{
  value = Serial.parseInt();
}
char c = Serial.read();
switch(c)
{
case 'a':
  value = Serial.parseInt();
  break;
case 'b':
  value = Serial.parseInt();
  break;
}

EDIT: corrected the case example, see reply #5 by econjack.

Cheers for the reply guys,

you might be able to tell I spend a bit too much time with Python. I realised shortly after posting, the code is just terrible.

Initially I wanted to just type "all" into the serial to choose that function, but after a short bit of research I found that it would involve writing a seperate function to read in the serial data into a string and then perform string comparison operations to determine which function to run.

This is unnecessary so I just decided to use integers to make function selections.

I do still have a question though, is there a way to attempt a conditional statement but fall to another function in case an error is returned? (for example if I were to accidentally write a letter instead of an integer into the serial buffer)

Something like the try/except function with python.

Here is the updated code:

#include <Servo.h>
#include <string.h>

int value = 0; // 750 - 2000
int in; //Serial read value

Servo ESCa, ESCb, ESCc, ESCd;

void setup() {

  ESCa.attach(6);
  ESCb.attach(9);
  ESCc.attach(10);
  ESCd.attach(11);
  Serial.begin(9600);


  ESCa.writeMicroseconds(value);
  ESCb.writeMicroseconds(value);
  ESCc.writeMicroseconds(value);
  ESCd.writeMicroseconds(value);

}

void currentstatus(){
  Serial.print("1: ");
  Serial.print(ESCa.readMicroseconds());
  Serial.print("2: ");
  Serial.print(ESCb.readMicroseconds());
  Serial.print("3: ");
  Serial.print(ESCc.readMicroseconds());
  Serial.print("4: ");
  Serial.println(ESCd.readMicroseconds());
}  


void loop() {
  
  // start by checking which motors to alter
  
  if(Serial.available()) 
    {
      in = Serial.parseInt();
      
      if(in == 0)
      {
        Serial.println("You got all da motors hynere");
        Serial.flush();
        while (!Serial.available());
        value = Serial.parseInt();
        ESCa.writeMicroseconds(value);
        ESCb.writeMicroseconds(value);
        ESCc.writeMicroseconds(value);
        ESCd.writeMicroseconds(value);
        Serial.print("All motors set to: ");
        Serial.println(value);
        currentstatus();
      }
  
  
    if(in == 1)     // once the appropriate motor(s) have been selected, write the following integer
      { 
        Serial.println("Motor Uno...");
        Serial.flush();
        while (!Serial.available());
        value = Serial.parseInt();
        ESCa.writeMicroseconds(value);
        Serial.print("Motor Uno is now running at a full ");
        Serial.println(value);
        currentstatus();
      }
    else if(in == 2) 
      {
        Serial.println("Nummber two...");
        Serial.flush();
        while (!Serial.available());
        value = Serial.parseInt();
        ESCb.writeMicroseconds(value);
        Serial.print("Second motor now spizzing at ");
        Serial.println(value);
        currentstatus();
      }
    else if(in == 3) 
      {
        Serial.println("Pull up on da terd won");
        Serial.flush();
        while (!Serial.available());
        value = Serial.parseInt();
        ESCc.writeMicroseconds(value);
        Serial.print("Twee no spinning at ");
        Serial.println(value);
        currentstatus();
      }
    else if(in == 4) 
      {
        Serial.println("Number Four to dutyyy!!!");
        Serial.flush();      
        while (!Serial.available());
        value = Serial.parseInt();
        ESCd.writeMicroseconds(value);
        Serial.print("Four now humming at ");
        Serial.println(value);
        currentstatus();
      }
    }
}

And thanks again for your help :slight_smile:

Just a few suggestions for Peter_n's code:

change:

char c = Serial.read();

to:

char c = tolower(Serial.read());

Now it doesn't matter if they use upper or lower case letters. Also, the switch expression needs a minor correction and the case expressions need single quotes around them:

char c = tolower(Serial.read());
switch (c)
{
   case 'a':
     value = Serial.parseInt();
     break;
   case 'b':
     value = Serial.parseInt();
     break;
   default:          // Good idea to always have one of these
     value = -1;   // ...or some value that flags an error
     break;
}

So you use now : 2,10000 ? For servo '2' value 10000 ?
What if things get mixed (for example an error in the sketch) ? For example when you send 2,10000\r\n3,750. And you are reading out of sync half in between that servo 10000 should get value 3.
Could you ever get in sync again ?

You can add a start and stop character: 3,12000

Both the if-else-if-else and the 'case' have a fall-through-the-remaining-rest option.

if ( n == 0)
{
  value = Serial.parseInt();
} 
else if ( n == 1)
{
  value = Serial.parseInt();
} 
else
{
  // Everything else comes here.
  Serial.println("I'm sorry, Dave. I'm afraid I can't do that");
}
n = Serial.parseInt();
switch(n)
{
case 0:
  value = Serial.parseInt();
  break;
case 1:
  value = Serial.parseInt();
  break;
default:
  // Everything else comes here
  Serial.println("You talkin' to me ?");
  break;
}

econjack, I already noticed that my 'case' example was bad :-[ Thanks, I will correct to so I don't give the wrong ideas.

econjack:
// ...or some value that flags an error

This is what I mean. If the if/elseif/else statement encounters a reading error, will the conditional statement just fall apart or will the else conditional execute? I'm starting to think switch/case might be easier...

Peter_n what I aim to do is type a number, and then the program will wait until I enter the desired setting for the motor. I assume thats how it works though.

I won't be able to test this code out on my arduino until tomorrow, but I am wondering. Will this code continuously send whatever microseconds I write to each motor? Or will the signal cease while it executes "while (!Serial.available());"? I suppose my question is if .writeMicroseconds(value) means only a single pulse is sent in each iteration of the loop.

For the remaining options the if-else will execute the last 'else', just as the 'case' will execute the 'default'. In software they are used in the same way. The difference is that the 'case' is used with numbers and the if-else can have complex conditions.

Waiting for a settings ? Please don't do that. The parseInt() has a timeout (I think one second).
It is better and easier to send a single command in a single line.
When you want to wait for a settings, you can to write a serial interface. When you want to do that, there are libraries, perhaps even libraries with menus for that.

The loop() is executed over and over again. It is up to you to update the microSeconds just once at a command (good) or all the time (not good).

You code requires a function for setting a servo.
For example: setMyServo (int n, int value);
with n=0...3 or n=1...4 for the servo number.

PaulS:

  if(Serial.available() == 'all')

Two problems here. First, single quotes are for single characters. Please post a picture of your keyboard with the all key circled.

It's right next to the 'any' key, as in "Press any key". :slight_smile: