0

I am having trouble with a "simple" command parser; I'm guessing I'm doing something wrong with memory allocation - appreciate any pointers :)

For context: I'm running on an Arduino Uno (if it matters). The commands I expect are of the form <key1=value1;key2=value2;key3=value3>. For simplicity here (as well as for my own failed attempts at debugging), I've removed everything extraneous, so that this code just prints back to the serial monitor whatever the current value associated with the key "text" is every time a command enclosed in < > is encountered.

This seems to work correctly until after the first time the key is found; then it behaves somewhat unpredictably - haven't yet been able to discern the pattern. But, for instance, if I send:

<a=5>
-> EMPTY
<b=7;text=here;d=9>
-> here
<d=abc>
-> here
<d=ghi;e=afds>
-> afds

That last displayed value is wrong, since no new key of text was sent over serial. The complete code is here:


// command indicators
char START_MARKER = '<';
char END_MARKER = '>';

const int MAX_KEY_VALUE_PAIRS = 3;  // maximum number of key-value pairs in message
const int MAX_ELEMENT_CHARS = 25;  // the maximum number of characters (+1 for terminator) in a key or a value

// message format: <key1=value1;key2=value2;key3=value3>
const int MAX_MESSAGE_CHARS = (MAX_KEY_VALUE_PAIRS * (MAX_ELEMENT_CHARS + 1)) * 2 + (MAX_KEY_VALUE_PAIRS - 1) + 2;  // maximum message size

char received_chars[MAX_MESSAGE_CHARS];  // <- EDIT#1
char received_chars[30] ={'\0'}; 
bool new_data = false;

char *text;  // the value identified by the "text" keyword, i.e.: in <text=abc;time=814a>, text would be abc.

void ParseData(char *str) {
  // This picks off the ;-delimited key-value pairs and assigns them to a multi-dim array
  char * pch;
  int pairs_count = 0;
  char config[MAX_KEY_VALUE_PAIRS][2][MAX_ELEMENT_CHARS];

  pch = strtok(str, "=");

  while (pch != NULL)
  {
    strcpy(config[pairs_count][0], pch);
    pch = strtok(NULL, ";");
    if (pch == NULL) break;

    strcpy(config[pairs_count][1], pch);
    pairs_count++;
    pch = strtok(NULL, "=");
    if (pch == NULL) break;
  }

  for(int i=0;i<pairs_count;i++) {
    if (strcmp(config[i][0], "text")==0) text = config[i][1];    
  }
  Serial.println(text);
}

void ReadSerial(){
  // After calling ReceiveText to empty the buffer, if a complete command has been found,
  // parse that command.
  new_data = ReceiveText();
  if (new_data == true) {
    char temp_chars[MAX_MESSAGE_CHARS];  // temporary array for use when parsing
    strcpy(temp_chars, received_chars);
    received_chars[0] = '\0';  // <- EDIT#2
    ParseData(temp_chars);
  }
}

boolean ReceiveText() {
  // This dumps the characters on the buffer so far received_chars, searching for an END_MARKER
  // along the way; if it finds one, it goes back to find a START_MARKER; if that is also found,
  // the string within is the ;-delimited set of key value pairs
  static boolean recv_in_progress = false;
  static byte ndx = 0;
  char rc;

  boolean new_data = false;
  while (Serial.available() > 0 && new_data == false) {
    rc = Serial.read();

    if (recv_in_progress == true) {
      if (rc != END_MARKER) {
        received_chars[ndx] = rc;
        ndx++;
        if (ndx >= MAX_MESSAGE_CHARS) {
          ndx = MAX_MESSAGE_CHARS - 1;
        }
      } else {
        received_chars[ndx] = '\0'; // terminate the string
        recv_in_progress = false;
        ndx = 0;
        new_data = true;
      }
    } else if (rc == START_MARKER) {
        recv_in_progress = true;
    }
  }
  return new_data;
}

void setup() {  
  Serial.begin(9600);
  text = (char*)malloc(25);
  strcpy(text, "EMPTY");
  received_chars[0] = '\0';
}

void loop() {
  ReadSerial();
}

Thanks!

4
  • Are your "keys" pre-defined values? Or is this intended to allow absolutely anything to be entered as a "key"?
    – Majenko
    Apr 6 '20 at 9:53
  • At present, they're defined so as to allow anything to be entered as a key.
    – David W
    Apr 6 '20 at 10:49
  • Is that really what you want? Personally I usually define a struct to take the data and do string matching on the key to work out which entry in the struct to assign the value to. You also parse the value according to what the type is, so numbers are stored as integers, etc.
    – Majenko
    Apr 6 '20 at 11:28
  • The way you use malloc and changing the pointer of dynamic created variable will cause memory leak. As for the problem, you are picking up the memory content left by the previous buffer.
    – hcheung
    Apr 6 '20 at 11:32
