Arduino seems to miss data that was send? Or can't handle it?

Hi there,
-not sure if this is the right section, sorry if it is.-

my younger brother and I are working on a quizsystem. He wrote a program in C# which communicates via USB with the Arduino Uno.
-Press button
-Arduino sends data to pc
-Pc sends data back to arduino to control the LEDS and more.

This all worked fine.

We were one port to short so we added a shifter to the arduino, this controls all the "status LEDs" now. These are the LEDs on the box so you can see which button pressed first.

Arduino also controls a rgb led string.

Like I said before, it worked all fine till the shifter was added. It look like Arduino now misses some data it gets from the pc.
Before it was like IOIOIOIO for the leds blinking, but now its more like IOOIOIIIOO and all sorts of variations. It sometimes doesn't get the "out"-command or the "on"command.

This is the data the pc sends to Arduino.

@0+ 
#240000000
@0- 
#000000000
@0+ 
#240000000
@0- 
#000000000
@0+ 
#240000000
@0- 
#000000000
@0+ 
#240000000
@0- 
#000000000
@0+ 
#240000000
@0- 
#000000000

And this is the Arduino code:

const int amountButtons = 8;

// set the first pin of the row of buttons
const int PinButton[] = {A0, A1, A2, A3, A4, A5, 12, 13};    // *

// what are the pins the leds are in, has to be the size of the buttons *
const int PinLed[] = {0, 1, 2, 3, 4, 5, 6, 7};
// what character corresponds with what led (led 0 -> 0, led 1 -> 1 etc) *
const char LedByte[] = {'0', '1', '2', '3', '4', '5', '6', '7'};
// the pins that the rgp strip is connected to (Red = 1st, Green = 2nd, Blue = 3rd)
const int PinRGBStrip[] = {10, 9, 11};



//Pin connected to latch pin (ST_CP) of 74HC595
const int latchPin = 2;
////Pin connected to Data in (DS) of 74HC595
const int dataPin = 3;
//Pin connected to clock pin (SH_CP) of 74HC595
const int clockPin = 4;

// set the standalone mode, whether it needs a computer to operate
const int PinStandalone = 8;

// after how many miliseconds can someone press the button again
const int buttonPressDelay = 2000;

const int ledBlinkDelay = 500;
const int ledBlinkTimes = 5;

// this is the first symbol that is defining the message is a color
const char colorSign = '#';
// this is the character that determines that the next bytes are for leds
const char ledSign = '@';

// dont fiddle with these
boolean appendreceive = false;
String receiveMessage;
boolean buttonPressed[] = {false, false, false, false};    // *
long previousMillisButton[] = {0,0,0,0,0,0,0,0};       // *
long previousMillisLed[] = {0,0,0,0,0,0,0,0};          // *
boolean nextIsLed = false;
int ledNum = -1;
boolean nextIsLedState = false;
boolean standalone = true;

////////////////////////////////////////////////////////////////////
// * = array has to be the same length as amount of buttons
////////////////////////////////////////////////////////////////////


// the setup routine runs once when you press reset:
void setup() 
{
  // loop #buttonAmount times to set some stuff
  for(int i=0; i<amountButtons; i++)
  {
    // set all the leds as output
    //pinMode(PinLed[i], OUTPUT);    // removed since we use a shifter now
    // set all the buttons as input
    pinMode(PinButton[i], INPUT);
    // set all the button state's to high
    digitalWrite(PinButton[i], HIGH);
  }
  
  pinMode(latchPin, OUTPUT);
  pinMode(dataPin, OUTPUT);  
  pinMode(clockPin, OUTPUT);
  
  if(false)    // just so we are able so skipy the startup test sequence
  {
    // turn all the lights on at the start, this way we can check if all the lights are connected properly
    for(int n=0; n<amountButtons; n++)
    {
      SetLedState(PinLed[n], true);
      delay(1000);
      SetLedState(PinLed[n], false);
      delay(100);
    }
    
    for(int m=0; m<5; m++)
    {
      switch (m)
      {
        case 0:
          SetLedStripColor(255, 255, 255);
          break;
        case 1:
          SetLedStripColor(255, 0, 0);
          break;
        case 2:
          SetLedStripColor(0, 255, 0);
          break;
        case 3:
          SetLedStripColor(0, 0, 255);
          break;
        case 4:
          SetLedStripColor(0, 0, 0);
          break;
      }
      
      delay(1000);
    }
  }
}

