Timer0 callback function on pic18f46k22

[nikouz]

Hi happy new year all

I am trying to set an interrupt and a callback using the mplab code configurator. There is something that i cannot understand. The timer overfow every 500ms and blinking an led. The callback comes after 15s and blinks an led in the main.c setting a variable. Now i have set as TMR0_SetInterruptHandler(TMR0_ISR); Inside the TMR0_ISR calls the TMR0_CallBack(void) to set the callback variable for the main.c. The problem is:
If i set the callback variable in the TMR0_CallBack(void) Then the timer runs ok the led1 blinking 500ms but when the callback kicks in the timer is stucking and the led1 stays on.
Now if inside the TMR0_ISR instead of calling the TMR0_CallBack(void) i set the variable everything works fine means led1 blinkiking @500ms and the led2 @15s. I enclose the codes: first is the main.c and then the tmr0.c made first with the way is not working and then with the way is working. In the code i mark with orange the changes to be easier to see. If someone can explain what am i doing wrong.

main.c

#include "mcc_generated_files/mcc.h"

char ioc_interrupt = 0, tmr0_callback = 0;

/*
                         Main application
 */
void main(void)
{
    // Initialize the device
    SYSTEM_Initialize();

    // If using interrupts in PIC18 High/Low Priority Mode you need to enable the Global High and Low Interrupts
    // If using interrupts in PIC Mid-Range Compatibility Mode you need to enable the Global and Peripheral Interrupts
    // Use the following macros to:

    // Enable high priority global interrupts
    INTERRUPT_GlobalInterruptHighEnable();

    // Enable low priority global interrupts.
    //INTERRUPT_GlobalInterruptLowEnable();

    // Disable high priority global interrupts
    //INTERRUPT_GlobalInterruptHighDisable();

    // Disable low priority global interrupts.
    //INTERRUPT_GlobalInterruptLowDisable();

    // Enable the Peripheral Interrupts
    INTERRUPT_PeripheralInterruptEnable();

    // Disable the Peripheral Interrupts
    //INTERRUPT_PeripheralInterruptDisable();

    while (1)
    {
        // Add your application code
        if(tmr0_callback)
        {
            tmr0_callback = 0;
            LED3_RD3_SetHigh();
            __delay_ms(2000);
            LED3_RD3_SetLow();
        }
    }
}
/**
 End of File
*/

tmr0.c bad version

/**
  Section: Included Files
*/

#include <xc.h>
#include "tmr0.h"
#include "mcc.h"

extern char tmr0_callback;

/**
  Section: Global Variables Definitions
*/

void (*TMR0_InterruptHandler)(void);

volatile uint16_t timer0ReloadVal;

/**
  Section: TMR0 APIs
*/


void TMR0_Initialize(void)
{
    // Set TMR0 to the options selected in the User Interface

    //Enable 16bit timer mode before assigning value to TMR0H
    T0CONbits.T08BIT = 0;

    // TMR0H 11;
    TMR0H = 0x0B;

    // TMR0L 219;
    TMR0L = 0xDB;

    
    // Load TMR0 value to the 16-bit reload variable
    timer0ReloadVal = (uint16_t)((TMR0H << 8) | TMR0L);

    // Clear Interrupt flag before enabling the interrupt
    INTCONbits.TMR0IF = 0;

    // Enabling TMR0 interrupt.
    INTCONbits.TMR0IE = 1;

    // Set Default Interrupt Handler
    TMR0_SetInterruptHandler(TMR0_ISR);

    // T0PS 1:2; T08BIT 16-bit; T0SE Increment_hi_lo; T0CS FOSC/4; TMR0ON enabled; PSA assigned;
    T0CON = 0x90;
}

void TMR0_StartTimer(void)
{
    // Start the Timer by writing to TMR0ON bit
    T0CONbits.TMR0ON = 1;
}

void TMR0_StopTimer(void)
{
    // Stop the Timer by writing to TMR0ON bit
    T0CONbits.TMR0ON = 0;
}

