Continue to Site

Welcome to our site!

Electro Tech is an online community (with over 170,000 members) who enjoy talking about and building electronic circuits, projects and gadgets. To participate you need to register. Registration is free. Click here to register now.

  • Welcome to our site! Electro Tech is an online community (with over 170,000 members) who enjoy talking about and building electronic circuits, projects and gadgets. To participate you need to register. Registration is free. Click here to register now.

C18 compiler shortcomings.

Status
Not open for further replies.

Pommie

Well-Known Member
Most Helpful Member
I have today converted a project from BoostC to C18 and had a rather nasty bug. I finally tracked it down to one bit of code that wasn't working correctly. Here is that code in isolation,
Code:
#include <p18f1320.h>
#pragma config WDT = OFF, LVP = OFF, OSC = INTIO2

void main(){
char count;
unsigned int temp;
    OSCCON=0x60;                //Osc=4MHz
    ADCON1=0x7f;                //All digital
    count=0;
    temp=0b0000100000000000;
    while((temp&(1<<count))==0){
        count++;
    }
    while(1);
}
What this code should do is find the first non zero bit in temp and so count should contain 11 after it executes. Well, it doesn't, it contains 7. C18 works with bytes when it does 1<<count. What is even more peculiar is that temp=1<<11 will place zero in temp not 0x0800 as you would expect.

You can fix the above by doing while((temp&((int)1<<count))==0) or adding -Oi command line option.

So, why am I telling you this. Cause it took me a long time to find it and I just had to get it of my chest. Plus, it may help others.

BTW, the above worked fine in BoostC and it is the C18 compiler that doesn't comply with ISO.

Edit, One good point about C18 is that it's free.

Mike.
 
Last edited:
Sure, add it to that thread. It's not really a bug but can cause some nasty bugs.

Who would have thought that writing,
Code:
unsigned int temp=10*60;
would not be the same as,
Code:
unsigned int temp=600;

I always assumed that constants would be treated as 32 (or even 64) bits on the PC and cast to the appropriate size during assignment.

Mike.
 
It's not a bug, the C18 spec says right there it doesn't follow the ISO requirement of performing calcs at INT or above without the -Oi option. That's because it's an 8-bit core.

In an 8-bit core, automatically promoting every apparently 8-bit operand to 16-bits is potentially catastrophic for efficiency and code size. Every time you add a char to a char and put it in a char, it'd be promoting both to 16 bit only to ditch the upper byte anyways. C18 made a GOOD decision there.

The -Oi option is there, but it's not a really good idea and a decent code writer should cast how he wants a calculation to work.

I mean, I looked and saw right there "wow, this shift should not be coded like this without a cast".
 
Last edited:
It's not a bug, the C18 spec says right there it doesn't follow the ISO requirement of performing calcs at INT or above without the -Oi option. That's because it's an 8-bit core.

In an 8-bit core, automatically promoting every apparently 8-bit operand to 16-bits is potentially catastrophic for efficiency and code size. Every time you add a char to a char and put it in a char, it'd be promoting both to 16 bit only to ditch the upper byte anyways. C18 made a GOOD decision there.

The -Oi option is there, but it's not a really good idea and a decent code writer should cast how he wants a calculation to work.

I mean, I looked and saw right there "wow, this shift should not be coded like this without a cast".

I think I pointed out that it wasn't a bug and the available command line switch. I also agree that promoting everything to ints will be inefficient on an 8 bit core. When I converted the code it didn't throw up any warnings and appeared to run correctly. As you are a decent code writer, maybe you can answer how it managed to come out of the while loop with a value of 7 (BTW, I do know why). With the byte default it should have been an infinite loop.

What is even more disturbing is, int temp = 10*60 should place 600 in temp and not 88. The calculation of constants is carried out by the compiler (on the PC) and so doesn't effect the run time performance. Making constants 8 bit is just plain dumb. C18 made a REALLY BAD decision here.

