working with data from serial works then doesn't

Projects Programming
21

What I am looking for is to have a set of variables set to 'something' when the program starts and then as input from serial comes in those start-up values are replaced with the new values from serial.

Below is some code for controlling servos via position commands received by the serial port.

//zoomkat 11-22-12 simple delimited ',' string parse 
//from serial port input (via serial monitor)
//and print result out serial port
//multi servos added 
// Powering a servo from the arduino usually *DOES NOT WORK*.

String readString;
#include <Servo.h> 
Servo myservoa, myservob, myservoc, myservod;  // create servo object to control a servo 

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

  //myservoa.writeMicroseconds(1500); //set initial servo position if desired

  myservoa.attach(6);  //the pin for the servoa control
  myservob.attach(7);  //the pin for the servob control
  myservoc.attach(8);  //the pin for the servoc control
  myservod.attach(9);  //the pin for the servod control 
  Serial.println("multi-servo-delimit-test-dual-input-11-22-12"); // so I can keep track of what is loaded
}

void loop() {

  //expect single strings like 700a, or 1500c, or 2000d,
  //or like 30c, or 90a, or 180d,
  //or combined like 30c,180b,70a,120d,

  if (Serial.available())  {
    char c = Serial.read();  //gets one byte from serial buffer
    if (c == ',') {
      if (readString.length() >1) {
        Serial.println(readString); //prints string to serial port out

        int n = readString.toInt();  //convert readString into a number

        // auto select appropriate value, copied from someone elses code.
        if(n >= 500)
        {
          Serial.print("writing Microseconds: ");
          Serial.println(n);
          if(readString.indexOf('a') >0) myservoa.writeMicroseconds(n);
          if(readString.indexOf('b') >0) myservob.writeMicroseconds(n);
          if(readString.indexOf('c') >0) myservoc.writeMicroseconds(n);
          if(readString.indexOf('d') >0) myservod.writeMicroseconds(n);
        }
        else
        {   
          Serial.print("writing Angle: ");
          Serial.println(n);
          if(readString.indexOf('a') >0) myservoa.write(n);
          if(readString.indexOf('b') >0) myservob.write(n);
          if(readString.indexOf('c') >0) myservoc.write(n);
          if(readString.indexOf('d') >0) myservod.write(n);
        }
         readString=""; //clears variable for new input
      }
    }  
    else {     
      readString += c; //makes the string readString
    }
  }
}
22

Ok, so I tried this all again. It works better now.

#define debug 1
#define holdingAreaSize 26
#define strtokTokens ".,| "
#define smallStorageSize 8

char copyOfSerialHoldingArea[holdingAreaSize];
char serialHoldingArea[holdingAreaSize];
char firstStrtokData[smallStorageSize] = "empty";
char secondStrtokData[smallStorageSize] = "empty";
char thirdStrtokData[smallStorageSize] = "empty";
int bufferReadingPosition = 0;
boolean shouldPrint = true;


void setup() {
  Serial.begin(9600);
  Serial.println ("<Arduino Active>");
}

void loop() {
  if (shouldPrint == true){
    outputDataOnMonitor();
  }
  checkSerialForData();

}
void checkSerialForData(){
  while (Serial.available() > 0){
    char charBuffer = Serial.read();
    if (charBuffer == '\n'){
      serialHoldingArea[bufferReadingPosition] = '\0';
      bufferReadingPosition = 0;
      strlcpy(copyOfSerialHoldingArea, serialHoldingArea, sizeof(copyOfSerialHoldingArea));
      splitDataReceived(copyOfSerialHoldingArea);
    }
    else if (bufferReadingPosition < (holdingAreaSize)){
      serialHoldingArea[bufferReadingPosition++] = charBuffer;
    }
  }
}// End checkSerialForData()

void splitDataReceived(char * dataToProcess){
  char* temp1 = strtok(dataToProcess, strtokTokens);
  strlcpy (firstStrtokData, temp1, sizeof(firstStrtokData));
  //free(temp1);
  char* temp2 = strtok(NULL,strtokTokens);
  strlcpy (secondStrtokData, temp2, sizeof(secondStrtokData));
  //free(temp2);
  char* temp3 = strtok(NULL,strtokTokens);
  strlcpy (thirdStrtokData, temp3, sizeof(thirdStrtokData));
  //free(temp3);
  outputDataOnMonitor();
}//End splitDataReceived

void outputDataOnMonitor(){
  Serial.print ("A= ");
  Serial.print (firstStrtokData);
  Serial.print (" B= ");
  Serial.print (secondStrtokData);
  Serial.print (" C= ");
  Serial.println (thirdStrtokData);
  shouldPrint = false;
  
  #if debug
  Serial.print("Free Ram = "); 
  Serial.println(freeRam(), DEC);
  #endif
  
}//End outputDataOnMonitor

