65C02 and IRQ service routine trouble

[jgroth]

I have an ISR that is working but the vector to the routine is hard coded and I would like to be able to change it so I would like to have the IRQ vector in RAM instead. I thought that wouldn't be that hard to code but I'm stuck because the code simply doesn't work.
So the reset code looks like this:

Code: Select all

1383    .8849                   COLDSTART
1384    .8849   d8                      CLD           ;Disable decimal mode
1385    .884a   a2 ff                   LDX  #$FF     ;Initialize STACK POINTER
1386    .884c   9a                      TXS
1388    .884d   a5 ab                   LDA  <DOINTERRUPT
1389    .884f   8d 02 03                STA  INTERRUPTVECTOR
1390    .8852   a5 89                   LDA  >DOINTERRUPT
1391    .8854   8d 03 03                STA  INTERRUPTVECTOR+1
....

COLDSTART is the reset vector which initialise the IRQ vector.
The ISR looks like this.

Code: Select all

1601    .89a8                   INTERRUPT
1602    .89a8   6c 02 03                JMP  (INTERRUPTVECTOR)
1604    .89ab                   DOINTERRUPT
1605    .89ab   85 ed                   STA  AINTSAV  ;Save ACCUMULATOR
1606    .89ad   86 ee                   STX  XINTSAV  ;Save X-REGISTER
1607    .89af   84 ef                   STY  YINTSAV  ;Save Y-REGISTER
1608    .89b1   ad e1 7f                LDA  SIOSTAT  ;Read 6551 ACIA status register
1609    .89b4   29 88                   AND  #$88     ;Isolate bits. bit 7: Interrupt has occured and bit 3: receive data register full
1610    .89b6   49 88                   EOR  #$88     ;Invert state of both bits
.....

and so on. The ISR just receives bytes from the ACIA and puts them in a buffer.
The initialisation of the vectors looks like this

Code: Select all

3232    >fffa   00 03                    .word $0300        ;NMI
3233    >fffc   49 88                    .word COLDSTART    ;RESET
3234    >fffe   a8 89                    .word INTERRUPT    ;IRQ

So why is it that if I change the IRQ vector to DOINTERRUPT everything works but if it is INTERRUPT nothing works?
I know I'm missing something trivial but I can't find figure out why it is not working.
Any ideas anyone?

/Jgroth

[Dr Jefyll]

Code: Select all

1388    .884d   a5 ab                   LDA  <DOINTERRUPT          <------- opcode should be $A9 not $A5
1389    .884f   8d 02 03                STA  INTERRUPTVECTOR
1390    .8852   a5 89                   LDA  >DOINTERRUPT          <------- opcode should be $A9 not $A5
1391    .8854   8d 03 03                STA  INTERRUPTVECTOR+1

I think you intended these two LDA instructions to use immediate address mode. But you've omitted the # symbol and consequently the assembler gave you z-pg mode instead.

Darn assembler! Insists on giving you what you asked for (instead of what you wanted)!!

[jgroth]

Dr Jefyll wrote:

Code: Select all

1388    .884d   a5 ab                   LDA  <DOINTERRUPT          <------- opcode should be $A9 not $A5
1389    .884f   8d 02 03                STA  INTERRUPTVECTOR
1390    .8852   a5 89                   LDA  >DOINTERRUPT          <------- opcode should be $A9 not $A5
1391    .8854   8d 03 03                STA  INTERRUPTVECTOR+1

I think you intended these two LDA instructions to use immediate address mode. But you've omitted the # symbol and consequently the assembler gave you z-pg mode instead.

Darn assembler! Insists on giving you what you asked for (instead of what you wanted)!!

Slapping palm against forehead! That was it. I knew it was something simple. Thank you so much!

[Dr Jefyll]

You're welcome -- glad to help! I admit I had to stare at this for a while.

[ramble]

When I first learned to code it was a pencil and paper affair. This meant manually looking up every opcode that was required. (No assembler, eh?) To slightly reduce the drudgery I memorized a dozen or so of the most common opcodes.

This was a cloud that didn't seem to have any silver lining. But today it was the a5 ab in the object code of your program that eventually jumped out at me, not the missing # in the corresponding source. Maybe the LDA <DOINTERRUPT ought to have been conspicuous (ie: no #), but frankly I got entirely distracted wondering about the < operator (not the #).

I don't know if there's any moral to all this... [/ramble]

-- Jeff

