serial mixing up, or arduino memory running out?

Hi,

I’m building a project which control 4 RGB LED strips and 2 white , my problems are now:
1- when I send too many commands, the serial start to mix them up, which miss everything up.
2- even when I send the commands slowly,after a while the arduino freeze, and no it’s not freezing from pc end.

Here is the code, please take a look and tell me if I made something in a stupid way.

the problem is only when I use the :cmd: mode, as I need to send many and arduino should update colors fast without a problem.

Thank you in advance, and yes I have already searched a lot over the net before running to you guys.

char params[100];
String valid;
char* p;

void setup() {
Serial.begin(57600);

//front strip
pinMode(2, OUTPUT); // r
pinMode(3, OUTPUT); // g
pinMode(4, OUTPUT); // b

//back
pinMode(5, OUTPUT); //r
pinMode(6, OUTPUT); //g
pinMode(7, OUTPUT); //b

//left
pinMode(8, OUTPUT); //r
pinMode(9, OUTPUT); //g
pinMode(10, OUTPUT); //b

//right
pinMode(11, OUTPUT); //r
pinMode(12, OUTPUT); //g
pinMode(13, OUTPUT); //b

//white1
pinMode(44, OUTPUT);
//white2
pinMode(45, OUTPUT);
//not used yet
pinMode(46, OUTPUT);

onMode();

}

boolean isValid(){
if(valid.length()>3)return true;
while (Serial.available()) {

delay(10); //small delay to allow input buffer to fill

char c = Serial.read(); //gets one byte from serial buffer

if (c==’|’) {
break;
} //breaks out of capture loop to print readstring
valid += c;
}

if(valid.startsWith(":")){
return true;
}
else {
valid="";
return false;
}

}

void loop() {

if(isValid()){
Serial.println(valid); //prints a valid command only string to serial port out

(valid.substring(5)).toCharArray(params,valid.length());
p=params;

if(valid.startsWith(":cmd")){
cmdMode(p);
}

else if(valid.startsWith(":onn")){
onMode();
}
else if(valid.startsWith(":off")){
offMode();
}

else {
valid="";
}

// free(p);
}

}

//----------------Modes---------

void onMode(){
valid="";

sendWhite(“w1”,255);
delay(900);
sendWhite(“w2”,255);
delay(900);
sendColor(“fr”,255,0,0);
delay(900);
sendColor(“fr”,255,255,255);
delay(900);
sendColor(“ba”,0,255,0);
delay(900);
sendColor(“ba”,255,255,255);
delay(900);
sendColor(“le”,255,255,0);
delay(900);
sendColor(“le”,255,255,255);
delay(900);
sendColor(“ri”,0,0,255);
delay(900);
sendColor(“ri”,255,255,255);
delay(900);
}
void offMode(){
valid="";

sendWhite(“w1”,0);
delay(800);
sendWhite(“w2”,0);
delay(800);
sendColor(“fr”,0,0,0);
delay(800);
sendColor(“ba”,0,0,0);
delay(800);
sendColor(“le”,0,0,0);
delay(800);
sendColor(“ri”,0,0,0);

}

void cmdMode(char* pa){
valid="";
//ex: fr;20;150;222
//ex: ba;0;255;222
//ex: w1;20
String p1,p2,p3,p4;
p1=subStr(pa ,";",1);
p2=subStr(pa ,";",2);
p3=subStr(pa ,";",3);
p4=subStr(pa ,";",4);
// Serial.println("p1 “+p1+” p2 "+p2+ " p3 “+p3+” p4 "+p4);
if(p1.startsWith(“w”)){
sendWhite(p1,p2.toInt());
}
else{
sendColor(p1,p2.toInt(),p3.toInt(),p4.toInt());
}

}

//-------------------------