uint16_t TMR0_ReadTimer(void)
{
    uint16_t readVal;
    uint8_t readValLow;
    uint8_t readValHigh;

    readValLow  = TMR0L;
    readValHigh = TMR0H;
    readVal  = ((uint16_t)readValHigh << 8) + readValLow;

    return readVal;
}

void TMR0_WriteTimer(uint16_t timerVal)
{
    // Write to the Timer0 register
    TMR0H = timerVal >> 8;
    TMR0L = (uint8_t) timerVal;
}

void TMR0_Reload(void)
{
    // Write to the Timer0 register
    TMR0H = timer0ReloadVal >> 8;
    TMR0L = (uint8_t) timer0ReloadVal;
}

void TMR0_ISR(void)
{
    static volatile uint16_t CountCallBack = 0;

    // clear the TMR0 interrupt flag
    INTCONbits.TMR0IF = 0;

    // reload TMR0
    // Write to the Timer0 register
    TMR0H = timer0ReloadVal >> 8;
    TMR0L = (uint8_t) timer0ReloadVal;

    // callback function - called every 30th pass
    if (++CountCallBack >= TMR0_INTERRUPT_TICKER_FACTOR)
    {
        // ticker function call
        TMR0_CallBack();

        // reset ticker counter
        CountCallBack = 0;
    }

    // add your TMR0 interrupt custom code
    LED1_RC4_Toggle();
}

void TMR0_CallBack(void)
{
    // Add your custom callback code here
    tmr0_callback = 1;

    if(TMR0_InterruptHandler)
    {
        TMR0_InterruptHandler();
    }
}

void TMR0_SetInterruptHandler(void (* InterruptHandler)(void)){
    TMR0_InterruptHandler = InterruptHandler;
}

void TMR0_DefaultInterruptHandler(void){
    // add your TMR0 interrupt custom code
    // or set custom function using TMR0_SetInterruptHandler()
}

/**
  End of File
*/

tmr0.c working version

/**
  Section: Included Files
*/

#include <xc.h>
#include "tmr0.h"
#include "mcc.h"

extern char tmr0_callback;

/**
  Section: Global Variables Definitions
*/

void (*TMR0_InterruptHandler)(void);

volatile uint16_t timer0ReloadVal;

/**
  Section: TMR0 APIs
*/


void TMR0_Initialize(void)
{
    // Set TMR0 to the options selected in the User Interface

    //Enable 16bit timer mode before assigning value to TMR0H
    T0CONbits.T08BIT = 0;

    // TMR0H 11;
    TMR0H = 0x0B;

    // TMR0L 219;
    TMR0L = 0xDB;

    
    // Load TMR0 value to the 16-bit reload variable
    timer0ReloadVal = (uint16_t)((TMR0H << 8) | TMR0L);

    // Clear Interrupt flag before enabling the interrupt
    INTCONbits.TMR0IF = 0;

    // Enabling TMR0 interrupt.
    INTCONbits.TMR0IE = 1;

    // Set Default Interrupt Handler
    TMR0_SetInterruptHandler(TMR0_ISR);

    // T0PS 1:2; T08BIT 16-bit; T0SE Increment_hi_lo; T0CS FOSC/4; TMR0ON enabled; PSA assigned;
    T0CON = 0x90;
}

void TMR0_StartTimer(void)
{
    // Start the Timer by writing to TMR0ON bit
    T0CONbits.TMR0ON = 1;
}

void TMR0_StopTimer(void)
{
    // Stop the Timer by writing to TMR0ON bit
    T0CONbits.TMR0ON = 0;
}

uint16_t TMR0_ReadTimer(void)
{
    uint16_t readVal;
    uint8_t readValLow;
    uint8_t readValHigh;

    readValLow  = TMR0L;
    readValHigh = TMR0H;
    readVal  = ((uint16_t)readValHigh << 8) + readValLow;

    return readVal;
}

