How can I make my serial protocol more efficient?

I’m rather inexperienced with C++/AVR development and this is my first foray into developing a serial communication protocol of any kind. I have an 8 relay board connected to an Arduino Duemilanove, amongst various sensors, and my serial code is already turning into an inefficient mess, I think.

Relevant parts of my setup and loop is as follows

#define RELAY_ON 0
#define RELAY_OFF 1
#define Relay_1  12
#define Relay_2  11
#define Relay_3  10
#define Relay_4  9
#define Relay_5  8
#define Relay_6  7
#define Relay_7  6
#define Relay_8  5

bool * getRelayState()
  static bool relayState[8];

  for (int memoryAddr=0; memoryAddr <= 7; memoryAddr++) 
    relayState[memoryAddr] =;

  return relayState;

void setup(void)
  // start serial port

  // load relay state from eeprom
  bool *relayState;
  relayState = getRelayState();

  digitalWrite(Relay_1, relayState[0]);
  digitalWrite(Relay_2, relayState[1]);
  digitalWrite(Relay_3, relayState[2]);
  digitalWrite(Relay_4, relayState[3]);
  digitalWrite(Relay_5, relayState[4]);
  digitalWrite(Relay_6, relayState[5]);
  digitalWrite(Relay_7, relayState[6]);
  digitalWrite(Relay_8, relayState[7]);

  pinMode(Relay_1, OUTPUT);
  pinMode(Relay_2, OUTPUT);
  pinMode(Relay_3, OUTPUT);
  pinMode(Relay_4, OUTPUT);
  pinMode(Relay_5, OUTPUT);
  pinMode(Relay_6, OUTPUT);
  pinMode(Relay_7, OUTPUT);
  pinMode(Relay_8, OUTPUT);

void loop(void)
  String serialIn;

  //Verify connection by serial
  while (Serial.available() > 0) {
    //Read Serial data and allocate on serialIn
    serialIn = Serial.readString();

    This can be done much more cleanly. I just know.

  if (serialIn == "o1")
    digitalWrite(Relay_1, RELAY_ON);
    EEPROM.update(0, RELAY_ON);
  else if (serialIn == "o2")
    digitalWrite(Relay_2, RELAY_ON);
    EEPROM.update(1, RELAY_ON);
  else if (serialIn == "o3")
    digitalWrite(Relay_3, RELAY_ON);
    EEPROM.update(2, RELAY_ON);
  else if (serialIn == "o4")
    digitalWrite(Relay_4, RELAY_ON);
    EEPROM.update(3, RELAY_ON);
  else if (serialIn == "o5")
    digitalWrite(Relay_5, RELAY_ON);
    EEPROM.update(4, RELAY_ON);
  else if (serialIn == "o6")
    digitalWrite(Relay_6, RELAY_ON);
    EEPROM.update(5, RELAY_ON);
  else if (serialIn == "o7")
    digitalWrite(Relay_7, RELAY_ON);
    EEPROM.update(6, RELAY_ON);
  else if (serialIn == "o8")
    digitalWrite(Relay_8, RELAY_ON);
    EEPROM.update(7, RELAY_ON);

  else if (serialIn == "c1")
    digitalWrite(Relay_1, RELAY_OFF);
    EEPROM.update(0, RELAY_OFF);
  else if (serialIn == "c2")
    digitalWrite(Relay_2, RELAY_OFF);
    EEPROM.update(1, RELAY_OFF);
  else if (serialIn == "c3")
    digitalWrite(Relay_3, RELAY_OFF);
    EEPROM.update(2, RELAY_OFF);
  else if (serialIn == "c4")
    digitalWrite(Relay_4, RELAY_OFF);
    EEPROM.update(3, RELAY_OFF);
  else if (serialIn == "c5")
    digitalWrite(Relay_5, RELAY_OFF);
    EEPROM.update(4, RELAY_OFF);
  else if (serialIn == "c6")
    digitalWrite(Relay_6, RELAY_OFF);
    EEPROM.update(5, RELAY_OFF);
  else if (serialIn == "c7")
    digitalWrite(Relay_7, RELAY_OFF);
    EEPROM.update(6, RELAY_OFF);
  else if (serialIn == "c8")
    digitalWrite(Relay_8, RELAY_OFF);
    EEPROM.update(7, RELAY_OFF);

I’ve removed all code not relevant to this particular instance, but other serial commands follow the same design pattern of two bytes/characters per command. It’s a mess of elseif blocks and redundant code use and only getting worse the more I add to it.

Additionally, with how this is set up currently, I have issues when sending multiple commands within a very short period of time, they all end up combined in the buffer and nothing is executed. I’m using this in a hydroponics controller + dosing system driven by a Raspberry Pi and would prefer it to be as robust as possible.

Another related question, is there any way in C++ to refer to a constant dynamically, as in, can I refer to constants using a concatenated string? If not, is there a better way to define my relay pins and refer to them without lengthy if/else blocks?

Thanks a lot for any help and advice you can supply!

Have a look at the examples in Serial Input Basics - simple reliable ways to receive data.

Don’t use the String (capital S) class on an Arduino - it is not suited to the small memory and can cause corruption. Just use C strings (small s) which are char arrays terminated with 0.

Can you describe how your system of commands is designed? Is there any system to it?

Could you send a single message that has data for all the relays - perhaps something like <0,1,1,0,0,1>. That could be stored in an array and your code could just iterate throught the array to set the outputs - not a single IF or SWITCH/CASE needed.


I would suggest switch case statements, which will make it more readable:

int Relay_pin;
int EEProm_location;

switch (serialIn[1]) {
  case "1":
     Relay_pin = Relay_1;
     EEProm_location = 0;

  case "2":
     Relay_pin = Relay_2;
     EEProm_location = 1;

  case "3":
     Relay_pin = Relay_3;
     EEProm_location = 2;

  case "4":
     Relay_pin = Relay_4;
     EEProm_location = 3;

  case "5":
     Relay_pin = Relay_5;
     EEProm_location = 4;

  case "6":
     Relay_pin = Relay_6;
     EEProm_location = 5;

  case "7":
     Relay_pin = Relay_7;
     EEProm_location = 6;

  case "8":
     Relay_pin = Relay_8;
     EEProm_location = 7;

} // end switch serialIn[1]

switch (serialIn[0]) {
 case "o":
   digitalWrite(Relay_pin, RELAY_ON);
   EEPROM.update(EEProm_location, RELAY_ON);

 case "i":
   digitalWrite(Relay_pin, RELAY_OFF);
   EEPROM.update(EEProm_location, RELAY_OFF);

} // end switch serialIn[0]

I think you meant single-quoted character literals:

switch (serialIn[1]) {
  case '1':

Why do you have an array for the states but not for the pin numbers? Your code would be a lot simpler with an array of pin numbers.