| View previous topic :: View next topic |
| Author |
Message |
matthewsachs
Joined: 19 Jun 2016
Posts: 22
|
| Compiler Bug or Do I Need to Get Myself a Good C Book |
 Posted: Sat Oct 26, 2019 11:56 pm |
 |
|
PCWH v5.090
I spent hours tracking down this bug.
The original function looked like this:
| Code: |
int16 ADCtoFLOW(int16 ADCReading, int16 ScaleFactor)
{
int32 res = 0L;
res = ((int32)(ADCReading)) * ((int32)(1000L));
if(res <= ((int32)(ScaleFactor))) res = 0L;
else res = res / ((int32)(ScaleFactor));
return (int16)res;
}
|
but it returned garbage all the time. It seems that using a cast of the function's second argument (i.e. (int32)(ScaleFactor)) in 2 places was causing the problem.
Changing the function by adding another temporary variable fixed the issue:
| Code: |
int16 ADCtoFLOW(int16 ADCReading, int16 ScaleFactor)
{
int32 res = 0L;
int32 SFCT = 0L;
SFCT = (int32)ScaleFactor;
res = ((int32)(ADCReading)) * ((int32)(1000L));
if(res <= SFCT) res = 0L;
else res = res / SFCT;
return (int16)res;
}
|
Am I missing something obvious? |
|
 |
