Converting PBP interrupt to ASM - 18F25K20

[achilles03]

I have a couple interrupt routines using DT's instant interrupts in PBP that I'd like to convert to asm to reduce execution time. One of the ISRs uses an external interrupt to time pulses and store the results in an array (ppm) and increments to the array index (ppm_n). Bit 4 of ppm_n is set to 0 so that ppm_n rolls over from 15 to 0. I read the array outside of the interrupt and verify the bit ppmwrite hasn't changed while reading the data, else the interrupt could have occured while reading the data.

This compiles but it appears none of the array values are updating except for index 15, which appears to be randomly assigned and never changes even though the array is cleared at startup. However it appears the index ppm_n is incrementing and ppmwrite is being toggled. Typical output from the code below looks like this (first 16 values are the ppm array, red is ppm_n, and blue is ppmwrite):

0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 29184 8 0
0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 29184 6 0
0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 29184 5 1
0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 29184 3 1
0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 29184 2 0

I cobbled together the code below from some helpful posts in these forums and the datasheet. I still include "ReEnterPBP-18.bas" since I use that for another ISR that still uses PBP, but that is not included in this test code. I'm not that experienced with asm (yet), so any pointers are greatly appreciated!



pausetime var word
ppm var word[16]
ppm_n var byte
n var byte
ppmwrite var bit

T3CON = %10110000 'ppm timer: Prescaler = 8:1, Timer off; bit 5-4 is prescaler (00=1:1, 01=2:1 ... 11=8:1)

INCLUDE "DT_INTS-18.bas" ' Base Interrupt System
INCLUDE "ReEnterPBP-18.bas" ' Include if using PBP interrupts

'----[High Priority PPM Interrupt]----------------------------------------------
ASM
INT_LIST macro ; IntSource, Label, Type, ResetFlag?
INT_Handler INT0_INT, _PULSE_WIDTH, ASM, yes
endm
INT_CREATE ; Creates the interrupt processor
ENDASM

'clear ppm array
for n=0 to 15
ppm[n]=0
next n

ppm_n=0

@ bsf T3CON, TMR3ON ; Start timer
@ INT_ENABLE INT0_INT ; enable external (INT) interrupts

ppmloop:
pause 100
@ INT_DISABLE INT0_INT ; disble external (INT) interrupts
serout2 PIN_OUT,6,[dec ppm[0]," ",dec ppm[1]," ",dec ppm[2]," ",dec ppm[3]," ",dec ppm[4]," ",dec ppm[5]," ",dec ppm[6]," ",dec ppm[7]," ",dec ppm[8]," ",dec ppm[9]," ",dec ppm[10]," ",dec ppm[11]," ",dec ppm[12]," ",dec ppm[13]," ",dec ppm[14]," ",dec ppm[15]," ",dec ppm_n," ",dec ppmwrite,13,10]
@ INT_ENABLE INT0_INT ; enable external (INT) interrupts
goto ppmloop

PULSE_WIDTH:
ASM
movff TMR3L, _pausetime ; Read LB and store in variable 'pausetime' - also loads TMR3H into buffer
movff TMR3H, _pausetime + 1 ; Read HB from buffer
movf _ppm_n, W ; Get index value from variable 'ppm_n' for array address, store in WREG
rlncf WREG, W ; Multiply by 2 since ppm array is words (16bit)
addlw _ppm ; add base address of ppm array to offset in W
movwf FSR0 ; store the calculated address in FSR0
movf _pausetime, W ; load pausetime LB into WREG
movwf INDF0 ; move LB into ppm[ppm_n]
incf FSR0 ; move to next pointer loation for HB
movf _pausetime + 1, W ; load pausetime HB into WREG
movwf INDF0 ; move HB into ppm[ppm_n]
incf _ppm_n, f ; increment array index variable ppm_n
bcf _ppm_n, 4 ; clear bit 4 so array starts over after index 15 (b1111)
btg _ppmwrite ; toggle ppmwrite to check if interrupt occured in main program
INT_RETURN
ENDASM

