Errors in my code. Help Please !

Using Arduino Programming Questions
1

Evening All
Im trying to pars a string received through the serial port. The fields are separated by # and the command ends with a .
Ive cut the code down to make it more readable but the problems are the same. the 'sdt#1234' command only has 2 fields in it and it seems to work. (Although something freezes up occasionally).
The second command 'ov#1#345*' , usually the 'ov' part is identified, but the program mostly seems to freeze while extracting the '1' and has never got as far as the 3rd field.
Ive done VERY little embedded programming although have an amount of experience with VC++. (I really miss my de-bug environment !!).
Id be very greatfull if anyone can help me out with this, its driving me nuts !!!!!
the code below will compile and run if you have the inclination. the strings Im sending down the serial port are
sdt#1234*
ov#1#345*

Thanks Very much in advance for your time !
Phil

/* COMMS MESSAGE

sdt#VALUE*                                       Sets Sequence Delay to a given value *=End Marker
dc#DRIP_ID#START|STOP(1 | 0)#FORM_TIME#DELAY*    Drip Control Empty field = no change
sys#START|STOP(1 | 0)*                           Start or stop running
dd#NUMBER*                                       do 'number' amount of drips
ov#TIME*                                         open valve for TIME ms

*/
  struct open_valve_struct{
    int open_valve;
    int run_time;
  } Open_valve = {0,5000};

  struct do_drip_struct{
    boolean do_drip;
    int drip_number;
  } Do_Drip = {0,5};    

  struct drip_struct{
    boolean active;
    int drip_delay;
    int drip_form_time;
  } drip_1 = {1,0,30},
    drip_2 = {0,85,30},
    drip_3 = {0,85,30},
    drip_4 = {0,85,30};
    
    drip_struct* DripArray[] = {&drip_1,&drip_2,&drip_3,&drip_4};
  
  int ValvePin = 3; 
  unsigned long Sequence_Delay = 3500; //ms
  int DripsInSequence = 1;
  String CmdInStr; 
  boolean MsgComplete = false;
  int Running = 0;
  boolean Ready = false;
  long previousMillis = 0;
  boolean TimeToGo = false;
  String MsgDest;

void setup()
{
   // initialize serial communication:
   Serial.begin(9600);
   Serial.flush();
  
   pinMode(ValvePin, OUTPUT);
   digitalWrite(ValvePin, LOW);
}

void ReadSerialPort()
{
    while (Serial.available()&& MsgComplete == false)
    {
      char c = Serial.read();
      CmdInStr += c;
      if(c == '*')//msg end
       MsgComplete = true;
    } //while

}

void GetActionCmd()
{
  String tmpStr1;
  int pos1 = CmdInStr.indexOf('#');
  if(pos1 == -1)  
    pos1 = CmdInStr.indexOf('*');
  MsgDest = CmdInStr.substring(0,pos1);
  tmpStr1 = CmdInStr.substring(pos1 + 1, CmdInStr.length());
  CmdInStr = tmpStr1;
  Serial.print(F("CmdInStr =  "));Serial.print(CmdInStr);Serial.print(F("\r\n"));
  tmpStr1 = "";
}

void ActionMessage()
{
  
  int drip_id =0;
  void* tp;
 
  MsgDest = "";
  GetActionCmd();

  if(MsgDest == "sdt")//sequ delay time
  {
    GetActionCmd();
    tp = malloc(MsgDest.length());
    MsgDest.toCharArray((char*)tp,MsgDest.length()+1);
    Sequence_Delay = atoi((char*)tp);
    Serial.print(F("Sequence_Delay =  "));Serial.print(Sequence_Delay);Serial.print(F("\r\n"));
    CmdInStr = "";
    free((void*) tp);
    MsgDest = "";
  }

   if(MsgDest == "ov")//open valve
  {
      Serial.print("ov \r\n");
    GetActionCmd();
    tp = malloc(MsgDest.length());
    MsgDest.toCharArray((char*)tp,MsgDest.length()+1);
    Open_valve.open_valve = atoi((char*)tp);
    Serial.print(F(" Open_valve.open_valve =  "));Serial.print( Open_valve.open_valve);Serial.print(F("\r\n"));
    CmdInStr = "";
    free((void*) tp);
    MsgDest = "";
    
    GetActionCmd();
    tp = malloc(MsgDest.length());
    MsgDest.toCharArray((char*)tp,MsgDest.length()+1);
    Open_valve.run_time = atoi((char*)tp);
    Serial.print(F("Open_valve.run_time =  "));Serial.print(Open_valve.run_time);Serial.print(F("\r\n"));
    CmdInStr = "";
    free((void*) tp);
    MsgDest = "";
  }   

 MsgComplete = false; // re-set so new commands can be recieved
}

void loop()
{
  
  ReadSerialPort();
  if (MsgComplete == true)
  {
    ActionMessage();
    MsgComplete = false;
    Serial.print(F(". .... \r\n"));
  }
}
2

Without looking in great detail my guess is you are running out of memory. The String class, especially when you concatenate bytes at a time, is a big memory hog.

Try using a static char buffer, adding to that, and then processing.

As described here, for example:

3 
    while (Serial.available()&& MsgComplete == false)
    {
      char c = Serial.read();
      CmdInStr += c;
      if(c == '*')//msg end
       MsgComplete = true;
    } //while

If you see a * in the incoming stream of data, make note of that fact and keep reading. Well, OK. I would think that you would want to break out of the while loop if you found an end of packet marker.

4

@Paul:

    while (Serial.available()&& MsgComplete == false)
5

Well, OK, that works. I use break, instead. Seems clearer, to me.

6

Thanks for the reply guys...
Memory.... ummm, I guess it could be. I've tried to be careful lol but I'm used to PC programming. Proper Bloat ware !
Ill try and re structure it, see what happens !

Cheers