Reading a packet of data in one call

Hey guys! So, I have not programmed arduinos in a while, and now that I'm getting back into it I hit a roadblock. I am trying to send data from my C# program to my arduino, and then the arduino will execute something depending on the data sent. My C# program seems to work, but I get weir errors with the arduino. Here's my code:

#include <Servo.h> 

byte myVersion = 1;
byte testPin = 13; //A reference to the led pin
byte jawPin = 9;
byte neckPin = 10;

//PLC pins
byte plcPin1 = 40;
byte plcPin2 = 41;
byte plcPin3 = 42;
byte plcPin4 = 43;

byte data[64]; //holds data to be written to the serial port
byte tempBuffer[64]; //A temporary buffer to hold data being read from the serial
boolean digitals[50]; //Holds the data for all digital pins
byte analogs[20]; //Holds all the analog pin states
byte PLC[5]; //holds the PLC pin values
int lum = 0; //Value of pin 13's fade
byte jawID=0;
byte neckID=1;
Servo servos[10];
boolean inc = true;

//writes the buffer dat[] to the serial
void writeBuffer(byte dat[], int bufferSize)
{
  int i;
  for(i = 0; i < bufferSize; i++)
  {
    Serial.write(data[i]); 
  }
}

//sets pin pinID to dat
void setPin(boolean analog, byte pinID, byte dat)
{
  //handle analog pins
  if (analog == true)
  {
    analogWrite(pinID,dat);
    analogs[pinID] = dat;
  }

  //handle digital pins 
  else
  {
    if (dat>=128)
    {
      digitalWrite(pinID,HIGH);
      digitals[pinID] = true;
    }
    else
    {
      digitalWrite(pinID,LOW); 
      digitals[pinID] = false;
    }
  }
}

//assigns a servo to a pin
void assignServo(byte servoID, byte pinID)
{
  servos[servoID].attach(pinID); 
}

//returns the state of analog pin pinID
byte getAnalogPin(byte pinID)
{
  return analogs[pinID];
}

//returns 255 if digital pin pinID is HIGH
byte getDigitalPin(byte pinID)
{
  if (digitals[pinID] == true) 
  {
    return 255; 
  }
  else
  {
    return 0; 
  }
}

//sets a servo to the setting
void setServo(Servo servo, int pos)
{
  servo.write(pos);
}

//gets a servo's rotation
byte getServo(Servo servo)
{
  return servo.read(); 
}

void setup()
{
  pinMode(testPin, OUTPUT);
  Serial.begin(9600);
}

void loop()
{  
  int dataLength = Serial.available();
  char buffer[dataLength];
  while(dataLength>0)
  {
    Serial.readBytes(buffer,Serial.available());
    switch (buffer[0])
    {
      //writes a ping back
    case 0:
      {
        Serial.write(255);
      }
      break;

      //sets an analog pin
    case 1:
      {
        analogWrite(buffer[1],buffer[2]);
      }
      break;

      //sets a digital pin
    case 2:
      {
        pinMode(buffer[1],OUTPUT);
        if (buffer[2]>=128)
        {
          digitalWrite(buffer[1],HIGH); 
        }
        else
        {
          digitalWrite(buffer[1], LOW);
        }
      }
      break;

      //request for an analog pin's value
    case 3:
      {
        Serial.write(3);
        Serial.write(getAnalogPin(buffer[1]));
      }
      break;

      //request for an digital pin's value
    case 4:
      {
        Serial.write(4);
        Serial.write(getDigitalPin(buffer[1]));
      }
      break;

      //sets a servo
    case 5:
      {
        setServo(servos[buffer[1]],buffer[2]);
      } 
      break;
      
      case 6:
      {
       assignServo(buffer[1],buffer[2]); 
      }

    default:
      byte buff[2];
      buff[0]=255;
      buff[1]=0;
      Serial.write(buff,2);
      break;
    }
  }
}

