Arduino Pin value gets stuck

Question

I'm using a Python program to send some message over serial port to Arduino. This message contains the pin number of the pin and whether it should be HIGH or LOW . Eg: Sending 2H\n should set pin 2 to HIGH.

pins array maps the pin numbers in the messages to the actual pin number of arduino. 1H corresponds to pin 22 on Arduinio Mega.

When manually sending one message at a time, things work fine. However when Python sends a series of 30 such messages one after the other in a loop with no delays, pin 1 is always stuck at whichever value it is set to on the first series of messages.

Example:

1L\n
2H\n
3H\n
4H\n
5H\n
...

followed by

1H\n
2H\n
3H\n
4H\n
5H\n
...

will cause pin 1 to be stuck at LOW when it should be high.

On arduino, this is the code that parse the message and sets the pin values.

void setPinValues(String message) {

  for(int i = 1; i <= sizeof(pins) / sizeof(pins[0]); i++ ) {
    String pinNumber = String(i);

    if( message == pinNumber + "H\n" ) {
      pinValues[i] = HIGH;
    }
    if( message == pinNumber + "L\n" ) {
      pinValues[i] = LOW;
    }
  }

}


void loop(){
    if( Serial.available() > 0 ) {
        received = Serial.read();
        message += received;
        if(received == '\n') {

            // Set pin values
            setPinValues(message);

            // Write to pins        
            for (int i = 1; i <= sizeof(pins) / sizeof(pins[0]); i++) {
                digitalWrite(pins[i], pinValues[i]);
            }

            // Clear buffer
            message = "";

        }       

    }

}

Arduino Mega communicates with Windows 8 x64 system over USB using a baud rate of 57600. When using a lower baud rate of 9600, this problem is not seen.

Furthermore, at baud rate of 57600, if I were to replace setPinValues with the following code, pin 1 is properly switched on and off.

void setPinValues(String message) {

  for(int i = 1; i <= sizeof(pins) / sizeof(pins[0]); i++ ) {
    if( message == String(1) + "H\n" ) {
      pinValues[1] = HIGH;
    }
    if( message == String(1) + "L\n" ) {
      pinValues[1] = LOW;
    }
    if( message == String(2) + "H\n" ) {
      pinValues[2] = HIGH;
    }
    if( message == String(2) + "L\n" ) {
      pinValues[2] = LOW;
    }
    if( message == String(3) + "H\n" ) {
      pinValues[3] = HIGH;
    }
    if( message == String(3) + "L\n" ) {
      pinValues[3] = LOW;
    }
  }

}

Doesnt both versions of setPinValues do the same? Why does lowering the baud rate prevent the problem? I cannot use a lower baud rate because the USB buffer gets filled up and things slow to a crawl.

---

To compare the last char H or L:

String pinValueString = message.substring(message.length() - 1);
char pinValueBuffer[2];
pinValueString.toCharArray(pinValueBuffer,2);

if(pinValueBuffer[0] == 'H') {
    digitalWrite(pins[pinNumber], HIGH);
}

Answer 1

Not sure if this is your problem, but setPinValues looks weird to me:

void setPinValues(String message) {
  for(int i = 1; i <= sizeof(pins) / sizeof(pins[0]); i++ ) {
    String pinNumber = String(i);

    if( message == pinNumber + "H\n" ) {
      pinValues[i] = HIGH;
    }
    if( message == pinNumber + "L\n" ) {
      pinValues[i] = LOW;
    }
  }
}

Going out of bounds

Each time you call setPinValues, i will range from 1 to length(pins), so if pins was declared as pins[5], i will be 1, 2, 3, 4 and 5. Assuming length(pins) == length(pinValues) this is a problem in itself, because pinValues[5] would not even exist!

pinValues[5] only creates pinValues[0] to pinValues[4] (yes, I know it's weird, you're declaring 5 elements but, since C is 0-indexed, they're 0, 1, 2, 3, and 4), so i should NEVER reach 5 or you will be out of bounds which is undefined behavior (and might well be the source of your problems) because you're writing to the wrong memory location.

The correct way to do this is looping from 0 to i < sizeof(pins) / sizeof(pins[0]) (notice the strict less-than < operator) which ranges from 0 to length(pins) - 1 and then just String pinNumber = String(i + 1);. That way, 1H will change the value of pinValues[0], mapping you 1-indexed serial messages to 0-indexed C arrays.

Looping is unnecessary

I'm not sure why you're looping over pins. You're doing unnecessary work and probably hogging the uC (specially since, as Dave Tweed points out in the other answer, you're using expensive functions such as String and string comparison with ==).

message has a single instance, e.g., 1H\n so there's no loop needed. char pinNumber = message[0] - '0'; will store the integer value of the first character in pinNumber, with which you're then able to index the array.

How does that work?

• message[0] would be a char type (e.g. '1')
• '0' is also a char, which corresponds to the ASCII value of 0 (48 in decimal).
• If message[0] contains the char '0', then the result is '0' - '0' == 48 - 48 == 0
• Since numbers are contiguous and increasing in the ASCII set the result of the subtraction is the actual number as a single byte integer.
• '1' - '0' == 49 - 48 == 1 and on and on.

You might also want to reject bogus input:

if (pinNumber >= sizeof(pins) / sizeof(pins[0]))
  return;

More spurious work

At each loop() iteration you do this:

// Write to pins        
for (int i = 1; i <= sizeof(pins) / sizeof(pins[0]); i++) {
  digitalWrite(pins[i], pinValues[i]);
}

Which has the same problem with the out-of-bounds as before.

It also does unnecessary work, since you're writing ALL pin values, whether they changed or not! Why don't you digitalWrite(pins[pinNumber], HIGH or LOW); directly in setPinValues and avoid this loop? You could even get rid of the pinValues array if you don't really need to store them (or you can keep it if you need it for some reason).

My alternative setPinValues

void setPinValue(char pin, char value) {
  char pinNumber = pin - '0';  // Get the integer value of the ASCII char

  if (pinNumber >= sizeof(pins) / sizeof(pins[0]))  // Reject out-of-bounds input
    return;

  switch (value) {
    case 'H':
      pinValues[pinNumber] = HIGH;  // Not necessary if you don't want to store the pin values
      digitalWrite(pins[pinNumber], HIGH);
      break;
    case 'L':
      pinValues[pinNumber] = LOW;  // Not necessary if you don't want to store the pin values
      digitalWrite(pins[pinNumber], LOW);
  }
}

And then just call it from loop() as setPinValue(message[0], message[1]);

Let me know if this solves your problem.

Answer 2

It's a performance issue. The first version of setPinValues() is making many calls to String() each time it is called, and this is a nontrivial function. This causes setPinValues() to take much longer than you think to finish, so it isn't too surprising that the Arduino eventually can't keep up with the incoming messages at 57600 bps.

In the second version of setPinValues(), the arguments to the String() calls are all constants, so they get evaluated at compile time. This version runs much faster, and it is able to keep up at 57600.