int freeRam () 
{
  extern int __heap_start, *__brkval; 
  int v; 
  return (int) &v - (__brkval == 0 ? (int) &__heap_start : (int) __brkval); 
}

Questions I have now:
First, is this the proper way to take strtok output and put into a regular array?

char* temp1 = strtok(dataToProcess, strtokTokens);
  strlcpy (firstStrtokData, temp1, sizeof(firstStrtokData));

I initially tried having the strtok() output go directly into my firstStrtokData[8] array. This would not compile due to type mismatch char* to char[8]. So I tried with this method.
I found I could cause havoc in the output when using strncpy() if either my serialHoldingArea array was at it's limit or if say thirdStrtokData array got passed something too large so no NULL termination was put on the string. Strlcpy() seemed to address that.

My second question is about the same area of code.

void splitDataReceived(char * dataToProcess){
  char* temp1 = strtok(dataToProcess, strtokTokens);
  strlcpy (firstStrtokData, temp1, sizeof(firstStrtokData));
  //free(temp1);
  char* temp2 = strtok(NULL,strtokTokens);
  strlcpy (secondStrtokData, temp2, sizeof(secondStrtokData));
  //free(temp2);
  char* temp3 = strtok(NULL,strtokTokens);
  strlcpy (thirdStrtokData, temp3, sizeof(thirdStrtokData));
  //free(temp3);
  outputDataOnMonitor();
}//End splitDataReceived

The example I read suggesting using a temporary pointer to hold strtok() output said that free() would need to be used on the temporary pointer so it would not eat up memory. But the example didn't really specify how to implement that.
If I uncomment the free(temp1) free(temp2) and free(temp3) lines the program works correctly once. But then further serial input seems to be ignored.
However when commented out like it is now the program seems to be behaving fine. I can enter new input many times and it will continue to update on my serial output.
So is free() something that can only be used once and then that variable can not be reused?
Or should I have free() in a different spot, or be using something other than free()?

And I have no idea if the debug freeram() thing works or not. I saw it in an example code and the author seemed to be saying that it would monitor available ram as the program went along. So I put it in to see if not having the free() working would cause the available ram to lower. I went through 10+ serial inputs and the number didn't change. But that doesn't necessarily mean all is well.

23

Your code is so complicated, it makes my brain hurt.

This is what I would do.

#define BUF1 70   // arbitrary sizes for input buffer  and words A B C
#define BUF2 30

char buf[BUF1] ;
char words[3][BUF2] ;
int p=0 ;

void loop( )
{
     bool eol=false ;
     char c;
     while( Serial.available()>0 && p<BUF1 )
     {
         c = Serial.read();
         buf[p++]=c ;
         if ( c == '\n' ) break ;
     }
     if ( p == BUF1 ) Serial.println("Input buffer overflow!");
     if ( c=='\n' ) 
     {
          char* q = &buf[0] ;
          int w=0 ;
          intj=0 ;
          while ( w < 3 && q!=&buf[p] )
          {
              if ( *q == ',' || *q == '.' || *q='|' )
              {
                  words[w][j]='\0' ;
                  w++ ;
                  j=0 ;
              }
              else
              {
                    if ( j < BUF2 ) words[w][j++] = *q ;
                    else  Serial.println("Word buffee overflow!");
              }
              q++;

              if ( w == 3 )
              {
                  for ( int k=0 ; k<3 ; k++ ) Serial.println( &words[k][0] );
                  p=0 ;  // ignore anything else on the line
              }
          }
     }
     else delay(10) ;   // wait for more data
}
24

michinyon:
Your code is so complicated, it makes my brain hurt.

This is what I would do.

You are probably very experienced with programming. So what you wrote made sense to you. It is probably super efficient and works great(if fixed).

