0
\$\begingroup\$

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

\$\endgroup\$
  • 3
    \$\begingroup\$ I'm not very good with Assembly, but do you ever clear the interrupt flag in the ISR? \$\endgroup\$ – DerStrom8 Sep 18 '15 at 16:32
  • 2
    \$\begingroup\$ 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. \$\endgroup\$ – jippie Sep 18 '15 at 16:35
4
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.