11
\$\begingroup\$

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; }
\$\endgroup\$
3
  • \$\begingroup\$ Should really open this up to C developers. Nothing C++ about it. \$\endgroup\$ Commented Mar 21, 2014 at 19:19
  • \$\begingroup\$ @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. \$\endgroup\$ Commented Mar 22, 2014 at 4:39
  • \$\begingroup\$ @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. \$\endgroup\$ Commented Mar 22, 2014 at 6:36

3 Answers 3

Reset to default
8
\$\begingroup\$

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?

\$\endgroup\$
3
  • \$\begingroup\$ 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. \$\endgroup\$ Commented Mar 20, 2014 at 15:03
  • \$\begingroup\$ @AJMansfield Of course. It's true that I didn't pay any attention to it. \$\endgroup\$ Commented Mar 20, 2014 at 15:17
  • \$\begingroup\$ Yeah in this case it is desired to use C instead of C++ for string handling. \$\endgroup\$ Commented Mar 20, 2014 at 15:47
5
\$\begingroup\$

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.

\$\endgroup\$
2
\$\begingroup\$

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.

\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.