However, I am new to programming. I have been learning for about the last 2 months (mostly in C#).
I think know what your code is supposed to do in general(assuming it has a similar result to what mine does). But as far as understanding it, I can't even fix the things to get it to compile to see if it works.

It first stuck on intj=0;

Hey, I kinda see maybe that's a typo so I change it to int j=0;

But now it is stuck here...

 if ( *q == ',' || *q == '.' || *q='|' )

with the error

Arduino: 1.6.4 (Windows 8.1), Board: "Arduino Uno"

testingVariPlacement.ino: In function 'void loop()':
testingVariPlacement:27: error: lvalue required as left operand of assignment
lvalue required as left operand of assignment

I figure it is either they are supposed all have = and not == or just the opposite of that. But since I only have a vague idea that this line might somehow be about the tokens for strtok() to read to, I don't know what to do to 'fix' it. And I could be completely wrong about what the error is and what this line of code is for. Especially since I don't see strtok() anywhere in the code so I have no idea how the serial data is going to be parsed and then copied into the words array.

Honestly to a person as inexperienced with programming as me it looks like a cat walked on a keyboard instead of being computer code. To an experienced coder it probably makes sense. I dunno.

I assume you are tying to help, and I do appreciate that.
But your example is not helpful because I simply can't understand it.

25 
 if ( *q == ',' || *q == '.' || *q='|' )

He meant ==. As in:

 if ( *q == ',' || *q == '.' || *q=='|' )

Clearly that line is comparing things, so == is appropriate here.

26 
  else delay(10) ;   // wait for more data

Why wait?

Honestly to a person as inexperienced with programming as me it looks like a cat walked on a keyboard instead of being computer code. To an experienced coder it probably makes sense. I dunno.

Read my page Gammon Forum : Electronics : Microprocessors : How to process incoming serial data without blocking

Also Gammon Forum : Electronics : Microprocessors : State machines

Your code is incredibly complex. No wonder it doesn't work. I would rewrite it, in your shoes. And not just because I have been programming for a while.

There have been times in the past when I have dug a deep hole in writing code that is:

  • complex
  • doesn't work

There are times when you say "when you are deep in a hole, stop digging". This is one of those times. I've rewritten code, with the benefit of knowing what doesn't work. You can too.

27

But the code I posted does work as written.
I simply asked if I copied the data in to the real arrays(that you told me to use instead of pointers) in a semi-efficient way. Basically you said use arrays(which was helpful) but didn't mention how to copy strtok() output to them(which left me needing to figure out how that was accomplished and not being able to figure that out before is how I ended up using the pointers /sigh). So I was asking if my copy method was ok to use. It seems to work just dandy.
I also asked about how to properly impliment free() since it was mentioned in someone else's post that memory might be an issue eventually. Most examples don't seem to bother with it, but I tried to use it and posted my results.
You say my code is too complicated. To me it seems much simpler than the last example provided that didn't work as written and I didn't understand it anyway.

And the delay was not my last code. I used a simple bool switch to display my initial values only once without needing delay or millis().

28 
 char* temp2 = strtok(NULL,strtokTokens);

strlcpy (secondStrtokData, temp2, sizeof(secondStrtokData));

strtok can return NULL. You should not do a strlcpy from a NULL pointer. At the very least, test for the NULL.

29

Thank you for the information.

Is it just strlcpy() where this is a concern or would strncpy() or strcpy() have the same pitfall?

Also would this be a sufficient way to test for NULL?

char* temp2 = strtok(NULL,strtokTokens);
  if(strlen(temp2) > 0){
  strlcpy (secondStrtokData, temp2, sizeof(secondStrtokData));
  }

Also since I used strlcpy() where I made my copy of the input from serial I added the same type of check there as well. Would that be the proper thing to do?

if (charBuffer == '\n' && (strlen(serialHoldingArea)) > 0)

I have a feeling using strlen() is probably bad since it seems that is just the way my luck goes :sunglasses:
But again, thank you.

30

Is it just strlcpy() where this is a concern or would strncpy() or strcpy() have the same pitfall?

Anything that expects a non-NULL pointer.

Also would this be a sufficient way to test for NULL?

No, because strlen expects a non-NULL pointer. More like:

char* temp2 = strtok(NULL,strtokTokens);
if (temp2 == NULL)
  ...   // problem
31

So would this code cover everything?
Or is it too much?

char* temp2 = strtok(NULL,strtokTokens);
  if(temp2 != NULL){
  strlcpy (secondStrtokData, temp2, sizeof(secondStrtokData));
  }
  else if(temp2 == NULL){
    temp2 = "error";
    strlcpy (secondStrtokData, temp2, sizeof(secondStrtokData));

See I'm thinking that if I had left the !=NULL bit out of an if statement the code would go right into the strlcpy() even if strtok() returned a NULL.
And now that my former 2 lines of code is now 6 lines....yeah it's looking a bit complicated lol
And I freely admit I could not figure out how to set secondStrtokData = "error"; directly without using a for loop and that made it look even more complicated.

And I have learned more in this thread than I did trying google a lot of this.
So thank you all.

32 
 if(temp2 != NULL){
  strlcpy (secondStrtokData, temp2, sizeof(secondStrtokData));
  }
  else if(temp2 == NULL){

You don't need to test for something and then its opposite. Just use "else":

 if(temp2 != NULL){
  strlcpy (secondStrtokData, temp2, sizeof(secondStrtokData));
  }
  else {
...