void sendColor(String channel,int red,int green,int blue){
if(channel.equals(“fr”)){
analogWrite(2, red);
analogWrite(3, green);
analogWrite(4,blue);
}
if(channel.equals(“ba”)){
analogWrite(5, red);
analogWrite(6, green);
analogWrite(7,blue);
}
if(channel.equals(“le”)){
analogWrite(8, red);
analogWrite(9, green);
analogWrite(10,blue);
}
if(channel.equals(“ri”)){
analogWrite(11, red);
analogWrite(12, green);
analogWrite(13,blue);
}
}

void sendWhite(String channel,int shade){
if(channel.equals(“w1”)){
analogWrite(44, shade);
}
if(channel.equals(“w2”)){
analogWrite(45, shade);

}

}

char* subStr (char* str, char *delim, int index) {
char *act, *sub, *ptr;
static char copy[10];
int i;

// Since strtok consumes the first arg, make a copy
strcpy(copy, str);

for (i = 1, act = copy; i <= index; i++, act = NULL) {
//Serial.print(".");
sub = strtok_r(act, delim, &ptr);
if (sub == NULL) break;
}
return sub;

}

Lots of delays in there.

OnMode and OffMode, aren't even called! and they cause no problems anyway, they are just an effect when I turn the lights On or Off, I can remove them from the code, but usually people ask for full code, and when you put full code, they look into another thing!

My problem is with cmdMode, I need to call it too many times, so I'm sending

:cmd:le;255;0;0|:cmd:fr;255;0;0|:cmd:ri;255;0;0|:cmd:ba;255;255;0|

which says( cmdMode:make left R=255 , G= 0 , B =0 etc..)

when I send those commands from a processing program for example, if I send it slowly, allowing some time between each command set (like 600ms) then it's OK, but when I send faster (like 400ms) the valid command prints mixed strings, for example this is out put is taken from putty, while I was doing paste that string slowly at the beginning then then clicked faster, it mixed up everything:

:cmd:le;255;0;0 :cmd:fr;255;0;0 :cmd:ri;255;0;0 :cmd:ba;255;255;0 :cmd:le;255;0;0 :cmd:fr;255;0;0 :cmd:ri;255;0;0 :cmd:ba;255;255;0 :cmd:le;255;0;0 :cmd:fr;r:cmd:le;255;0;:cmd:le;255;2:cmd:le;255;0;;:cmd:le;255;c:cmd:le;255;0r:cmd:le;255;0c:cmd:le;255;0d:cmd:le;255;0;m:cmd:le;255;0;0

I really need to get this done, I wasted 3 weeks building and stuff, and now 3 days for this software issue, please take a real look on the code, I'm ready to provide any info which might help solving this, I saw some posts and modified the code according them, but the same problem exists.

There is absolutely nothing that the String class is doing that char arrays wouldn't be equally good for. If you don't know how to deal with char arrays, you should learn.

strtok_t() is the thread-safe version of strtok(). On a single threaded Arduino, do you really need the overhead of a thread-safe version of a function?

Since you are sending all kinds of delimiters already, add another one to define the end of a packet. Then, read data until that end of packet marker arrives. The stupid delay() in isValid() is not needed.

if that delay is so stupid why I see it everywhere? even in arduino examples they have it

