WREG saving in interrupts?

[blackcattech]

I've been having a problem where a Word variable which is access in my ISR sometimes isn't set properly outside the ISR. I finally worked out that if the ISR call coincides with the write to the variable then only the lower byte is written correctly.

I've found a fix for this but I am confused as to why it is necessary. I have changed the start of the ISR to

Code: Select all

Save(0,FSR0,FSR1,WREG)

and all now works as expected.

What is confusing me is that the Swordfish help files suggest that WREG is saved as standard. Has something changed, or is there a known issue with some PICs that prevents this happening?

I'm using a PIC18F26K22. The datasheet suggests that WREG, STATUS and BSR are saved on the 'Fast Return Stack' but that if fast return from interrupt is not used then these may need to be saved by the user.

Does Swordfish use the fast return from interrupt mechanism, or could it be this that is causing WREG to not be saved? The Swordfish help file suggests these registers are stored in hardware shadow registers when an ISR is called - maybe the storage mechanism has changed for more modern PICs?

[David Barker]

Assuming Microchip hasn't changed anything since I last looked, registers will only be saved in shadow RAM for a single interrupt. If another interrupt occurs during the servicing of another interrupt, then the registers need to be saved in software. If you examine the ASM, I'm sure you will find that Swordfish is correctly context saving WREG, if required. You mentioned touching a word variable outside of the ISR which is used from within an ISR - this is a likely cause of your problem. Don't forget, an ISR happens at the assembler level so writing to a word variable may not have completed when the ISR is invoked. I suspect if you disable the ISR when writing to the variable, it will probably fix the problem.

[blackcattech]

I only have one ISR defined in this program, although it services multiple interrupts. However, isn't GIE disabled as soon as you enter the ISR and not re-enabled until the interrupt exits?

Am I right in my understanding that you are talking about using both interrupt priority levels when you talk about servicing another interrupt, not if another interrupt of the same level happens? I'd assume in that case the ISR would just be called again immediately after it exits.

I found two ways to fix the problem, as you suggested I could disable interrupts before writing to the word variable but also adding WREG to the Save at the start of the ISR also cured it.

[David Barker]

> isn't GIE disabled as soon as you enter the ISR

That's not what I meant...

> I've been having a problem where a Word variable which is
> access in my ISR sometimes isn't set properly outside the ISR.

You could be in the middle of reading a word variable, you have just read a byte of the word variable when - INTERRUPT - the value gets changed, then you return to your code. You then read the next byte for the comparison. But you have read half a word before the ISR and half a word after the ISR!! Hence:

> ...my ISR sometimes isn't set properly outside the ISR

That's why disabling the ISR BEFORE you test the word value and then enabling it again is what you should be doing. This is good practice. Simply saving WREG again sounds like a fudge to me and more than likely bite you again further down the road.

[blackcattech]

I don't know if it makes any difference, but I only write the variable outside the ISR.

It is a 'timer'. The ISR is called at a fixed frequency. It tests if the variable is non-zero and if so decrements it, setting a flag when it reaches zero.

What seems to be happening is that the lower byte is being written OK but the upper byte isn't, so the delay is much shorter than it should be. Looking at the assembler code it appears that the W register is loaded with one byte of the word then this is written to the appropriate RAM location. The only way it could be getting corrupted is if the value in the W register was not being saved and restored during the interrupt.

My question really is why adding a Save (WREG) is fixing the problem if WREG is supposed to be saved anyway. I made the change last week and haven't experienced any issues since during my testing. It may be that this change is masking some underlying problem of course.

All that said, yesterday I changed the code to make use of the extra timers on the PIC18F26K22 so I only need to use bytes to store the delay period, not words which should eliminate the issue anyway. The query is more out of interest to see if it is an issue with the new PICs or something more subtle wrong with my code.

[Jerry Messina]

There were some PIC18's that had issues with 2-cycle instructions and the fast interrupt stack. I was unlucky enough to use one of them... an 18F4680. But, that was quite a while ago and there's no such errata for the 26K22.

