0

I am trying to communicate with a usb dongle via serial communication. My communication works, however I cant get the device to correctly parse the communication. My devices reads the message and compares it with a hardcoded c-string. It parses and recognizes that it's the correct string, but when I try to parse the value after the : character, it returns 0x00000000 and I have no idea why. I've tried using char cast and use atoi, I tried using a simple ascii translation, and even doing a bitwise addition operation as shown here: convert subset of vector<uint8_t> to int

For example:
I send "Heart Rate:55" It parses and recognizes that "Heart Rate:" but when I tell it to go find the 55 and bring it back to do stuff with it, it gives me a 0x00000000
Heres a snippet:

const uint8_t hrmSet[] = "Heart Rate:";

/** Find the : character in the string and break it apart to find if it matches, 
and determine the value of the value of the desired heart rate. **/
int parse(uint8_t *input, uint8_t size)
{
    for (uint8_t i = 0; i < size; i++)
    {
        if (input[i] == ':')
        {
            if (compare_string(input, hrmSet, i) == 0)
            {
                int val = 0;
                for (int j = i+1; j < size; j++)
                {
                    if (!isdigit(input[j]))
                    {
                        for (int k = i; k < j; k++)
                        {
                            val <<= 8;
                            val |= input[k];
                        }
                    }   
                }
                return val;
            }
            return -1;
        }
    }
    return -1;
}

Compare string function

/** Compare the input with the const values byte by byte to determine if they are equal.**/
int compare_string(uint8_t *first, const uint8_t *second, int total)
{
    for (int i = 0; i < total; i++)
    {
        if (*first != *second)
        {
            break;
        }

        if (*first == '\0' || *second == '\0')
        {
            break;
        }

        first++;
        second++;
    }

    if (*first == ':' && *second == ':')
    {
        return 0;
    }
    else
    {
       return -1;
    }
}
Community
  • 1
  • 1
Ronin
  • 93
  • 1
  • 2
  • 11
  • Why do you have that string comparison there _after_ you've located the colon? – 500 - Internal Server Error Oct 22 '14 at 22:53
  • 1
    Why are you checking for **not** `isdigit`, and what's the purpose of the `k` loop? – user3386109 Oct 22 '14 at 22:55
  • Your `compare_string` iterates from the start of both strings (`input` and `hrmSet`) each time it's called. – 500 - Internal Server Error Oct 22 '14 at 22:57
  • The `compare_string` function could be improved by `break`ing the loop when a colon is found in either string. That way, you wouldn't have to search for the colon before calling `compare_string`, and you wouldn't need `total` as an argument. – user3386109 Oct 22 '14 at 23:03
  • I find the colon, to set the bounds of the compare_string test. Because I do not know how long the value will be. In the future, this will check multiple different strings for matches.
    I check for not isdigit to find the end of the string, again, because I dont know how long it will be every time.
    The K loop is there because of this related question: http://stackoverflow.com/questions/17813350/convert-subset-of-vectoruint8-t-to-int?lq=1
    – Ronin Oct 22 '14 at 23:15
  • You're right, if I look for just the colon, and break after, that would eliminate some of complexity finding the bounds and everything. Thanks for the constructive advice! But I do not believe that will fix the issue at hand. – Ronin Oct 22 '14 at 23:19

2 Answers2

1
int val = 0;
for(int j = i+1; j < size; j++){
    if(isdigit(input[j] )){
        val = val * 10 + input[j]-'0';// val = (val << 8) | input[j];
    }   
}
return val;
user3386109
  • 34,287
  • 7
  • 49
  • 68
BLUEPIXY
  • 39,699
  • 7
  • 33
  • 70
  • 2
    @meaning-matters Although I personally agree with your coding style (with regards to the placement of braces), you should realize that it is considered extremely rude to edit someone's post merely to impose your preferred style. – user3386109 Oct 23 '14 at 00:02
  • 1
    Your answer below shows that instead we don't agree on coding style. But I agree that it was rude; will think twice next time. (I strongly believe that bad coding style is a cause of a lot of crap in software land. Extreme clean/consistent formatting is part of craftsmanship.) – meaning-matters Oct 23 '14 at 10:45
  • Code drawling long vertical interfere with thinking. – BLUEPIXY Oct 23 '14 at 11:04
1

The problem here is that you are using nested loops to perform tasks that should be done with sequential loops.

For example, the i loop searches for the colon, and then the loop in compare_string searches for the colon again. You could run the i loop first, and then call compare_string after the i loop finishes. But a better design is to have compare_string search for the colon while comparing, and then return the index of the character after the colon (or -1 if the colon is not found).

The same is true of the j and k nested loops. The j loop is searching for the end of the number. The k loop only runs once after the j loop is finished, and so the k loop should be after the j loop, not nested. But a better design is a single loop that converts the number while searching for the end of the number.

The code below demonstrates one possible implementation using the techniques I've described.

const uint8_t hrmSet[] = "Heart Rate:";

int compare_string( uint8_t *input, const uint8_t *expected, int size )
{
    for ( int i = 0; i < size; i++ )
    {
        if ( *input != *expected || *expected == '\0' )
            return( -1 );

        if ( *input == ':' && *expected == ':' )
            return( i + 1 );

        input++;
        expected++;
    }

    return( -1 );
}

int parse( uint8_t *input, uint8_t size )
{
    int i, val;

    if ( (i = compare_string( input, hrmSet, size )) < 0 )
        return( -1 );

    val = 0;
    for ( ; i < size && isdigit( input[i] ); i++ )
        val = val * 10 + input[i] - '0';

    return( val );
}

int main( void )
{
    uint8_t input[] = "Heart Rate:75";
    int rate = parse( input, sizeof(input) - 1 );
    printf( "%d\n", rate );
}
user3386109
  • 34,287
  • 7
  • 49
  • 68
  • Thanks so much! Looking at my code, I think that all the loops break like they're suppose to, but something is obviously not working as intended and it's way to complex. Simplicity is the ultimate sophistication after all. – Ronin Oct 23 '14 at 14:28