// the loop routine runs over and over again forever:
void loop() 
{
  // Loop through buttons to get if they are pressed
  for(int i=0; i<amountButtons; i++)
  {
    // if button pressed
    if(digitalRead(PinButton[i]) == LOW && buttonPressed[i] == false)
    {
      // send the button number over serial
      if(!standalone)
        Serial.println(i);
      else
        FlashLights(PinLed[i], 500, 500, 5);
        
      buttonPressed[i] = true;
      
      // set the time for the button press delay
      previousMillisButton[i] = millis();
    }
  }
  
  if(!standalone)
  {
    // check if there is a message to receive
    if (Serial.available() > 0) 
    {
      char incomingByte = (char)Serial.read();
      
      // check if the character is the colorSign OR if we want to collect the characters
      if(incomingByte == colorSign || appendreceive)
      {
        if(incomingByte == colorSign)
          appendreceive = true;
        else
        {
          if(receiveMessage.length() < 9)
            receiveMessage += incomingByte;
            
          if(receiveMessage.length() >= 9)
          {
            SetLedStripColor(receiveMessage.substring(0, 3).toInt(), receiveMessage.substring(3, 6).toInt(), receiveMessage.substring(6, 9).toInt());
            receiveMessage = "";
            appendreceive = false;
          }
        } 
      }
      
      if(incomingByte == ledSign || nextIsLed || nextIsLedState)
      {
        if(incomingByte == ledSign)
        {
          nextIsLed = true;
        }
          
        if(nextIsLed)
        {
          if(nextIsLedState)
          {
            SetLedState(ledNum, (incomingByte == '+' ? true : false));
            nextIsLed = false;
            nextIsLedState = false;
          }
          
          if(!nextIsLedState)
          {
            // loop # amount of times to check if there is a led linked to the received byte
            for(int n=0; n<amountButtons; n++)
            {
              // check the byte to the current led
              if(incomingByte == LedByte[n])
              {
                ledNum = n;
                nextIsLedState = true;
              }
            }
          }
        }
      }
    }
  }
  // BUTTONS STATE SET
  for(int a=0; a<amountButtons; a++)
  {
    // check if the current button is pressed
    if(buttonPressed[a] == true)
    {
      unsigned long currentMillis = millis();
      
      // check if enough time has passed to set the buttonpressed to false
      if(currentMillis - previousMillisButton[a] > buttonPressDelay)
      {
        buttonPressed[a] = false;
        previousMillisButton[a] = 0;
      }
    }
  }
  
  if(digitalRead(PinStandalone) == HIGH)
  {
    if(standalone)
    {
      Serial.begin(57600);
      standalone = false;
    }
  }
  else if(digitalRead(PinStandalone) == LOW)
  {
    if(!standalone)
    {
      Serial.end();
      standalone = true;
    }
  }
}

void FlashLights(int led, int onTime, int offTime, int amount)
{
  for(int i=0; i<amount; i++)
  {
    SetLedState(led, true);
    SetLedStripColor(255, 255, 255);
    delay(onTime);
    SetLedState(led, false);
    SetLedStripColor(0, 0, 0);
    delay(offTime);
  }
}

void SetLedState(int led, boolean on)
{
  if(on)
  {
    //digitalWrite(PinLed[led], HIGH);    // Removed since we use a shifter now
    registerWrite(PinLed[led], HIGH);
  }
  else
  {
    //digitalWrite(PinLed[led], LOW);    // Removed since we use a shifter now
    registerWrite(PinLed[led], LOW);
  }
}

void SetLedStripColor(int red, int green, int blue)
{
  analogWrite(PinRGBStrip[0], red);
  analogWrite(PinRGBStrip[1], green);
  analogWrite(PinRGBStrip[2], blue);
}



// This method sends bits to the shift register:     (Origional arduino code)

