8
\$\begingroup\$

Microcontroller receive data on uart and call parser_i2c function when user presses enter e.g. parser_i2c(I2C scan)

I want to parse following input from user communicated through uart

I2C scan

I2C read read_adress

I2C Write Write_address data

I have wrote following code

        void parser_i2c(unsigned char str[20])
    {
       //unsigned char str[20]  
       unsigned char s[2] = " ";
       unsigned char *token;
       unsigned char output[10][10] ;
        U8 i_i2c=0;
       /* get the first token */
       token = strtok(str, s);
      
       /* walk through other tokens */
       while( token != NULL ) {
      //    printf( " %s\n", token );
        strcpy(output[i_i2c],token);
         
         i_i2c++ ;
          token = strtok(NULL, s);
     
       }
     if(!(strcmp(output[0],"I2C")))
      {
         if(!(strcmp(output[1],"READ")))
            {
                if(*output[2]!='\0')
                {
                i2c_read(output[2]) ;
                }
                else{
            
                        printf(COLOR_RED "Bad command" COLOR_RESET);
                        
                }
            }
         else if(!(strcmp(output[1],"WRITE")))
         {
            if(((*output[2])||(*output[3]))!='\0')
            {
            i2c_write(output[2],output[3]);
            }
            else{

                        printf(COLOR_RED "Bad Command" COLOR_RESET);
                
            }
         }
         
         else if(!(strcmp(output[1],"SCAN")))
         {
         i2c_scan();
         }
         else{
      
                        printf(COLOR_RED "Bad Command" COLOR_RESET);
                    
         }
         
      }
      else {
                            printf(COLOR_RED "Bad Command" COLOR_RESET);
                    
      }
memset(output, '\0', 10*sizeof(arr[0]));
    
    }

How to improve it ? in terms of reducing code lines and code size and memory utilization ? or any other way to parse ? Is there any basic library available ?

\$\endgroup\$
2
  • 2
    \$\begingroup\$ Is the indentation of your code the same as shown on this page as it is in your editor on your own machine? \$\endgroup\$
    – G. Sliepen
    Aug 5, 2020 at 19:13
  • 1
    \$\begingroup\$ @AustinHastings please leave the indentation as is- changing it would invalidate the answer. For more context, please see this meta answer. \$\endgroup\$ Aug 5, 2020 at 22:34

2 Answers 2

8
\$\begingroup\$

Here are some things that may help you improve your program.

Use consistent formatting

The code as posted has inconsistent indentation which makes it hard to read and understand. Pick a style and apply it consistently.

Provide complete code to reviewers

This is not so much a change to the code as a change in how you present it to other people. Without the full context of the code and an example of how to use it, it takes more effort for other people to understand your code. This affects not only code reviews, but also maintenance of the code in the future, by you or by others. One good way to address that is by the use of comments. Another good technique is to include test code showing how your code is intended to be used.

Use const where practical

The s string is set and never altered, so it could be const or even better in this case, static const.

Consider re-entrancy

In an embedded system, there are often many things happening at once. The usual case is that many events are event-driven and so the order in which parts of code are executed is uncertain. For that reason, you should be wary about using non-reentrant calls such as strtok. Also, we don't have the complete context, but it may be worth making a copy of a buffered line of data so that subesequent data the comes in does not overwrite a line as it is being parsed.

Consider passing a length

This code makes the tacit assumption that the passed array is a '\0'-terminated text string. If you can guarantee that is always the case, then the code is fine as it is, but a more robust system might be to explicitly pass both a pointer and a length.

Understand system variations

It is implementation-defined whether char is signed or unsigned. If it happens to be signed on your platform, you will get warnings about sign mismatches for parameters passed to strtok and strcpy. If you system uses unsigned chars by default, you will get no such warning, but you should be aware that this is at least a portability concern.

Simplify your algorithm

This could be simpler and more maintainble in the future if it used a state machine for parsing. For this simple grammar we have the following valid sentences:

  1. I2C SCAN
  2. I2C READ n
  3. I2C WRITE n n

I am assuming the n is a numeric value, but it's not clear from the context. Here's the corresponding state machine:

simple state machine Here's one way to code that:

void parser_i2c(mychar str[20])
{
    static const mychar s[2] = " ";
    enum { start, i2c, r1, w1, w2, error, done } state = start;
    mychar *n1 = NULL;
    for (mychar *token = strtok(str, s); state != done; token = strtok(NULL, s)) {
        switch (state) {
            case start:
                if (strcmp(token, "I2C") == 0) {
                    state = i2c;
                } else {
                    state = error;
                }
                break;
            case i2c:
                if (strcmp(token, "READ") == 0) {
                    state = r1;
                } else if (strcmp(token, "WRITE") == 0) {
                    state = w1;
                } else if (strcmp(token, "SCAN") == 0) {
                    i2c_scan();
                    state = done;
                } else {
                    state = error;
                }
                break;
            case r1:
                /* check for number? */
                i2c_read(token);
                state = done;
                break;
            case w1:
                /* check for number? */
                n1 = token;
                state = w2;
                break;
            case w2:
                /* check for number? */
                i2c_write(n1, token);
                state = done;
                break;
            default:
                printf(COLOR_RED "Illegal state" COLOR_RESET);
                state = done;
        }
        if (state == error) {
            printf(COLOR_RED "Bad Command" COLOR_RESET);
            state = done;
        }
    }
}

I've use mychar as a typedef for char on my machine. On yours, it looks like you could use unsigned char instead.

Note that now, the flow is very easy to follow and it would not be at all difficult to make an alteration to the grammar to accommodate some other command. Because the tokens are handled as they're parsed, there's no need to store any except the first parsed number which we need for i2c_write(). If it gets any more complex than this, one could use a more sophisticated parser or use a tool like flex.

\$\endgroup\$
3
  • \$\begingroup\$ The sample input in the question is mixed case, but the code only checks for upper case commands. \$\endgroup\$
    – RootTwo
    Aug 6, 2020 at 4:13
  • \$\begingroup\$ The original code also only checks for uppercase. If it works, so does this. \$\endgroup\$
    – Edward
    Aug 6, 2020 at 7:55
  • \$\begingroup\$ Interesting one liner enum { start, i2c, r1, w1, w2, error, done } state = start;. Hmmm. \$\endgroup\$ Aug 6, 2020 at 16:26
1
\$\begingroup\$

Some important remarks regarding performance:

  • Calling strcmp repeatedly on the same data in some if - else if listing is naive and very inefficient. This will kill significant amounts of execution time, particularly on low-end microcontrollers.

    Instead you should have all valid strings stored in a sorted look-up table. Then use binary search on that table instead. You can use standard C bsearch or implement it yourself - it's not hard to do.

  • You probably don't want to use strtok since it destroys the data passed, which in turn means that you'll have to make additional hard copies of it in advance, which is inefficient. A simple while(*ptr == ' ' && *ptr != '\0') {} could be used instead.

  • In general, you shouldn't need to make any calls to strcpy once you have copied the data from the I2C hardware buffers into RAM variables. Instead of shovelling whole data strings around, copy pointers if needed. Similarly, there should be no need to memset anything to zero.

\$\endgroup\$
4
  • \$\begingroup\$ is there any example , cause for using bsearch i need to use strcmp or any other comparison method ,and strcmp will be used same number of time as using if(strcmp ... \$\endgroup\$
    – G.ONE
    Aug 11, 2020 at 9:46
  • \$\begingroup\$ @G.ONE If rolling out the binary search manually, you don't need strcmp, which has the disadvantage of starting over from the first letter each time. Another alternative is a hash table, but that's mainly effective for larger amounts of strings. \$\endgroup\$
    – Lundin
    Aug 11, 2020 at 9:53
  • \$\begingroup\$ What do you mean by manually rolling it,bsearch function requires a comparison function itself ?Please provide an example \$\endgroup\$
    – G.ONE
    Aug 11, 2020 at 10:05
  • \$\begingroup\$ @G.ONE Manually implementing binary search, by keeping track of low/high/mid indices in a loop. \$\endgroup\$
    – Lundin
    Aug 11, 2020 at 10:07

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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