Here's my C# class for communicating:

            static SerialPort _serialPort;
            String[] messages = new String[100];
            int messagecount = 0;
            static byte[] serialData;
            StringComparer stringComparer = StringComparer.OrdinalIgnoreCase;
            Thread readThread = new Thread(Read);

            byte[] pwmPins = new byte[] { 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 44, 45, 46 };
            public byte[] analogPins = new byte[46];
            public bool[] digitalPins = new bool[54];
            public byte[] servoStates = new byte[8];

            //command 0
            public void getPing()
            {
                byte[] buffer = new byte[] { 0 };
                Write(buffer);
                byte[] retunValues = ReadSerial();
            }

            //command 1
            public void setPinAnalog(byte pinID, byte newState)
            {
                if (pwmPins.Contains(pinID))
                {
                    byte[] buffer = new byte[] { 1, pinID, newState };
                    Write(buffer);
                    analogPins[pinID - 1] = newState;
                }
            }

            //command 2
            public void setPinDigtal(byte pinID, bool newState)
            {
                byte[] buffer;
                if (newState)
                {
                    digitalPins[pinID] = true;
                    buffer = new byte[] { 2, pinID, 255};
                }
                else
                {
                    digitalPins[pinID] = false;
                    buffer = new byte[] { 2, pinID, 0};
                }
                Write(buffer);
            }

            //command 3
            public byte getPinAnalog(byte pinID)
            {
                byte[] buffer = new byte[] {3,pinID};
                Write(buffer);
                byte[] returnArray = ReadSerial();
                analogPins[pinID-1] = returnArray[1];
                return returnArray[1];
            }

            //command 4
            public byte getPinDigital(byte pinID)
            {
                byte[] buffer = new byte[] { 4, pinID };
                Write(buffer);
                byte[] returnArray = ReadSerial();
                if (returnArray[1] == 255)
                    digitalPins[pinID] = true;
                else
                    digitalPins[pinID] = false;
                return returnArray[1];
            }

            //command 5
            public void setServo(byte servoID, byte dir)
            {
                byte[] buffer = new byte[] { 5, servoID, dir };
                Write(buffer);
            }

            //Serial Commands
            public bool Init()
            {
                foreach (string mess in SerialPort.GetPortNames())
                {
                    Debug.WriteLine(mess);
                }

                if (SerialPort.GetPortNames().Length > 0)
                {
                    // Create a new SerialPort object with default settings.
                    _serialPort = new SerialPort();

                    // Allow the user to set the appropriate properties.
                    _serialPort.PortName = SerialPort.GetPortNames()[0];
                    Debug.WriteLine(_serialPort.PortName.ToString());
                    _serialPort.BaudRate = 9600;
                    _serialPort.DataBits = 8;

                    // Set the read/write timeouts
                    _serialPort.ReadTimeout = 500;
                    _serialPort.WriteTimeout = 500;
                    try
                    {
                        _serialPort.Open();
                    }
                    catch (UnauthorizedAccessException)
                    {
                        Debug.WriteLine("Failed to open port!");
                    }
                    readThread.Start();


                    Console.Write("Name: ");
                    //name = Console.ReadLine();

                    Console.WriteLine("Type QUIT to exit");
                    return true;
                }
                return false;
            }

            public void Close()
            {
                readThread.Join();
                _serialPort.Close();
            }

            public byte[] ReadSerial()
            {
                Read();
                if (serialData.Length > 0)
                    handleErrors(serialData);
                return serialData;
            }

            public static void Read()
            {
                try
                {
                    serialData = new byte[_serialPort.BytesToRead];
                    _serialPort.Read(serialData, 0, _serialPort.BytesToRead);
                }
                catch (TimeoutException)
                {
                    Console.WriteLine("Serial read timed out.");
                }
            }

            public void Write(byte[] packet)
            {
                try
                {
                    if (_serialPort != null)
                        _serialPort.Write(packet, 0, packet.Length);
                }
                catch (TimeoutException) { Console.WriteLine("Serial write timed out."); }
            }

Any big mistakes that I missed?
Thanks.

char buffer[Serial.available()];
  while(Serial.available())

These two function calls may not return the same values. Data may arrive while the while loop is running, allowing you to overflow your buffer.

Dynamically sizing the buffer this way is a really bad idea. A statically sized, global buffer is a much better idea.

    Serial.readBytes(buffer,Serial.available());

Another call, which can return a 3rd value. Do NOT do this. If you MUST use a dynamic array, make ONE call to Serial.available() and record that value. Ditch the while loop, using an if statement, instead.

