michael brown wrote: > Here is the most minimal code for a 16F88 (I just tested) that exhibits > the same type of behavior: I copied your code, deleted most of the wrapped lines (this wouldn't have been a problem if you just attached it instead of posting it in the body of the message), deleted the commented out code, and ran it thru my ASPIC_FIX program to put everything into one consistent format. I have some comments on the resulting code: > STARTOFRAM equ 0x20 ;First Usable RAM Location > ENDOFRAM equ 0x6F ;Last Usable RAM Location > FIXEDAREA equ 0x70 ;Start of shared memory > BANK1RAM equ 0xA0 ;Start of Bank 1 RAM > ENDOFBANK1 equ 0xEF ;End of Bank 1 RAM > > cblock FIXEDAREA > INT_FLAGS > W_TEMP > STATUS_TEMP > FSR_TEMP > PCLATH_TEMP > endc Give me a break! This sort of manual RAM allocation is a bug waiting to happen. In this particular case it looks like you got what you intended, but there is nothing guaranteeing that other "variables" won't be created with the same addresses or that you didn't overrun the 70h-7Fh unbanked region. You should be using the RES directive, especially if you care about your credibility on a public forum. > org 0x000 > goto Start This is a bug waiting to happen. The 16F88 has two pages, but you are doing nothing to make sure PCLATH is set properly for jumping to START. Again it works in this particular case because the code is so small, but that doesn't make it a good idea. > org 0x004 > ... > bcf STATUS, RP0 ;Back to Bank 0 Or bank 2. This could be a serious bug waiting to happen since you are relying on the state of RP1 on entry to the interrupt routine. This can be easily fixed by doing CLRF STATUS instead of the BCF. > btfsc PIR1, TMR1IF ;Is it a TMR1 interrupt > goto Timeout ;Yes PIR1 is only mapped to bank 0, but you could be in bank 0 or 2 here. > Timeout > > ; toggle LED pin > movfw PORTA > xorlw 0x01 > movwf PORTA There is a serious problem here. You are making the implicit assumption that the current bank is bank 0, but there is no mention of that at this entry point. If someone changed the earlier code or copied this snippet somewhere else, it could stop working. If a particular bank setting is required at an entry point, a comment should clearly state this restriction. > bcf PIR1, TMR1IF ;Clear interrupt flag > > ; Comment the next three lines for 2 second rollover or load W with 0x00 > movlw 0xC0 ;Preload for 1/2 second rollover > movwf TMR1H You are assuming TMR1H is in bank 0. I just checked and it is, but there is no evidence here that you thought about it. > Start > bsf STATUS, RP0 ;Switch to Bank 1 Or bank 3. > movlw b'00000000' ;All pins output > movwf TRISA & 0x7F Why are you ORing the address of TRISA with 7Fh? The instruction will only contain the 7 bit offset, so this does no harm, but makes the code confusing and hard to read. I had to think about it to realize that it does nothing. > movlw 0x00 ;All pins digital i/o for now > movwf ANSEL & 0x7F And we can see that you thought about the bank setting for ANSEL and that it is correct how? > movlw b'01100000' ;Set internal osc to 4Mhz > movwf OSCCON & 0x7F Bank for OSCCON? > bcf STATUS, RP0 ;Switch to Bank 0 Or bank 2. > clrf PORTA ;Drive all pins low > clrf PORTB > > call TMR1Init Page of TMR1INIT? > movlw b'00001110' ;TMR1 oscillator off, and timer > ; |||||||+-------------- TMR1ON (started later) > ; ||||||+--------------- TMR1CS (external 32768 crystal) > ; |||||+---------------- /T1SYNC (unsynchronized for > sleep) > ; ||||+----------------- T1OSCEN (enabled) > ; |||+------------------ T1CKPS0 > ; ||+------------------- T1CKPS1 > ; |+-------------------- TMR1GE (T1RUN on 16F88) > ; +--------------------- T1GINV (Unused on 16F88) Showing each bit is a good idea. This is way better than just writing a HEX value in the source code, which we see all too often. However your formatting doesn't leave a lot of room for comments for each bit and I personally find it visually a little confusing. Here is how I would have written this: dbankif t1con ;set bank for access to T1CON movlw b'00001110' ;configure timer 1 ; X------- unused bit ; -0------ system clock not from timer 1 ; --00---- select prescaler divide of 1 ; ----1--- enable timer 1 oscillator ; -----1-- do not sync to instruction clock ; ------1- clock source from timer 1 oscillator ; -------0 keep timer off for now movwf t1con This is just pointing out an alternative. I recognize this is largely a personal style issue. > ; Clear the timer registers > clrf TMR1H > clrf TMR1L Bank settting? > ; Clear the interrupt flag > bcf PIR1, TMR1IF > > ; Enable peripheral interrupts > bsf INTCON, PEIE > > ; Enable TMR1 interrupts > bsf STATUS, RP0 ;Select Bank 1 Or bank 3. > bsf PIE1 & 0x07F, TMR1IE ;Turn on TMR1 ints > bcf STATUS, RP0 ;Select Bank 0 Or bank 2. In general you need to pay more attention to the bank setting. For small test programs where neither space nor speed is critical use BANKSEL liberally. It proves that you thought about it and makes it clear the bank is set properly, whether it causes extra instructions or not. Also, use relocatable mode and the RES directive. You should want to do this for your own reasons, but at least do it when posting code publicly if you want to have any credibility. ****************************************************************** Embed Inc, Littleton Massachusetts, (978) 742-9014. #1 PIC consultant in 2004 program year. http://www.embedinc.com/products -- http://www.piclist.com PIC/SX FAQ & list archive View/change your membership options at http://mailman.mit.edu/mailman/listinfo/piclist