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] = EEPROM.read(memoryAddr);
  }

  return relayState;
}

void setup(void)
{
  // start serial port
  Serial.begin(115200);
  Serial.setTimeout(100);

  // 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();
    serialIn.trim();
  }

  /*
    RELAY CONTROL
    This can be done much more cleanly. I just know.
   */

  // ON COMMANDS
  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);
  }

  // OFF COMMANDS
  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.

…R

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;
     break;

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

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

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

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

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

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

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

             
} // end switch serialIn[1]

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


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

} // 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.