Author Topic: PIC XC8 goes crazy when avoiding local variables  (Read 9424 times)

0 Members and 1 Guest are viewing this topic.

Offline miceuzTopic starter

  • Frequent Contributor
  • **
  • Posts: 387
  • Country: lt
    • chirp - a soil moisture meter / plant watering alarm
PIC XC8 goes crazy when avoiding local variables
« on: January 17, 2015, 02:44:41 pm »
I have a couple of simple functions:

Code: [Select]
uint16_t adcRead(uint8_t channel) {
    ADCON0bits.CHS = channel;
    ADCON0bits.GO=1;//start atod
    while(ADCON0bits.GO);
    return(ADRES);
}

uint16_t toMillivolts(uint16_t lsb) {
    return (unsigned int) (((unsigned long) 5000L * (unsigned long) lsb) / 1024L);//millivolts
}

When I do:

Code: [Select]
int temp = toTemperature(toMillivolts(adcRead(SENSOR_LIMIT)));

Function toTemperature gets '5000' as an argument, i.e. toMillivolts yelds 0. I've stepped thru the code and have seen that adcRead() does return non zero value that should translate into ~900mV.

When I do this, everything works as expected.:
Code: [Select]
    uint16_t lsb = adcRead(SENSOR_ROOM);
    uint16_t millivolts = toMillivolts(lsb);
    if(SENSOR_ROOM == sensor) {
        millivolts = 5000 - millivolts;
    }
    int temp = toTemperature(millivolts);


I'm using XC8 v1.33. I'm new to the Microchip world, is their compiler really such a crap or am I missing something? What about PRO version? Are they going to throw such shit at me even after I pay 2k$ for a licence?

Offline dannyf

  • Super Contributor
  • ***
  • Posts: 8221
  • Country: 00
Re: PIC XC8 goes crazy when avoiding local variables
« Reply #1 on: January 17, 2015, 03:16:52 pm »
Quote
am I missing something?

More likely.

If you can establish the issue, the story is more believable.

================================
https://dannyelectronics.wordpress.com/
 

Offline amyk

  • Super Contributor
  • ***
  • Posts: 8232
Re: PIC XC8 goes crazy when avoiding local variables
« Reply #2 on: January 17, 2015, 04:38:02 pm »
Look at the instructions it generated.
 

Offline macboy

  • Super Contributor
  • ***
  • Posts: 2250
  • Country: ca
Re: PIC XC8 goes crazy when avoiding local variables
« Reply #3 on: January 19, 2015, 02:14:17 pm »
Your function:
Code: [Select]
uint16_t toMillivolts(uint16_t lsb) {
    return (unsigned int) (((unsigned long) 5000L * (unsigned long) lsb) / 1024L);//millivolts
}
...has a return value of "uint16_t".
But it is casting the result of the cacluation as "unsigned int" before returning it.  :o

What size is an "int" (and an "unsigned int") in XC8? Is it 8 or 16 bits? Are you sure? ANSI requires at least 16 bits, but did you do everything necessary to make the compiler enforce ANSI in these types? Did you include limits.h?

I would guess that changing the cast to a uint16_t will fix the problem.

Avoid confusion and never use the implementation specific "int" types; use the fixed size types uint16_t, etc., according to the size you actually need.
 

Offline Volta

  • Newbie
  • Posts: 5
Re: PIC XC8 goes crazy when avoiding local variables
« Reply #4 on: January 19, 2015, 03:05:03 pm »
In the compiler manual http://ww1.microchip.com/downloads/en/DeviceDoc/52053B.pdf, page 143, is stated that a int is 16 bits wide.

I suggest really dissecting that toMillivolts function.
Maybe by splitting the calculation and then stepping through it.
Or what I like to do is to write the offending function in a 'standard' pc program like python or just c and debugging it there.
Also try playing with various optimization settings of xc8

And yes, in my experience xc8 is not very good. For the 8bit PIC's I tend to stick to assembly, or just use the much better PIC32 or PIC24 series and compilers.
 

Offline dannyf

  • Super Contributor
  • ***
  • Posts: 8221
  • Country: 00