[achilles03]

I might have figured it out. It looks like I wasn't loading the full pointer into FSR0 since it's a 12-bit pointer. Also I hit on the POSTINC0 option, which is nifty. I believe the ISR should be something like this (see below). I am currently assuming that word arrays in PBP are allocated with the low byte in the lower address and the high byte at the next address (I need to verify this). Any errors in the code or any suggestions to consolidate instructions are much appreciated!


PULSE_WIDTH:
ASM
movff TMR3L, _pausetime ; Read LB and store in variable 'pausetime' - also loads TMR3H into buffer
movff TMR3H, _pausetime + 1 ; Read HB from buffer
lfsr FSR0, _ppm ; Load the full 12-bit address of ppm into FSR0
movf _ppm_n, W ; Get index value from variable 'ppm_n' for array address, store in WREG
rlncf WREG, W ; Multiply by 2 since ppm array is words (16bit)
addwf FSR0L, f ; Add offset to low byte of pointer
movlw 0 ; clear WREG although carry flag is still active in STATUS REGISTER bit 0 (pretty sure that's how it works?)
addwfc FSR0H, f ; Add carry to high byte of pointer
movf _pausetime, W ; Load pausetime LB into WREG
movwf POSTINC0 ; Store pausetime LB in ppm location and automatically increment FSR0
movf _pausetime + 1, W ; Load pausetime HB into WREG
movwf INDF0 ; Store pausetime HB in ppm location
incf _ppm_n, f ; increment array index variable ppm_n
bcf _ppm_n, 4 ; clear bit 4 so array starts over after index 15 (b1111)
btg _ppmwrite ; toggle ppmwrite to check if interrupt occured in main program
INT_RETURN
ENDASM

[richard]

looks close but if the vars used in asm are not in access ram you still need to ensure the correct bank is set, it may look likes it works depending on where the compiler places your vars but its not safe

eg

movf _ppm_n, W // movf takes an 8bit address field, ppm would need to be in the currently set bank

ditto for bcf,btg etc

[achilles03]

Thank you for your feedback! The test code seems to work ok, but when I roll this ISR into my main program, it could occur at any time so it seems setting the correct bank would be important.

What I have now puts variable ppm_n at 000000DFh and ppm at 000001F6h. Since ppm is 16 words (32 bytes), I believe the ppm array rolls over into bank 2. The next variable starts at 00000216h, so I believe that it did roll over.

It looks like I'm not completely filling bank 2 according to the .LST file, so would it be better to allocate the variables ppm and ppm_n to bank 3 and have this particular ISR always point to bank 3? Or should I add in some operations to point to the correct bank? It seems calculating the correct bank could require more time especially if the array straddles different banks?

Thanks again for the feedback!
Dave

[richard]

The Access Bank consists of the first 96 bytes of memory(00h-5Fh) so if you create your asm vars in that space you can use the opcode access field rather than needing to switch banks


eg
ppm_n var byte $5f [in access bank]
pausetime var byte $5d [in access bank]

asm
movf _ppm_n, W ,a
...
movf _pausetime, W,a

or let pbp store the vars anywhere it likes

ppm_n var byte [anywhere]
pausetime var byte

asm
banksel _ppm_n
movf _ppm_n, W
...
banksel _pausetime
movf _pausetime, W

or store all the asm vars in the same bank


ppm_n var byte bank0
pausetime var byte bank0

asm
banksel 0
movf _ppm_n, W
...
movf _pausetime, W

[achilles03]

Thanks for the insight! I ended up putting all the variables used in the ISR in bank 3 and starting the interrupt with "banksel _ppm_n" in case I every move them to a different bank. I may not need the subsequent "movlw 0" and "addwfc FSR0H, f" operations since the variables are now in the same bank?

I also consolidated a few operations. The ISR now is able to initiate, run, and return to the main program in ~127 instruction cycles, which is great:


PULSE_WIDTH:
asm
banksel _ppm_n ; Variables used in interrupts are stored one bank via config definition - can change bank # as long as vars are in same bank
movf _ppm_n, W ; Get index value from variable 'ppm_n' for array address, store in WREG
incf _ppm_n, f ; increment array index variable ppm_n
bcf _ppm_n, 4 ; clear bit 4 so array starts over after index 15 (b1111)
rlncf WREG, W ; Multiply WREG by 2 since ppm array is words (16bit)
lfsr FSR0, _ppm ; Load the full 12-bit address of ppm into FSR0
addwf FSR0L, f ; Add offset from WREG to low byte of pointer
movlw 0 ; clear WREG although carry flag is still active in STATUS REGISTER bit 0
addwfc FSR0H, f ; Add carry to high byte of pointer
movff TMR3L, POSTINC0 ; Move TMR0L into ppm and automatically increment FSR0
movff TMR3H, INDF0 ; Move TMR0H into ppm
clrf TMR3H ; reset TMR0H by loading 0 into TMR3H buffer
clrf TMR3L ; reset TMR0 by loading 0 into low byte and pushing in high byte buffer
btg _ppmwrite ; toggle ppmwrite to check if interrupt occured in main program
INT_RETURN
endasm

Now I'm working on converting the other ISR to asm, and am having some issues. I suspect it's how I'm trying to update the outputs on PORTC or how I'm trying to preload the highbyte of timer1 (timer1 is configured for two 8-bit R/W operations). I think the issues with the code are in red? PORTC is configured entirely as outputs and is updated with a precalculated byte from motor[mcount] that has zeros for the 4 LSBs (i.e. %XXXX0000). The value in preload[mcount] is also precalculated. Any thoughts or suggestions are much appreciated!



mcount var byte bank3
motor var BYTE[6] bank3
preload var BYTE[7] bank3

T1CON = %00010000
TRISC = %00000000

'----------[High Priority Motor Interrupt]------------
ASM
INT_LIST macro ; IntSource, Label, Type, ResetFlag?
INT_Handler TMR1_INT, _MOTOR_CONTROL, ASM, yes
endm
INT_CREATE
ENDASM

MOTOR_CONTROL:
ASM
banksel _mcount ; Variables used in interrupts are stored one bank via config definition - can change bank # as long as vars are in same bank
movf _mcount, W ; Get index value from variable 'mcount' for array address, store in WREG
lfsr FSR0, _motor ; Load the full 12-bit address of motor array into FSR0
addwf FSR0L, f ; Add offset to low byte of pointer
movlw 0 ; clear WREG although carry flag is still active in STATUS REGISTER bit0
addwfc FSR0H, f ; Add carry to high byte of pointer
MOVFF INDF0, LATC ; Change PORTC output to adjust which motors are powered
incf _mcount, f ; Increment mcount
movf _mcount, W ; Get index value from variable 'mcount' for array address, store in WREG
lfsr FSR0, _preload ; Load the full 12-bit address of preload array into FSR0
addwf FSR0L, f ; Add offset to low byte of pointer
movlw 0 ; clear WREG although carry flag is still active in STATUS REGISTER bit0
addwfc FSR0H, f ; Add carry to high byte of pointer
MOVFF INDF0, TMR1H ; Load preload byte into TMR1H
INT_RETURN
endasm

[richard]

I may not need the subsequent "movlw 0" and "addwfc FSR0H, f" operations since the variables are now in the same bank?

whether the vars are in the same bank or not is irrelevant with indirect addressing, the point is that with pic18 chips an array can be more than 255 bytes in length and may cross a bank boundary. when adding an 8 bit offset to a 16 bit fsr register it is always prudent to check if the addition caused the low byte to overflow and increment the high byte if it did.
bugs caused by incorrect fsr manipulation are tricky to find

the code you have posted is not complete or even compliable i have no idea what you are even trying to do

[richard]

as an example this is what i came up with for your first question




#CONFIG
CONFIG FOSC=INTIO67, FCMEN=OFF, IESO=OFF, PWRT=OFF, BOREN=SBORDIS, BORV=18
CONFIG WDTEN=ON, WDTPS=512, CCP2MX=PORTC, PBADEN=OFF, LPT1OSC=OFF, HFOFST=ON
CONFIG MCLRE=ON, STVREN=ON, LVP=OFF, XINST=OFF, DEBUG=OFF, CP0=OFF, CP1=OFF
CONFIG CP2=OFF, CP3=OFF, CPB=OFF, CPD=OFF, WRT0=OFF, WRT1=OFF, WRT2=OFF
CONFIG WRT3=OFF, WRTC=OFF, WRTB=OFF, WRTD=OFF, EBTR0=OFF, EBTR1=OFF
CONFIG EBTR2=OFF, EBTR3=OFF, EBTRB=OFF
#ENDCONFIG
DEFINE OSC 16


pausetime var word bank0
ppm var word[16]
ppm_n var byte bank0
aflags var byte bank0
ppmwrite var aflags.0
LED VAR LATA.0


T3CON = % 10110000 'ppm timer: Prescaler = 8:1, Timer off; bit 5-4 is prescaler (00=1:1, 01=2:1 ... 11=8:1)
TRISA = % 11111110
ANSEL = $CF


OSCCON = $70
RCSTA = $90 ' Enable serial port & continuous receive
TXSTA = $20 ' Enable transmit, BRGH = 0
SPBRG = 25 ' 38400 Baud @ 16MHz, 0.16%
SPBRGH = 0
BAUDCON.3 = 1 ' Enable 16 bit baudrate generator






INCLUDE "DT_INTS-18.bas" ' Base Interrupt System
;INCLUDE "ReEnterPBP-18.bas" ' Include if using PBP interrupts


'----[High Priority PPM Interrupt]----------------------------------------------
ASM
INT_LIST macro ; IntSource, Label, Type, ResetFlag?
INT_Handler INT0_INT, _PULSE_WIDTH, ASM, yes
endm
INT_CREATE ; Creates the interrupt processor
ENDASM


PAUSE 1000
HSErout [ "READY",13,10]


ppm_n=0


@ bsf T3CON, TMR3ON ; Start timer
@ INT_ENABLE INT0_INT ; enable external (INT) interrupts


ppmloop:
if ppmwrite then
@ INT_DISABLE INT0_INT ; disble external (INT) interrupts
;HSErout [dec ppm_N,13,10]
HSErout [13,10,13,10]
; print timer value at capture
HSErout [dec ppm[0]," ",dec ppm[1]," ",dec ppm[2]," ",dec ppm[3]," ",dec ppm[4]," ",dec ppm[5]," ",_
dec ppm[6]," ",dec ppm[7]," ",dec ppm[8]," ",dec ppm[9]," ",dec ppm[10]," ",dec ppm[11]," ",dec ppm[12]," ",_
dec ppm[13]," ",dec ppm[14]," ",dec ppm[15],13,10]
; print interval between interrupts
HSErout [dec ppm[1]- ppm[0] ," "]
HSErout [dec ppm[3]- ppm[2] ," "]
HSErout [dec ppm[5]- ppm[4] ," "]
HSErout [dec ppm[7]- ppm[6] ," "]
HSErout [dec ppm[9]- ppm[8] ," "]
HSErout [dec ppm[11]- ppm[10] ," "]
HSErout [dec ppm[13]- ppm[12] ," "]
HSErout [dec ppm[15]- ppm[14] ,13,10]
intcon.1=0 ;clr int0 flag
ppmwrite = 0
@ INT_ENABLE INT0_INT ; enable external (INT) interrupts
endif
goto ppmloop




PULSE_WIDTH:
ASM
BTG LATA,0,A
movff TMR3L, _pausetime ; Read LB and store in variable 'pausetime' - also loads TMR3H into buffer
movff TMR3H, _pausetime + 1 ; Read HB from buffer
banksel 0
lfsr FSR0, _ppm ; set fsr0
rlncf _ppm_n, W ; Multiply index by 2 since ppm array is words (16bit)
addwf FSR0L, F ; Add offset to low byte of pointer
movlw 0 ; clear WREG although carry flag is still active in STATUS REGISTER bit 0 (pretty sure that's how it works?)
addwfc FSR0H, F ; Add carry to high byte of pointer
movf _pausetime, W ; Load pausetime LB into WREG
movwf POSTINC0 ; Store pausetime LB in ppm location and automatically increment FSR0
movf _pausetime + 1, W ; Load pausetime HB into WREG
movwf INDF0 ; Store pausetime HB in ppm location
incf _ppm_n,F ; increment index ppm_n
BTFSS _ppm_n, 4
GOTO WWW
clrf _ppm_n ; clear index after index == 16
bsf _aflags,0 ; set ppmwrite to signal main program data captured
WWW
INT_RETURN
ENDASM



10037

[richard]

maybe like this

https://youtu.be/PQrvgA-w0gk

steps through an array to set portb pin and another array to set timer interval for next interrupt , repeats every 6th pass

ra0 toggles every interrupt ra1 every 6th

[achilles03]

Sorry I didn't get back to this thread sooner, but I got both interrupts working well for this application. For reference, these interrupts are specific to a quadcopter. The "PULSE_WIDTH" interrupt is reading pulse lengths from an RC reciever (using the "ppm" protocol which puts all channels on a single pin) and the "MOTOR_CONTROL" interrupt is driving mosfets that are connected to PORTC pins 4-7. I've run several verification tests to make sure they are working as intended, and it appears they are. Here is a video of the quadcopter flying with both new asm interrupts implemented:


https://www.youtube.com/watch?v=waSne5kYlvA

The landing was a little rough, but that blame falls on the pilot (me). For completeness, here is the final code I am using for both interrupts:


MOTOR_CONTROL:
ASM
banksel _mcount ; Variables used in interrupts are stored one bank via config definition - can change bank # as long as vars are in same bank
movf _mcount, W ; Get index value from variable 'mcount' for array address, store in WREG
incf _mcount, f ; Increment mcount before changing banks
lfsr FSR0, _motor ; Load the full 12-bit address of motor array into FSR0
addwf FSR0L, f ; Add offset to low byte of pointer
movlw 0 ; clear WREG although carry flag is still active in STATUS REGISTER bit0
addwfc FSR0H, f ; Add carry to high byte of pointer
movf INDF0, W ; Move INDF0 to WREG
banksel LATC ; Change bank to LATC
movwf LATC ; Change PORTC outputs based on WREG
banksel _mcount
movf _mcount, W ; Get index value from variable 'mcount' for array address, store in WREG
lfsr FSR0, _preload ; Load the full 12-bit address of preload array into FSR0
addwf FSR0L, f ; Add offset to low byte of pointer
movlw 0 ; clear WREG although carry flag is still active in STATUS REGISTER bit0
addwfc FSR0H, f ; Add carry to high byte of pointer
movf INDF0, W ; Move INDF0 to WREG
banksel TMR1H ; Change bank to TMR1H
movwf TMR1H ; Load preload byte into TMR1H
INT_RETURN
endasm

And the RC interrupt:


PULSE_WIDTH:
asm
banksel _ppm_n ; Variables used in interrupts are stored one bank via config definition - can change bank # as long as vars are in same bank
movff TMR3L, _nowtmr3 ; Read LB and store in variable 'pausetime' - also loads TMR3H into buffer
movff TMR3H, _nowtmr3 + 1 ; Read HB from buffer
movf _ppm_n, W ; Get index value from variable 'ppm_n' for array address, store in WREG
rlncf WREG, W ; Multiply WREG by 2 since ppm array is words (16bit)
lfsr FSR0, _ppm ; Load the full 12-bit address of ppm into FSR0
addwf FSR0L, f ; Add offset from WREG to low byte of pointer
movlw 0 ; clear WREG although carry flag is still active in STATUS REGISTER bit 0
addwfc FSR0H, f ; Add carry to high byte of pointer - pointer is now set to corret register!
;subtraction and store
movf _lasttmr3, W ; W = Low byte of lasttmr3
subwf _nowtmr3, W ; Subtract WREG from f and store in WREG -> WREG = nowtmr3_L - lasttmr3_L
movwf POSTINC0 ; Store result low byte of array
movf _lasttmr3 + 1, W ; W = High byte of subtrahend
btfss STATUS, C ; Did LSB subtraction produce a borrow? (C=0)
incfsz _lasttmr3 + 1, W ; Yes: W = lasttmr3 + 1 (adds borrow)
subwf _nowtmr3 + 1, W ; Subtract WREG from f and store in WREG -> WREG = nowtmr3_H - lasttmr3_H
movwf INDF0 ; Store result high byte
;subtraction end
incf _ppm_n, f ; increment array index variable ppm_n
movlw 18h ; move 24 (18h) into WREG to compare with ppm_n
cpfslt _ppm_n, 1 ; check if ppm_n < WREG (24), skip next instruction if ppm_n < WREG
clrf _ppm_n ; clear ppm_n if above condition is met
btg _ppmwrite ; toggle ppmwrite to check if interrupt occured in main program
bcf _RCtickle ; clear RCtickle to show RC is still receiving signals
movff _nowtmr3, _lasttmr3 ; Move current timer3 val to last timer3 val for next interrupt
movff _nowtmr3 + 1, _lasttmr3 + 1 ; Move current timer3 val to last timer3 val for next interrupt
INT_RETURN
endasm

[achilles03]

Also I forgot to mention: thanks Richard for the help!

[richard]

for what it worth you might save a nano second or two

i'm not exactly sure how the assembler works but SFR's that are in the access bank can be referenced without needing to select bank 15 explicitly, not all SFR's are in access bank
notice that addwf FSR0L, f works ok with out a banksel 15.
Technically movwf LATC,f,a would be more correct but the ass seems to interpret movwf LATC correctly no matter what bank is selected





MOTOR_CONTROL:ASM
banksel _mcount ; Variables used in interrupts are stored one bank via config definition - can change bank # as long as vars are in same bank
movf _mcount, W ; Get index value from variable 'mcount' for array address, store in WREG
incf _mcount, f ; Increment mcount before changing banks
lfsr FSR0, _motor ; Load the full 12-bit address of motor array into FSR0
addwf FSR0L, f ; Add offset to low byte of pointer
movlw 0 ; clear WREG although carry flag is still active in STATUS REGISTER bit0
addwfc FSR0H, f ; Add carry to high byte of pointer
movf INDF0, W ; Move INDF0 to WREG
;banksel LATC not needed LATC is in access bank ; Change bank to LATC
movwf LATC ; Change PORTC outputs based on WREG
banksel _mcount
movf _mcount, W ; Get index value from variable 'mcount' for array address, store in WREG
lfsr FSR0, _preload ; Load the full 12-bit address of preload array into FSR0
addwf FSR0L, f ; Add offset to low byte of pointer
movlw 0 ; clear WREG although carry flag is still active in STATUS REGISTER bit0
addwfc FSR0H, f ; Add carry to high byte of pointer
movf INDF0, W ; Move INDF0 to WREG
;banksel TMR1H not needed TMR1H is in access bank ; Change bank to TMR1H
movwf TMR1H ; Load preload byte into TMR1H
INT_RETURN
endasm