Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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.

Suggestion are welcome.

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;
}

Edit - new Function 1:

float *Receiver::parse_pid_substr(char* buffer) {
  const uint_fast8_t iSize    = 4;
  static float rgfPIDS[iSize] = { 0.f, 0.f, 0.f, 0.f };
  memset(rgfPIDS, 0, sizeof(rgfPIDS) );

  char rgcPIDS[iSize][32];
  memset(rgcPIDS, 0, sizeof(rgcPIDS) );

  size_t i = 0, iPIDcstr = 0, iPID = 0;
  for(; i < strlen(buffer); i++) {
    // String ended here
    if(buffer[i] == '\0') {
      break;
    }
    // Avoid buffer overflow
    else if(iPID >= iSize) {
      break;
    }
    // Reached new variable; Go over to next char
    else if(buffer[i] == ',') {
      iPID++;
      iPIDcstr = 0;
      continue;
    }
    // Read the current variable
    else {
      rgcPIDS[iPID][iPIDcstr]   = buffer[i];
      rgcPIDS[iPID][++iPIDcstr] = '\0';
    }
  }
  for (int i = 0; i < iSize; i++) {
    rgfPIDS[i] = atof(rgcPIDS[i]);
  }
  return rgfPIDS;
}
share|improve this question
    
Should really open this up to C developers. Nothing C++ about it. –  Loki Astari Mar 21 at 19:19
    
@LokiAstari It may be idiomatically closer to C than C++, but the method signature makes it definitely C++, as the :: makes it fail to parse as C. –  200_success Mar 22 at 4:39
    
@200_success: True. But that will not help the OP. What he rely need is C help not C++ help. The function signatures are just 3 lines and play no significant part in the code. Thus you are preventing qualified C people from spotting it and helping by applying the C++ tag and not the C tag. –  Loki Astari Mar 22 at 6:36

3 Answers 3

up vote 7 down vote accepted

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?

share|improve this answer
    
Could you please adjust the position of the braces at the last for loop to be consistent with the rest of the code? It is really bothering me. –  AJMansfield Mar 20 at 15:03
    
@AJMansfield Of course. It's true that I didn't pay any attention to it. –  Morwenn Mar 20 at 15:17
    
Yeah in this case it is desired to use C instead of C++ for string handling. –  dgrat Mar 20 at 15:47

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.

share|improve this answer

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.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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