Post by thread:[PIC] C18 comparison bug?

Hmmm... still tinkering with C18 and ran into this oddity... #define S2_MIN 90 #define S2_MAX 125 #define S2_STEP 5 .... void main(void) { ... S2 = S2_MIN; // Initialize ... // (In button responder routine...) S2 += S2_STEP; // Increment to next value if (S2 >= (S2_MAX + S2_STEP)) // If max passed... S2 = S2_MIN; // ...cycle back to start ... } Every time S2 is incremented, the comparison checks to see if the max value has been exceeded, and if so, it will reset it to the min value. But it fails. If I change the comparison to this, it works... if (S2 > S2_MAX) S2 = S2_MIN; // Cycle back around to start Disassembly of the non-working code (with comments I've now added). 0xc0 and 0xbf are the upper and lower bytes of S2 resp. Computations in comments are at S2=130. 845: if (S2 >= (S2_MAX + S2_STEP)) 0B16 0E82 MOVLW 0x82 // 130, correct. 0B18 5DBF SUBWF 0xbf, W, BANKED // Result=0, so C=1, Z=1 0B1A 0EFF MOVLW 0xff 0B1C 59C0 SUBWFB 0xc0, W, BANKED // 511-0 is +ve, so C=1, Z=0.. 0B1E E303 BNC 0xb26 // So this should not branch...? 847: S2 = S2_MIN; 0B20 0E5A MOVLW 0x5a 0B22 6FBF MOVWF 0xbf, BANKED 0B24 6BC0 CLRF 0xc0, BANKED If I understand what's happening here, the lower bytes are subtracted first, and C is carried over into the next operation where the upper byte is subtracted. Not sure why 255 is being used for that though. But at S2=130, C should be 1 when it reaches the BNC instruction. But's it's skipping the code after that branch. What's really confusing me here is the SUBWFB instruction. I've looked at the datasheet for it and the examples provided for its use, and can't see how they get those results. Huh?

On Tue, Jan 24, 2012 at 4:27 PM, PICdude <spam_OUTpicdude3TakeThisOuTnarwani.org> wrote: {Quote hidden}> Hmmm... still tinkering with C18 and ran into this oddity... > > #define S2_MIN 90 > #define S2_MAX 125 > #define S2_STEP 5 > ... > void main(void) > { > ... > S2 = S2_MIN; // Initialize > ... > // (In button responder routine...) > S2 += S2_STEP; // Increment to next value > if (S2 >= (S2_MAX + S2_STEP)) // If max passed... > S2 = S2_MIN; // ...cycle back to start > ... > } > > Every time S2 is incremented, the comparison checks to see if the max > value has been exceeded, and if so, it will reset it to the min value. > But it fails. > > If I change the comparison to this, it works... > > if (S2 > S2_MAX) > S2 = S2_MIN; // Cycle back around to start > > Disassembly of the non-working code (with comments I've now added). > 0xc0 and 0xbf are the upper and lower bytes of S2 resp. Computations > in comments are at S2=130. > > 845: if (S2 >= (S2_MAX + S2_STEP)) > 0B16 0E82 MOVLW 0x82 // 130, correct. > 0B18 5DBF SUBWF 0xbf, W, BANKED // Result=0, so C=1, Z=1 > 0B1A 0EFF MOVLW 0xff > 0B1C 59C0 SUBWFB 0xc0, W, BANKED // 511-0 is +ve, so C=1, Z=0. > 0B1E E303 BNC 0xb26 // So this should not branch...? > 847: S2 = S2_MIN; > 0B20 0E5A MOVLW 0x5a > 0B22 6FBF MOVWF 0xbf, BANKED > 0B24 6BC0 CLRF 0xc0, BANKED > > > If I understand what's happening here, the lower bytes are subtracted > first, and C is carried over into the next operation where the upper > byte is subtracted. Not sure why 255 is being used for that though. > But at S2=130, C should be 1 when it reaches the BNC instruction. > But's it's skipping the code after that branch. What's really > confusing me here is the SUBWFB instruction. I've looked at the > datasheet for it and the examples provided for its use, and can't see > how they get those results. > > Huh? > How is "S2" declared? If it is an int8_t (aka char) then 125+5 = -126, which is always going to be less than S2_MIN, so S2 will remain at 90. -- Regards, Mark markrages@gmail