Re: PIC XC8 goes crazy when avoiding local variables
« Reply #5 on: January 19, 2015, 03:19:43 pm »
"I  suggest really dissecting that toMillivolts function. "


I tend to think before you attempt to solve a probleem, you have to establish that the said problem actually exists.

I for one don't think that such a problem actually exists.

Yes, I have done my homework on this, from xc8 1.12, 1.32 to 1.33.
================================
https://dannyelectronics.wordpress.com/
 

Offline jerry507

  • Regular Contributor
  • *
  • Posts: 247
Re: PIC XC8 goes crazy when avoiding local variables
« Reply #6 on: January 20, 2015, 03:21:02 am »
Two minor issues. First, as macboy said, stick with the stdint types. There really is no reason to use most of the C data types (int, long, etc). The stdint types (int8_t etc) are always preferred.

The second is to avoid these ridiculously long lines.

Code: [Select]
return (unsigned int) (((unsigned long) 5000L * (unsigned long) lsb) / 1024L);//millivolts
Really?

Code: [Select]
uint16_t toMillivolts(uint16_t lsb) {
    uint16_t value = (uint32_t)5000 * (uint32_t)lsb;
    value /= (uint32_t)1024;
    return value;//millivolts
}

Does exactly the same thing but is considerably easier to single step though. Personally I consider it easier to read, and that's an opinion. But to step through that combined calculation to figure out if you've got some type related issue would require instruction stepping, vs just setting a breakpoint. Like I said, minor issues but I consider the extra simplicity of the breakpoints a lifesaver. You don't need to theorycraft why it might not work, you just step through and find the line with the problem. Then you can figure out why it didn't work with a more specific intent.
 

Offline miguelvp

  • Super Contributor
  • ***
  • Posts: 5550
  • Country: us
Re: PIC XC8 goes crazy when avoiding local variables
« Reply #7 on: January 20, 2015, 06:04:48 am »
@jerry507

That uint16_t value is going to hold a truncated value, so it should be uint32_t and then cast the result to uint16_t
 

Offline miceuzTopic starter

  • Frequent Contributor
  • **
  • Posts: 387
  • Country: lt
    • chirp - a soil moisture meter / plant watering alarm
Re: PIC XC8 goes crazy when avoiding local variables
« Reply #8 on: January 20, 2015, 10:09:02 pm »
Actually those problems are hounting me in this project. No compiler warnings, no nothing, but sometimes it seems I get problems by inlining function calls or calculations. Just drives me crazy. One before that was that suddenly LCD was displaying garbage if I just add another global 8, 16 or 32 bit variable. No problem if I add a MenuItem_t struct variable. Tracked it down to a function
Code: [Select]
uint8_t isDialog() {
    return 0 != menuGetCurrent()->dialogHandler;
}
It was returning true for cases when there really was no dialogHandler.

After replacing menuGetCurrent call with a local variable - bah - everything works just nice.

One another thing - debugger gets thrown out from time to time - just breaks not on the line where the breakpoint is. At least it displays it's on a different line. Crazy I know.

Offline Howardlong

  • Super Contributor
  • ***
  • Posts: 5313
  • Country: gb
Re: PIC XC8 goes crazy when avoiding local variables
« Reply #9 on: January 20, 2015, 10:35:07 pm »
I have to agree with Amyk: look, and step through, at the code that's generated, it can reveal an awful lot. Turn any optimisations off that you may be using first to ease the pain.
 

Offline jerry507

  • Regular Contributor
  • *
  • Posts: 247
Re: PIC XC8 goes crazy when avoiding local variables
« Reply #10 on: January 21, 2015, 02:16:16 am »
@jerry507

That uint16_t value is going to hold a truncated value, so it should be uint32_t and then cast the result to uint16_t

I generally dislike adding extra things that aren't needed. Your point about the truncated value is good, so here is the modification:

Code: [Select]
uint16_t toMillivolts(uint16_t lsb) {
    uint16_t value = (uint32_t)5000 * (uint32_t)lsb;
    value = (uint32_t)value / (uint32_t)1024;
    return value;//millivolts
}

