Calculation Error XC8 MPLABX

Thread Starter

Youngjohnnie

Joined Feb 11, 2016
3
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
 

ErnieM

Joined Apr 24, 2011
8,377
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.
 

WBahn

Joined Mar 31, 2012
29,976
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?
 

JohnInTX

Joined Jun 26, 2012
4,787
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.

C:
#include <xc.h>
//int freq[] =  { 0,0,0,0,0,0,0,0};
int freq[] = {0,0,5,8,0,0,0,0};
long frequency;

void main(void) {

    while(1){
        frequency = 0L;  // clear each loop
        // do by steps for debug
        frequency =  freq[7] * 1L;
        frequency += freq[6] * 10L;
        frequency += freq[5] * 100L;
        frequency += freq[4] * 1000L;
        frequency += freq[3] * 10000L;
        frequency += freq[2] * 100000L;
        frequency += freq[1] * 1000000L;
        frequency += freq[0] * 10000000L;
        PORTA = (char) frequency;  // do something with the variable so it doesn't get optimized out.
                                   // makes a good breakpoint
    }// while
}// main
Edit: others thought so too while I was posting this.
 
Last edited:

WBahn

Joined Mar 31, 2012
29,976
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

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.
 

JohnInTX

Joined Jun 26, 2012
4,787
Are 'int' values 16-bit or 32-bit?
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:
C:
#include <xc.h>
//int freq[] =  { 0,0,0,0,0,0,0,0};
unsigned char freq[] = {0,0,5,8,0,0,0,0};
unsigned long frequency;

void main(void) {
   
    while(1){
        frequency = 0L;  // clear each loop
                        // do by steps for debug
        frequency =  freq[7] * 1L;
        frequency += freq[6] * 10L;
        frequency += freq[5] * 100L;
        frequency += freq[4] * 1000L;
        frequency += freq[3] * 10000L;
        frequency += freq[2] * 100000L;
        frequency += freq[1] * 1000000L;
        frequency += freq[0] * 10000000L;
        PORTA = (char) frequency;  // do something with the variable so it doesn't get optimized out.
                                   // makes a good breakpoint
    }// while
}// main
 
Last edited:

Thread Starter

Youngjohnnie

Joined Feb 11, 2016
3
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.

C:
#include <xc.h>
//int freq[] =  { 0,0,0,0,0,0,0,0};
int freq[] = {0,0,5,8,0,0,0,0};
long frequency;

void main(void) {

    while(1){
        frequency = 0L;  // clear each loop
        // do by steps for debug
        frequency =  freq[7] * 1L;
        frequency += freq[6] * 10L;
        frequency += freq[5] * 100L;
        frequency += freq[4] * 1000L;
        frequency += freq[3] * 10000L;
        frequency += freq[2] * 100000L;
        frequency += freq[1] * 1000000L;
        frequency += freq[0] * 10000000L;
        PORTA = (char) frequency;  // do something with the variable so it doesn't get optimized out.
                                   // makes a good breakpoint
    }// while
}// main
Edit: others thought so too while I was posting this.
Thanks so much for this. Yes it works fine in my code. Thanks to all others who replied.
 

WBahn

Joined Mar 31, 2012
29,976
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.
 

WBahn

Joined Mar 31, 2012
29,976
14464=80000-65536.
14464+500000=514464.
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
 

JohnInTX

Joined Jun 26, 2012
4,787
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.
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.
 

nsaspook

Joined Aug 27, 2009
13,079
again thankyou for the explanation. Another bit of C learned and definitely will use explicit casts in the future
You should also use the stdint.h types if possible.

http://ww1.microchip.com/downloads/en/DeviceDoc/52053B.pdf
2.4.6 Sizes of Types The sizes of the basic C types, for example char, int and long, are not fully defined by the CCI. These types, by design, reflect the size of registers and other architectural features in the target device. They allow the device to efficiently access objects of this type. The ANSI C Standard does, however, indicate minimum requirements for these types, as specified in . If you need fixed-size types in your project, use the types defined in , e.g., uint8_t or int16_t. These types are consistently defined across all XC compilers, even outside of the CCI. Essentially, the C language offers a choice of two groups of types: those that offer sizes and formats that are tailored to the device you are using; or those that have a fixed size, regardless of the target.
 
Top