SMS Phone

Hi all ! My first post and yes, another newbie! First time coding in C, Im a PLC man so be kind please.
I’ve built a controller for a GPRS modem for a house controller and would like some feedback please. What is the best way to take a text string, parse it and switch outputs. I have included my code which works everytime its tested, but have yet to do the control side yet.

Comments please…

SMS_Phone_Controller.pde (12.9 KB)

  if (startup == 0) { SendText("Arduino Re-booting."); startup = 1; }     // if startup = 0 we have booted. Send a text

Multiple statements on one line are hard to read. Why are you not doing this one-time-only operation in setup()?

boolean Check4Text()                                                      // Here we see if there is a text waiting
{
  delay(10);

Why is this delay here?

    int Numm=int(rx[loopcount-1]);                                        // Gets message number

After reading random amounts of text, how can you be certain that the message number is in the last character read?

      Phone_Serial.flush();                                               // Clear the serial store

You don't get to this statement until the while loop ends, and all data that was pending was read. Aside from copying the array of characters that was read into a String object, very few machine instructions have been executed, at the rate of 16000000 per second. How much data do you suppose has arrived in that time? In any case, why do you want to throw any of it away?

      int loopcount = 0; rx[0] = 0;                                       // init variables

Useful comment, huh? In loop(), you now have two different variables named loopcount. Why?

      while(Phone_Serial.available() > 0)                                 // loop to get characters in the serial buffer
      {
        rx[loopcount] = Phone_Serial.read();                              // Put recieved chars into the array
        loopcount ++;                                                     // increment counter
      }
      Phone_Serial.flush();                                               // Clear the buffers

As soon as you have read the last character, throw away any unread characters. Again, why?

void ParseText(char parse[])                                              // Now have the hard bit, stripping data
{
  String Text_RX = parse;                                                 // put the array into a string
  int firstcomma = Text_RX.indexOf(',');                                  // look for the first comma
  int secondcomma = Text_RX.indexOf(',', firstcomma + 1 );                // look for the second comma
  int thirdcomma = Text_RX.indexOf(',', secondcomma + 1 );                // look for the third comma
  int forthcomma = Text_RX.indexOf(',', thirdcomma + 1 );                 // look for the fourth comma
  int from = firstcomma + 2; int to = secondcomma - 1;                    // from and to hold where the phone no. is
  int Bypass = Text_RX.indexOf(10);                                       // Look for the first 'LINEFEED'
  int DataStreamStart = Text_RX.indexOf(10,Bypass + 1);                   // Get the 'text' index start
  int DataStreamFinish = Text_RX.indexOf(10, DataStreamStart + 1);        // Get the 'text' index finish
  String CalledByNumber = Text_RX.substring(from,to);                     // Strip out the phone No.
  String Data = Text_RX.substring(DataStreamStart+1,DataStreamFinish);    // Strip out the text
  CalledByNumber.toCharArray(Parse_from, 15);                             // put phone number in our array 'Parse_from'
  Data.toCharArray(Parse_Text,128);                                       // put phones text in our array 'Parse_Text'
}                                                                         // Phew!!!

You copied the character array into a String object so that you can re-implement the strtok function. Fine by me, but it adds a lot of unnecessary overhead. After re-implementing strtok, you then have to extract all the useful data from the String objects. All this copying is unnecessary if you simply passed this function the char array and used strtok.

    String stringOne = num2Dial;                                          // Check if its the allowed phone
    String stringTwo = Parse_from;
    if (stringOne == stringTwo)                                           // if the numbers match
    {

Now, you copy the extracted char arrays back to String objects, to avoid using strcmp. Is there a reason for this?

      delay(2000);                                                        // for 5 seconds

It's great when the comments match the code...

  String stringOne = test;
  String stringTwo = rx;
  if (stringOne == stringTwo) { return(true); } else { return(false); }

Or

return strcmp(test, rx) == 0;

Finally:

  char* alpha = err;                                                      // Get the command we errored on

Why don't you just print err?

Wow, thanks! As i said, this is my first time in this language. I might be long in the tooth but this is all new to me. (3 days!) I shall now look at your helpful suggestions and implement them. Once again PaulS, thank you! :) :)

Okay, now I get it! I was wondering where STRTOK was in the Arduino instructions. There's all the C library as well ! DOH. Got a lot to learn.......... :blush: