# Interrupt doesn't seem to reenable on PIC16F887

I'm building a timer using the PIC16F887, some push buttons, and seven-segment displays. I'm writing the code in assembly, and everything works great except the start/stop button which is handled as part of an interrupt service routine. The service routine complements the RUN variable which starts the timer/incrementer. The increment routine is skipped if the least significant bit of RUN is 0. I can start the timer, but I can't stop it. I clear the INTF flag in the INTCON register within the ISR, but it seems like the ISR remains disabled nonetheless.


;******************************************************
;PIC Configuration for PIC16F887
#include "p16F887.inc"

; CONFIG1
; __config 0x2032
__CONFIG _CONFIG1, _FOSC_HS & _WDTE_OFF & _PWRTE_OFF & _MCLRE_ON & _CP_ON & _CPD_ON & _BOREN_OFF & _IESO_OFF & _FCMEN_OFF & _LVP_OFF
; CONFIG2
; __config 0x3FFF
__CONFIG _CONFIG2, _BOR4V_BOR40V & _WRT_OFF
;******************************************************
DCOUNT       EQU 20H
CNT_100th    EQU 21H    ; counter for 0.01s
CNT_10th     EQU 22H    ; counter for 0.1s
CNT_sec      EQU 23H    ; counter for seconds
CNT_10sec    EQU 24H    ; counter for tens of seconds
CNT_min      EQU 25H    ; counter for minutes
CNT_10min    EQU 26H    ; counter for tens of minutes
RUN          EQU 70H    ;the run variable that does stuff to things
OCOUNT       EQU 28H
ICOUNT       EQU 29H

ORG  000H
GOTO MAIN

ORG   004H
GOTO  ISR

ORG   008H
GOTO  LOOP

;*******************************************************
MAIN
CLRF STATUS

CLRF  CNT_100th
CLRF  CNT_10th
CLRF  CNT_sec
CLRF  CNT_10sec
CLRF  CNT_min
CLRF  CNT_10min
CLRF  RUN

BSF    STATUS,RP1             ;change to bank 3
BSF    STATUS,RP0

CLRF   ANSEL                 ;Using all digital mode, turns off analog
CLRF   ANSELH                ;Make all analog ports digital

BCF    STATUS,RP1             ;change to correct bank to configure TRIS registers
;MAKE RA0 OUTPUT, RB0 INPUT, PORTC AND PORTD ALL OUTPUTS

CLRF  TRISA
CLRF  TRISB
COMF  TRISB,1
CLRF  TRISD

CLRF  INTCON

BSF   INTCON,7
BSF   INTCON,4
MOVLW 040H
MOVWF OPTION_REG

BCF   STATUS,RP0

GOTO  LOOP

LOOP
MOVF    CNT_100th,0 ; Put count of 1/100ths of second in W
CALL    TABLE         ; Get value to write out to PORTD from table
MOVWF   PORTD     ; Put value out to PORTD
BCF     PORTA,5     ; Turn on and off display to show digit
CALL    DELAY
BSF     PORTA,5

MOVF    CNT_10th,0 ; Put count of 1/100ths of second in W
CALL    TABLE         ; Get value to write out to PORTD from table
MOVWF   PORTD     ; Put value out to PORTD
BCF     PORTA,4     ; Turn on and off display to show digit
CALL    DELAY
BSF     PORTA,4

MOVF    CNT_sec,0 ; Put count of 1/100ths of second in W
CALL    TABLE         ; Get value to write out to PORTD from table
MOVWF   PORTD     ; Put value out to PORTD
BCF     PORTA,3     ; Turn on and off display to show digit
CALL    DELAY
BSF     PORTA,3

MOVF    CNT_10sec,0 ; Put count of 1/100ths of second in W
CALL    TABLE         ; Get value to write out to PORTD from table
MOVWF   PORTD     ; Put value out to PORTD
BCF     PORTA,2     ; Turn on and off display to show digit
CALL    DELAY
BSF     PORTA,2

MOVF    CNT_min,0 ; Put count of 1/100ths of second in W
CALL    TABLE         ; Get value to write out to PORTD from table
MOVWF   PORTD     ; Put value out to PORTD
BCF     PORTA,1     ; Turn on and off display to show digit
CALL    DELAY
BSF     PORTA,1

MOVF    CNT_10min,0 ; Put count of 1/100ths of second in W
CALL    TABLE         ; Get value to write out to PORTD from table
MOVWF   PORTD     ; Put value out to PORTD
BCF     PORTA,0     ; Turn on and off display to show digit
CALL    DELAY
BSF     PORTA,0

CALL    WAIT

BTFSC   RUN,0
CALL    INCCNT
GOTO    LOOP

WAIT
MOVLW     08H
MOVWF     OCOUNT
MOVLW     0FFH
MOVWF     ICOUNT

OLOOP
ILOOP
DECFSZ    ICOUNT,1
GOTO      ILOOP
DECFSZ    OCOUNT,1
GOTO      OLOOP

RETURN

ISR
COMF     RUN,1
BCF      INTCON,1
RETFIE



Any guidance would be greatly appreciated here. My suspicion is that there's something about the way I've structured the program which is causing this bug.

# Update

I solved the problem by clearing out the following registers associated with PORTB:

• CCP1CON
• IOCB
• WPUB

I also reformatted the code according to some of the stern advice below, and the PIC I'm using does have ANSEL and ANSELH registers. It's right there in black and white in the datasheet.

Thanks everyone

• I'm not very good with Assembly, but do you ever clear the interrupt flag in the ISR? – DerStrom8 Sep 18 '15 at 16:32
• I agree with the previous comment. Also it is good practice to minimize the code to the bare minimum required to reproduce the problem. The problem gets much more visible. – jippie Sep 18 '15 at 16:35

## 1 Answer

There is so much bad code here, it's hard to know where to begin. Answering the question directly in pointless until other things are fixed. You're worrying about what color socks to wear when jumping off a cliff.

I'll just mention a few things:

  DCOUNT       EQU 20H
CNT_100th    EQU 21H    ; counter for 0.01s
CNT_10th     EQU 22H    ; counter for 0.1s
CNT_sec      EQU 23H    ; counter for seconds
CNT_10sec    EQU 24H    ; counter for tens of seconds
CNT_min      EQU 25H    ; counter for minutes
CNT_10min    EQU 26H    ; counter for tens of minutes
RUN          EQU 70H    ;the run variable that does stuff to things
OCOUNT       EQU 28H
ICOUNT       EQU 29H


You seem to think you are allocating variables here, but all you are really doing is creating constants that have specific values. Only you know that these values are the addresses of memory locations. That's bad enough, but you are also specifying fixed addresses in the source code. You may have a reason to know what bank these variables are in, but there is no point specifying exact addresses. If you ever move this code to a different PIC, you may find that PIC doesn't have general RAM at 20h. Specify the bank if you need to, but let the linker place the variables within the bank.

The only way to allocate variables with MPASM/MPLINK is to use the RES directive.

Also, I see some comments, but "variable that does stuff to things"!? And what about DCOUNT, OCOUNT, and ICOUNT? What are those supposed to do.

I see you put RUN in the shared region, but didn't point this out. That's both sneaky and obnoxious.

  ORG   004H
GOTO  ISR


Not only is this silly, but also downright wrong on this machine. First, why not put the interrupt routine at 4? Some code will end up there, so it might as well be the one routine that actually benefits from being there. Second, this is outright wrong on a machine that has more than one code page. The GOTO instruction only encodes the low 11 bits of the address, with the upper 2 address bits coming from PCLATH. You don't know what PCLATH might be set to when a interrupt occurs, so you have one chance in 4 this will actually go to the right place.

Start the interrupt routine at 4 and let everything else flow around it.

  ORG   008H
GOTO  LOOP


What? Huh? This makes absolutely no sense whatsoever. Did you even look at the datasheet?

  BSF    STATUS,RP1             ;change to bank 3
BSF    STATUS,RP0


Don't do fixed bank setting. At the least use BANKSEL.

  CLRF   ANSEL                 ;Using all digital mode, turns off analog
CLRF   ANSELH                ;Make all analog ports digital


At least we've got some comments here so we can tell what you intended. However, this machine doesn't have ANSEL registers, which even a brief look at the datasheet would have revealed. To make all analog pins digital, you use the ADCON registers. Again, READ THE DATASHEET.

  BCF    STATUS,RP1             ;change to correct bank to configure TRIS registers


This will actually change the bank to 0 or 1, depending on the RP0 bit setting. The code above does set RP0, so this actually goes to bank 1. However, this dependency is not stated and this code will break if the code above it is modified to leave RP0 set to 0. Again, at least use BANKSEL. There are more clever and efficient ways to manage the bank setting, but BANKSEL is totally reliable and self-contained. When you're just starting, use it instead of trying to get tricky.

  GOTO  LOOP

LOOP


Where else did you think the code was going to go?

ISR
COMF     RUN,1
BCF      INTCON,1
RETFIE


In this case, you put RUN in the shared bank and INTCON is mapped to all bank, so these two instructions will actually work. However, you never cleared the interrupt condition! The processor will generate a new interrupt right after interrupts are enabled again by RETFIE. Once again, you actually have to read the datasheet, which is quite clear about how interrupts work.

Then there is the issue of debouncing, which I don't see any of. Even if you were to clear the interrupt condition properly, this still wouldn't work since you'll get multiple counts per button press and release.

I skipped over other issues, and there's more code I haven't even looked at, but I'm quitting here.