18f2420 & MCC18 - Is this ISR sensible?

[Angry Badger]

Hi,

Trying to get my first ISR in C18 working. Seems to now. Copied part of some code from here: Problem w/ PORT B Interrupt-On-Change

It's this from that site:

#include <p18f458.h>
 #include <max7221.h>
 #include <temp.h>
 #include <portb.h>
 
 /* Set configuration bits for use with ICD2 / PICDEM2 PLUS Demo Board:
  *  - set HS oscillator
  *  - disable watchdog timer
  *  - disable low voltage programming
  */
 #pragma config OSC = HS
 #pragma config WDT = OFF
 #pragma config LVP = OFF
 
 int i;
 unsigned char portB_val;
 
 void user_input_isr(void);
 
 #pragma code interrupt_vector = 0x08
 void high_interrupt(void)
 {
        _asm
        goto user_input_isr
        _endasm
 }
 #pragma code
 
 #pragma interrupt user_input_isr
 void user_input_isr(void)
 {
        portB_val = PORTB; //Read PORTB to enable the RBIF flag to be cleared (is this right?)
        DisplayTemp(20.3, 'C'); //Display the temp 20.3C to the LED display if we get in here
        INTCONbits.RBIF = 0; //Clear the PORTB RBIF flag
 }
 
 void EnableInterrupts (void)
 {
        INTCONbits.RBIE = 1; //Ensure PORTB interrupts enabled
        INTCONbits.RBIF = 0; //Clear the PORTB interrupt flag
        INTCONbits.PEIE = 1; //Enable peripheral interrupts (don't think i really need this one
        INTCONbits.GIE = 1; //Enable all interrupts
 }
 
 void main (void)
 {
        MAX7221_Init();
 
        OpenPORTB(PORTB_CHANGE_INT_ON & PORTB_PULLUPS_ON);
 
        EnableInterrupts(); //Setup the interrupts
 
        DisplayTemp(15.5, 'C');  //Display the temperature 15.5C to the LED display
 
        while(1);
 }

So I made it work in my little prog and found that it worked even if I negated some parts like

#pragma code interrupt_vector = 0x008

and;

void Interrupt(void)
{
_asm
goto button_ISR
_endasm
},

and;

#pragma code

Still finding my way around C so not sure why those were in the original code.

Is my code sensible?

#include <p18f2420.h>
#include <delays.h>                    //the code for the delay routines is in here

#pragma config OSC  = INTIO7         //Internal oscillator with Fosc/4 on RA6, I/O on RA7
#pragma config WDT = OFF
#pragma config PBADEN = ON            //portb bits 0 thru 4 as digital input (analog by default at POR)

#define button         PORTBbits.RB0    //pin 21
#define led            PORTAbits.RA5    //pin 7

void button_ISR(void);

//#pragma code interrupt_vector = 0x008 IS THIS REQUIRED? Works ok without.

//void Interrupt(void)    //REQUIRED? Works ok without. Does interrupt automatically go to 0x08 by default?
 //{
//    _asm 
//    goto button_ISR 
//    _endasm
 //}
 

//#pragma code    IS THIS REQUIRED? Works ok without.

#pragma interrupt button_ISR

void button_ISR (void)
{
            Delay1KTCYx(15);        //wait 15mS
            if (button ==0)            //check button still down
            {
                if (led == 0)        // and led is off
                led = 1;            // turn it on
                else if (led ==1)    // but if it's already on
                led = 0;            // turn it off
            }
        
        while (button ==0);            // stay here until the button is released
        Delay1KTCYx(15);            //wait 15mS
        INTCONbits.INT0IF = 0;        // clear the interrupt flag
}

void interrupts_init (void)
{    
    INTCON2bits.INTEDG0 = 0;        // interrupt on falling edge
    INTCONbits.INT0IE = 1;            // enables INT0 external interrupt (RB0)
    INTCONbits.INT0IF = 0;            // clear the interrupt flag
    INTCONbits.GIE = 1;                // enable all unmasked interrupts
}

void main (void)
{
    OSCCON = 0b01100010;             // set Fosc = 4Mhz = 1uS instruction cycle time
    ADCON1 = 0xF;                    // sets all analog channels as digital inputs
    
    INTCON2bits.RBPU = 0;            // enable PORTB internal pullups
    TRISBbits.TRISB0 = 1;           // PORTB bit 0 (connected to switch) is input (1)
    TRISAbits.TRISA5 = 0;            // PORTA bit 5 (connected to led) is an output (0)
    
    PORTA = 0;                        // (ensure led to be off at start up)
    PORTB = 0;                        // clear portb
    
    interrupts_init();                // do the interrupts enable routine

    while(1);                        // stay here until the button is pressed

}

Thanks in advance for any help.

 

[nsaspook]

As you say the compiler defaults to the high interrupt vector so unless your code uses both low and high vectors it's extra but I would always add it for completeness and readability.

You don't want to wait for button presses in the ISR routine. Set a flag or semaphore to be used by the main program flow with the button state if needed.