pic 18F4550: local variable problem:

Question

I'm using the pic18F4550 with microchip v8.63 and with the C 18 compiler. I'm using a LDR that retrieve the value of the led (not on my picdem board) (red, green and blue) these values are stored in a variable after each conversion. Afther that when I press the button S2, I come into the method ISR: this part works.

But now: I try to compare the variable red, green and blue in the if's: but I think that it not happen, he just go to my 'else' (led RB3 on my picdem board burns).

#include <p18f4550.h> 
/** V E C T O R R E M A P P I N G *******************************************/
extern void _startup (void); // See c018i.c in your C18 compiler dir
#pragma code _RESET_INTERRUPT_VECTOR = 0x001000
void _reset (void)
{
 _asm goto _startup _endasm
}
#pragma code
void ISR (void);
#pragma code _HIGH_INTERRUPT_VECTOR = 0x001008
void _high_ISR (void)
{
 _asm goto ISR _endasm
}
#pragma code _LOW_INTERRUPT_VECTOR = 0x001018
void _low_ISR (void)
{
 ;
}
#pragma code
/******************************************************************************/
// global variable, value off LDR.
unsigned int var1ADRESH = 0x00;
unsigned int color_red = 0;
unsigned int color_green = 0;
unsigned int color_blue = 0;
void main (void)
{ 
 TRISD = 0x00; // PORTD als uitgang
 RCONbits.IPEN = 0; // prioriteit uit
 INTCONbits.GIE = 1; // enable interrupt
 INTCONbits.RBIE = 1; // interrupt portB aan
 //= set up port =
 TRISAbits.TRISA0 = 1; // Set RA0/AN0 to input
 //leds
 TRISAbits.TRISA3 = 0;
 TRISAbits.TRISA4 = 0;
 TRISAbits.TRISA5 = 0;
 LATAbits.LATA3 = 1;
 LATAbits.LATA4 = 1;
 LATAbits.LATA5 = 1;
 ADCON0 = 0b00000000; // Set channel select to AN0
 ADCON1 = 0b00001110; // Configure RA0/AN0 as analogue
 ADCON2 = 0b10101010; // Right justified result
 // TAD 12 and FOSC 32 - may need to adjust this
 // depending on your clock frequency (see datasheet)
 while(1)
 { 
 _asm sleep _endasm 
 }
}
#pragma interrupt ISR
void ISR (void)
{
 if (INTCONbits.RBIF==1) {
 //conversie blauw
 LATAbits.LATA3 = 0;
 ADCON0bits.ADON = 1; // Enable ADC 
 // read LDR value.
 ADCON0bits.GO = 1; // Set the GO bit of the ADCON0 register to start the conversion.
 while (ADCON0bits.GO); // Wait until the conversion is complete.
 ADCON2bits.ADFM = 0; // read result as 8-bit. (dus data in ADRESH) ! 
 //= read data in ADRESH =
 var1ADRESH = ADRESH; // reading value from LDR
 color_blue = ADRESH; //waarde in blauw
 //conversie rood
 LATAbits.LATA3 = 1;
 LATAbits.LATA4 = 0;
 ADCON0bits.ADON = 1; // Enable ADC 
 // read LDR value.
 ADCON0bits.GO = 1; // Set the GO bit of the ADCON0 register to start the conversion.
 while (ADCON0bits.GO); // Wait until the conversion is complete.
 ADCON2bits.ADFM = 0; // read result as 8-bit. (dus data in ADRESH) ! 
 //= read data in ADRESH =
 var1ADRESH = ADRESH; // reading value from LDR
 color_red = ADRESH; //waarde in blauwe steken
 //conversie groen
 LATAbits.LATA4 = 1;
 LATAbits.LATA5 = 0;
 ADCON0bits.ADON = 1; // Enable ADC 
 // read LDR value.
 ADCON0bits.GO = 1; // Set the GO bit of the ADCON0 register to start the conversion.
 while (ADCON0bits.GO); // Wait until the conversion is complete.
 ADCON2bits.ADFM = 0; // read result as 8-bit. (dus data in ADRESH) ! 
 //= read data in ADRESH =
 var1ADRESH = ADRESH; // reading value from LDR
 color_green = ADRESH; //waarde in blauwe steken
 // alles uitzetten
 //PORTB = 0b1111111;
 LATAbits.LATA5 = 1;
 if(color_blue > color_red && color_blue > color_green){
 //blauw
 LATDbits.LATD0 = 1;
 }
 if(color_red > color_blue && color_red > color_green){
 //rood
 LATDbits.LATD1 = 1;
 }
 if(color_green > color_red && color_green > color_blue){
 //groen
 LATDbits.LATD2 = 1;
 } 
 else {
 LATDbits.LATD3 = 1;
 } 
 }
 INTCONbits.RBIF = 0;
}

• Kellenjb: I was going to go edit your code block to make it easier to read, but boy is it messed up. Might be easier for you to just do it yourself. Edit your question, copy your code in fresh, and then highlight it, and select the code button.
• Erik: Generally, an interrupt should be handled 'as quickly as possible'. Doing things like an A to D conversion in an interrupt is not considered good design. Have your interrupt change a state variable so that your processing can be done in your main loop.

Answer 1

Just to mak Martin's response clearer:

if(color_blue > color_red && color_blue > color_green){
 //blauw
 LATDbits.LATD0 = 1;
}
if(color_red > color_blue && color_red > color_green){
 //rood
 LATDbits.LATD1 = 1;
}
if(color_green > color_red && color_green > color_blue){
 //groen
 LATDbits.LATD2 = 1;
} 
else {
 LATDbits.LATD3 = 1;
} 

should become

 if(color_blue > color_red && color_blue > color_green){
 //blauw
 LATDbits.LATD0 = 1;
 }
 else if(color_red > color_blue && color_red > color_green){
 //rood
 LATDbits.LATD1 = 1;
 }
 else if(color_green > color_red && color_green > color_blue){
 //groen
 LATDbits.LATD2 = 1;
 } 
 else {
 LATDbits.LATD3 = 1;
 } 

Answer 2

The else only ties to the last if, not to all the preceeding ifs. If you want that it ties to all the preceeding ifs, use an if/else if/else if/else structure.