Any big mistakes that I missed?

You failed to post all of your Arduino code. You failed to post any of your C# code.

[quote author=PaulS link=topic=125578.msg944187#msg944187 date=1349342610]
Dynamically sizing the buffer this way is a really bad idea. A statically sized, global buffer is a much better idea.
Ditch the while loop, using an if statement, instead.
[/quote]
Uh, okay... not quite sure what your vision is here. I need to process all incoming packets at once, for timing reasons...

[quote author=PaulS link=topic=125578.msg944187#msg944187 date=1349342610]
[code]Any big mistakes that I missed?

You failed to post all of your Arduino code. You failed to post any of your C# code.
[/quote]

apologies, there is a lot of code, so I'm trying to only post relevant stuff. I will post more though[/code]

Uh, okay... not quite sure what your vision is here. I need to process all incoming packets at once, for timing reasons...

You changed code in your first post. Please don't do that again. Post the code again, if needed.

  int dataLength = Serial.available();
  char buffer[dataLength];
  while(dataLength>0)
  {
    Serial.readBytes(buffer,Serial.available());

Where in this while loop do you change the value in dataLength? If there is any serial data, you've just entered an endless loop.

Suppose that dataLength is 0. How big an array did you create?

Suppose that an interrupt occurs between the first call to Serial.available(), which say returned 3 and the second call which would now return 4. The amount of data that you just told Serial.readBytes() to read will now not fit in the array.

This dynamically sized array really is not a good idea. Look at the data that you will be sending this function. What is the longest string that you will be sending? Just make the array that size.

It looks to me like your C# application should really be sending an end-of-record marker, and you should be reading serial data until that end of record marker arrives. Only when it does should you use the data that you have received.

huh, never even thought of that... would I still be able to process multiple packets though?

would I still be able to process multiple packets though?

I don't understand this question. The code you posted processes one packet per call to the function. Nothing I've suggested would alter that fact.

Well, balls. that wasn't what i was aiming for at all :stuck_out_tongue:
I'm lost, is there any way you could direct me on how to process multiple packets?

I'm lost, is there any way you could direct me on how to process multiple packets?

I don't understand what you mean. Each packet should contain all the data to do something. You should read until the end of the packet arrives, then do that something.

I don't understand what you mean by "process multiple packets". Each one needs to be processed separately.

Apologies. What I'm trying to say is that everything should be processed in one call of the loop. IE: I sent 4 packets from the computer. All four packets process in the same call of loop().

All four packets process in the same call of loop().

I don't see what that is necessary, but, OK, you can make that happen. Simply put the code in loop() in a function, and call that function 4 times.

There are a number of things your code is trying to do that just is NOT going to work.
You are trying to transmit multiple types of data between the two systems, and you have no reliable identification of the types of data that are being transmitted. Not only that, but you want all these things to be handled in one call, which is still possible, but only magnifies the need to uniquely identify all the different types of data being transmitted.

For example: You have a request for analog read data that immediately reads what comes back from the Arduino and assumes it's your result from that request. Bad assumption.

In general, you have 4 different commands of varying lengths. 2 are expected results back also of varying lengths. Your serial comms need to include a few things in order to make such a scenario reliable.

  1. Unique start and end of packet markers. It is essential they be unique. In other words, these start and end of packet markers cannot be used anywhere within the packet itself.
  2. Unique headers for each of the command types and response types. The uniqueness is the same as for the start and end markers.
  3. A processing framework that either does not expect the result to be the very next packet of data received after the command is sent (flexible, but a bit more complicated to code), or strictly enforces a rigid command/response ordering for all commands (not as complicated to code, but introduces a lot of blocking and waiting, and isn't very efficient, responsive in general).

You are currently using binary serial comms, which makes a lot of the above more difficult (unique markers/headers in particular, since writing out an int value can generate any byte value output). Switching to human readable transmission makes most of the above easier, but introduces a layer of conversion/parsing. Both the Arduino Serial object and C# serial object provide provisions for converting, and both the Arduino and C# environment provide additional libraries for parsing. As a result, it's generally easier to get human readable Serial comms functional than it is to get binary Serial comms functional. It's also easier to debug human readable Serial comms since it's, well, human readable.

Sorry for the late reply. I have changed my code a bit but still cannot logically process why it is not working. The way I see it, it should work, but the way the arduino see's it it does not.

Here's the relevant C# code:

public class ArduinoCom
        {
            static SerialPort _serialPort;
            String[] messages = new String[100];
            int messagecount = 0;
            static byte[] serialData;
            StringComparer stringComparer = StringComparer.OrdinalIgnoreCase;
            Thread readThread = new Thread(Read);
            byte offByte = 253;

            byte[] pwmPins = new byte[] { 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 44, 45, 46 };
            public byte[] analogPins = new byte[46];
            public bool[] digitalPins = new bool[54];
            public byte[] servoStates = new byte[8];

            //command 0
            public void getPing()
            {
                byte[] buffer = new byte[] { 1, offByte };
                Write(buffer);
                byte[] retunValues = ReadSerial();
            }

            //command 1
            public void setPinAnalog(byte pinID, byte newState)
            {
                if (pwmPins.Contains(pinID))
                {
                    byte[] buffer = new byte[] { 2, pinID, newState, offByte };
                    Write(buffer);
                    analogPins[pinID - 1] = newState;
                }
            }

            //command 2
            public void setPinDigtal(byte pinID, bool newState)
            {
                byte[] buffer;
                if (newState)
                {
                    digitalPins[pinID] = true;
                    buffer = new byte[] { 3, pinID, 255, offByte };
                }
                else
                {
                    digitalPins[pinID] = false;
                    buffer = new byte[] { 3, pinID, 0, offByte };
                }
                Write(buffer);
            }

            //command 3
            public byte getPinAnalog(byte pinID)
            {
                byte[] buffer = new byte[] { 4, pinID, offByte };
                Write(buffer);
                byte[] returnArray = ReadSerial();
                analogPins[pinID-1] = returnArray[1];
                return returnArray[1];
            }

            //command 4
            public byte getPinDigital(byte pinID)
            {
                byte[] buffer = new byte[] { 5, pinID, offByte };
                Write(buffer);
                byte[] returnArray = ReadSerial();
                if (returnArray[1] == 255)
                    digitalPins[pinID] = true;
                else
                    digitalPins[pinID] = false;
                return returnArray[1];
            }

            //command 5
            public void setServo(byte servoID, byte dir)
            {
                byte[] buffer = new byte[] { 6, servoID, dir, offByte };
                Write(buffer);
            }

            public byte[] ReadSerial()
            {
                try
                {
                    Read();
                    if (serialData.Length > 0)
                        handleErrors(serialData);
                    return serialData;
                }
                catch (NullReferenceException)
                {
                    //debug.WriteLine("[ERROR] Data array not initialized!");
                    debug.WriteLine("[ERROR] FATAL ERROR! \nABORTING COMMUNICATION!");
                    return new byte[0];
                }
            }

            public static void Read()
            {
                try
                {
                    serialData = new byte[_serialPort.BytesToRead];
                    _serialPort.Read(serialData, 0, _serialPort.BytesToRead);
                }
                catch (NullReferenceException)
                {
                    debug.WriteLine("[ERROR] Serial not initialized!");
                }
                catch (TimeoutException)
                {
                    debug.WriteLine("Serial read timed out!");
                }
            }

            public void Write(byte[] packet)
            {
                try
                {
                    if (_serialPort != null)
                        _serialPort.Write(packet, 0, packet.Length);
                }
                catch (TimeoutException) { Console.WriteLine("Serial write timed out."); }
            }

            protected void handleErrors(byte[] data)
            {
                if (data.Length > 0)
                {
                    if (data[0] == 254)
                    {
                        debug.WriteLine("Ping");
                    }
                }
                //error messages
                if (data[0] == 255)
                {
                    if (data[1] != null)
                    {
                        //unknown
                        if (data[1] == 0)
                        {
                            debug.WriteLine("Unknown error from Arduino!");
                        }

                        //unknown
                        if (data[1] == 1)
                        {
                            debug.WriteLine("Set Pin High.");
                        }

                        //ping
                        if (data[1] == 2)
                        {
                            debug.WriteLine("~Ping~");
                        }
                    }
                    else
                    {
                        debug.WriteLine("FATAL ERROR HAS OCCURED! Closing\nserial port!");
                        _serialPort.Close();
                    }
                }
            }
        }

and the new arduino code:

#include <Servo.h> 

byte myVersion = 1;

byte incomingBuffer[64]; //holds data to be written to the serial port
Servo servos[10];
byte pointer = 0;
byte analogs[12];
boolean digitals[54];

//writes the buffer dat[] to the serial
void writeBuffer(byte dat[], int bufferSize)
{
  int i;
  for(i = 0; i < bufferSize; i++)
  {
    Serial.write(dat[i]); 
  }
}

//sets pin pinID to dat
void setPin(boolean analog, byte pinID, byte dat)
{
  //handle analog pins
  if (analog == true)
  {
    analogWrite(pinID,dat);
    analogs[pinID] = dat;
  }

  //handle digital pins 
  else
  {
    if (dat>=128)
    {
      digitalWrite(pinID,HIGH);
      digitals[pinID] = true;
    }
    else
    {
      digitalWrite(pinID,LOW); 
      digitals[pinID] = false;
    }
  }
}

//assigns a servo to a pin
void assignServo(byte servoID, byte pinID)
{
  servos[servoID].attach(pinID); 
}

//returns the state of analog pin pinID
byte getAnalogPin(byte pinID)
{
  return analogs[pinID];
}

//returns 255 if digital pin pinID is HIGH
byte getDigitalPin(byte pinID)
{
  if (digitals[pinID] == true) 
  {
    return 255; 
  }
  else
  {
    return 0; 
  }
}

//sets a servo to the setting
void setServo(Servo servo, int pos)
{
  servo.write(pos);
}

//gets a servo's rotation
byte getServo(Servo servo)
{
  return servo.read(); 
}

void setup()
{
  Serial.begin(9600);
}

void handlePacket(byte buffer[])
{
  switch (buffer[0])
  {
    //writes a ping back
  case byte(1):
    {
      Serial.write(254);
    }
    break;

    //sets an analog pin
  case byte(2):
    {
      analogWrite(buffer[1],buffer[2]);
      Serial.write(255);
      Serial.write(1);
    }
    break;

    //sets a digital pin
  case byte(3):
    {
      pinMode(buffer[1],OUTPUT);
      if (buffer[2]>=128)
      {
        digitalWrite(buffer[1],HIGH); 
      }
      else
      {
        digitalWrite(buffer[1], LOW);
      }
      
      Serial.write(255);
      Serial.write(1);
    }
    break;

    //request for an analog pin's value
  case byte(4):
    {
      Serial.write(3);
      Serial.write(getAnalogPin(buffer[1]));
    }
    break;

    //request for an digital pin's value
  case byte(5):
    {
      Serial.write(4);
      Serial.write(getDigitalPin(buffer[1]));
    }
    break;

    //sets a servo
  case byte(6):
    {
      setServo(servos[buffer[1]],buffer[2]);
    } 
    break;

  case byte(7):
    {
      assignServo(buffer[1],buffer[2]);
    }

  default:
    byte buff[2];
    buff[0]=255;
    buff[1]=0;
    Serial.write(buff,2);
    break;
  }
}

void loop()
{  
  byte dat = Serial.read();
  if (dat!=253)
  {
    incomingBuffer[pointer] = dat;
    pointer++; 
  }
  else
  {
    pointer = 0;
    handlePacket(incomingBuffer);
    for(byte i = 0; i<64;i++)
    {
      incomingBuffer[i]=0; 
    }
  }
}

I get the catch statement when trying to ping, and pin 13 is always high.

I must be missing something (or you are). I don't see where _serialPort is ever initialized. You can't expect to read from it, or write to is, without that.

Output the BytesToRead and you're going to find that there aren't any to read, even with a proper connection.

Your processor operates many many times faster than serial communications, even through the overhead of the .NET runtime engine. Not only are you not going to receive a result in the time it takes your code to get from the Write() operation to the Read() operation, but it's likely your command still hasn't even been sent by that point.

The recommended method of receiving serial data in C# is through the DataReceived event. This is because you can't really predict when the data is going to arrive.