PIC18 - problems getting inline assembler routine to work

Question

(EDIT: Added C18 disassembly code)

I need to optimize a function written in C for a PIC18f4585. I am using C18 to compile.

The function I am trying to re-write in assembly is:

void readResetTimers (void)
{
 register unsigned char tmr0_temp;
 register unsigned char tmr1_temp;
 tmr0_temp = TMR0L;
 TMR0L = 0;
 tmr1_temp = TMR1L;
 TMR1L = 0;
 tmr0_value = tmr0_temp;
 tmr1_value = tmr1_temp;
}

I thought that I could instead use assembly code to load the value of each registers into variables tmr0_value & tmr1_value as they are declared in the global namespace, however my system doesn't work when I substitute the following code in:

void readResetTimers (void)
{
_asm
 MOVF TMR0L, 0, 1
 MOVWF tmr0_value, 1
 CLRF TMR0L,0
 MOVF TMR1L, 0, 1
 MOVWF tmr1_value, 1
 CLRF TMR1L,0
_endasm
}

Any ideas would be appreciated, Cheers.

 void readResetTimers (void)
 04D4 CFD9 MOVFF 0xfd9, 0xfe6
 04D6 FFE6 NOP
 04D8 CFE1 MOVFF 0xfe1, 0xfd9
 04DA FFD9 NOP
 04DC 0E02 MOVLW 0x2
 04DE 26E1 ADDWF 0xfe1, F, ACCESS
405: {
406: register unsigned char tmr0_temp;
407: register unsigned char tmr1_temp;
408: 
409: /* Read the two timers into CPU registers and reset them
410: * as quickly as absolutely possible.
411: */
412: 
413: tmr0_temp = TMR0L;
 04E0 50D6 MOVF 0xfd6, W, ACCESS
 04E2 6EDF MOVWF 0xfdf, ACCESS
414: TMR0L = 0;
 04E4 6AD6 CLRF 0xfd6, ACCESS
415: tmr1_temp = TMR1L;
 04E6 0E01 MOVLW 0x1
 04E8 CFCE MOVFF 0xfce, 0xfdb
 04EA FFDB NOP
416: TMR1L = 0;
 04EC 6ACE CLRF 0xfce, ACCESS
417: 
418: tmr0_value = tmr0_temp;
 04EE CFDF MOVFF 0xfdf, 0xe4
 04F0 F0E4 NOP
419: tmr1_value = tmr1_temp;
 04F2 CFDB MOVFF 0xfdb, 0xe5
 04F4 F0E5 NOP
431: 
432: }

• Adam Lawrence: How does the C18 disassembly compare with your manual assembler?
• Kaz: I can't imagine why the compiler wouldn't already generate code which is at least as good as the approach being taken in the hand-written code.
• chib: I added the disassembly listing to the post.

Answer 1

Since you want to replace the whole subroutine, in-line assembler is a bit silly. Write the whole routine in assembler. That way the compiler can't get in the way doing stuff on entry and exit of the routine and the like.

It would also be better to explain what you really want this routine to do than to define it implicitly with C code. You have:

 tmr0_temp = TMR0L;
 TMR0L = 0;
 tmr1_temp = TMR1L;
 TMR1L = 0;
 tmr0_value = tmr0_temp;
 tmr1_value = tmr1_temp;

Apparently you want a snapshot of timer 0 in 16 bit mode into a separate variable. Your code above won't work for that in all cases since you didn't deal with the high byte incrementing between the two reads.

Unless you will access the two final registers in a routine that could get interrupted by this one, there is no need for the temporary register. Actually, without disabling interrupts you can't guarantee the final 16 bit value is written atomically anyway. Either you care about atomic update or you don't. Your method makes no sense as it seems to care in one part but in the end doesn't guarantee it.

You are also resetting the timer to 0, which is usually a bad idea. This is usually the result of wanting to make sequential time interval measurements without having thought it thru very well. If the reason you need this routine to run so quickly is because you want to minimize the instructions you lose due to resetting the timer, then this is definitely the wrong way to go about it. Take a snapshot at each point you want to measure the interval between, then do a unsigned 16 bit subtract. That will give you the interval without any cycles lost when the timer is reset.

If you can afford the timer to be off for a few cycles, then just shut it off, read it, clear it, and restart it. I really don't see a case where your method makes sense.

In any case, here is a example code snippet that takes a consistant snapshot of timer 0 while leaving it running (untested, not even syntax checked):

 extern t0snapshot ;16 bit snapshot of timer 0
;*********************************************************************
;
; Subroutine T0_GRAB
;
; Grab the current timer 0 value into T0SNAPSHOT. The timer will
; continue running the whole time.
;
 global t0_grab
t0_grab
 banksel t0snapshot ;set bank for access to output variable
t0_retry ;back here if high byte changed
 movf tmr0h, w ;get the timer high byte
 movwf t0snapshot+1 ;save it
 movff tmr0l, t0snapshot+0 ;grab the low byte
 xorwf tmr0h, w ;check the high byte again
 bnz t0_retry ;high byte changed, try again ?
 return

Note this construct called "comments" in this routine. Those are very useful things you need to learn about. Undocumented code is really no code at all.

• supercat: The code as shown reads the low byte of both TMR0 and TMR1. There may be some semantic significance to the fact that tmr0_value and tmr1_value are not updated until after the timers are cleared, though like you I regard the clearing of timers as dubious. If one did want to read and clear a timer with no prescalar rather than letting it free-run (e.g. because one wanted something to happen if the timer reached a certain value without being read and cleared), one could read the value, add three, and then subtract that from the timer.
• Olin Lathrop: @Super: Oops, you're right. Somehow I didn't notice he was reading the low byte of two different timers. That's particularly strange since timer 1 is 16 bits wide. I guess we need to hear from the OP what he is really trying to do. Going from undocumented code and no description isn't working.