Second clock/timer causes red light of death

[pogenwurst]

I'm working with Kevin Watson's 2008 codebase.

I've been fiddling with a clock (based on IFI's whitepaper on the subject) for far too much time I care to remember. I waded through many mistakes on my part along the way (as I am a relative neophyte in programming) but I'm finally stumped.

Here's my relevant code:

/*******************************************************************************
*
*	FUNCTION:		Initialize_Timer_1()
*
*	PURPOSE:		Initializes the timer 1 hardware.
*
*	CALLED FROM:	teleop.c/Initialization()
*
*	PARAMETERS:		None
*
*	RETURNS:		Nothing
*
*	COMMENTS:
*
*******************************************************************************/
#ifdef ENABLE_TIMER_1_ISR
void Initialize_Timer_1(void)  
{
	TMR1L = 0xED;			// least significant 8-bits of the timer 1 register (this is readable and writable)
	TMR1H = 0x85;			// most significant 8-bits of the timer 1 register (this is readable and writable)
							//
	T1CONbits.T1CKPS0 = 1;	// T1CSP1 T1CSP0
	T1CONbits.T1CKPS1 = 1;	//   0      0		1:1 prescaler (clock=10MHz/each tick=100ns)
							//   0      1		1:2 prescaler (clock=5MHz/each tick=200ns)
							//   1      0		1:4 prescaler (clock=2.5MHz/each tick=400ns)
							//   1      1		1:8 prescaler (clock=1.25MHz/each tick=800ns)
							//
	T1CONbits.T1OSCEN = 0;	// 0: timer 1 oscillator disabled (leave at 0 to allow the use of an external clock)
							// 1: timer 1 oscillator enabled (can't be used because of hardware constraints)
							//
	T1CONbits.TMR1CS = 0;	// 0: use the internal clock
							// 1: use an external clock on RC0/T1OSO/T13CLK (rc_dig_in14 on robot controller)
							//
	T1CONbits.RD16 = 1;		// 0: timer 1 register operations are done in two 8-bit accesses
							// 1: timer 1 register operations are done in one 16-bit access
							//    In this mode, reading TMR1L will latch a copy of TMR1H into a buffer
							//    mapped to the TMR1H memory address. Conversely, a write to the buffer
							//    followed by a write to the TMR1L register will update the entire 16-bit
							//    timer at once. This solves the problem where the timer may overflow
							//    between two 8-bit accesses. Here's an example of how to do a 16-bit read:
							//
							//    unsigned char Temp_Buf; // 8-bit temporary buffer
							//    unsigned int Timer_Snapshot; // 16-bit variable
							//
							//    Temp_Buf = TMR1L; // TMR1L must be read before TMR1H
							//    Timer_Snapshot = TMR1H;
		 					//    Timer_Snapshot <<= 8; // move TMR1H data to the upper half of the variable
							//    Timer_Snapshot += Temp_Buf; // we now have all sixteen bits  
							// 
	IPR1bits.TMR1IP = 0;	// 0: timer 1 overflow interrupt is low priority (leave at 0 on IFI controllers)
							// 1: timer 1 overflow interrupt is high priority
							//
	PIR1bits.TMR1IF = 0;	// 0: timer 1 overflow hasn't happened (set to 0 before enabling the interrupt)
							// 1: timer 1 overflow has happened
							//
							// Brian is stupid and doesn't read in-code documentation
							//
	PIE1bits.TMR1IE = 1;	// 0: disable timer 1 interrupt on overflow (i.e., a transition from FFFF->0)
							// 1: enable timer 1 interrupt on overflow (i.e., a transition from FFFF->0)
							//	
	T1CONbits.TMR1ON = 1;	// 0: timer 1 is disabled
							// 1: timer 1 is enabled (running)
}
#endif

/*******************************************************************************
*
*	FUNCTION:		Timer_1_ISR()
*
*	PURPOSE:		If enabled, the timer 1 interrupt handler is called when
*					the TMR1 register overflows	and rolls over to zero.
*
*	CALLED FROM:	ifi_frc.c/Interrupt_Handler_Low()
*
*	PARAMETERS:		None
*
*	RETURNS:		Nothing
*
*	COMMENTS:
*
*******************************************************************************/
#ifdef ENABLE_TIMER_1_ISR
#pragma tmpdata low_isr_tmpdata

// counter variables
volatile unsigned	char	timerTickCount = 0;
volatile unsigned	int		timerSecondCount = 0;

void Timer_1_ISR(void)
{
	// this function will be called when a timer 1 interrupt occurs
	
	if(timerTickCount > 39) // 40 * 25 ms = 1 sec
	{
		// reset tick count
		timerTickCount = 0;
		
		// increment second counter
		++timerSecondCount;
	}
	else
	{
		++timerTickCount;
	}
}
#pragma tmpdata
#endif