It's unnecessary to cast the result into a uint16_t, the assignment does that. Technically, the cast of value to a uint32_t is unnecessary too because C's type promotion rules will bring the uint16_t to a uint32_t before doing the division. However, that's a bit of a dark area in C so erring on the side of transparency is better.

Code: [Select]
uint8_t isDialog() {
    return 0 != menuGetCurrent()->dialogHandler;
}

Not to harp too much on the same idea, but you'll have a heck of a time debugging this one too. You can't simply find out what menuGetCurrent() returns, nor what menuGetCurrent()->dialogHandler is without nasty instruction single stepping.

Code: [Select]
uint8_t isDialog() {
    uint8_t returnValue = menuGetCurrent();
    returnValue = returnValue->dialogHandler;
    return (0 != returnValue);
}

Is EXACTLY the same thing. Now this is probably wrong for your specific code since I obviously don't know what menuGetCurrent() returns, but go ahead and explicitly create the variable. In nearly all cases, you're not consuming any additional memory unless it's a bad compiler or a very very limited platform. You're poking your code with a stick from 10 feet away (just changing the location of the variables etc), when you could just directly debug the return values and statements. It takes about 10-15 seconds more to write it explicitly, and saves at worst minutes, if not tens of minutes or hours, of debugging time just doing permutations of code.
 

Offline miceuzTopic starter

  • Frequent Contributor
  • **
  • Posts: 387
  • Country: lt
    • chirp - a soil moisture meter / plant watering alarm
Re: PIC XC8 goes crazy when avoiding local variables
« Reply #11 on: January 21, 2015, 06:57:31 am »

Code: [Select]
uint8_t isDialog() {
    return 0 != menuGetCurrent()->dialogHandler;
}

Not to harp too much on the same idea, but you'll have a heck of a time debugging this one too. You can't simply find out what menuGetCurrent() returns, nor what menuGetCurrent()->dialogHandler is without nasty instruction single stepping.

Code: [Select]
uint8_t isDialog() {
    uint8_t returnValue = menuGetCurrent();
    returnValue = returnValue->dialogHandler;
    return (0 != returnValue);
}

Is EXACTLY the same thing. Now this is probably wrong for your specific code since I obviously

Yes it is exactly the same thing except that (1) you create a local variable (2) you are assigning two things to the variable that are totally conceptually different. IMHO This is just a poor programming style, sorry.

I usually avoid creating local variables that don't make code stink less (as per M.Fawler "Refactoring: Improving the Design of Existing Code" -- great read) I'm not trying to save on code size here, I still have plenty; another variable - another potential problem to screw things up, another thing to manage in my head.

In this particular case I have to create a local variable because compiler makes wrong code otherwise or because I can't single-step it -- in both cases this is just wrong - I have to put a potential point of failure into my code for no good reason related to what I'm trying express.

My assembly skills are rusty - last time programmed anything in asm was 20 years ago, just little poking around in microcontroller code recently, but I will try to make a sample test bed and will see what is the difference.

Offline miguelvp

  • Super Contributor
  • ***
  • Posts: 5550
  • Country: us
Re: PIC XC8 goes crazy when avoiding local variables
« Reply #12 on: January 21, 2015, 08:09:34 am »
@jerry507

That uint16_t value is going to hold a truncated value, so it should be uint32_t and then cast the result to uint16_t

I generally dislike adding extra things that aren't needed. Your point about the truncated value is good, so here is the modification:

Code: [Select]
uint16_t toMillivolts(uint16_t lsb) {
    uint16_t value = (uint32_t)5000 * (uint32_t)lsb;
    value = (uint32_t)value / (uint32_t)1024;
    return value;//millivolts
}


Still will overflow if lsb is 14 or bigger so value has to be a uint32_t.
To illustrate: 14*5000 is equal to 70000 which is greater than 65535 (max uint16_t) so you will end up with 4464 instead divided by 1024 will return 4 instead of the real answer 68.
« Last Edit: January 21, 2015, 08:12:44 am by miguelvp »
 

Offline jerry507

  • Regular Contributor
  • *
  • Posts: 247