PCM programmer
Joined: 06 Sep 2003
Posts: 21708
|
|
| Posted: Sun Oct 27, 2019 1:26 am |
|
|
Post a couple sample numbers for ADCReading and the ScaleFactor
that caused the problem to occur. And tell us what the bad result
was, and what the expected result was.
In other words, allow us to duplicate your problem. |
|
|
Ttelmah
Joined: 11 Mar 2010
Posts: 19544
|
|
| Posted: Sun Oct 27, 2019 6:39 am |
|
|
Also tell us what chip is involved.
I had an issue a couple of compiler versions ago, with casting not working
correctly with signed valeus on a PIC24. But this should be fixed in .090 |
|
|
matthewsachs
Joined: 19 Jun 2016
Posts: 22
|
|
| Posted: Sun Oct 27, 2019 7:26 am |
|
|
#include <18F66K80.h>
#device adc=12
With the function implemented using 2 casts:
ADCtoFLOW(8, 7371) -> returned 7936 (instead of 1 expected)
ADCtoFLOW(2265, 7371) -> returned 36609 (instead of 307 expected)
ADCtoFLOW(4092, 7371) -> returned 28674 (instead of 555 expected)
Not that it matters, but in the actual code, this function is passed a pair of int16 global variables (not the hard coded values shown above) that a) hold the ADC Reading and b) the ScaleFactor both of which are guaranteed NOT to be changing while this function executes - especially not the "twice cast" ScaleFactor variable which is only set once on power-up. |
|
|
PCM programmer
Joined: 06 Sep 2003
Posts: 21708
|
|
| Posted: Sun Oct 27, 2019 7:37 am |
|
|
| matthewsachs wrote: |
ADCtoFLOW(8, 7371) -> returned 7936 (instead of 1 expected)
ADCtoFLOW(2265, 7371) -> returned 36609 (instead of 307 expected)
ADCtoFLOW(4092, 7371) -> returned 28674 (instead of 555 expected)
|
I ran it with all three sets of parameters and it worked fine.
It gave the results below in MPLAB vs. 8.92 simulator:
| Quote: |
result = 1
result = 307
result = 555
|
I used version 5.090:
| Code: |
CCS PCH C Compiler, Version 5.090, xxxxx 27-Oct-19 06:34
Filename: C:\Program Files\PICC\Projects\PCH_Test\PCH_Test.lst
ROM used: 772 bytes (1%)
Largest free fragment is 64764
RAM used: 7 (0%) at main() level
33 (1%) worst case
Stack used: 0 locations
Stack size: 31
|
Test program:
| Code: |
#include <18F66K80.h>
#device adc=12
#use delay(internal=4M)
#use rs232(baud=9600, UART1, ERRORS)
int16 ADCtoFLOW(int16 ADCReading, int16 ScaleFactor)
{
int32 res = 0L;
res = ((int32)(ADCReading)) * ((int32)(1000L));
if(res <= ((int32)(ScaleFactor))) res = 0L;
else res = res / ((int32)(ScaleFactor));
return (int16)res;
}
//====================================
void main(void)
{
int16 result;
result = ADCtoFLOW(8, 7371);
printf("result = %lu \r", result);
while(TRUE);
} |
|
|
|
Ttelmah
Joined: 11 Mar 2010
Posts: 19544
|
|
| Posted: Sun Oct 27, 2019 8:18 am |
|
|
I'm wondering if we have a similar issue to the one we had just a few
threads ago, with the install having an old DLL somehow (multiple compilers
or something not cleaned away in the install). It was 5.085 that had the
casting issue.
I suggest deleting your compiler, and doing a clean re-install and seeing
if the problem then disappears. |
|
|
matthewsachs
Joined: 19 Jun 2016
Posts: 22
|
|
| Posted: Sun Oct 27, 2019 8:22 am |
|
|
Thanks PCM.
This is more along the lines how this is used in my application:
| Code: | #include <18F66K80.h>
#device adc=12
#use delay(internal=4M)
#use rs232(baud=9600, UART1, ERRORS)
int16 ADCR = 8L;
int16 SF = 7371L;
int16 ADCtoFLOW(int16 ADCReading, int16 ScaleFactor)
{
int32 res = 0L;
res = ((int32)(ADCReading)) * ((int32)(1000L));
if(res <= ((int32)(ScaleFactor))) res = 0L;
else res = res / ((int32)(ScaleFactor));
return (int16)res;
}
//====================================
void main(void)
{
int16 result;
result = ADCtoFLOW(ADCR, SF);
printf("result = %lu \r", result);
while(TRUE);
} |
Should I try to post the generated assembly code I see on my end with the two versions of this function? |
|
|
matthewsachs
Joined: 19 Jun 2016
Posts: 22
|
|
| Posted: Sun Oct 27, 2019 8:23 am |
|
|
| Ttelmah wrote: | I'm wondering if we have a similar issue to the one we had just a few
threads ago, with the install having an old DLL somehow (multiple compilers
or something not cleaned away in the install). It was 5.085 that had the
casting issue.
I suggest deleting your compiler, and doing a clean re-install and seeing
if the problem then disappears. |
Very interesting. I will try that right away. |
|
|
temtronic
Joined: 01 Jul 2010
Posts: 9244
Location: Greensville,Ontario
|
|
| Posted: Sun Oct 27, 2019 11:03 am |
|
|
Since you always zero 'res' at the beginning..
you 'might' get tighter code by doing this...
if( ((int32)(ADCReading)) * ((int32)(1000L)) > ((int32)(ScaleFactor))) res = res / ((int32)(ScaleFactor));
I did 'copy/paste' the code so maybe too many brackets..... |
|
|
matthewsachs
Joined: 19 Jun 2016
Posts: 22
|
|
| Posted: Sun Oct 27, 2019 11:12 am |
|
|
Deleting the old compiler (PICC folder and all) and re-installing a fresh version of 5.090 did NOT solve the problem.
Not a big deal as I have a workaround, but I would love to find out the root cause. |
|
|
Ttelmah
Joined: 11 Mar 2010
Posts: 19544
|
|
| Posted: Sun Oct 27, 2019 11:14 am |
|
|
I'd suggest:
if( ((ADCReading * 1000LL) > (int32)ScaleFactor) res/ =(int32)ScaleFactor);
It seems wasteful to cast a constant to int32. Just declare it as int32 (LL).
Also on a lot of compilers it is more efficient to use /=, rather than
val=val/
Separately though the optimiser should be ignoring the extra brackets,
having the variable bracketed potentially means this has to be
'evaluated', then 'cast', when it is the variable that just wants to be cast
itself. I think the optimiser will ignore this, but it seems silly to be
relying on this... 
It's not actually needed to cast both sides up in a sum. Only one side
needs to be cast, the other is automatically done. |
|
|
matthewsachs
Joined: 19 Jun 2016
Posts: 22
|
|
| Posted: Sun Oct 27, 2019 11:21 am |
|
|
| temtronic wrote: | Since you always zero 'res' at the beginning..
you 'might' get tighter code by doing this...
if( ((int32)(ADCReading)) * ((int32)(1000L)) > ((int32)(ScaleFactor))) res = res / ((int32)(ScaleFactor));
I did 'copy/paste' the code so maybe too many brackets..... |
Thanks - your recommendation is a lot cleaner than my if/else
I changed it to this
| Code: |
res = ((int32)(ADCReading)) * ((int32)(1000L));
if(res > ((int32)(ScaleFactor))) res = res / ((int32)(ScaleFactor));
return (int16)res; |
But it still returns with incorrect values.
If I store (int32)(ScaleFactor) in another temp variable and then use it twice, all is well. |
|
|
matthewsachs
Joined: 19 Jun 2016
Posts: 22
|
|
| Posted: Sun Oct 27, 2019 11:29 am |
|
|
| Ttelmah wrote: | I'd suggest:
if( ((ADCReading * 1000LL) > (int32)ScaleFactor) res/ =(int32)ScaleFactor);
It seems wasteful to cast a constant to int32. Just declare it as int32 (LL).
Also on a lot of compilers it is more efficient to use /=, rather than
val=val/
Separately though the optimiser should be ignoring the extra brackets,
having the variable bracketed potentially means this has to be
'evaluated', then 'cast', when it is the variable that just wants to be cast
itself. I think the optimiser will ignore this, but it seems silly to be
relying on this...
It's not actually needed to cast both sides up in a sum. Only one side
needs to be cast, the other is automatically done. |
Thanks for the tip and the rationale.
I also didn't know that "LL" was an option - this is great info since I am casting constants all over the place :-(
I tried a variation of your suggestion
| Code: | int32 res = 0L;
res = ((int32)(ADCReading)) * (1000LL);
if(res > ((int32)(ScaleFactor))) res /= ((int32)(ScaleFactor));
return (int16)res; |
and it's still returning wrong values.
Yet
| Code: |
int32 res = 0LL;
int32 scl_fctr = 0LL;
scl_fctr = ((int32)ScaleFactor);
res = ((int32)(ADCReading)) * (1000LL);
if(res > scl_fctr) res /= scl_fctr;
return (int16)res;
|
works as expected. |
|
|
Ttelmah
Joined: 11 Mar 2010
Posts: 19544
|
|
| Posted: Sun Oct 27, 2019 12:04 pm |
|
|
Having to use an intermediate variable, was exactly the problem I was seeing.
What is odd, is I have run up the code, and like PCM_Programmer, it is
working on my system!...
Uuurgh!.
I've tried V5.076, 5.078 (gives the wrong result), 5.080, 5.085 & 5.090.
The only one that gives a wrong result is 5.078.
Show your actual test program. Not just the routine, but the code being used
to call it. I'm wondering if there is something else giving the issue on what
you are doing. Has the sort of 'smell' of a variable being corrupted by
an interrupt handler or some similar oddity. |
|
|
matthewsachs
Joined: 19 Jun 2016
Posts: 22
|
|
| Posted: Sun Oct 27, 2019 1:48 pm |
|
|
I'll post the code shortly.
I also thought it may be a case of the value getting corrupted, but the wrong values are not randomly (or ever) changing. For every unique ADC value I get the same unique (but wrong) function result no matter if I cycle power or let it run for hours. |
|
|
|
FAQ 
Search
Register
Profile 
Log in to check your private messages 
Log in