in timers.c

	// if Brian has stopped being an idiot (unlikely) this will move us for 5 seconds
	if(timerSecondCount < 6)
		pwm13 = pwm15 = 255;
	else
		pwm13 = pwm15 = 127;

in teleop.c within the Teleop() function

extern timerSecondCount;

in teleop.h

My code has been compiling fine (after much trial and tribulation ), but as indicated in the topic subject, it results in a red program state light.

When I disable the timer 1 overflow interrupt (PIE1bits.TMR1IE), the program state light is green (but as my count-incrementing code is within an ISR, doing so renders my code useless). Strangely, however, if I re-enable the interrupt but comment out the contents of the ISR function, I once again see red.

Can anyone point me in the right direction? Thanks.

[Kevin Sevcik]

Are you actually calling your timer interrupt handler from the Interrupt_Handler_Low() in ifi_frc.c? Also, your timer interrupt handler isn't clearing the Timer 1 Interrupt Flag. You need this:

PIR1bits.TMR1IF = 0;

in your interrupt handler to clear the interrupt, or you'll just end up right back in it. Also, also, if you're preloading TMR1L and TMR1H, then you need to preload them again in your interrupt handler, or they'll start counting again from 0 instead of your preloaded value.

[Kevin Watson]

Originally Posted by pogenwurst

Can anyone point me in the right direction?

Did you enable the ISR at the top of ifi_frc.h and the timer 1 ISR in timers.h? If not, Timer_1_ISR() won't be called from Interrupt_Handler_Low() and the interrupt flag won't get cleared, causing the RLOD.

Edit: Your code looks very good. The counter variables need to be global though. My bet is that you just forgot to enable the timer 1 ISR in ifi_frc.h.

-Kevin

[dcbrown]

A nit, but it could cause unpredictable program results - no red-light-of-death - but weird things on occasion.

The timerSecondCount is a multibyte variable. Reading it's value needs to be protected by disabling the timer interrupt enable to avoid issues.

For example, code reads lower byte and its 0xFF - before the code can operate on the upper byte a timer interrupt happens and the upper byte gets changed. The user code then reads the upper byte and completes whatever operation its working on but now it believes the timer count is a lot larger than it really is. (For example code thinks the value is 0x1FF when it is 0x100).

PIE1bits.TMR1IE = 0;
temp_int = timerSecondCount;
PIE1bits.TMR1IE = 1;

True, you're not likely to hit this... but its always better to practice safe variable access otherwise you can go bald pulling your hair out trying to find the glitch that happens every so often and apparently at random.

You only need to do this for multibyte variables that are written to within the ISR routine AND which you access at non-interrupt level.

[pogenwurst]

Thank you very much to everyone for your responses!

Originally Posted by Kevin Sevcik

Are you actually calling your timer interrupt handler from the Interrupt_Handler_Low() in ifi_frc.c?

Forgive me, but I'm afraid I don't quite understand what you mean. :/

My understanding is that after calling Initialize_Timer_1() within Initialize(), the ISR for timer 1 (if enabled) should be called every time timer 1 overflows (in this case, every 25ms). Am I missing something (or was it an issue of me miscommunicating by forgetting to mention that I initialized timer 1)?

Originally Posted by Kevin Sevcik

Also, your timer interrupt handler isn't clearing the Timer 1 Interrupt Flag.

Originally Posted by Kevin Sevcik

Also, also, if you're preloading TMR1L and TMR1H, then you need to preload them again in your interrupt handler, or they'll start counting again from 0 instead of your preloaded value.

Do'oh! Both those points make a lot of sense.

Originally Posted by Kevin Watson

My bet is that you just forgot to enable the timer 1 ISR in ifi_frc.h.

You bet right -- I did so in timers.h but not ifi_frc.h; I just need to uncomment "#define ENABLE_TIMER_1" in ifi_frc.h, correct?

Originally Posted by Kevin Watson

Your code looks very good.

Thank you!

Originally Posted by Kevin Watson

The counter variables need to be global though.

Please forgive my ignorance, but I don't understand why "extern timerSecondCount;" in teleop.c (although that will later be changed to autonomous.c) is not enough.

Originally Posted by dcbrown

The timerSecondCount is a multibyte variable. Reading it's value needs to be protected by disabling the timer interrupt enable to avoid issues.

Nice tip! That never would have occurred to me.

On a related note, how do I distinguish between a multibyte and a single-byte variable? I'm afraid I'm quite ignorant concerning this bit and byte business.

Also, to anyone who might be able to answer, was using "volatile" in my variable declaration correct (purely out of curiosity)?

Again, thank you very much to everyone who responded; I'll modify my code ASAP. All this is very frustrating, but I'm ecstatic at how much I've been learning!

[ericand]

I think volatile is correct in this context. It tells the compiler that it needs to read the memory every time it accesses a particular variable (not use a register cached value), since the variable may change outside of the compiler's ability to predict or controll. In this instance, the variable might be updated by your interrupt service routine.