If you're using the fast interrupt shadow registers, the chip will save the STATUS, BSR, and WREG for you. You can verify that's the case by looking at the return instruction for the isr... if it's 'RETFIE 1' then you're good to go. A plain 'RETFIE' indicates the shadow regs aren't being used. If you'd like to experiment you can disable the fast interrupt context save and force it to insert instructions to save the regs in software by using

Code: Select all

#option ISR_SHADOW = false

I'd heed David's advice. You've got to be really careful accessing a multi-byte object in both the isr and main context, and disabling the interrupt is probably the safest way to go. It's likely adding the 'SAVE(WREG)' is masking some other issue, like things moving around in memory.

I don't know if it makes any difference, but I only write the variable outside the ISR.

It is a 'timer'. The ISR is called at a fixed frequency. It tests if the variable is non-zero and if so decrements it, setting a flag when it reaches zero.

Sounds like you're writing the 'timer' variable INSIDE the isr to me. Do you still have snippets of the old code showing how the var was being used? Maybe we can figure out what's going on, if you're still interested.

[blackcattech]

I've checked the assembly code and it does do 'REFTIE 1'

However, something very odd. I can't see anything restoring WREG despite it being saved. The code only seems to restore '0', FSR0 and FSR1. Indeed, the code before the restoration code is a DECFSZ WREG command (followed by a BRA -6) so obviously WREG isn't saved in software.

Does Swordfish know that WREG is saved in hardware so ignores the additional command to save it in software?

That would confirm that adding the Save WREG was masking the problem, not fixing it.

Useful to know for future - as I commented, I ended up rewriting this ISR to only use byte variables to streamline things and try and reduce memory usage so the problem has been fixed that way.

[David Barker]

> However, something very odd. I can't see anything
> restoring WREG despite it being saved

I'm not seeing anything odd at all - you need to post some code if you think there is an issue. Here is what I see here:

Code: Select all

Interrupt OnTimer(1)
End Interrupt
Enable(OnTimer)

ISR_L00000000
RETFIE 1

Code: Select all

#option ISR_SHADOW = false
Interrupt OnTimer(1)
End Interrupt
Enable(OnTimer)

ISR_L00000000
MOVFF STATUS, F0_U08
MOVFF WREG, F1_U08
MOVFF BSR, F2_U08
MOVFF F2_U08, BSR
MOVFF F1_U08, WREG
MOVFF F0_U08, STATUS
RETFIE

both of the above look correct.

[Jerry Messina]

Just to add... this is what you see when using the addt'l 'save(0, fsr1, fsr2, wreg)'

Code: Select all

dim b as byte

interrupt isr()
    save(0, fsr1, fsr2, wreg)
    b = 0       // dummy code so the isr isn't empty
    restore
end interrupt

enable(isr)

Code: Select all

; ISR
ISR_L00000000
// fast shadow isr automatically saves STATUS, BSR, and WREG
    MOVLB 0
// save (0, FSR1, FSR2, WREG)...
// save FSR1
    MOVFF _FSR1L#M0_U16H,F0_U16H
    MOVFF _FSR1L#M0_U16,F0_U16
// save FSR2
    MOVFF _FSR2L#M0_U16H,F2_U16H
    MOVFF _FSR2L#M0_U16,F2_U16
// save WREG
    MOVWF F4_U08,0
// save system vars (save(0))
// save FSR0 since it's used to save the system block
    MOVFF FSR0L,F5_U08
    MOVFF FSR0H,F6_U08
    LFSR 0,F7_U200
    CLRF FSR1L,0
    CLRF FSR1H,0
    MOVLW 25
    MOVFF POSTINC1,POSTINC0
    DECFSZ WREG,1,0
    BRA $ - 6
// and PROD reg
    MOVFF PRODL,F32_U08
    MOVFF PRODH,F33_U08

?I000000_F000_000005_P000001 ; L#MK B = 0       // DUMMY CODE SO THE ISR ISN'T EMPTY
    CLRF M0_U08,0

// restore...
// restore PROD
    MOVFF F33_U08,PRODH
    MOVFF F32_U08,PRODL
// restore system vars
    CLRF FSR0L,0
    CLRF FSR0H,0
    LFSR 1,F7_U200
    MOVLW 25
    MOVFF POSTINC1,POSTINC0
    DECFSZ WREG,1,0
    BRA $ - 6