[BigDumbDinosaur]

Dr Jefyll wrote:

Code: Select all

1388    .884d   a5 ab                   LDA  <DOINTERRUPT          <------- opcode should be $A9 not $A5
1389    .884f   8d 02 03                STA  INTERRUPTVECTOR
1390    .8852   a5 89                   LDA  >DOINTERRUPT          <------- opcode should be $A9 not $A5
1391    .8854   8d 03 03                STA  INTERRUPTVECTOR+1

I think you intended these two LDA instructions to use immediate address mode. But you've omitted the # symbol and consequently the assembler gave you z-pg mode instead.

Darn assembler! Insists on giving you what you asked for (instead of what you wanted)!!

Good catch!

[BigDumbDinosaur]

jgroth wrote:

Code: Select all

1383    .8849                   COLDSTART
1384    .8849   d8                      CLD           ;Disable decimal mode
1385    .884a   a2 ff                   LDX  #$FF     ;Initialize STACK POINTER
1386    .884c   9a                      TXS
1388    .884d   a5 ab                   LDA  #<DOINTERRUPT
1389    .884f   8d 02 03                STA  INTERRUPTVECTOR
1390    .8852   a5 89                   LDA  #>DOINTERRUPT
1391    .8854   8d 03 03                STA  INTERRUPTVECTOR+1
....

I suggest you insert SEI as the first instruction in the above and then follow STA INTERRUPTVECTOR+1 with CLI. Once interrupt processing is in effect, re-executing the above code could result in an IRQ occurring between an LDA and an STA. If subsequent code pointed INTERRUPTVECTOR to a new location, the temporary inconsistency of the vector as it is being initialized will likely cause a crash.

Quote:

Code: Select all

1601    .89a8                   INTERRUPT
1602    .89a8   6c 02 03                JMP  (INTERRUPTVECTOR)
1604    .89ab                   DOINTERRUPT
1605    .89ab   85 ed                   STA  AINTSAV  ;Save ACCUMULATOR
1606    .89ad   86 ee                   STX  XINTSAV  ;Save X-REGISTER
1607    .89af   84 ef                   STY  YINTSAV  ;Save Y-REGISTER
1608    .89b1   ad e1 7f                LDA  SIOSTAT  ;Read 6551 ACIA status register
1609    .89b4   29 88                   AND  #$88     ;Isolate bits. bit 7: Interrupt has occured and bit 3: receive data register full
1610    .89b6   49 88                   EOR  #$88     ;Invert state of both bits
.....

Using RAM as temporary register storage within an ISR as you are doing is not particularly good practice, as it precludes the possibility of processing nested IRQs. I recommend you push them to the stack and then pull them (in reverse order) at the termination of your ISR. Also, don't save registers that your ISR will not use, as all you'll be doing is wasting clock cycles.

Your code will be slightly faster and more efficient with the following change:

Code: Select all

INTERRUPT   JMP  (INTERRUPTVECTOR)
DOINTERRUPT PHA                       ;save MPU state
            PHX                       ;save if necessary
            PHY                       ;save if necessary
            LDA SIOSTAT               ;Read 6551 ACIA status register
            BPL NOT_ACIA              ;skip ahead if ACIA is not interrupting
;
;...no reason to execute the following instructions if the ACIA wasn't the IRQ source...
;
            AND  #$88                 ;Isolate bits. bit 7: Interrupt has occured and bit 3: receive data register full
            EOR  #$88                 ;Invert state of both bits
...

Something else: values such as the two masks used in the final two instructions should not be hard-coded into your source. They should be symbolically defined and since they are masks, should be expressed in bit-wise notation so it is clear which bits are being manipulated. Although I and some others here can mentally translated hex values to binary equivalents (and in reverse), others may not. A year or two or three from now when you revisit the code you yourself may be pondering what you meant with the above code.

Documentation means not only commenting the intended effect of each instruction, it also means defining "magic numbers" referred to within your code. This is where INCLUDE files are put to use. A professional would put something like the following into an INCLUDE file somewhere and refer to it during assembly:

Code: Select all

ACIAMASK  = %10001000        ;mask used to analyze ACIA status

and later on:

Code: Select all

          AND #ACIAMASK                
          EOR #ACIAMASK

Of course, there is a more efficient way to analyze the 6551's status—you don't need to know about bit 7 once you fall through the BPL NOT_ACIA instruction. However, that's for you to work out.

