Newbie code attempt - advice please?

[APL]

Would appreciate it if some one could have a look at this code I have put together. It is supposed to read a latch that reflects the status of four switches (high nibble) and depending on which key is pressed, toggle the led at the low order nibble at the same address. What it does do is switch the leds on when you press the corresponding key, but doesn't toggle the led. If I press another key the led for that key goes on but only one led illuminates at one time. There is probably a lot wrong with the code it'd be great if you guys would give me an idea what I am doing wrong.

Code: Select all

ZPFPLED		EQU	$00		;Zero page location holds status of fp leds
FPLED		EQU	$A002
FBSWTCH		EQU	FPLED

		org	$E000		;Begining of ROM
;
;
;
SWCHTBL   	dw      0xE0D0		;Look up table for fp switches
                dw      0xB070		;End lookup table
SWCHLUP   	dw      0x0804		;Look up table for mask
                dw      0x0201		;End lookup table
SWCHCOM	dw      0xF7FB             ;Compliment of SWCHLUP table
                dw      0xFDFE		;End lookup table
                dw      0x0000
                dw      0x0000


Reset01     ;Initialise machine
		;
		CLC                     ;Clear carry flag
		CLD
		SEI                     ;disable interupts
		;
		LDX #$FF
		TXS                     ;Initialise Stack pointer
		;
		LDX #$0F
		;
		STX	ZPFPLED	  ;Initialise zero page location holds status of leds	
      		;
		;
		TXA
		STA FPLED		 ;Make sure all leds are off
		;
       		;
		CLI
		;
		;
		CLC
Labelend:	LDA FBSWTCH		;read switch on front pannel
		AND #$F0		        ;Clear lower four bits
                CMP #$F0		        ;Key pressed at all?
		BEQ NEXT001              ;If no key pressed exit
		LDX #$03
NEXT002:	CMP SWCHTBL,X		;Which Key was pressed
		BEQ NEXT004
		DEX
		BNE NEXT002		;Loop until we find match
NEXT004:	TXA			        ;X holds index to key pressed
		;	
		;
		;	Lamp control routine.
		;	that toggles individual Led's on or off.
		;
		TAX			;
		LDA SWCHLUP,X		; Get mask for from table according to index
		;
		BIT ZPFPLED 		;Test bits
		;
		BNE NEXT005
		;Bit result is zero. Led is on need to switch off
		LDA ZPFPLED
 		ORA SWCHLUP,X
		STA ZPFPLED
		BRA NEXT003

NEXT005:	; Bit is set. Led is off need to switch it on.
		LDA ZPFPLED
		AND SWCHCOM,X
		STA ZPFPLED
		;
		; 
NEXT003:	LDA ZPFPLED
		STA FPLED		;Output to panel leds
NEXT001:	NOP 

		;
		;
		JMP	Labelend

TIA

[GARTHWILSON]

I might be able to take more time with it later, but quickly now before I have to go (the first and last points and the comment at the end are most important, while observing the rest will make the code more concise and easier to look at and keep control of):

• I suspect that the dw directive will put low byte first as is normal for 6502, so your "dw 0xE0D0" will lay down 0D 0E, not 0E 0D.  (It looks like you got it straight in SWCHCOM.)  I would replace dw with db (define byte—I don't know if that's what your assembler uses), and change the order and put them in binary so it's more visible that the bits are marching across, one position per byte.
• The first CLC is irrelevant because it is not known yet if the first operation that needs it will need it set or cleared.
• The CLD is only necessary on NMOS 6502's.  CMOS come out of reset with D clear.
• The SEI is unnecessary because the processor comes out of RST with interrupts disabled anyway.
• Blank lines don't need to be commented out.
• The TXA is unnecessary because the next instruction can be STX instead of STA, and the value is not needed in A later either.
• The CLI farther down is unnecessary because you haven't set up any I/O ICs to produce interrupts anyway.
• At NEXT004, the TXA...TAX is unnecessary.  The index value you want next is already in X, and the contents of A are going to get replaced anyway.
• In your instruction BIT ZPFPLED, ZPFPLED is a constant and I don't think you meant it to be an address, so the BIT needs to be BIT #ZPFPLED (don't miss the "#").

As I explain in the keypads section of the circuit potpourri page of the 6502 primer though, you must debounce.  The discussion there tells why and gives ideas of how.

