0

I need to convert a hex string to a decimal value. I have used the following method. But sometimes it returns the wrong decimal value.

Hex string is in this format "00 00 0C 6E"

unsigned long hexToDec(String hexString) {
  unsigned long decValue = 0;
  int nextInt;
  for (long i = 0; i < hexString.length(); i++) {
    nextInt = long(hexString.charAt(i));
    if (nextInt >= 48 && nextInt <= 57) nextInt = map(nextInt, 48, 57, 0, 9);
    if (nextInt >= 65 && nextInt <= 70) nextInt = map(nextInt, 65, 70, 10, 15);
    if (nextInt >= 97 && nextInt <= 102) nextInt = map(nextInt, 97, 102, 10, 15);
    nextInt = constrain(nextInt, 0, 15);
    decValue = (decValue * 16) + nextInt;
  }
  return decValue;
}
dda
  • 6,030
  • 2
  • 25
  • 34
Kavinda Gehan
  • 4,668
  • 2
  • 17
  • 20
  • 1
    What is the type `String`? What does the function `map()` do? Is this really C given `hexString.length()`? It could be, except that `nextInt = long(hexString.charAt(i));` is definitely invalid in C (though it could be valid in C++). You've not shown how the code is called, or sample inputs which convert OK, or sample inputs which fail (it isn't clear whether `"00 00 0C 6E"` passes or fails). – Jonathan Leffler Jul 30 '17 at 04:07
  • "sometimes it returns wrong decimal value." Start by finding a specific example which returns the wrong decimal value. Then use a debugger or add `printf()` statements to your code to view the values of variables as it runs. As you step through your code, find what values are unexpected and then figure out why they are not what you expect. – Code-Apprentice Jul 30 '17 at 04:41
  • Possible duplicate of [convert HEX string to Decimal in arduino](https://stackoverflow.com/questions/31830143/convert-hex-string-to-decimal-in-arduino) – TomServo Jul 30 '17 at 10:19
  • but there is no correct answer for that – Kavinda Gehan Aug 02 '17 at 10:10

3 Answers3

5

The whitespace is fouling the conversion. Try this:

#include <iostream>
#include <string>
#include <cctype>
using namespace std;

unsigned long hexToDec(string hexString) {
  unsigned long decValue = 0;
  char nextInt;
  for ( long i = 0; i < hexString.length(); i++ ) {
    nextInt = toupper(hexString[i]);
    if( isxdigit(nextInt) ) {
        if (nextInt >= '0' && nextInt <= '9') nextInt = nextInt - '0';
        if (nextInt >= 'A' && nextInt <= 'F') nextInt = nextInt - 'A' + 10;
        decValue = (decValue << 4) + nextInt;
    }
  }
  return decValue;
}

int main() {
    string test = "00 00 00 0c 1e";
    printf("'%s' in dec = %ld\n", test.c_str(), hexToDec(test));
    return 0;
}

output:
'00 00 00 0c 1e' in dec = 3102
dda
  • 6,030
  • 2
  • 25
  • 34
Grifplex
  • 192
  • 1
  • 12
  • Given that you are checking valid ranges for the conversion you can do without the `isxdigit`; the `toupper` can be avoided as well by simply replicating the second `if` with lowercase characters. Finally, *you must always cast `char` to `unsigned char`* when calling character classification/conversion functions from `ctype.h`. – Matteo Italia Jul 30 '17 at 10:09
  • Have you tried it? The `isxdigit` block is there to ignore the whitespace which includes the left shift and the original issue. The `toupper` is a tradeoff that can go either way that now as I think about it is more clear but less efficient. – Grifplex Jul 30 '17 at 15:25
  • About the `isxdigit`, as I said it's useless; you can simply remove it, change the second `if` into an `else if` (as it should already be) and add an `else` with a `continue`. For the `toupper`, exactly, it's just less efficient. But again, most importantly, if you choose to use those functions you must cast to `unsigned char`. – Matteo Italia Jul 30 '17 at 15:52
2
  1. Why are you hardcoding ASCII values? Just use char literals - '0', '9', 'a', 'f', ...; they are way more readable.
  2. There's no need to all those map, simple math operations will do; also, there's no need for constrain, it's only hiding your problem (see next point).
  3. If you meet a character that is not in the [0-9a-fA-F] range instead of skipping it you are just constraining its ASCII value to 0-15, which is definitely not what you want to do.

So:

unsigned long hexToDec(String hexString) {
    unsigned long ret;
    for(int i=0, n=hexString.length(); i!=n; ++i) {
        char ch = hexString[i];
        int val = 0;
        if('0'<=ch && ch<='9')      val = ch - '0';
        else if('a'<=ch && ch<='f') val = ch - 'a' + 10;
        else if('A'<=ch && ch<='F') val = ch - 'A' + 10;
        else continue; // skip non-hex characters
        ret = ret*16 + val;
    }
    return ret;
}

Notice that this will provide the correct result if the data you are getting is a big-endian dump of some integer.

Matteo Italia
  • 123,740
  • 17
  • 206
  • 299
1

Your hex string contains whitespace, but your code doesn't skip them and that constrain() function effectively turns a white space into a 0xF which ultimately results in wrong results. Other than that your code is OK.

If I were to suggest the minimal amount of changes I'd say, after:

nextInt = long(hexString.charAt(i));

Add this to the code:

if(isspace(nextInt))
   continue;

And note that the isspace() function is a C Standard Library function that is prototyped in the ctype.h header file.

But if I'm to comment on the whole code, you are calling too many functions to do a job that can be done without any function call.

dda
  • 6,030
  • 2
  • 25
  • 34
m0h4mm4d
  • 400
  • 4
  • 12