Jump to content
Sign in to follow this  
ppulle

Rtc Code Interrupt During Table Lookup, Pcl Fiddling Seems Bad

Recommended Posts

Hi,

 

I have an interrupt servicing a RTC (real time clock) routine. However it uses a lookup table to determine the days of the month that does a call/PCL jump/retlw technique that doesn't seem to work.

 

I'm using TMR1 to set flags to do ADC samples (polled in main code) as well as do the RTC. The samples are sub multiples of the RTC clock. On each second various variables (seconds, minutes, hours, days, months, year) are adjusted to the time.

 

TMR0 is used to call a USB service routine.

 

Here is the gist of the code, various flags and variables have been omitted:

 

void days(void)
{

asm {
;_days:
	addwf	_pcl,F	; Number of days per month
	retlw	0x00	; Leap-day	29	
	retlw	0x1f	; January	31
	retlw	0x1c	; February	28
	retlw	0x1f	; Mars		31
	retlw	0x1e	; April		30
	retlw	0x1f	; May		31
	retlw	0x1e	; June		30
	retlw	0x1f	; July		31
	retlw	0x1f	; August	31
	retlw	0x1e	; September	30
	retlw	0x1f	; October	31
	retlw	0x1e	; November	30
	retlw	0x1f	; December	31
}

}