void TMR0_WriteTimer(uint16_t timerVal)
{
    // Write to the Timer0 register
    TMR0H = timerVal >> 8;
    TMR0L = (uint8_t) timerVal;
}

void TMR0_Reload(void)
{
    // Write to the Timer0 register
    TMR0H = timer0ReloadVal >> 8;
    TMR0L = (uint8_t) timer0ReloadVal;
}

void TMR0_ISR(void)
{
    static volatile uint16_t CountCallBack = 0;

    // clear the TMR0 interrupt flag
    INTCONbits.TMR0IF = 0;

    // reload TMR0
    // Write to the Timer0 register
    TMR0H = timer0ReloadVal >> 8;
    TMR0L = (uint8_t) timer0ReloadVal;

    // callback function - called every 30th pass
    if (++CountCallBack >= TMR0_INTERRUPT_TICKER_FACTOR)
    {
        // ticker function call
        tmr0_callback = 1;

        // reset ticker counter
        CountCallBack = 0;
    }

    // add your TMR0 interrupt custom code
    LED1_RC4_Toggle();
}

void TMR0_CallBack(void)
{
    // Add your custom callback code here

    if(TMR0_InterruptHandler)
    {
        TMR0_InterruptHandler();
    }
}

void TMR0_SetInterruptHandler(void (* InterruptHandler)(void)){
    TMR0_InterruptHandler = InterruptHandler;
}

void TMR0_DefaultInterruptHandler(void){
    // add your TMR0 interrupt custom code
    // or set custom function using TMR0_SetInterruptHandler()
}

/**
  End of File
*/

.c working version

 

[KlausST]

Hi,

there are a lot of mistakes and misunderstandings ..and missing informations.

if(tmr0_callback)

(please avoid using the same name for two different things, just differentiated by capitals.)

a callback is a function .. containing code that is supposed to be processed.
but you treat it as a variable.

Thus I´m confused.
* do you want it to be a variable ... then don´t name it "callback"
* do you want a function... then treat it as a function. Here the code generator even generated a frame of code ... and it says you should include YOUR function code there.

