Parsing comma-separated floats and semicolon-delimited commands

Question

I wrote a cstring parser. It has to work with a relatively wide amount of arguments (usually 3 or 4, but maybe in future with a different amount), which are separated by an , or ;. My wish is to make at least Function 1 less static and save some code. However, I don't really want to use dynamic memory allocation (just if it not to avoid and would bring a benefit). The whole code should run on ATMega2560. This is why I cannot use higher library functions.

Function 1:

float *Receiver::parse_pid_substr(char* buffer) {
  static float pids[8];
  memset(pids, 0, 8*sizeof(float) );

  char cI[32], cII[32], cIII[32], cIV[32], cV[32], cVI[32], cVII[32], cVIII[32];
  size_t i = 0, c = 0, p = 0;
  for(; i < strlen(buffer); i++) {
    if(buffer[i] == '\0') {
      break;
    }
    else if(buffer[i] != ',') {
      switch(p) {
        case 0:
          cI[c] = buffer[i];
          cI[c+1] = '\0';
          break;
        case 1:
          cII[c] = buffer[i];
          cII[c+1] = '\0';
          break;
        case 2:
          cIII[c] = buffer[i];
          cIII[c+1] = '\0';
          break;
        case 3:
          cIV[c] = buffer[i];
          cIV[c+1] = '\0';
          break;
        case 4:
          cV[c] = buffer[i];
          cV[c+1] = '\0';
          break;
        case 5:
          cVI[c] = buffer[i];
          cVI[c+1] = '\0';
          break;
        case 6:
          cVII[c] = buffer[i];
          cVII[c+1] = '\0';
          break;
        case 7:
          cVIII[c] = buffer[i];
          cVIII[c+1] = '\0';
          break;
      }
      c++;
    } else {
      p++;
      c = 0;
      continue;
    }
  }

  pids[0] = atof(cI);
  pids[1] = atof(cII);
  pids[2] = atof(cIII);
  pids[3] = atof(cIV);
  pids[4] = atof(cV);
  pids[5] = atof(cVI);
  pids[6] = atof(cVII);
  pids[7] = atof(cVIII);

  return pids;
}

Function 2:

bool Receiver::parse_pid_conf(char* buffer) {
  if(m_pHalBoard == NULL) {
    return false;
  } 
  else if(m_rgChannelsRC[2] > RC_THR_MIN) {        // If motors run: Do nothing!
    return false;
  }

  // process cmd
  bool bRet = false;
  char *str = strtok(buffer, "*");                  // str = roll, pit, thr, yaw
  char *chk = strtok(NULL, "*");                    // chk = chksum

  if(verf_chksum(str, chk) ) {                    // if chksum OK
    char *cstr;

    for(uint_fast8_t i = 0; i < PID_ARGS; i++) {
      if(i == 0)
        cstr = strtok (buffer, ";");
      else cstr = strtok (NULL, ";");

      float *pids = parse_pid_substr(cstr);
      switch(i) {
      case 0:
        m_pHalBoard->m_rgPIDS[PID_PIT_RATE].kP(pids[0]);
        m_pHalBoard->m_rgPIDS[PID_PIT_RATE].kI(pids[1]);
        m_pHalBoard->m_rgPIDS[PID_PIT_RATE].imax(pids[2]);
        break;
      case 1:
        m_pHalBoard->m_rgPIDS[PID_ROL_RATE].kP(pids[0]);
        m_pHalBoard->m_rgPIDS[PID_ROL_RATE].kI(pids[1]);
        m_pHalBoard->m_rgPIDS[PID_ROL_RATE].imax(pids[2]);
        break;
      case 2:
        m_pHalBoard->m_rgPIDS[PID_YAW_RATE].kP(pids[0]);
        m_pHalBoard->m_rgPIDS[PID_YAW_RATE].kI(pids[1]);
        m_pHalBoard->m_rgPIDS[PID_YAW_RATE].imax(pids[2]);
        break;
      case 3:
        m_pHalBoard->m_rgPIDS[PID_THR_ACCL].kP(pids[0]);
        m_pHalBoard->m_rgPIDS[PID_THR_ACCL].kI(pids[1]);
        m_pHalBoard->m_rgPIDS[PID_THR_ACCL].imax(pids[2]);
        break;
      case 4:
        m_pHalBoard->m_rgPIDS[PID_PIT_STAB].kP(pids[0]);
        m_pHalBoard->m_rgPIDS[PID_ROL_STAB].kP(pids[1]);
        m_pHalBoard->m_rgPIDS[PID_YAW_STAB].kP(pids[2]);
        m_pHalBoard->m_rgPIDS[PID_THR_STAB].kP(pids[3]);
        bRet = true;
        break;
      }
    }
  }
  return bRet;
}

Answer 1

Two-dimesional array

Your first function can be aggressively reduced by replacing cI and friends by a 2D array:

float *Receiver::parse_pid_substr(char* buffer) {
  static float pids[8];
  memset(pids, 0, 8*sizeof(float) );

  char rgcPIDS[8][32];
  size_t i = 0, c = 0, p = 0;
  for(; i < strlen(buffer); i++) {
    if(buffer[i] == '\0') {
      break;
    }
    else if(buffer[i] != ',') {
      rgcPIDS[p][c] = buffer[i];
      rgcPIDS[p][c+1] = '\0';
      c++;
    } else {
      p++;
      c = 0;
      continue;
    }
  }

  for (int j = 0 ; j < 8 ; ++j) {
    pids[j] = atof(chars[j]);
  }

  return pids;
}

continue

In the following piece of code:

} else {
  p++;
  c = 0;
  continue;
}

The else clause is the last of your loop, and continue is the last instruction of that clause. You can remove continue: even without it, your function will still work and behave the same way as before.

Zero-initialization

Instead of using memset to zero-initialize your arrays, you can use the language zero-initialization for arrays whose bound is known at compile-time:

char rgcPIDS[8][32] = { 0 };

Names

The names you use are all but explicit. While i is good as a loop iterator, c and p should have more explicit names.

NOTE: Hadn't it been for the Receiver::, I would have thought that your code was written in plain old C, and not in C++. Is it intentional?

Answer 2

At the top, you are declaring eight 32-character arrays, which later you use a switch with to decide which array to use. Instead, you should declare an array of arrays, the Morwenn did it, or you can use just one 256-character array. You can then just add a multiple of the p to the index in the first big switch block, reducing it to only one case:

cArrays[p<<5 + c] = buffer[i];
cArrays[p<<5 + c+1] = '\0';

There are a number of this type of optimization that you can do in your code.

Answer 3

Without going into too much detail about your concrete code (I agree with the others that an array makes a lot more sense), I'd like to point out that many parser generators (flex, bison, re2c) generate essentially standalone code and don't need runtime library support. You may need to move some of the tables they generate from RAM to PROGMEM (i.e. flash), but other than that, such parsers should work fine.