void registerWrite(int whichPin, int whichState) {
// the bits you want to send
  byte bitsToSend = 0;

  // turn off the output so the pins don't light up
  // while you're shifting bits:
  digitalWrite(latchPin, LOW);

  // turn on the next highest bit in bitsToSend:
  bitWrite(bitsToSend, whichPin, whichState);

  // shift the bits out:
  shiftOut(dataPin, clockPin, MSBFIRST, bitsToSend);

    // turn on the output so the LEDs can light up:
  digitalWrite(latchPin, HIGH);
}

I really hope you can find a bug or something like it. If it's not totally clear what the problem is I can upload a short movie of the problem.
It can't be the Arduino being to slow to handle it, it's not much it gets to process??

This is the data the pc sends to Arduino.

Which really doesn't tell us anything. What is that supposed to mean?

const int PinLed[] = {0, 1, 2, 3, 4, 5, 6, 7};

You have LEDs attached to the serial port pins that the PC is using to talk to the Arduino? Well, no wonder stuff doesn't work.

I'm sorry if I wasn't specific enough, I'll try to be more now.

PaulS:

This is the data the pc sends to Arduino.

Which really doesn't tell us anything. What is that supposed to mean?

The @ means the data is for the statusled, not for the RGBleds. The zero means that the data is for LED zero. The plus or minus is for On/off.
The # means the data is for the RGBleds, 240000000 = red240, green000, blue000. Just the RGB colors. (000000000 is black=off)

PaulS:

const int PinLed[] = {0, 1, 2, 3, 4, 5, 6, 7};

You have LEDs attached to the serial port pins that the PC is using to talk to the Arduino? Well, no wonder stuff doesn't work.

No, I don't have LEDs attached to the serial port pins? The 0-7 correspond to the 8 statusLEDs. I think they are defined somewhere in the code?

Here is an image, it's totally not complete but you get the idea I think. In real, the shifter is of course connected correctly.

Hope this helps

Problem is solved. For those who have put time in it: Thank you anyway :slight_smile:

const char LedByte[] = {'0', '1', '2', '3', '4', '5', '6', '7'};

So, LedByte[n] is equal to n + '0'. No need for that array.

  if(!standalone)
  {
    // check if there is a message to receive
    if (Serial.available() > 0) 
    {
      char incomingByte = (char)Serial.read();
      
      // check if the character is the colorSign OR if we want to collect the characters
      if(incomingByte == colorSign || appendreceive)
      {

On any pass through loop, you read one character available from the serial port. At a minimum, you should read all the available data.

          if(nextIsLedState)
          {
            SetLedState(ledNum, (incomingByte == '+' ? true : false));
            nextIsLed = false;
            nextIsLedState = false;
          }
          
          if(!nextIsLedState)
          {

If nextIsLedState is not true, can it be anything but not true?

if(nextIsLedState)
{
   // do something
}
else
{
   // do something else
}
        if(incomingByte == colorSign)
          appendreceive = true;
        else
        {
          if(receiveMessage.length() < 9)
            receiveMessage += incomingByte;
            
          if(receiveMessage.length() >= 9)
          {
            SetLedStripColor(receiveMessage.substring(0, 3).toInt(), receiveMessage.substring(3, 6).toInt(), receiveMessage.substring(6, 9).toInt());
            receiveMessage = "";
            appendreceive = false;
          }
        }

Using a String to hold a fixed amount of data is horrid. Using a char array is a much better idea. But, since you are sending fixed length strings, there is no reason to store them at all. Simply count them. Less than 3, red gets multiplied by 10, and the new value (minus '0') gets added. More than 3 and less than 6, green gets manipulated. More than 6, and blue gets manipulated. After the call to SetLedStripColor, reset the count, red, green, and blue to 0.

Appending a character to a String takes a fair amount of time. Extracting sub-strings as Strings takes time. Converting a String to an int takes more time than converting a string to an int (which takes more time than converting on the fly, as defined above).

All these things point to serial overflow as a possibility.

Another problem that you aren't dealing with, but should be, is that serial data delivery is NOT guaranteed. Bytes can get corrupted, and discarded. You are assuming that that will never happen. Perhaps it hasn't yet, but, as the investment commercials are all prompt to point out, past performance is not indicative of future performance.

I agree with PaulS.. You need a Protocol for serial transferring the data. Without handshake you may get trouble..especially when you use Shiftregisters..