void TMR0_CallBack(void) { // Add your custom callback code here

***
This:

tmr0_callback = 0; 
LED3_RD3_SetHigh();
 __delay_ms(2000); 
LED3_RD3_SetLow();

exactly is what a interrupt function is meant to avoid. You don´t want to use a "delay(2000)" at all, because it stalls your processor for a total of 2 seconds .. and this only to switch OFF a LED.
During this time your microcontroller could do several million of USEFUL instructions ... while you waste the processing power.

Btw: a "2000 ms" will never lead to a 500ms blinking time. I don´t understand the idea.

***

static volatile uint16_t CountCallBack = 0;

I don´t think this makes sense. (But I´m not a C expert)
You declared this variabe INSIDE a function. Thus I expect it not to be accessable outside this fucntion.
--> declare this variable as a global variable.

***
A lot of informations are missing.
Like we can not see the meaning of the tmr0 initialisation.
I don´t know if it´s up or down counting, what is your system clock frequency, do you expect it to repeat -or is it a one shot?
--> add comments to your code, especially when you want other people to understand it

***

There are more issues ...

***
I guess you are not familiar yet how interrupts work and how to use them.
--> there are tutorials explaining this topic. Even videos --> go through some of them.
Choose the source that suits you most, provide a link, and ask specific questions on a topic referring to the source.

Klaus

 

[nikouz]

Hi all
I am sorry but i think my question create more issues because is difficult to explain sometimes by writing. And this is my mistake so i will try to take it better to help all yous trying to help me and not to torture you. Forget what i wrote in #1. Below is a main.c which is working fine. This IOC interrupt for testing reasons is turning on an a LED. What i am trying to do is to express the same code using the MCC configurator but i am failing.
The push button logic is 1.

This is working

#include "mcc_generated_files/mcc.h"
volatile char previous_PORTB_RB4_State = 1; // saves the prevoius RB4 state

/*
                         Main application
 */
void main(void)
{
    // Initialize the device
    SYSTEM_Initialize();
    INTCON2bits.nRBPU = 1;
    // initialize the interrupts
    RCONbits.IPEN = 1;   // enable priorities
    INTCONbits.GIEH = 1; // enable high priority interrupt
    // initialize the IOC interrupt
    IOCBbits.IOCB4 = 1;  // enable IOC interrupt on RB4 pin
    IOCBbits.IOCB5 = 1;  // enable IOC interrupt on RB5 pin
    IOCBbits.IOCB6 = 1;  // enable IOC interrupt on RB6 pin
    IOCBbits.IOCB7 = 1;  // enable IOC interrupt on RB7 pin
    INTCON2bits.RBIP = 1;// configure as high priority
    INTCONbits.RBIF = 0; // clear the IOC interrupt flag
    INTCONbits.RBIE = 1; // enable the interrupt

    // If using interrupts in PIC18 High/Low Priority Mode you need to enable the Global High and Low Interrupts
    // If using interrupts in PIC Mid-Range Compatibility Mode you need to enable the Global and Peripheral Interrupts
    // Use the following macros to:

    // Enable the Global Interrupts
    //INTERRUPT_GlobalInterruptEnable();

    // Disable the Global Interrupts
    //INTERRUPT_GlobalInterruptDisable();

    // Enable the Peripheral Interrupts
    //INTERRUPT_PeripheralInterruptEnable();

    // Disable the Peripheral Interrupts
    //INTERRUPT_PeripheralInterruptDisable();

    while (1)
    {
        // Add your application code
        __delay_ms(1000);
        LED_RC4_SetLow();
    }
}

void __interrupt(high_priority) ISR_high()
{
    // read current state of PORTB
    char current_PORTB_state = PORTB;
    // we extract the state of the pins separate
    char current_RB4_state = (current_PORTB_state & 0b00010000) >> 4;
    // compare the results and take action
    if(previous_PORTB_RB4_State != current_RB4_state){
        LED_RC4_SetHigh();
    }
    __delay_ms(1000);
    // read PORTB before exiting
    current_PORTB_state = PORTB;
    // save the new state as previous
    current_RB4_state = (current_PORTB_state & 0b00010000) >> 4;
    // clear the interrupt flag
    INTCONbits.RBIF = 0;
}

/**
 End of File
*/

--- Updated Wednesday at 1:45 PM ---

Hi,

there are a lot of mistakes and misunderstandings ..and missing informations.

(please avoid using the same name for two different things, just differentiated by capitals.)

a callback is a function .. containing code that is supposed to be processed.
but you treat it as a variable.

Thus I´m confused.
* do you want it to be a variable ... then don´t name it "callback"
* do you want a function... then treat it as a function. Here the code generator even generated a frame of code ... and it says you should include YOUR function code there.

***
This:

tmr0_callback = 0;
LED3_RD3_SetHigh();
 __delay_ms(2000);
LED3_RD3_SetLow();

exactly is what a interrupt function is meant to avoid. You don´t want to use a "delay(2000)" at all, because it stalls your processor for a total of 2 seconds .. and this only to switch OFF a LED.
During this time your microcontroller could do several million of USEFUL instructions ... while you waste the processing power.

Btw: a "2000 ms" will never lead to a 500ms blinking time. I don´t understand the idea.

***

I don´t think this makes sense. (But I´m not a C expert)
You declared this variabe INSIDE a function. Thus I expect it not to be accessable outside this fucntion.
--> declare this variable as a global variable.

***
A lot of informations are missing.
Like we can not see the meaning of the tmr0 initialisation.
I don´t know if it´s up or down counting, what is your system clock frequency, do you expect it to repeat -or is it a one shot?
--> add comments to your code, especially when you want other people to understand it

***

There are more issues ...

***
I guess you are not familiar yet how interrupts work and how to use them.
--> there are tutorials explaining this topic. Even videos --> go through some of them.
Choose the source that suits you most, provide a link, and ask specific questions on a topic referring to the source.

Klaus

Hi Klaus and happy new year. I mess the question too much please check #3

 

[KlausST]

Hi,

in post#3 you did not explain anthing ... and did not ask anything. So I don´t know what information you need for us.

Below is a main.c which is working fine.

for us ... this gives no information. We don´t know what you expect, we don´t know how you tested it, we don´t know whether you tested it long enough to see the "drift in timing".

***
Anyways. Still you post code (btw without any comments I asked for)
This code shows a couple lines of code that make no sense.
No application would satisfactory work like this.

You say "the code is working" ... but I´m quite sure "it only looks like so" .. but with a deeper understanding you will recognize it does not work the way you think it works.

*** example1)

    while (1)
    {
        // Add your application code
        __delay_ms(1000);
        LED_RC4_SetLow();
    }