if (Serial.available() > 0) { // get incoming byte: inByte = Serial.read(); // read first analog input, divide by 4 to make the range 0-255: firstSensor = analogRead(A0)/4; // delay 10ms to let the ADC recover: delay(10); ..... ...

Would tell me why it's stupid, and give me simple example of a while loop without the delay and it's working and print back what you sent! because if I simple remove the delay(10) then the while loop would break so fast before even catching that there is still some stuff in the reading buffer.

Another thing, is that I've already said that I looked into all posts related and I saw you comments about those, but I've already used the char array instead of string, and nothing changed.

And I made another version of the code where I didn't use strtok_r at all,but same problem.

I mean I just tell me what could cause the problem, I don't even know what causing the problem so I start looking into solving it.

You’re watching a ball game, and other scores are scrolling by on the bottom of the screen. Do you read one letter, and then close your eyes. Open them, read another letter, and close your eyes again? No, of course not, you read all the data that is present. Well, that delay is closing your Arduino’s eyes on reading serial data or watching the ball game or listening to your wife nagging or anything else.

because if I simple remove the delay(10) then the while loop would break so fast before even catching that there is still some stuff in the reading buffer.

No. What it would do is race through reading everything in the buffer, before the sender finished sending everything. That’s OK though, as long as you KNOW that the the packet is incomplete, and don’t try to process an incomplete packet.

What that delay is doing is causing you to read serial data so slowly that all the data has arrived before you finish reading it. At least, that’s the hope. If you change the serial speed, you need to figure out a new delay value, so you don’t miss anything or read the data too fast.

Since you have no idea whether you have a complete packet, or not, you’re screwed if something interferes with the sender sending data, since you assume that a complete packet is everything that arrives in some fixed period of time. Anything more or less than that causes you problems.

There are better ways.

#define SOP <
#define EOP >

bool started = false;
bool ended = false;

char inData[80];
byte index;

void setup()
{
   Serial.begin(57600);
   // Other stuff...
}

void loop()
{
  // Read all serial data available, as fast as possible
  while(Serial.available() > 0)
  {
    char inChar = Serial.read();
    if(inChar == SOP)
    {
       index = 0;
       inData[index] = '\0';
       started = true;
       ended = false;
    }
    else if(inChar == EOP)
    {
       ended = true;
       break;
    }
    else
    {
      if(index < 79)
      {
        inData[index] = inChar;
        index++;
        inData[index] = '\0';
      }
    }
  }

  // We are here either because all the serial data
  // has been read OR because an end of packet 
  // marker arrived. Which is it?
  if(started && ended)
  {
    // The end of packet marker arrived. Process the packet

    // Reset for the next packet
    started = false;
    ended = false;
    index = 0;
    inData[index] = '\0';
  }
}

PaulS: Since you are sending all kinds of delimiters already, add another one to define the end of a packet. Then, read data until that end of packet marker arrives.

I usually use end of line cr or lf to check that I got the whole line but having start and end is much better. It reminds me of serial start and stop bits, next stop is parity or CRC!

I usually use end of line cr or lf to check that I got the whole line but having start and end is much better.

The start and end markers can be anything, including cr or lf. I like < and > because they are not characters I use much for anything else.

OK, thank you, your answer was clear, indeed, the delay is stupid :slight_smile:

applying that code, improved the speed, but didn’t eliminate the problem, I made 2 very simple programs so anyone can see:

Arduino side:

#define SOP ‘<’
#define EOP ‘>’

boolean started = false;
boolean ended = false;

char inData[80];
byte index;

void setup()
{
Serial.begin(57600);
// Other stuff…
}

void loop()
{
// Read all serial data available, as fast as possible
while(Serial.available() > 0)
{
char inChar = Serial.read();
if(inChar == SOP)
{
index = 0;
inData[index] = ‘\0’;
started = true;
ended = false;
}
else if(inChar == EOP)
{
ended = true;
break;
}
else
{
if(index < 79)
{
inData[index] = inChar;
index++;
inData[index] = ‘\0’;
}
}
}

// We are here either because all the serial data
// has been read OR because an end of packet
// marker arrived. Which is it?
if(started && ended)
{
// The end of packet marker arrived. Process the packet

Serial.println(inData);
// Reset for the next packet
started = false;
ended = false;
index = 0;
inData[index] = ‘\0’;
}
}

Processing side:

import processing.serial.*;

Serial myPort;
void setup() {
size(200, 200);
background(127);

myPort = new Serial(this, “COM21”, 57600);
myPort.write("");
}

void draw() {

}

void mouseReleased() {
String ch= {“fr”, “ba”, “le”, “ri”};

while (true) {
delay(100); // no problem
// delay(10); // problem
int v=(int)random(255);
String colorran ="<cmd:"+ch[(int)random(4)]+";"+ ((int)random(255) )+ “;” +((int)random(255))+ “;” + ((int)random(255)) + “>”;

println("S "+colorran);

myPort.write(colorran );

while (myPort.available () > 0) {
char inByte = myPort.readChar();
print(inByte);
}
}
}

Just copy paste,modify according your COMM then one click on the processing program screen,you can uncomment the delay line in the processing side to witness the problem.

So now I’m going to tell what I understand from this, and please correct me:
The arduino side now is doing its best at full speed, but the coming data is much faster than it could do, so the buffer is getting full, that causing the read process to mix stuff and act crazy. that’s why I have to put a delay on the sender side which suits the speed.

So now I’m going to tell what I understand from this, and please correct me:
The arduino side now is doing its best at full speed, but the coming data is much faster than it could do, so the buffer is getting full, that causing the read process to mix stuff and act crazy. that’s why I have to put a delay on the sender side which suits the speed.

This sounds about right. And, of course, there is a solution. When the Arduino is ready to receive data, it sends a message to Processing. When Processing gets a message from the Arduino, it sends a packet and waits for another request for data.

In setup(), add

Serial.println("Ready");

Add that same code in the if(started && ended) block, after resetting the array.

In the Processing application, add:

myPort.bufferUntil('\n');

to setup(), after the myPort = statement.

Add a global variable (before setup()):

bool clearToSend = false;

Add a new method:

void serialEvent(Serial myPort)
{
  // read the serial buffer:
  String myString = myPort.readStringUntil('\n');

  // make sure you have a valid string:

  if (myString != null)
  {
    // trim off the whitespace (linefeed, carriage return) characters:
    myString = trim(myString);

    if(myString == "Ready")
      clearToSend = true;
  }
}

In the while loop, check that clearToSend is true, before constructing the string to write and sending it. After sending the string, set clearToSend back to false.

Having an application-level handshake like that will give you flow control, but introduces another problem: if there are any communication errors you could end up with a livelock where both sides are ready to proceed but neither side realises it. If you take that approach, you might find it's best to use an XON/XOFF style protocol, and implement a timeout on either side to resync in case the XON gets lost.

PeterH: if there are any communication errors you could end up with a livelock where both sides are ready to proceed but neither side realises it.

You have good point, specially that I'm using a SPP over bluetooth, and there is a lot of wifi, wireless mouse and keyboard, etc there is some chance that something will drop, I'll try to implement some solution without the timeout, and test it for some days.

  [color=#7E7E7E]// We are here either because all the serial data[/color]

  [color=#7E7E7E]// has been read OR because an end of packet [/color]
  [color=#7E7E7E]// marker arrived. Which is it?[/color]
  [color=#CC6600]if[/color](started && ended)
  {
    [color=#7E7E7E]// The end of packet marker arrived. Process the packet[/color]

    [color=#CC6600][b]Serial[/b][/color].[color=#CC6600]println[/color](inData);
    [color=#7E7E7E]// Reset for the next packet[/color]
    started = [color=#CC6600]false[/color];
    ended = [color=#CC6600]false[/color];
    index = 0;
    inData[index] = [color=#006699]'\0'[/color];
  }

You might be there because no characters are available yet there are more to come.
And you -only- code for start and end correct which means that you don’t reset started, ended, or index on error. Not only that but because of

      if(index < 79)
      {
        inData[index] = inChar;
        index++;
        inData[index] = '\0';
      }

you may be keeping your buffer from overflowing but you’re throwing read characters out.

So it keeps running until it finds SOP,

    if(inChar == SOP)
    {
       index = 0;
       inData[index] = '\0';
       started = true;
       ended = false;
    }

and starts reading good data again.

Well 57,600 is fast but that’s over 17 microseconds per character. The Arduino has 277 cycles to to process each. Maybe the serial chip takes too many of those or maybe it falls flat on its face when trying to print a line out. That’s probably where you’re losing data. It could be missing the next or the next few SOP’s.

If the sender takes a millisecond or 5, maybe 10, off between sending packets you might not have these problems.

PS – don’t/can’t the serial chips do handshaking when receive buffers are full? Is it really a protocol problem?