Re: PIC XC8 goes crazy when avoiding local variables
« Reply #13 on: January 21, 2015, 04:54:25 pm »
The variable is created either way. You don't think somehow you used less memory by writing it that way, do you? Like I said, it depends on the compiler, but there are temporary variables generated to hold the results. If you are using a fairly old compiler, or a very basic one, it may be simple and create the variable that otherwise wouldn't be necessary. But in general, that doesn't happen anymore. There are tons of implementation details about whether it does use more memory or not, but unless you're absolutely that starved, there's no reason.

Likely, the problem is your code. Refactor it and make sure it works, and then you can simplify it again.

And no, it won't overflow. Two uint32_t's on the right side will yield a result that is a uint32_t. The cast happens during the assignment of the result to the left hand side. There's no need to use another uint32_t to store the result temporarily.
 

Offline miguelvp

  • Super Contributor
  • ***
  • Posts: 5550
  • Country: us
Re: PIC XC8 goes crazy when avoiding local variables
« Reply #14 on: January 21, 2015, 05:11:51 pm »
It won't overflow on the internal memory but when you assign it it will truncate it to 16 bits, so it will have the same result as an overflow.

So this:
uint16_t value = (uint32_t)5000 * (uint32_t)lsb;

Only have valid results for lsb [0-13] and will have truncated results from [14-65535].

Not sure how you can't see this.

 

Offline jerry507

  • Regular Contributor
  • *
  • Posts: 247
Re: PIC XC8 goes crazy when avoiding local variables
« Reply #15 on: January 22, 2015, 03:26:41 am »
I see what you're talking about, but since I am only refactoring the code I am not changing the function. Macboy touched on the overflow issue, so why would I? The overflow is there regardless of whether you assign it to a temp variable or return it from a function with a return value of uint16_t.
 

Offline miguelvp

  • Super Contributor
  • ***
  • Posts: 5550
  • Country: us
Re: PIC XC8 goes crazy when avoiding local variables
« Reply #16 on: January 22, 2015, 03:43:16 am »
I see what you're talking about, but since I am only refactoring the code I am not changing the function. Macboy touched on the overflow issue, so why would I? The overflow is there regardless of whether you assign it to a temp variable or return it from a function with a return value of uint16_t.

Not really, his code will allow inputs of lsb from [0-13421] before overflowing or truncating the result allowing results from 0V to 65532mV at an average of 4.88 mV increments truncated to the lower integral value which is better than from [0-13] in your refactoring by three orders of magnitude.

If you look at the original return instruction you'll see that the internal math is all done in unsigned 32 bit and only truncated after the 1024 division or shifting the multiplication result 10 bits to the right.

return (unsigned int) (((unsigned long) 5000L * (unsigned long) lsb) / 1024L);//millivolts
 

Offline jerry507

  • Regular Contributor
  • *
  • Posts: 247
Re: PIC XC8 goes crazy when avoiding local variables
« Reply #17 on: January 22, 2015, 04:35:20 am »
Do you even see what I was trying to get at with the refactor? My code sample doesn't match original functionality, but it wasn't the point. Just illustration. Forum pedantics...
 

Offline miguelvp

  • Super Contributor
  • ***
  • Posts: 5550
  • Country: us
Re: PIC XC8 goes crazy when avoiding local variables
« Reply #18 on: January 22, 2015, 04:42:03 am »
That's why I mentioned that your value variable should be declared as a uint32_t and the return casted to a uint16_t so that it returns the same value range and operates the same as the original function.

I do agree on not doing a single one liner and I agree that it's easier to debug when doing intermediate operations, I never disagreed on that, actually I strongly agree with it.

That said, forum pedantics are the difference of having a function that can accurately give you measurements from 0V to 65532mV to one that can only accurately give you measurements from 0V to 63mV.
 

Offline jerry507

  • Regular Contributor
  • *
  • Posts: 247
Re: PIC XC8 goes crazy when avoiding local variables
« Reply #19 on: January 22, 2015, 04:46:49 am »
Sorry miguelvp! That was a misunderstanding on my part. I imagined disagreement where there was none. Thanks for the correction :)
 


Share me

Digg  Facebook  SlashDot  Delicious  Technorati  Twitter  Google  Yahoo
Smf