It makes no sense to put a 1s dely in a LOOP .. that only switches OFF a LED.
What it does .. it stalls the main loop for 1s and then switches OFF the LED .. and this repeatedly.

There are two cases:
1) no ISR does never switch ON the LED .. then write it this way:

    __delay_ms(1000);
    LED_RC4_SetLow();
    while (1)
    {
        // Add your application code
    }

2) there is an ISR that switches ON the LED.
Then the 1s delay is not valid. The LED ON time is unpredictable.

*****
*** example2)

void __interrupt(high_priority) ISR_high()
{
    // read current state of PORTB
    char current_PORTB_state = PORTB;
    // we extract the state of the pins separate
    char current_RB4_state = (current_PORTB_state & 0b00010000) >> 4;
    // compare the results and take action
    if(previous_PORTB_RB4_State != current_RB4_state){
        LED_RC4_SetHigh();
    }
    __delay_ms(1000);

--> general recommendation is to keep an ISR as short in time as possible. Thus in 99.999% of all cases a "delay_ms(1000)" has to be avoided.
What it does: It completely destroys the function of the main loop during this 1 second. .. additionally it destroys the timing in the mina loop.
All in all it is very likely that it will mess up the whole system.

****
--> I´ve written my recommendation in post#2.


Klaus

 

[Aussie Susan]

(BTW and IMHO this is the reason why MCC is a big pile of rubbish.)
I think you need to understand exactly how MC works. Basically it tries to hide a lot of the internal handling of the various modules (in this case the timer) so that you can concentrate on the 'user' level coding. It generally does this by internally handling the interrupts and only telling you what has happened via callbacks.
(@KlausST - this does lead to the use of code such as 'if( callback_function)...' because it is checking to see if the callback has been defined and therefore does not call a null function pointer.)
Therefore you need to declare your callback function and tell MCC about it through the appropriate function call where you pass the callback function as the parameter.
As the MCC code executes (and in particular the ISR code) it will call your callback function where you do whatever you need to before returning back to the MCC code. (Therefore you DON'T write your own ISR - the MCC code does that for you.)
And herein lies the trap for young players - the callback function is (generally) called in the interrupt context but you are not really aware of this. Therefore you need to follow all of the rules about how to code anISR within you callback, namely: keep it short; NEVER call a delay function; (depending on your MCU) don't call other functions (as they might call delays etc. or exceed the stack size....).
Also, when timing is important, remember that the MCC code adds (LOTS of) code before and after it calls your callback function and you don't always know what that codes is and how long it takes.
All of this is why (again IMHO) for simple modules such as the timer (and most others except Ethernet and USB modules) it is far better to forget MCCm and interact with the modules directly, and also use your own ISR.
(I actually think you have discovered this for yourself looking at the 'working' code you posted in Post #3.)
Susan

 

[KlausST]

(@KlausST - this does lead to the use of code such as 'if( callback_function)...' because it is checking to see if the callback has been defined and therefore does not call a null function pointer.)

I agree .. it could be used this way. ... but ... there are two "but"s:

1) then the user does not define/declare the variable (that´s what a "char" is) ... like the OP did several times

extern char tmr0_callback;

2)
Instead the code generator generates the function:

void TMR0_CallBack(void)

And the existance of this function needs to be checked by

if (TMR0_CallBack()) ...

since C is case sensitve an "IF" on [all_lower_case_name] won´t correctly report whether the function is defined.

***
How I see it:
The user declared a variable with an all lower case name ... and he checked on the variable with an all lower case name.

Klaus

--- Updated Thursday at 10:44 PM ---

Therefore you need to declare your callback function and tell MCC about it through the appropriate function call where you pass the callback function as the parameter.

I don´t think this is how the conde generator is meant to work.

I see the code generator already defines the callback function .... and tells (with a comment) the user to put HIS callback code there.
****

And herein lies the trap for young players - the callback function is (generally) called in the interrupt context but you are not really aware of this.

Is it really a trap?
I guess it is well documented ... with examples. Not reading the documentation ... I don´t call "a trap".
I did a search for "MCC interrupt call back" and the first hit was this: (a two seconds job!! )

https://developerhelp.microchip.com/xwiki/bin/view/software-tools/mcc/interrupts/

(followed by several additional MICROCHIP documents and videos)
i says:

When using MCC, an extra jump occurs when servicing interrupts. The code generated by MCC for each interrupt source creates a callback function and places a jump to callback at the interrupt vector. MCC inserts a directive in the code making the interrupt jump to the callback function. The callback function will then jump to the application's developer-written ISR. MCC-based projects utilize two jumps before the application's ISR executes.

So basically they recommend to just put a (second) call to your private function there .... but for sure you could add any other code there, too.
The callback is called by the ISR ... so one has to expect it to be in interrupt context.
Pictures also illustrate this.

****
The bigger problem - in my eyes - is
* not reading the documentation
* .. and thus not understanding the interrupt functionality and it´s benefits.
Interrupts enable a fast reacton on asynchronous events .. while still other jobs can be processed ... with the result of a reliably working system and efficiently using the processing power (not wasting processsing power for a [delay_ms(2000) ] for example)

Also, when timing is important, remember that the MCC code adds (LOTS of) code before and after it calls your callback function and you don't always know what that codes is and how long it takes.

Why "you don´t know"? ... all the code is shown. You could even check the compiled code .. and even use a simulator to step thorugh the code and check the timing.
It´s not hidden at all.

Klaus

 

[Aussie Susan]

It´s not hidden at all.

I agree but it seems that many people (my assumption based on the questions iI see in the Microchip forum and here) don't read the documentation and don't read the generated code - I think they must assume that it does whatever they think it should (my editorialising here!!).
I think they also don't understand the purpose of ISRs (as you have said above) and also don't realise that the callback is called in the ISR context.
Susan

 

[KlausST]

Hi,

Yes, I have the same assumption. They don´t read documentation, don´t read tutorials ... but spent much time on (wrong) coding, spend time to write in a forum ... want individual support.
This individual treatment takes much more time on both sides of the forum ... and the forum posts can never be as complete as a tutorial usually is.

Another point is: many people don´t divide the software into smaller tasks ... don´t code them individually and don´t test them individually.
And "testing" .. and I mean "deep testing" is extremely important if you want to write reliable code.
This "deep testing" does not mean you need to spend much time .. nor does it mean you need extra hardware or simulation software.
Often a LED connected to a "debug pin" is all you need. Or a couple of them.
For sure ... a scope or logic analyzer has it´s benefits.

For me: testing on the real hardware ... gives better feedback than any simulator can. Better in the meaning of reliability.
Example: If a pin on a simulation goes ON and OFF .. does not mean it will switch ON/OFF a LED on the real hardware. There are so many things that a simulation does not capture, like: bad wiring, bad power supply, wrong system clock, interferance caused by bad PCB layout, crosstalk, left floating inputs ... and so on.

Klaus