Arduino serial data input output

Question

I have created an Arduino sketch that I am using to send and receive data via serial. It works pretty well and doesn't seem go wrong as long as the correct formats are received. Is there anything I could do to improve this/shrink it to clear up some code smells?

String dataString;
void setup() {
 Serial.begin(9600);

 //setup digital pins as inputs pin 3 - 13
 for (int i = 2; i <= 7; i++) {
    pinMode(i, INPUT);
  }
  //setup digital pins as OutPuts  pin 22 - 53
  for (int i = 7; i <= 13; i++) {
    pinMode(i, OUTPUT);
  }



}
void loop() {
  serialDataOutput();
  serialDataInput();
  delay(100);

}


void serialDataOutput() {
  dataString = "";
  for (int i = 0; i <= 13; i++) {
    dataString.concat(i);
    dataString.concat("," + String(digitalRead(i)) + "/");
  }

  for (int i = 14; i <= 18; i++) {
   dataString.concat(i);
   dataString.concat("," + String(analogRead(i - 14)) + "/");
  }
   Serial.println(dataString);
  delay(0);
    }
      void serialDataInput() {
  String Input;
   if (Serial.available() > 0) { // If data is available to read,
    Input = Serial.readStringUntil('\n'); // read it and store it in val
  }
  //  Input = "13,1";
  int commaIndex = Input.indexOf(',');
  //  Search for the next comma just after the first
  int secondCommaIndex = Input.indexOf(',', commaIndex + 1);
  String firstValue = Input.substring(0, commaIndex);
  String secondValue = Input.substring(commaIndex + 1, secondCommaIndex);
  digitalWrite(firstValue.toInt(), secondValue.toInt());
}

Answer 1

You claim that this code works. However, since you haven't really specified exactly what the intended behaviour is, I can't verify whether it does what it's supposed to do. There are some very strong code smells that lead me to suspect that this code is completely broken, or at least doing something silly.

• Non-idiomatic loop limits: The idiomatic way to write a loop that does something 14 times, especially when counting from 0, is

  for (int i = 0; i < 14; i++) {
      …
  }
  

  This tip would be merely a nitpick, except that I suspect you have a bug in setup(): is pin 7 supposed to be in INPUT or OUTPUT mode?

• Apparent mismatch of code and comments: In setup(), you wrote:

  //setup digital pins as inputs pin 3 - 13
  for (int i = 2; i <= 7; i++) {
  

  What is the relationship between 3-13 and 2-7? The comment is either erroneous or fails to fully explain the code.

• Misuse of String.concat(): dataString, being a String, is immutable. Therefore, dataString.concat(i) is a no-op, and dataString.concat("," + String(digitalRead(i)) + "/") is a convoluted way of just calling digitalRead(i) and discarding the result. The final outcome is that Serial.println(dataString) is equivalent to Serial.println("").

In addition, I see some code hygiene issues, such as inconsistent indentation and using Input instead of input for a variable name.