void interrupt(void)
{
//Clock code from Microchip datasheet, calendar code from Jaakko Ala-Paavola
asm
{
	btfss	_pir1,TMR1IF	; check if this is a timer interrupt
	goto	usb_service	; no so do usb stuff

	bsf	_nRTCFlags,fSAMPLE_READY; sets a flag which is polled in main code to do an ADC sample
	movf	_nRTCTMR1H,W	;
	movwf	_tmr1h

	bcf	_pir1,TMR1IF	;clear interrupt flag

	incf	_nRTCRecord,F
	movf	_nRTCRecord,W
	subwf	_nRTCRecordRate,W; more ADC stuff.....no relevant to forum question
	btfss	_status,Z
	goto 	int_dortc
	bsf	_nRTCFlags, fREPORT_READY
	clrf	_nRTCRecord

;This is the RTC stuff
int_dortc:
	incf	_nRTCSubSecs,F
	movf	_nRTCSubSecs,W
	subwf	_nRTCSampleRate,W
	btfss	_status,Z	;number of samples per second elapsed?

	goto	int_exit	;no
	clrf	_nRTCSubSecs

;Now we adjust the RTC clock on each second interval
	incf	_nRTCSecs,F	;increment seconds
	movf	_nRTCSecs,W
	sublw	60		;
	btfss	_status,Z	;60 seconds elapsed??
	goto	int_exit
	clrf	_nRTCSecs	;yes, so clear seconds

	incf	_nRTCMins,F
	movf	_nRTCMins,W
	sublw	60		;60 minutes elapsed??
	btfss	_status,Z
	goto	int_exit	;no so return
	clrf	_nRTCMins
	incf	_nRTCHours,F	;increment hours
	movf	_nRTCHours,W
	sublw	24		;24 hours elapsed??
	btfss	_status,Z
	goto	int_exit
	clrf	_nRTCHours

; Calendar code from Jaakko Ala-Paavola http://users.tkk.fi/~jalapaav/Electronics/Pic/Clock/index.html
	bsf	_nRTCFlags,dayf	;		  dayf = 1;
	incf	_nRTCDay,F	;	   day++;
	movf	_nRTCMonth,W	;		  ACCU = month;
	sublw	0x02		;	   
	btfss	_status,Z	;	   if ((ACCU - 2) == 0)
	goto	_noleap		;	   {
	btfsc	_nRTCFlags,lyf	;		 if (leap_year)
	andlw	0x00		;		   ACCU = 0;
	goto	_leap		;		  }else
_noleap:
	movf	_nRTCMonth,W	;		 ACCU = month;
_leap:
;>>>>>>>>>>>>>> this seems to be the problem code <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
	call	days		;		  ACCU = days[ACCU];
;>>>>>>>>>>>>>>				 <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
	subwf	_nRTCDay,W	;		  if ((day-ACCU) != 0)
	btfss	_status,Z	;		 return;
	goto	int_exit	;		  else {
	movlw	0x01		;			ACCU = 1;
	clrf	_nRTCDay	;		 day = ACCU;
	bsf	_nRTCFlags,monthf;			monthf = 1;
	incf	_nRTCMonth,F	;			month++;
	movf	_nRTCMonth,W	;			ACCU = month;
	sublw	0x0d		;			if ((ACCU-13) != 0)
	btfss	_status,Z	;		return;
	goto	int_exit	;			else {
	movlw	0x01		;		ACCU = 1;
	movwf	_nRTCMonth	;		month = ACCU;
	bsf	_nRTCFlags,yearf;		yearf = 1;
	incf	_nRTCYear,F	;		year++;
	bcf	_nRTCFlags,lyf	;		lyf = 0;
	movf	_nRTCYear,W	;		ACCU = year;
	andlw	0x03		;			   if ((ACCU & 00000011) == 0)
	btfsc	_status,Z	;		{
	bsf	_nRTCFlags,lyf	;		  lyf = 1;}
	movf	_nRTCYear,W	;		ACCU = year;
	sublw	0x64		;		if ((ACCU-100) != 0)
	btfss	_status,Z	;		  return;
	goto	int_exit	;		else {
	clrf	_nRTCYear	;		  year = 0;
	incf	_nRTCCentury,F	;		  century++;	
	bcf	_nRTCFlags,lyf	;		  lyf = 0;
	movf	_nRTCCentury,W	;		  ACCU = century;
	andlw	0x03		;				 if ((ACCU & 00000011) == 0)
	btfsc	_status,Z	;		  {
	bsf	_nRTCFlags,lyf	;			lyf = 1;
	goto	int_exit

usb_service:
	btfsc	_intcon,TMR0IF
	call	cdc_service
	bcf	_intcon,TMR0IF	;clear sample interrupt flag
	goto	fin_int
int_exit:
;had some stuff here before	
fin_int:

}
}

 

The code basically checks if a TMR1 interrupt has occured, if it has it does some flag setting and RTC updates. If not, then it checks if TMR0 interrupt has occured and does a usb service call.

 

The problem is that when the clock clicks over from 23:59:59 to the next day, some undefined chaos occurs and my USB stops working, probably because it misses a service call or doesn't get reset (so never TMR0 interrupt never occurs again).

 

If I comment out all the lines in the 'days' routine, and replace it with a fixed line like RETLW 0x1f, all is well.

 

It appears it is the table lookup method using the line:

addwf	_pcl,F	; Number of days per month

that is the problem. Is fiddling with the pcl during an interrupt routine a bad thing? Could it be bollocksing up something that interferes with everything else?

 

Is there an alternative to using a PCL/RETLW sequence to do a table lookup? I've looked at using the TBLRD opcode, but have no experience with using table reads and am looking for some advice on setting them up.

 

I don't think it's a timing issue, because if I replace all the RETLW and addwf PCL,F statements with loads of nops everything works.

eg:

asm
{
	nop
	nop
	nop
	nop
	nop
	nop
	nop
	nop
	nop
	nop
	nop
	nop
	nop
	nop
	nop
	nop
	nop
	nop
	nop
	nop
	retlw 0x1f
}

presumably this would actually take longer to execute than the original code.

 

So the only thing I can think of is adding something to the pcl during the interrupt routine (servicing TMR1..the RTC) then TMR0 goes off.

 

So I suppose there are a couple of questions:

- what can I replace my ADDWF PCL,F/RETLW sequence with that is safe during an interrupt, can anyone provide a short code fragment please.

- what is causing the problem in the first place. Obviously making calls during an interrupt is OK (though not advised)...it's just the PCL fiddling.

 

Thanks

Phil

Share this post


Link to post
Share on other sites

I presume from the mention of USB you are using a PIC18 chip.

 

You are calling a routine that is almost certainly outside the current page then adding W to PCL. The assembler does *NOT* recognise addition as requiring code page fixups and nothing in the code you've shown triggers any (look in the .lst file) so PCLATH and PCLATU are either undefined or pointing at the page your calling routine is in. PC gets loaded from PCLATU, PCLATH and PCL on any write to PCL. Quite simply it performs a random jump or if you prefer, goes off into LaLa land. Additionally (PIC18 only), you *MUST* double W first as each instruction takes 2 bytes and any attempt to jump to an odd address caused a bus fault and resets the processor.

 

The trouble is the compiler usually handles all code page switching so if you start doing it manually, you'll need to preserve the old value of the registers and restore it ASAP. You need also to keep the call/return within the same function (put the table in the same asm block) and interrupts would need disabling (only if they weren't already) *AND* use the complicated code Microchip recommend for tables that cross page boundaries as you can't

guarantee where it will get put.

 

Its a 'C' compiler: Write 'C' whenever possible!

 

IMHO This code is pathologically complicated assembler and has no place inline in a C compiler. I believe the whole 1 second routine should be written in C. If timing is really that tight, and you cant afford the minuscule extra overhead of doing it in C, remember you only need 'days' to decide when to roll 'day' over.

 

You could read it from a ROM array,

rom char* days_in_month={
; Number of days per month
	/* Leap-day */ 29,	
	/* January */ 31,
	/* February */ 28,
	/* March */ 31,
	/* April */ 30,
	/* May */ 31,
	/* June */ 30,
	/* July */ 31,
	/* August */ 31,
	/* September */ 30,
	/* October */ 31,
	/* November */ 30,
	/* December */ 31 };
...
// code to handle the /4, /100 and /400 rules for leap years
...
days=days_in_month[m]; // m is the month except for February in leap years

*NOT* a lookup table, in the main loop and store it in a global for the ISR to use. You could even set a flag in the increment month part of the ISR to request a new 'days' value so it only gets run when required once on the first of the month but you'd also then need to store it during your clock set routine.

Edited by IanM

Share this post


Link to post
Share on other sites

Hi IanM,

 

Thanks for you reply. I agree the code is complex, I'd like to make it simpler which is usually important in an interrupt routine....that's what you get using cut and paste from different sources I suppose (I sloppy way of saying 'don't blame me....I didn't write it). Oops I'm sorry I forgot to mention it's a 18F4450.

 

Interestingly the fault doesn't manifest is such a dramatic way as a reset or bus fault. I would have noticed that early on. FYI I have a logging system. The RTC chugs along quite nicely for days on end till the battery runs out, when logging events occur they are logged with the correct time and date. It's only when I start up the USB system again there is a problem. Presumably, as I mentioned, my usb service interrupt gets slyly disabled.

 

Looking at my .lst the days routine; although it is outside the interrupt routine page it doesn't get put across a page boundary...but I get your point, it might, depending on the compilers whim, so PCL lookups are not good. It shall be removed.

 

Looks like I'll have to re-write my RTC clock/calendar.....in 'C'....where possible.....because it's a C compiler!

(just being frivolous above...no offense.....please don't wield the dreaded big bold font at me.....arrrrggghghghh mercy )

 

I suppose it's tempting fate to wonder if anyone has a safe 18F4550 clock calendar code example, I'm wryly ruing wrangling the code from a 16F84 example.

 

Phil

Share this post


Link to post
Share on other sites

I put your days() code in a test program with two calls to it, one in C in main and one in assembler. I padded main with 4K of 128 byte data strings top and bottom to force days() to be in a different page. No PCLATH or PCLATU handling was found in the .lst file. *HOW* it works normally on the midnight rollover is a mystery. I guess you've lucked out with the powerup values. Testing it properly is beyond me as I don't want to spend days writing a test harness for it.

 

Your USB code (if it doesn't do so already) needs to detect connection and fully reinitialise the USB. Then at least it will work if you plug it in after midnight even if it crashes if USB is active at midnight. From your description, I think you may well have a 'nasty' in the USB code as well.

 

You could try the following UNTESTED code to replace the call to days()

 

1. insert the definition i gave earlier for days_in_month and define w_temp:

char w_temp at the beginning of interrupt()

2. replace the call days as shown. (... is existing code etc.)

void interrupt(void)
{
rom char *days_in_month={
; Number of days per month
...
	/* December */ 31 };
char w_temp;

//Clock code from Microchip datasheet, calendar code from Jaakko Ala-Paavola
asm
{
	btfss	_pir1,TMR1IF  ; check if this is a timer interrupt
	goto	usb_service  ; no so do usb stuff
...
_leap:
;>>>>>>>>>>>>>> this seems to be the problem code;<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
;		call	days	  ;		  ACCU = days[ACCU];
;>>>>>>>>>>>>>>			;<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
;***** PLAN B - do a little bit of it in C *****
  movwf _w_temp
}
w_temp=days_in_month[w_temp];
asm {
  movf _w_temp,W
;************************** 
   subwf	_nRTCDay,W  ;		  if ((day-ACCU) != 0)
...

Edited by IanM

Share this post


Link to post
Share on other sites
....

void days(void)
{

asm {
;_days:
	addwf	_pcl,F; Number of days per month
	retlw	0x00; Leap-day	29	
	retlw	0x1f; January	31
	retlw	0x1c; February	28
	retlw	0x1f; Mars		31
	retlw	0x1e; April		30
	retlw	0x1f; May		31
	retlw	0x1e; June		30
	retlw	0x1f; July		31
	retlw	0x1f; August	31
	retlw	0x1e; September	30
	retlw	0x1f; October	31
	retlw	0x1e; November	30
	retlw	0x1f; December	31
}

}
...

Code like this is not recommended as the linker may end up putting the table in a place where it crosses a 256 byte boundary.

You can improve the situation by placeing the code at a fixed address by using the @ symbol after the function name:

void days(void)@0x1000
{
...

Also returning the value in the w register also assumes that the compiler/linker hasn't used it for something else, which probably it has not on this occassion, but you need to be aware.

 

Regards

Dave

Share this post


Link to post
Share on other sites

Hi,

 

Thanks for the suggestion on using the array. As a quick fix it should work. I must admit to forgetting that Sourceboost can now properly reference variables in asm code.....I've been using it for a while and remember having to use globals for that sort of thing (on a very old version), so have been a bit shy to do so.

 

I'll get to it as soon as possible and come back with the result. I share your mystification on how the code works, one reason why if I get the chance I'll do a re-write. I suppose we can summarize some lessons for other users:

- be wary of using PCL lookup tables, in or out of interrupt routines because of the page boundary problem

- be careful of dropping in someone elses code, particularly if you don't fathom it entirely, even more particularly if it's been ported from another chip

- if you must use PCL lookups, use the fixed function location as Dave suggests.

 

Anyway, thanks all.

 

Phil

Share this post


Link to post
Share on other sites

*Great* I'm glad the fix worked.

 

Now you get the pleasure of converting the rest of that calender code to C. Its fairly well commented and the algorithm is pretty obvious as you aren't trying to do day of the week stuff. You *KNOW* it will be easier to maintain when its in 'C' and its no longer a cut and paste job. If you tie the calender code to the 60 second rollover for testing instead of the normal 24 hour rollover, you can run through a whole years testing in just over 6 hours. Bit of work with a debugger pre-loading the date and you'll have the leap year algorithm tested as well.

 

One of the things I like about this compiler is the absence of opaque libraries. I have a NOVO licence so can look at the source if I am perplexed and as it doesn't try too hard to insulate you from the hardware, when I do look at the source, its modular and I can understand it if I work hard enough, unlike code from some of the big open source projects where the last person to fully understand the gnarly bits retired five years ago and is now living on top of a mountain somewhere off the net.; :-)

 

Ian.

Share this post


Link to post
Share on other sites

Join the conversation

You are posting as a guest. If you have an account, sign in now to post with your account.
Note: Your post will require moderator approval before it will be visible.

Guest
Reply to this topic...

×   Pasted as rich text.   Paste as plain text instead

  Only 75 emoji are allowed.

×   Your link has been automatically embedded.   Display as a link instead

×   Your previous content has been restored.   Clear editor

×   You cannot paste images directly. Upload or insert images from URL.

Loading...
Sign in to follow this  

×
×
  • Create New...