0

I want to combine two bytes into one unsigned long variable, my current code does not work. I am using MPLAB C18 compiler, this is my code.

    unsigned long red = 0;
    BYTE t[2];


    t[0]  = 0x12;
    t[1]  = 0x33;

    red = 100 * t[0] + t[1];
    printf("%lu", red);

Please let me know why I am not getting 1233 as my output.

panhandel
  • 674
  • 13
  • 23
cookie monster
  • 77
  • 1
  • 10

3 Answers3

5

You are multiplying t[0] by 100 when you should be multiplying it by 256. A better way would be to shift t[0] by 8 bits to the left and add t[1].

red = ( t[0] << 8 ) | t[1];
unxnut
  • 8,509
  • 3
  • 27
  • 41
  • Neither of these ways worked. I am getting **51** for red using the better way? – cookie monster Jun 11 '13 at 19:39
  • 2
    @cookiemonster Hexadecimal is still not decimal. –  Jun 11 '13 at 19:49
  • 1
    You need to modify your `printf` to print hexadecimal values. You should say `printf ( "%x", red );` – unxnut Jun 11 '13 at 21:30
  • you need to typecast t[0] to something larger before doing a shift like that, the standard does not insure that works (implementation defined), some compilers happen to allow that to work (gcc). Note this solution is much much safer than the pointer solution as the pointer solution can fail with optimization. – old_timer Jun 12 '13 at 17:46
1

You'll note that the values in your array are specified with 0x prefixes. 100 is not equal to 0x100.

Your compiler is non-compliant. Additionally, binary operators have poorly defined aspects for signed integer types, of which BYTE might be. Here's how I'd write that code, to be on the safe side:

unsigned long red = 0;
BYTE t[2];

t[0]  = 0x12;
t[1]  = 0x33;

red = (unsigned char) t[0];
red = red * 0x100;
red = red + (unsigned char) t[1];

printf("Decimal: %lu\n"
       "Hexadecimal: 0x%lx\n", red, red);
autistic
  • 1
  • 3
  • 35
  • 80
  • I am getting **4659** as my value for red? – cookie monster Jun 11 '13 at 19:55
  • That's correct. What does this print? `printf("%lu\n", 0x1233UL);` – autistic Jun 11 '13 at 19:57
  • Remember, `100` is not equal to `0x100`. Likewise, `1233` is not equal to `0x1233`. – autistic Jun 11 '13 at 19:58
  • Look [here](http://ideone.com/Dv3cNx)... See the first number printed? See the second number printed? Which one do you expect? Why do you suppose they're different? – autistic Jun 11 '13 at 20:07
  • Hint: `100` is not equal to `0x100`. – autistic Jun 11 '13 at 20:08
  • so "%lx" gave me 1233 for red which is amazing. How come it doesn't come through the if statement .. if (red == 1233) { printf"yes";} – cookie monster Jun 11 '13 at 20:24
  • 1
    ... `if (red == 0x1233UL) { ... }` WTF? Why haven't you learnt that `1233` IS NOT `0x1233`, yet? – autistic Jun 11 '13 at 20:25
  • I think I confused myself from the beginning .. I wanted 1233 to be in decimal but I initialized it in Hex `t[0] = 0x12;` and `t[1] = 0x33;` – cookie monster Jun 11 '13 at 21:59
  • 1
    @unxnut According to the C standard, the format specifier `%x` corresponds to an `unsigned int` typed argument. Which type does `red` have? Comment before you edit, **please**. – autistic Jun 12 '13 at 02:28
  • 2
    @JoshuaTaylor By approving that edit, you allowed undefined behaviour to be introduced into my answer! WTF? – autistic Jun 12 '13 at 02:34
  • 1
    @Jean-BernardPellerin ^----- Please ensure you approve less undefined behaviour! – autistic Jun 12 '13 at 02:35
  • 1
    @greeness That was sort of like saying "hey, hey... Die in a fire, would you?". Look at my screen-name! Please don't approve edits for code written in programming languages you don't know; You might cause some actual damage. – autistic Jun 12 '13 at 02:38
0

First of all note that MPLAB C18 has some divergences to the C standard. Read it's user manual, especially section 2.7 labeled "ISO divergences".

So pay attention to that C18 does char promotion instead of integer promotion which also applies to literals. So if you add 0x80 and 0x80 together you get 0x00. The way to avoid this is the same as how it can be worked around normally if you need larger constants Use 'L' or 'UL' postfixes to force calculating in 32 bits, maybe also casts where needed.

The compiler's optimizer may be somewhat imperfect, you will need to check the disassembly. If you work with 32bit literals, it may emit 32bit code even if, say, only 8bit would be needed. You may work this around by casting the result of a calculation such as:

#define C8BIT ((unsigned char)(((0x12UL * 0x100UL) + 0xC0UL)>>6))

(Just an example arbitrary calculation which would overflow otherwise)

In the example you posted the problem is similar, you essentially work with unsigned chars, the compiler promotes to unsigned char, and you except it to be calculated with at least 16bit depth. A proper and clean solution for the problematic line might be:

red = (0x100UL * (unsigned long)(t[0])) + ((unsigned long)(t[1]));

If performance matters however (assuming t[0] and t[1] there just got some example values, so may get arbitrary contents in the real program) you may want to check disassembly and finally resort to this:

red = (0x100U * t[0]) + t[1];

or

red = (((unsigned short)(t[0]))<<8) + t[1];

A sane compiler (even including C18) should give proper result on these. I also showed a version with shifting as C18 is not too strong on optimization and might include a library call to a multiply routine there. I omitted casting t[0] and t[1] as the compiler should properly promote them (the 0x100U demands at least 16bit promotion), and the cast might confuse the compiler to add extra 16bit logic (C18 is very weak in this term!) with literal '0' operands. If performance matters, check disassembly, otherwise just go with the clean solution!

You may also want to check that BYTE type. It should be unsigned char, but not necessarily. You might want to define types with fixed sizes (such as typedef unsigned char uint8; and so) for a consistent use.

Jubatian
  • 2,171
  • 16
  • 22