BTW, have you studied Garth Wilson's interrupt primer?

[jgroth]

BigDumbDinosaur wrote:

jgroth wrote:

Code: Select all

1383    .8849                   COLDSTART
1384    .8849   d8                      CLD           ;Disable decimal mode
1385    .884a   a2 ff                   LDX  #$FF     ;Initialize STACK POINTER
1386    .884c   9a                      TXS
1388    .884d   a5 ab                   LDA  #<DOINTERRUPT
1389    .884f   8d 02 03                STA  INTERRUPTVECTOR
1390    .8852   a5 89                   LDA  #>DOINTERRUPT
1391    .8854   8d 03 03                STA  INTERRUPTVECTOR+1
....

I suggest you insert SEI as the first instruction in the above and then follow STA INTERRUPTVECTOR+1 with CLI. Once interrupt processing is in effect, re-executing the above code could result in an IRQ occurring between an LDA and an STA. If subsequent code pointed INTERRUPTVECTOR to a new location, the temporary inconsistency of the vector as it is being initialized will likely cause a crash.

Good point. SEI and CLI added.

Quote:

Code: Select all

1601    .89a8                   INTERRUPT
1602    .89a8   6c 02 03                JMP  (INTERRUPTVECTOR)
1604    .89ab                   DOINTERRUPT
1605    .89ab   85 ed                   STA  AINTSAV  ;Save ACCUMULATOR
1606    .89ad   86 ee                   STX  XINTSAV  ;Save X-REGISTER
1607    .89af   84 ef                   STY  YINTSAV  ;Save Y-REGISTER
1608    .89b1   ad e1 7f                LDA  SIOSTAT  ;Read 6551 ACIA status register
1609    .89b4   29 88                   AND  #$88     ;Isolate bits. bit 7: Interrupt has occured and bit 3: receive data register full
1610    .89b6   49 88                   EOR  #$88     ;Invert state of both bits
.....

BigDumbDinosaur wrote:

Using RAM as temporary register storage within an ISR as you are doing is not particularly good practice, as it precludes the possibility of processing nested IRQs. I recommend you push them to the stack and then pull them (in reverse order) at the termination of your ISR. Also, don't save registers that your ISR will not use, as all you'll be doing is wasting clock cycles.

Will take that into consideration

BigDumbDinosaur wrote:

Your code will be slightly faster and more efficient with the following change:

Code: Select all

INTERRUPT   JMP  (INTERRUPTVECTOR)
DOINTERRUPT PHA                       ;save MPU state
            PHX                       ;save if necessary
            PHY                       ;save if necessary
            LDA SIOSTAT               ;Read 6551 ACIA status register
            BPL NOT_ACIA              ;skip ahead if ACIA is not interrupting
;
;...no reason to execute the following instructions if the ACIA wasn't the IRQ source...
;
            AND  #$88                 ;Isolate bits. bit 7: Interrupt has occured and bit 3: receive data register full
            EOR  #$88                 ;Invert state of both bits
...

Something else: values such as the two masks used in the final two instructions should not be hard-coded into your source. They should be symbolically defined and since they are masks, should be expressed in bit-wise notation so it is clear which bits are being manipulated. Although I and some others here can mentally translated hex values to binary equivalents (and in reverse), others may not. A year or two or three from now when you revisit the code you yourself may be pondering what you meant with the above code.

Yeah, should be constant with a meaningful name (haven't gotten that far yet).

BigDumbDinosaur wrote:

Documentation means not only commenting the intended effect of each instruction, it also means defining "magic numbers" referred to within your code. This is where INCLUDE files are put to use. A professional would put something like the following into an INCLUDE file somewhere and refer to it during assembly:

Code: Select all

ACIAMASK  = %10001000        ;mask used to analyze ACIA status

and later on:

Code: Select all

          AND #ACIAMASK                
          EOR #ACIAMASK

Of course, there is a more efficient way to analyze the 6551's status—you don't need to know about bit 7 once you fall through the BPL NOT_ACIA instruction. However, that's for you to work out.

BTW, have you studied Garth Wilson's interrupt primer?

No, where can I find it?

[GARTHWILSON]

jgroth wrote:

BigDumbDinosaur wrote:

BTW, have you studied Garth Wilson's interrupt primer?

No, where can I find it?

Here. (Enjoy the outdated cartoons! Actually, that's where Mike Naberezny pulled my avatar from.)