Maybe you think that this is bad code,
Code:
#define minutes 10
void delaySeconds(unsigned int);

    delaySeconds(minutes*60);
Because it will delay 88 seconds on the C18 compiler, not 10 minutes as intended.

And you think it's OK because they mention it in the manual. The constants being 8 bit looks like a nasty bug waiting to bite unsuspecting programmers. Even MPASM treats constants correctly.

Mike.
 
Last edited:
At a minimum it should produce a compile-time warning. I find those extremely useful and can filter them very quickly by eye.

It should definitely be promoting to int when you are calling a function that is defined as taking an int as an argument!
 
A good reason to use 8-bit constants or data when transferring data between an ISR and main loop code is because operations can be atomic without disabling interrupts and increasing latency.

This is exactly why the volatile keyword exists. It really doesn't matter if you "transfer" one-byte or a hundred, you need to ensure that:

1) your ISR isn't going to get called again (disabling interrupts is correct in almost all cases)
2) the compiler is aware that the storage you are using may change due to external factors, so it can adjust optimization and access schemes appropriately

I'd ask what the difference is between the following two pieces of code:

Code:
// Assume char is 8 bits
volatile unsigned char rx_flag;
volatile unsigned char ready_flag;
volatile unsigned char coffee_time;

void interrupt() {
  // disable interrupts
  // check interrupt source etc.

  rx_flag = 1;
  ready_flag = 1;
  coffee_time = 1;
}

Code:
// Assume int on this platform is 16 bits
volatile unsigned int status;

void interrupt() {
  // disable interrupts
  // check interrupt source etc.
  status = 0x0003;
}

As long as you mark the variable as volatile, I don't see how the second example is any different. The compiler should ensure any action to modify it is atomic.

Am I wrong?
 
edeca,

In the second example,
Code:
// Assume int on this platform is 16 bits
volatile unsigned int status;

void interrupt() {
  // disable interrupts
  // check interrupt source etc.
  status = 0x0003;
}
If the previous status was 0x500, the main code reads the low byte of status, an interrupt occurs and then the high byte is read, status will have been read as 0x503 which may be invalid. This would (probably) only happen on very rare occasions and so would be a very hard bug to find. I have not come across a (pic 8 bit) compiler that will ensure 16 bit variables are written atomically.

A little trick that can be used on the Pic18 series is to make use of a free 16 bit timer to transfer 16 bits atomically. They can be read/written in a single instruction.

BTW, this thread had nothing to do with atomic/interrupt matters but it's an interesting subject anyway.

Mike.
 
edeca,

In the second example,
Code:
// Assume int on this platform is 16 bits
volatile unsigned int status;

void interrupt() {
  // disable interrupts
  // check interrupt source etc.
  status = 0x0003;
}
If the previous status was 0x500, the main code reads the low byte of status, an interrupt occurs and then the high byte is read, status will have been read as 0x503 which may be invalid. This would (probably) only happen on very rare occasions and so would be a very hard bug to find. I have not come across a (pic 8 bit) compiler that will ensure 16 bit variables are written atomically.

A little trick that can be used on the Pic18 series is to make use of a free 16 bit timer to transfer 16 bits atomically. They can be read/written in a single instruction.

BTW, this thread had nothing to do with atomic/interrupt matters but it's an interesting subject anyway.

Mike.

This is one reason for the decision to try and keep data 8 bits and not freely promote things to 16 bit unless it's clearly defined or overridden.
 
A little trick that can be used on the Pic18 series is to make use of a free 16 bit timer to transfer 16 bits atomically. They can be read/written in a single instruction.

Thanks for the explanation, that makes sense.

BTW, this thread had nothing to do with atomic/interrupt matters but it's an interesting subject anyway.

It's just one example of why you wouldn't want to automagically do it. I still think that the compiler should be sensible enough to look at the destination variable type (or function argument) to make a decision though. Sorry to thread hijack! :)
 
