Take the 2-minute tour ×
Electrical Engineering Stack Exchange is a question and answer site for electronics and electrical engineering professionals, students, and enthusiasts. It's 100% free, no registration required.

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);
}
share|improve this question
1  
Remember that you are on an embedded platform. You have to care about how efficient your code is. Here I would use an interrupt and buffer all received values. This way I would ensure that none of the received chr could be lost. And then, in a main loop I would process the content of the buffer and act on the pins accordingly. I could also light up a LED in case of the buffer gets full (to let you know). Or I could ensure that the processing is always faster than the rate at which the chr are received in the worst case. –  Blup1980 Mar 14 at 12:42
    
@Blup1980 IIRC Arduino takes care of the serial interrupt for you. Serial.available() and Serial.read() are just wrappers around the serial buffer attached to the interrupt by Arduino when calling Serial.begin(), so he's actually already doing what you suggest :) The problem probably lies in the 128-byte buffer filling and dropping bytes due to the very slow processing. –  DuckTyped Mar 14 at 13:43
    
Just FYI. There is now a stack dedicated to Arduino arduino.stackexchange.com –  Nick Alexeev Mar 14 at 17:11

2 Answers 2

up vote 7 down vote accepted

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.

share|improve this answer
    
Thank you! If there are more than 10 pins so theres the possibility of receiving 13H\n, will char pinNumber = message[0] - '0'; still work? I'm thinking of splitting a message like 13H\n into 13 and H using strstr() but I'm not sure how it can be done –  Nyxynyx Mar 14 at 13:36
    
@Nyxynyx not without a loop scanning all characters. In that case String.toInt() is your best friend. IIRC, you don't even need to split the string since .toInt() will parse up to the first non-numeric character so, as long as your string starts with the pin number it will be alright. It'd be great if you confirm if .toInt() works like that, for future reference! –  DuckTyped Mar 14 at 13:38
    
Yes String.toInt() does work as expected! To compare the last char H or L, I use the code I've just updated the original question with. Will that be optimal? Used if conditionals because when using switch, I received the error switch quantity not an integer –  Nyxynyx Mar 14 at 14:34
2  
That's such a thorough answer it deserves a +2! Numerous coding sins covered and corrected in style. –  John U Mar 14 at 15:07
1  
If I was using more than 10 pins, I'd change the message format to always use two digits for the pin number: 01H, 05L, 13H, etc. That way, the H/L is always string[2], for easy access. –  Peter Bennett Mar 14 at 23:39

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.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.