> Disassembly of the non-working code (with comments I've now added). > 0xc0 and 0xbf are the upper and lower bytes of S2 resp. Why are you using a 16-bit variable instead of an 8-bit (unsigned) variable? The subwfb instruction is correct - you do want to subtract the borrow from the most significant byte calculation. What is confusing me is the MOVLW 0xff. If I were writing this in assembly, I would use MOVLW 0x00. -Scot

Quoting Mark Rages <markragesKILLspamgmail.com>: > On Tue, Jan 24, 2012 at 4:27 PM, PICdude <.....picdude3KILLspam.....narwani.org> wrote: >> Hmmm... still tinkering with C18 and ran into this oddity... >> ... > > How is "S2" declared? If it is an int8_t (aka char) then 125+5 = > -126, which is always going to be less than S2_MIN, so S2 will remain > at 90. > "unsigned int", because the full range of values will be 0 to 350.

I'm using unsigned int... 16bit unsigned. Quoting Scott <EraseMEgoldscottspam_OUTTakeThisOuTgmail.com>: {Quote hidden}>> Disassembly of the non-working code (with comments I've now added). >> 0xc0 and 0xbf are the upper and lower bytes of S2 resp. > > Why are you using a 16-bit variable instead of an 8-bit (unsigned) variable? > > The subwfb instruction is correct - you do want to subtract the borrow > from the most significant byte calculation. What is confusing me is > the MOVLW 0xff. If I were writing this in assembly, I would use MOVLW > 0x00. > > -Scott >

Have you tried with a modification like this: #define S2_MIN ( (unsigned) 90 ) #define S2_MAX ( (unsigned) 125 ) #define S2_STEP ( (unsigned) 5 ) Tamas On 25 January 2012 01:09, PICdude <picdude3spam_OUTnarwani.org> wrote: {Quote hidden}> Quoting Mark Rages <@spam@markragesKILLspamgmail.com>: > > > On Tue, Jan 24, 2012 at 4:27 PM, PICdude <KILLspampicdude3KILLspamnarwani.org> wrote: > >> Hmmm... still tinkering with C18 and ran into this oddity... > >> ... > > > > > How is "S2" declared? If it is an int8_t (aka char) then 125+5 = > > -126, which is always going to be less than S2_MIN, so S2 will remain > > at 90. > > > > "unsigned int", because the full range of values will be 0 to 350. > > > > >

Hmmm... that works. Some other interesting points to note... Original, which failed... if (S2 >= (S2_MAX + S2_STEP)) S2 = S2_MIN; This disassembled to... 0B16 0E82 MOVLW 0x82 0B18 5DBF SUBWF 0xbf, W, BANKED 0B1A 0EFF MOVLW 0xff 0B1C 59C0 SUBWFB 0xc0, W, BANKED 0B1E E303 BNC 0xb26 ' ... With the type-casting done in the definition as you suggested, the same statement disassembles to... 0B30 0E82 MOVLW 0x82 0B32 5DBF SUBWF 0xbf, W, BANKED 0B34 0E00 MOVLW 0 0B36 59C0 SUBWFB 0xc0, W, BANKED 0B38 E303 BNC 0xb40 ... Here, the calculation of 125+5 is still correct (130 = 0x82), but the 3rd instruction is different. I can see now what it's thinking here, that 125 will fit in a signed byte, so it uses that. And if I change the statement to "if (S2 >= 130)", then it knows that a signed byte won't work, so it interprets it as unsigned, and works correctly. But this is so not intuitive. When the compiler does the math on the defines and comes up with 130, it should know then that a signed byte won't work. I'm going to reserve space on this list now for a future rant on this, when this bites me in a big way. I just know that's coming. Thanks, -Neil. Quoting Tamas Rudnai <RemoveMEtamas.rudnaiTakeThisOuTgmail.com>: {Quote hidden}> Have you tried with a modification like this: > > #define S2_MIN ( (unsigned) 90 ) > #define S2_MAX ( (unsigned) 125 ) > #define S2_STEP ( (unsigned) 5 ) > > Tamas > > > On 25 January 2012 01:09, PICdude <spamBeGonepicdude3spamBeGonenarwani.org> wrote: > >> Quoting Mark Rages <TakeThisOuTmarkragesEraseMEspam_OUTgmail.com>: >> >> > On Tue, Jan 24, 2012 at 4:27 PM, PICdude <RemoveMEpicdude3TakeThisOuTnarwani.org> wrote: >> >> Hmmm... still tinkering with C18 and ran into this oddity... >> >> ... >> >> > >> > How is "S2" declared? If it is an int8_t (aka char) then 125+5 = >> > -126, which is always going to be less than S2_MIN, so S2 will remain >> > at 90. >> > >> >> "unsigned int", because the full range of values will be 0 to 350. >> >> >> >> >> -

On Tue, 24 Jan 2012, PICdude wrote: > I can see now what it's thinking here, that 125 will fit in a signed > byte, so it uses that. And if I change the statement to "if (S2 >= > 130)", then it knows that a signed byte won't work, so it interprets > it as unsigned, and works correctly. But this is so not intuitive. It may not be intuitive, but it's documented in the C18 user's guide. By default, C18 behaves in a non-standard way with regard to integer promotions. You can request standard C behavior with a command-line switch. > When the compiler does the math on the defines and comes up with 130, > it should know then that a signed byte won't work. That's not how C works. Even if you enable standard behavior in C18, you'll still run into situations like this. When you add two ints, the addition is performed with int precision. If the addition doesn't fit in an int, you have an overflow. It doesn't matter that you're comparing the result to a "long" or assigning it to a "long". If you need an operation to be performed with higher precision, you must explicitly tell the compiler that. Incidentally, the fact that you're using #defines here isn't relevant to the problem. The preprocessor just does a simple text substitution with these. -- John W. Temples, II