This is exactly why the volatile keyword exists. It really doesn't matter if you "transfer" one-byte or a hundred, you need to ensure that:

1) your ISR isn't going to get called again (disabling interrupts is correct in almost all cases)
2) the compiler is aware that the storage you are using may change due to external factors, so it can adjust optimization and access schemes appropriately

I'd ask what the difference is between the following two pieces of code:

Code:
// Assume char is 8 bits
volatile unsigned char rx_flag;
volatile unsigned char ready_flag;
volatile unsigned char coffee_time;

void interrupt() {
  // disable interrupts
  // check interrupt source etc.

  rx_flag = 1;
  ready_flag = 1;
  coffee_time = 1;
}
Code:
// Assume int on this platform is 16 bits
volatile unsigned int status;

void interrupt() {
  // disable interrupts
  // check interrupt source etc.
  status = 0x0003;
}
As long as you mark the variable as volatile, I don't see how the second example is any different. The compiler should ensure any action to modify it is atomic.

Am I wrong?
YUP, you're wrong! ;-) This is a common misunderstanding.

volatile does NOTHING to resolve an interrupt jumping in during the write of a variable and thus the ISR may read an incoherent mix of bytes, or may write it while the main() is reading or writing it, resulting in an incoherent value in the variable.

volatile is a directive to prevent optimization from assuming a variable has the same value it used to have. e.g.:
a=5;
b=a+5;

Will prevent any optimization from making this into b=10 (constant) in asm. No telling if your compiler would have done that in the first place... its exact implications are vague. But no, it does not attempt to resolve ISR conflicts at all, ever.

To prevent the ISR conflict, you MUST disable the relevant interrupt (or all interrupts) while reading variables which are written by the ISR, or while writing variables which may be read or written by an ISR. Then immediately reenable them. Or find some sort of guarantee in the program flow that this ISR simply can't occur while working with that variable.

Sometimes you want to be certain the ISR responds quickly, but the math equation to modify that variable is long. In this case you might calculate the value to a temp variable, disable the ISR, copy to the intended variable, and reenable the ISR. As such the ISR is disabled for only a few cycles.

Warning: in some cases, you might have ISRs getting enabled/disabled regularly for other reasons. Be careful you are not in a period where the interrupt was already disabled and needs to stay disabled, yet blindly disable, then reenable it as part of the main() math.

Also, note this problem is NOT limited to multi-byte variables. Consider:
In main():
volatile char a=50; //volatile will NOT help you at all
a+=2;

And the ISR clears "a" around about the same time.

Now depending on your compiler and PIC instruction set, it MIGHT use at atomic instruction to increment "a" in a file register. The PIC24 instruction set can do that if the compiler's smart enough. In that case, the code will either give a 2 or a 0, depending on exactly what cycle that ISR occurred on. But, if it's NOT atomic (and it probably won't be), you could get 52, and the ISR's clearing it will be lost forever. Because the main() will mov the value from the file register to wx, add 2, ISR clears the file register, then main(), unaware that this just happened, writes the a+2 value it's working on back to the file register, clobbering the value. Again, the point here is that this is NOT a multi-byte variable problem. Char.

When you go through it, there's no way what you assume is happening would be possible to implement.
There's no atomic-write or reads for multi-bytes in the instruction set. All it could do is copy the current ISR enable bit, clear the interrupt enable, then copy the current enable back, and that'd certainly cause problems for many people. Interrupts shouldn't spontaneously disable themselves like that.
And it wouldn't be guaranteed to fix things:
volatile a=50;
volatile b;
b=a;
b+=2;
;<<ISR clears "a" here
a=b;

Well, a simple, "dumb" automatic disabling of interrupts for the line where "a" is written will not fix this. The value from "a" was temporarily preserved elsewhere and will come back to clobber the ISR's clear, but there's no way the compiler could see that.
 
Last edited:
Status
Not open for further replies.

Latest threads

New Articles From Microcontroller Tips

Back
Top