Calculation Error XC8 MPLABX

Discussion in 'Programmer's Corner' started by Youngjohnnie, Feb 11, 2016.

  1. Youngjohnnie

    Thread Starter New Member

    Feb 11, 2016
    3
    1
    I am using xc8 v1.35 and MPLABX V3.2 and an 18f2420 pic. I have set double to be 32 bits.

    I have the digits of a frequency stored in an interger array.
    int freq[]
    { 0,0,0,0,0,0,0,0}

    I calculate the frequency thus
    long frequency = 0;
    freq[7] * 1 +
    freq[6] * 10 +
    freq[5] * 100 +
    freq[4] * 1000 +
    freq[3] * 10000 +
    freq[2] * 100000 +
    freq[1] * 1000000 +
    freq[0] * 10000000;

    However if I have
    freq = {0,0,5,8,0,0,0,0}
    and do the caluclation
    I get 514614 instead of 580000
    What stupid mistake am I making.
    John
     
  2. ErnieM

    AAC Fanatic!

    Apr 24, 2011
    7,386
    1,605
    Your code fragment has no l value, so show what your code is really doing, like do a copy and paste of the real code.

    Other than that you are mixing int and long types, using int to make a long. You may need to cast the intermediate products.
     
  3. WBahn

    Moderator

    Mar 31, 2012
    17,737
    4,789
    We definitely need more context. In particular, you don't show that you every set frequency to anything once you initialize it to zero. You just have a multiline expression sitting there doing nothing.

    But you can explore things yourself a bit. Run it several times with all zeros except for one 1 and move that 1 through all of the positions. Do you get the expected result?
     
  4. JohnInTX

    Moderator

    Jun 26, 2012
    2,345
    1,025
    Its because your constants are not 'long'. Without it, I got what you got. I added the L specifier and it works fine. Interesting note: I found it worked when only the first multiply by a non-zero array value was specified as 'L'. The subsequent non-zero multiplies worked correctly 'L' or not. Go figure.
    I would also expressly cast the array value to long as well even though the default casting works.
    This compiles and you can step through each line in MPSIM while watching the values. It gets the correct answer.

    Code (C):
    1. #include <xc.h>
    2. //int freq[] =  { 0,0,0,0,0,0,0,0};
    3. int freq[] = {0,0,5,8,0,0,0,0};
    4. long frequency;
    5.  
    6. void main(void) {
    7.  
    8.     while(1){
    9.         frequency = 0L;  // clear each loop
    10.         // do by steps for debug
    11.         frequency =  freq[7] * 1L;
    12.         frequency += freq[6] * 10L;
    13.         frequency += freq[5] * 100L;
    14.         frequency += freq[4] * 1000L;
    15.         frequency += freq[3] * 10000L;
    16.         frequency += freq[2] * 100000L;
    17.         frequency += freq[1] * 1000000L;
    18.         frequency += freq[0] * 10000000L;
    19.         PORTA = (char) frequency;  // do something with the variable so it doesn't get optimized out.
    20.                                    // makes a good breakpoint
    21.     }// while
    22. }// main
    Edit: others thought so too while I was posting this.
     
    Last edited: Feb 11, 2016
  5. WBahn

    Moderator

    Mar 31, 2012
    17,737
    4,789

    Are 'int' values 16-bit or 32-bit?

    If they are 16-bit then literals could get reduced modulo 65536 (with anything over 32767 being interpreted as a negative). So your expression might evaluate as
    8 * (10000 % 65536) + 5 * (100000 % 65536)
    8 * (10000) + 5 * (-31072) = 252320
    14464 + (-24288) = -9824

    So that doesn't seem to be the problem.
     
  6. JohnInTX

    Moderator

    Jun 26, 2012
    2,345
    1,025
    In XC8, ints are 16 bits, longs are 32 bits. A 'short long' is 24 bits. All can be signed or unsigned.

    'double' is a floating point number that can be 24 or 32 bits which has no bearing on this particular problem.

    It works fine with the more compact unsigned char/long as well:
    Code (C):
    1. #include <xc.h>
    2. //int freq[] =  { 0,0,0,0,0,0,0,0};
    3. unsigned char freq[] = {0,0,5,8,0,0,0,0};
    4. unsigned long frequency;
    5.  
    6. void main(void) {
    7.    
    8.     while(1){
    9.         frequency = 0L;  // clear each loop
    10.                         // do by steps for debug
    11.         frequency =  freq[7] * 1L;
    12.         frequency += freq[6] * 10L;
    13.         frequency += freq[5] * 100L;
    14.         frequency += freq[4] * 1000L;
    15.         frequency += freq[3] * 10000L;
    16.         frequency += freq[2] * 100000L;
    17.         frequency += freq[1] * 1000000L;
    18.         frequency += freq[0] * 10000000L;
    19.         PORTA = (char) frequency;  // do something with the variable so it doesn't get optimized out.
    20.                                    // makes a good breakpoint
    21.     }// while
    22. }// main
     
    Last edited: Feb 11, 2016
  7. Youngjohnnie

    Thread Starter New Member

    Feb 11, 2016
    3
    1
    Thanks so much for this. Yes it works fine in my code. Thanks to all others who replied.
     
  8. WBahn

    Moderator

    Mar 31, 2012
    17,737
    4,789
    Glad that solved it, but it would be nice to understand WHY it is producing that particular wrong value. If we can figure that out we may well then understand why the partial patch that JohnInTX described works.
     
  9. dannyf

    Well-Known Member

    Sep 13, 2015
    1,782
    360
    Integer promotion.
     
  10. WBahn

    Moderator

    Mar 31, 2012
    17,737
    4,789
    Please show how integer promotion (1) accounts for the result in the original post, and (2) produces the correct result in the reduced case described by JohnInTX.
     
  11. dannyf

    Well-Known Member

    Sep 13, 2015
    1,782
    360
    14464=80000-65536.
    14464+500000=514464.
     
  12. WBahn

    Moderator

    Mar 31, 2012
    17,737
    4,789
    Okay, that certainly helps set the focus.

    I certainly understand the 14464. What I don't understand is why the 100000 isn't reduced modulo 65536.

    Oh, I think I see. The compiler doesn't need the constants to be marked with the 'L', per se. It recognizes that 100000 has to be a long and, therefore, promotes the freq[2] to a long before performing the multiplication in that term. But since 10000 is representable by an int, freq[3] is not promoted and so the result of that multiplication remains an int and is reduced.

    I wonder if that behavior is standards compliant. I could easily see something like this being implementation defined.

    This also explains what JohnInTX observed, though it has nothing to do with the (quite reasonable) explanation he offered up.

    Here would be a test that would confirm (pretty much) this explanation: Don't put the 'L' suffix on anything and try the following:

    {0,0,0,3,2,7,6,7} => freq = 32768
    {0,0,0,3,2,7,6,8} => freq = -32768
    {0,0,0,6,5,5,3,5} => freq = -1
    {0,0,0,6,5,5,3,6} => freq = 0

    This is assuming that int is, by default, signed.

    The "best" solution is to make the freq[] array of type long, but that chews of memory which may be in short demand on an MCU. If so, my next recommendation would be to both put the 'L' suffix on all of the constants and cast each freq[] reference to a long before the multiplication occurs.

    But, if we really wanted to push things, as an absolute minimum either the 10000 has to have the suffix or freq[3] has to be cast to a long since that is the first term that could cause an overflow of an int and, even if it doesn't, adding the sum of the prior terms to it (which, by themselves, can't overflow) could. Even if you reverse the order of the terms, you still need to either put the suffix on 10000 or cast freq[3] since that term, by itself, could still overflow before it is promoted in order to add it to the prior running sum.

    Thanks, @dannyf
     
  13. JohnInTX

    Moderator

    Jun 26, 2012
    2,345
    1,025
    I took another look at it and that's what's happening.

    Constants with no explicit suffix are automatically assigned the smallest type that they will fit into. That action goes back to K&R. In this example 10000 fits into int, 100000 is stored as long and the compiler does that as it should.
    Integers of different types in an expression are promoted to the biggest type in the expression i.e. char * long gets promoted to long * long = long - also K&R.

    The problem is that each statement is evaluated right to left and something like 8 * 10000 is evaluated as int*int = int which overflows 16 bits. When the constants are explicitly typed as long i.e. 1000L, the expression is evaluated as long * long = long and no overflow occurs in any of the calculations. Note that the compiler doesn't look ahead to see what the type of the result variable is, it just promotes as it evaluates right to left. Note, too, that the array can be 'char' to save space if the constants are explicitly 'long'. The right to left evaluation will cause auto promotion of char to long and it works as it should with the result being type 'long'.

    I verified this (after brushing up on K&R and XC8) by setting various array entries to overflow 16bits at various points and the results agree with the above. Sorry if my early analysis caused confusion. I usually use explicit casts and types so get a little fuzzy on the implicit rules after awhile.
     
    MMcLaren likes this.
  14. Youngjohnnie

    Thread Starter New Member

    Feb 11, 2016
    3
    1
    again thankyou for the explanation. Another bit of C learned and definitely will use explicit casts in the future
     
    JohnInTX likes this.
  15. nsaspook

    AAC Fanatic!

    Aug 27, 2009
    2,907
    2,167
    You should also use the stdint.h types if possible.

    http://ww1.microchip.com/downloads/en/DeviceDoc/52053B.pdf
     
    GopherT, MMcLaren and JohnInTX like this.
Loading...