0

Welcome to the I forgot to empty my buffer char club (I lost some days finding a simlar problem)

 strcpy(temp_chars, received_chars)  // strcpy empties temp_chars and adds the received_chars

but you have no (for me) visible routine to empty received_chars
Do this either by setting

received_chars[0] ='\0'; // set the pointer to first index

or

 received_chars ="0";

or

strcpy(received_chars,"0");

after you used the buffer content. By using

static byte ndx = 0;

you just set the pointer and then start to write, as a safequard you should first reset the end terminator and then start to write. This is probably the reason you see characters even if no key is pressed due to a still "full" charbuffer.
To prevent heap corruption define your received_chars as a fixed arraybefore setup:

char received_chars[128] ={'\0'}; // 127 chars + terminator

if you use malloc you have to use free, but doing that with a lot of dynamic data causes heap fracturing -> crash in the end

EDIT
The program does now the following

  • ignores all char not enclosed by <>
  • check for text and displays everything after = and before;
  • ignores everything after ; if its not text= again
  • if its text= again ignores the first and only displays the second param

You do not need strtok here as it deletes chars I use a modified substring routne for char arrays. Here is the code working as described above:

// command indicators
const char START_MARKER = '<';
const char END_MARKER = '>';

const uint8_t MAX_KEY_VALUE_PAIRS = 3;  // maximum number of key-value pairs in message
const uint8_t MAX_ELEMENT_CHARS = 25;  // the maximum number of characters (+1 for terminator) in a key or a value

// message format: <key1=value1;key2=value2;key3=value3>
const uint8_t MAX_MESSAGE_CHARS = (MAX_KEY_VALUE_PAIRS * (MAX_ELEMENT_CHARS + 1)) * 2 + (MAX_KEY_VALUE_PAIRS - 1) + 2;  // maximum message size

//char received_chars[MAX_MESSAGE_CHARS] = {'\0'};// <- EDIT#1
char received_chars[256] = {'\0'};
bool new_data = false;
char text[25] =  {'\0'};      // the value identified by the "text" keyword, i.e.: in <text=abc;time=814a>, text would be abc.

void ParseData(char *str) {
  // This picks off the ;-delimited key-value pairs and assigns them to a multi-dim array
  char * pch;
  int pairs_count = 0;
  char config[MAX_KEY_VALUE_PAIRS][2][MAX_ELEMENT_CHARS];

  pch = strtok(str, "=");

  while (pch != NULL)  {
    strcpy(config[pairs_count][0], pch);
    pch = strtok(NULL, ";");
    if (pch == NULL) break;

    strcpy(config[pairs_count][1], pch);
    pairs_count++;
    pch = strtok(NULL, "=");
    if (pch == NULL) break;
  }

  for (uint8_t i = 0; i < pairs_count; i++) {
    if (strcmp(config[i][0], "text") == 0) strcpy (text,config[i][1]);
  }
  Serial.println(text);
}

void ReadSerial() {
  // After calling ReceiveText to empty the buffer, if a complete command has been found,
  // parse that command.
  new_data = ReceiveText();
  if (new_data == true) {
    char temp_chars[MAX_MESSAGE_CHARS];  // temporary array for use when parsing
    strcpy(temp_chars, received_chars);
    received_chars[0] = '\0';  // <- EDIT#2
    ParseData(temp_chars);
  }
}

bool ReceiveText() {
  // This dumps the characters on the buffer so far received_chars, searching for an END_MARKER
  // along the way; if it finds one, it goes back to find a START_MARKER; if that is also found,
  // the string within is the ;-delimited set of key value pairs
  static boolean recv_in_progress = false;
  static byte ndx = 0;
  char rc;

  bool new_data = false;
  while (Serial.available() > 0 && new_data == false) {
    rc = Serial.read();

    if (recv_in_progress == true) {
      if (rc != END_MARKER) {
        received_chars[ndx] = rc;
        ndx++;
        if (ndx >= MAX_MESSAGE_CHARS) {
          ndx = MAX_MESSAGE_CHARS - 1;
        }
      } else {
        received_chars[ndx] = '\0'; // terminate the string
        recv_in_progress = false;
        ndx = 0;
        new_data = true;
      }
    } else if (rc == START_MARKER) {
      recv_in_progress = true;
    }
  }
  return new_data;
}