[MichaelM]

Generally, most of this forum's members don't provide solutions when it appears that it is a homework problem. However, it appears that you've made an attempt to solve this problem, so I'll provide you a few pointers.

First, you need to look into the instruction set a bit more carefully. This problem has a fairly straight forward solution. Many years ago, my lab partners and I, pressed for time, solved a similar problem on an old SDK-85 trainer board. However, our solution required virtually all of the available memory in the device. We thought we were kind of special since we were the only ones to complete the assignment. The professor quickly brought us back down to earth when he presented the solution in class as an in-line solution of 16 instructions versus our solution of nearly 256 bytes.

Your initialization is correct, except for one nit picky issue. You use a CLI instruction to enable interrupts, but don't don't include an interrupt service routine in your code. You are also using two different styles for the definitions of your hexadecimal constants. Some assemblers recognize and support multiple styles, but I recommend using a single style throughout any one program.

Now to your approach. In general, the approach you have selected is not incorrect. However, the solution does not require any loops or complex masks as you have implemented. To arrive at the optimal solution, you will need to review the problem statement. The clue to the solution is right in the problem statement that you posted along with your code.

There is one important concept missing from your solution and that is how to toggle an LED.

You have isolated the switch using an active low mask because the switch is apparently closed to ground, and sensed by the processor as a logic 0. Dealing with active low or negative logic will be with you in any hardware design or programming you may do in your career. However, I've learned that it's easy to fall into a logical trap when mixing active low and active high signals.

Therefore, my recommendation is that all asserted signals be converted into a common basis. The processor that you are working with uses a logic 1 as the asserted state for its instructions and internal signals. This means that you need to isolate the switches with a mask, and then convert all 0s into 1s and vice-versa. There is a specific instruction in the 6502 instruction set for doing this required operation.

Quote:

It is supposed to read a latch that reflects the status of four switches (high nibble) and depending on which key is pressed, toggle the led at the low order nibble at the same address.

Second, you need to re-read the problem statement and then look for specific instructions or short sequences of instructions that will allow you move the bits of the inverted switches into alignment with the LEDs. Once this is accomplished, you only need to use a single instruction to toggle the state of the LEDs, store the resulting value into the zp working register you have already allocated, and then write that same value to the LED output register.

I have written a solution for this problem, and the solution does not require any loops over the bits representing the switches, and it only requires approximately 10 instructions and 19 bytes for the main program loop.

Hope this helps.

PS: A final solution may also require the switch debouncing that Garth alludes to in his response. I will assume for this excercise that the trainer that you are working with is taking care of this critical issue for you at this time.

[APL]

GARTHWILSON wrote:

As I explain in the section of the 6502 primer though, you must debounce.

That was an informative reference.

MichaelM wrote:

Generally, most of this forum's members don't provide solutions when it appears that it is a homework problem.

I know this is a simple problem, but I was just getting very frustrated with it. I'd characterize it as a hobby project rather than something that might lead to anything as useful as a certificate.

Thanks to both of you for your replies.

[MichaelM]

If it's not for a "certificate", then take a look at my solution, untested as it is, which I posted to GitHUB here.

I started my initial review of your approach and slipped off the rails as well. I realized that I was making the problem more complicated than necessary. I was using bit masks and counts from registers and the stack to walk across each bit just as you were attempting.

In the process of optimizing the solution, I realized from re-reading your problem statement, that this was a simple problem that does not require looping over the bits to solve.

[APL]

MichaelM wrote:

If it's not for a "certificate", then take a look at my solution, untested as it is, which I posted to GitHUB.

Thank you Michael, I had a look at your code and tried it in my machine. I had written something similar with five 'ROL's in an earlier test routine but hadn't thought to use the EOR instruction in this context.

Actually, I am starting to wonder if my hardware is at fault, as your code doesn't work on this machine either - It does illuminate the leds when I press a key but not in a systematic way, at least not a system I can make out.

MichaelM wrote:

I started my initial review of your approach and slipped off the rails as well. I realized that I was making the problem more complicated than necessary. I was using bit masks and counts from registers and the stack to walk across each bit just as you were attempting.

I have an ulterior motive for doing it that way, I have a plan in my mind that eventually I will be able to call the routine that just deals with the leds from any process that may be running on the cpu - with, for example - the number of the led to toggle in the x register.

[Dr Jefyll]

Quote:

and depending on which key is pressed, toggle the led

The wording of the problem needs to be considered more carefully. Seems to me it's implied that the led is supposed to toggle at the instant when the switch closure occurs. After that, nothing should change until another closure, meaning the button must be released and re-applied. I think Michael's solution continues toggling the leds for the entire duration of the switch closure.

This solution features edge detection on the closure; also switch debouncing:

Code: Select all

; Before getting a new input sample, make a copy of SwStatus:
    lda SwStatus   
    sta SwStatus_old

; I included some debounce code with this part. It's optional,
; but you do need to keep the instructions marked "<----"
    lda sample1     ;Move all samples one place thru a fifo
    sta sample2     ;
    lda sample0     ;
    sta sample1     ;
    lda IOPORT      ;Get the input sample...  <-------
    sta sample0     ;...into the fifo

    tax             ;A=X= sample0
    and sample1
    and sample2     ;For each bit, all of whose samples are =1,
    ora SwStatus    ; set that bit in the stored image.
    sta SwStatus
    txa             ;A=X= sample0
    ora sample1
    ora sample2     ;For each bit, all of whose samples are =0,
    and SwStatus    ; clear that bit in the stored image.
    sta SwStatus    ;SwStatus contains debounced pushbutton status <----

; A=SwStatus= switch status for this time. SwStatus_old= switch status from last time.
    eor SwStatus_old ;Each 1 bit in A indicates a switch transition, either 0->1 or 1->0
    and SwStatus     ;Only keep 0->1 transitions
    lsr
    lsr
    lsr
    lsr
    eor IOPORT       ;Compute new lo nibble
    sta IOPORT       ;Write both nibbles (wr data to high nibble is don't-care)

This routine needs to execute periodically but not at a high rate of speeed. For example use a timer interrupt or else wrap this routine in a loop that also includes a time delay. Debouncing is a waiting game, and you need some milliseconds to elapse between successive samples (executions of the lda IOPORT instruction).

It's handy that we can debounce eight inputs at once, although for this problem we only use 4 bits of result. BTW the same debounce technique also exists in hardware. Check out this funky old 4000 series cmos chip! http://www.onsemi.com/pub_link/Collateral/MC14490-D.PDF

cheers
-- Jeff
(Edits for commenting)

[MichaelM]

Jeff is right on point. Good catch. Thanks.

[APL]

Thank you all for you ideas and code suggestions. I think Garth, you were on to the problem with the alignment of the lookup bits in the tables. Anyway this is what is working now.

Code: Select all

LEDROUTINE:
;Lamp control routine that toggles individual Led's on or off.
;**Call with X reg holding index of the Led to address**

                LDA ZPFPLED             	; get image of leds held in zero page.

                BIT SWCHLUP,X          	; Match between Acc and mask 
                BEQ NEXT005

                AND SWCHCOM,X		; reset bit
                BRA NEXT003

NEXT005:   ORA SWCHLUP,X		; set the bit

NEXT003:   AND #$0F
                STA ZPFPLED	
                STA FPLED                 ;Output to front panel led 

		RTS

The next problem was that my control routine picking up the switch status was supposed to control the iteration through my lamp routine so that it only went through once when a key was actually pressed. My logic was 'all over the shop' there, as a result, it was going through the lamp routine all the time. Doh!

[BitWise]

If all you are doing is inverting a bit why not use EOR?

Code: Select all

  LDA ZPFPLED
  EOR SWCHLUP,X
  AND #$0F
  STA ZPFPLED
  STA FPLED
  RTS

[APL]

BitWise wrote:

If all you are doing is inverting a bit why not use EOR?

Code: Select all

  LDA ZPFPLED
  EOR SWCHLUP,X

  STA ZPFPLED
  STA FPLED
  RTS

A couple of reasons, I guess. Insufficient familiarity with the instruction set, I noticed Jeff and Mike used the instruction, but I guess I had fastened on BIT and was trying to get an idea of how that instruction worked.

But yes, EOR does the trick.

I have also identified an error in my hardware, where the switch latch doesn't get cleared until another switch is pressed. It is trivial to fix but my solution may mean I will have to read the key latch before writing to the LEDs.

I had intended to include a HD4480A00 on the keyboard too, but from what I gather from some of Garth's postings on the topic, this type of lcd controller isn't going to work on the processor bus on a 4MHz system.