// restore FSR0
    MOVFF F6_U08,FSR0H
    MOVFF F5_U08,FSR0L
// restore WREG
    MOVF F4_U08,0,0
// restore FSR2
    MOVFF F2_U16H,_FSR2L#M0_U16H
    MOVFF F2_U16,_FSR2L#M0_U16
// restore FSR1
    MOVFF F0_U16H,_FSR1L#M0_U16H
    MOVFF F0_U16,_FSR1L#M0_U16
?I000001_F000_000007_P000001 ; L#MK END INTERRUPT
// fast shadow return restores STATUS, BSR, and WREG
    RETFIE 1

If you leave out saving WREG using 'save(0, FSR1, FSR2)', the only thing that gets dropped out is the

Code: Select all

// save WREG
    MOVWF F4_U08,0
...
// restore WREG
    MOVF F4_U08,0,0
....

but WREG is saved/restored by the fast shadow feature

[blackcattech]

OK, I've edited out the actual content of the ISR, but my code is:

Code: Select all

Interrupt OnInt()
   Dim C As Byte

   Save(0,FSR0,FSR1,WREG) 
                                                     
   If Timer0IF = true Then                           // Timer 0 called 10 times a second. Decrements timers.
     Timer0H = T0ReloadH                             // Reload

...

     Timer4IF = false
   EndIf
     
   Restore
End Interrupt

And the ASM output is

Code: Select all

; ONINT
ISR_L00000281
        MOVFF FSR0__FSR0L#M0_U16H,F141_U16H
        MOVFF FSR0__FSR0L#M0_U16,F141_U16
        MOVFF FSR1__FSR1L#M0_U16H,F143_U16H
        MOVFF FSR1__FSR1L#M0_U16,F143_U16
        MOVWF F145_U08
        LFSR 0,F146_U200
        CLRF FSR1L,0
        CLRF FSR1H,0
        MOVLW 25
        MOVFF POSTINC1,POSTINC0
        DECFSZ WREG,1,0
        BRA $ - 6
        MOVFF PRODL,F171_U08
        MOVFF PRODH,F172_U08
?I000902_F000_000285_P000215 ; L#MK If TIMER0IF = TRUE Then                                  // ...
        BTFSS INTCON,2,0
        BRA L00000646
?I000903_F000_000286_P000215 ; L#MK TIMER0H = T0RELOADH                                    // RE...
        MOVLW 60
        MOVWF TMR0H,0

...

?I000955_F000_000361_P000215 ; L#MK TIMER4IF = FALSE
        BCF PIR5,0,0
L00000682
        MOVFF F172_U08,PRODH
        MOVFF F171_U08,PRODL
        CLRF FSR0L,0
        CLRF FSR0H,0
        LFSR 1,F146_U200
        MOVLW 25
        MOVFF POSTINC1,POSTINC0
        DECFSZ WREG,1,0
        BRA $ - 6
        MOVF F145_U08,0
        MOVFF F143_U16H,FSR1__FSR1L#M0_U16H
        MOVFF F143_U16,FSR1__FSR1L#M0_U16
        MOVFF F141_U16H,FSR0__FSR0L#M0_U16H
        MOVFF F141_U16,FSR0__FSR0L#M0_U16
?I000956_F000_000365_P000215 ; L#MK End Interrupt
        RETFIE 1

I don't fully understand the ASM code, but I can't see anything that appears to save or restore WREG. I don't have ISR_SHADOW set to false in code, but I do include WREG in the Save ... Restore block

Does Swordfish optimise out the Save ... WREG command because it knows it will be saved in hardware?

[Jerry Messina]

I don't fully understand the ASM code, but I can't see anything that appears to save or restore WREG

WREG is done by

Code: Select all

...
        MOVWF F145_U08
...
        MOVF F145_U08,0

But, it should be redundant because of the RETFIE 1

[blackcattech]

Ah, OK. I was expecting WREG to be in there somewhere. I guess MOVWF is obvious, but the restoring code isn't so obvious unless you know more about PIC assembly.