void setup() {
  Serial.begin(115200);
   strcpy (text, "EMPTY"); 
 //  text = (char*)malloc(25);
//  received_chars[0] = '\0';
}

void loop() {
  ReadSerial();
}

If you could describe what you really want in the end I'm sure we can help with some basic routines.

5
  • Thanks for the idea - I've made those changes, both to initialize in setup, and immediately after the strcopy, with the received_chars[0] = '\0'; suggestion. Alas, I still get the same bad behavior. But perhaps I misunderstood your suggestion.
    – David W
    Apr 6 '20 at 11:24
  • Please edit your question with the updated code Apr 6 '20 at 11:31
  • Done - yes, that probably helps you see exactly what I did (wrong) :P
    – David W
    Apr 6 '20 at 17:05
  • Thank you - following your code highlighted my errant ways. Specifically, once I found the keyword "text", I was using text = config[i][1];, whereas I should have been using strcopy(text, config[i][1];. Appreciate the help!
    – David W
    Apr 6 '20 at 21:56
  • Please mark as solved/accepted - Thanks Apr 7 '20 at 7:21
0

The problem was that, in the variables I wanted to keep (i.e.: text in the code below), I was assigning the value of a pointer to the parsed string which is reused the next time a command is found, rather than copying the string.

Fixed code below - see comment Fig#1. There's also a second fix, although that wasn't causing the problem behavior.

// command indicators
char START_MARKER = '<';
char END_MARKER = '>';

const int MAX_KEY_VALUE_PAIRS = 3;  // maximum number of key-value pairs in message
const int MAX_ELEMENT_CHARS = 25;  // the maximum number of characters (+1 for terminator) in a key or a value

// message format: <key1=value1;key2=value2;key3=value3>
const int MAX_MESSAGE_CHARS = (MAX_KEY_VALUE_PAIRS * (MAX_ELEMENT_CHARS + 1)) * 2 + (MAX_KEY_VALUE_PAIRS - 1) + 2;  // maximum message size

char received_chars[MAX_MESSAGE_CHARS];
bool new_data = false;

char *text;  // the value identified by the "text" keyword, i.e.: in <text=abc;time=814a>, text would be abc.

void ParseData(char *str) {
  // This picks off the ;-delimited key-value pairs and assigns them to a multi-dim array
  char * pch;
  int pairs_count = 0;
  char config[MAX_KEY_VALUE_PAIRS][2][MAX_ELEMENT_CHARS];

  pch = strtok(str, "=");

  while (pch != NULL)
  {
    strcpy(config[pairs_count][0], pch);
    pch = strtok(NULL, ";");
    if (pch == NULL) break;

    strcpy(config[pairs_count][1], pch);
    pairs_count++;
    pch = strtok(NULL, "=");
    if (pch == NULL) break;
  }

  for(int i=0;i<pairs_count;i++) {
    if (strcmp(config[i][0], "text")==0) strcpy(text, config[i][1]);  // Fix#1
  }
  Serial.println(text);
}

void ReadSerial(){
  // After calling ReceiveText to empty the buffer, if a complete command has been found,
  // parse that command.
  new_data = ReceiveText();
  if (new_data == true) {
    char temp_chars[MAX_MESSAGE_CHARS];  // temporary array for use when parsing
    strcpy(temp_chars, received_chars);
    received_chars[0] = '\0';  // Fix #2 - wasn't causing this problem, but still was wrong
    ParseData(temp_chars);
  }
}

boolean ReceiveText() {
  // This dumps the characters on the buffer so far received_chars, searching for an END_MARKER
  // along the way; if it finds one, it goes back to find a START_MARKER; if that is also found,
  // the string within is the ;-delimited set of key value pairs
  static boolean recv_in_progress = false;
  static byte ndx = 0;
  char rc;

  boolean new_data = false;
  while (Serial.available() > 0 && new_data == false) {
    rc = Serial.read();

    if (recv_in_progress == true) {
      if (rc != END_MARKER) {
        received_chars[ndx] = rc;
        ndx++;
        if (ndx >= MAX_MESSAGE_CHARS) {
          ndx = MAX_MESSAGE_CHARS - 1;
        }
      } else {
        received_chars[ndx] = '\0'; // terminate the string
        recv_in_progress = false;
        ndx = 0;
        new_data = true;
      }
    } else if (rc == START_MARKER) {
        recv_in_progress = true;
    }
  }
  return new_data;
}

void setup() {  
  Serial.begin(9600);
  text = (char*)malloc(25);
  strcpy(text, "EMPTY");
  received_chars[0] = '\0';
}

void loop() {
  ReadSerial();
}

Thanks to Codebreaker007 for the help.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.