Em 25/01/2012 06:18, John Temples escreveu: {Quote hidden}> On Tue, 24 Jan 2012, PICdude wrote: > >> I can see now what it's thinking here, that 125 will fit in a signed >> byte, so it uses that. And if I change the statement to "if (S2 >= >> 130)", then it knows that a signed byte won't work, so it interprets >> it as unsigned, and works correctly. But this is so not intuitive. > It may not be intuitive, but it's documented in the C18 user's guide. > By default, C18 behaves in a non-standard way with regard to integer > promotions. You can request standard C behavior with a command-line > switch. > >> When the compiler does the math on the defines and comes up with 130, >> it should know then that a signed byte won't work. > That's not how C works. Even if you enable standard behavior in C18, > you'll still run into situations like this. When you add two ints, > the addition is performed with int precision. If the addition doesn't > fit in an int, you have an overflow. It doesn't matter that you're > comparing the result to a "long" or assigning it to a "long". If you > need an operation to be performed with higher precision, you must > explicitly tell the compiler that. I may be wrong, but the C standard states that operands must be promoted "at least" to int, or to the type of the operand with highest precision. Also, the OP could use: #define S2_MIN 90u #define S2_MAX 125u #define S2_STEP 5u > > Incidentally, the fact that you're using #defines here isn't relevant > to the problem. The preprocessor just does a simple text substitution > with these. > > -- > John W. Temples, III

Isaac Marino Bavaresco wrote: > I may be wrong, but the C standard states that operands must be promoted > "at least" to int, or to the type of the operand with highest precision. > > Also, the OP could use: > > #define S2_MIN 90u > #define S2_MAX 125u > #define S2_STEP 5u I think there may be a more subtle issue going on here. The expression (S2_MAX + S2_STEP) is evaluated at compile time, not run time, as we can see from the generated code. My memory's a little hazy on this point, but doesn't the C standard state that compile-time operations are to be done with the largest available size of each type; i.e., longs for integers and doubles for floating-point? It looks like the compiler in this case assumed that a compile-time constant calculation was to be done with signed char, which it then sign-extended to 16 bits for the run-time comparison. At the very least, this violates the principle of "least surprise". -- Dave Twee

On 25 January 2012 14:22, Dave Tweed <picEraseME.....dtweed.com> wrote: > My memory's a little hazy on this point, but doesn't the C standard state > that > compile-time operations are to be done with the largest available size of > each > type; i.e., longs for integers and doubles for floating-point? > Should be used 'int' type, yes. > It looks like the compiler in this case assumed that a compile-time > constant > calculation was to be done with signed char, which it then sign-extended to > 16 bits for the run-time comparison. At the very least, this violates the > principle of "least surprise". > I think in C18 the default is 'char' -- maybe to save program memory and runtime --, maybe if integer promotion was used then it would go to the C89 standard (have not checked that). I vaguely remember discussing about such kind of issues here in this list few years back. Tamas > > -- Dave Tweed >

> I think there may be a more subtle issue going on here. The expression > (S2_MAX + S2_STEP) is evaluated at compile time, not run time, as we can > see > from the generated code. > > My memory's a little hazy on this point, but doesn't the C standard state > that > compile-time operations are to be done with the largest available size of > each > type; i.e., longs for integers and doubles for floating-point? Compile time computations are not required by the standard, except in places that require a constant value (for example: the values of an enum, the size of an array, the case labels in a switch). Other than these sorts of applications, folding compile time constants is just a performance optimization (albeit one that nearly all(?) compilers do. Where compile-time operations do occur that are supposted to follow all the same rules as runtime computation. This is so that it doesn't matter whether they are optimized away or not (principal of least surprise?). A problem with C18, and many other 8-bit microcontroller C's is that they do *not* follow the standard when it comes to type promotion. The standard says that *every* arithmetic operation must be done using at least int precision (or the results must be "as if" it was done in int precision). However this would be very costly on a 8-bit micro, so most (many?) C compilers for them will not promote byte-sized arguments to int in the absence of a specific cast. Some compilers (C18??) provide switches to force int promotion, but setting such a switch can have a strongly negative effect on performance. -- Bob Ammerman RAm Systems

On Wed, 25 Jan 2012, Dave Tweed wrote: > My memory's a little hazy on this point, but doesn't the C standard state that > compile-time operations are to be done with the largest available size of each > type; i.e., longs for integers and doubles for floating-point? No. Anything smaller than an "int" is promoted to "int", and there's nothing special about "compile-time operations." -- John W. Temples, II