Looking for some constructive criticism

[andrew]

Hey Everyone!
This is my first post here, hopefully this style of question is ok, if not I can post elsewhere or change it a bit.
I've been working through Rodney Zak's book Programming the 6502, and attempting the questions within in. One question asks the reader to write some code to find the largest value within a block.
I have written something I believe works, but as I am relatively new to writing low level languages, I was hoping for some friendly, but constructive, feedback.

The sections lowList and highList are mean to create fill a section of memory with the values: [5,4,3,2,1,15,14,13,12,11]. Then startSearch will move a part of this to $0220 (where I want to store the highest value) to give me something to compare too. compare and foundHigh then loop through the values I have stored in memory while foundHigh changes $0220 when appropriate.

Code: Select all

  LDX #05
  LDY #00
lowList:     ; deal with 5 to 1
  TXA
  INY
  STA $0205, Y
  DEX
  BNE lowList
  LDX #15
highList:     ; deal with 15 to 11
  TXA
  INY
  STA $0205, Y
  DEX
  CPX #10
  BEQ startSearch
  JMP highList
startSearch:  
  LDA $0205, Y ;this should be the largest num
  STA $0220  ; where we'll store highest number
  DEY
compare:   
  LDA $0205, Y
  CMP $0220   
  BCS foundHigh ; if A>$0220, jump to foundHigh
  DEY
  BNE END ; check to see if we've gone through whole list
  JMP compare
foundHigh:
  STA $0220  ;update $0220 with new largest value
  DEY
  JMP compare ;back to compare loop
END:
  RTS

I've been using https://skilldrick.github.io/easy6502/ to test it out, and it looks like it works!

I'm still getting to grips with the language and trying to find the best instructions to use when, and just get a feel for the style you should be writing in. Any feedback, tips, or thoughts would be really appreciated!

[BigEd]

Welcome! You will no doubt find there are so many suggestions that almost all bytes of your program change - but don't be discouraged! There are always many ways to tackle a problem, and the 6502's very limited registers mean that the best way is often different from any given attempt. There are wizards who are able to use the registers in an optimal way.

So, if you have any register transfers like TYA or similar, there's a fair chance you can rejig your code to avoid them. If you have both a count and an index - in your case a count in X and an index in Y - you can often combine them. Counting down to zero, or down to -1, can make the branching simpler - if you count up, you'll often use a comparison as well as a branch.

I notice you have one or two cases of branching over a jump: often you can swap those two instructions for a single branch, of the opposite sense.

[andrew]

BigEd wrote:

I notice you have one or two cases of branching over a jump: often you can swap those two instructions for a single branch, of the opposite sense.

That makes sense, I've changed the code to do that now.

BigEd wrote:

So, if you have any register transfers like TYA or similar, there's a fair chance you can rejig your code to avoid them. If you have both a count and an index - in your case a count in X and an index in Y - you can often combine them. Counting down to zero, or down to -1, can make the branching simpler - if you count up, you'll often use a comparison as well as a branch.

Again, that makes sense! I'll have to have a think how to change my code though for this one...

Thank you for the reply. Going from python,R and c++ to 6502asm is a big (but fun) jump. Lots to learn!

[BigEd]

(It would be good, I think, if you could leave the code in your post in its original state (which I think you have) and to post your improved revisions in this thread. Lots of people will be able to learn from the successive improvements!)

[Chromatix]

The way I would approach this is to keep the maximum value thus far in the accumulator, which you shouldn't need for indexing over the array. Initialise it to zero just before entering the array. Then you only need to compare it to the current array item, and if the latter is larger, load it into the accumulator to keep track. If you also need to track the index of the largest value, this is also the moment to save the index register in RAM.

On the 6502 it's also idiomatic to use zero-page memory for single, short-term variables like that. ZP accesses are slightly faster and result in smaller code, and you can indirect through pointers placed there for more flexible data access (instead of hard-coding addresses as you presently do). Indirect addressing may be a topic you haven't encountered in the book yet, but you will.

For a bigger challenge, consider how you might deal with multi-byte values. The 6502 doesn't make this particularly easy, but you'll need to master it eventually.

[BitWise]

Keeping the biggest in A gives you something like this:

Code: Select all

  ldx #LEN-1
  lda LIST,x   ; Assume last is biggest
.loop:
  dex  ; Reached the end?
  bmi .done
  cmp LIST,x  ; No, compare with next
  bcc .loop
  lda LIST,x  ; Bigger (or same) replace besy
  bra .loop
.done:
  rts  ; Done

[Chromatix]

Fettling that a bit:

Code: Select all

  ldx #LEN-1
  bra .bigger   ; Assume last is biggest
.loop:
  cmp LIST,x  ; compare with next
  bcc .smaller
.bigger
  lda LIST,x  ; Bigger (or same) replace best
.smaller
  dex  ; Reached the first?
  bpl .loop
  rts  ; Done

That's probably easier to read, smaller *and* faster!

But note: both version use the 65C02 BRA instruction. On an old NMOS 6502, replace mine with BNE, or just with LDA #0.

[andrew]

So, I've made some changes. I'm a little busy at work at the moment, so I haven't been able to implement everything.

Code: Select all

  LDX #05
  LDY #00
lowList:     ; deal with 5 to 1
  TXA
  INY
  STA $0205, Y
  DEX
  BNE lowList
  LDX #15
highList:     ; deal with 15 to 11
  TXA
  INY
  STA $0205, Y
  DEX
  CPX #10
  BCS highList ; if X > 10, loop
  LDX #10   ; reload x
  LDA #00   ; Assume last is biggest
loop:
  CMP $0205, X  ; compare with next
  BCS smaller
bigger:
  LDA $0205, X  ; Bigger (or same) replace best
smaller:
  DEX  ; Reached the first?
  BPL loop
  STA $0220
  RTS  ; Done

So far I've:

• (at BigEd's suggestion) swapped instructions where I've branched over a jump for a single instruction
  (inspired by Chromtix and Bitwise) Kept the largest value in A

I still need to:

• combine my x and y registers im using for counting at the start into 1
  Chromatix mentioned Indirect Addressing. Coincidentally, that was the last thing the book discussed before starting me on this problem. However, it might take me a while to properly get my head around.

A question for Chromtix and Bitwise, regarding their answer: you called the subroutines (thats the right word right?) things like ".loop" - is this "." a style decision, or does it serve another purpose? Also, you both used bra, as far as I can tell this is just a branch that must occur, so how does this differ from JMP? Then, I only managed to get this to work after I changed a BCC to a BCS, which I think makes sense - any thoughts?

Thanks for all the help so far! At BigEd's suggestion I've decided to post the code again in its not-totally-fixed-yet state.

[Chromatix]

The syntax for labels differs between assemblers, although many assemblers now accept multiple styles. Don't worry too much about it.

BRA has two significant differences from JMP: it uses Relative addressing like other Bxx instructions, and consequently occupies one fewer byte of program code. JMP normally uses Absolute addressing, though that's not the only addressing mode it supports. There is no significant difference in speed.

The big advantage of the Indirect addressing modes is that you get to put your data in arbitrary places. Two consecutive bytes of zero-page contain a pointer (LSB first). Often the most useful mode is the Post-Indexed Indirect, written (zp),Y. As part of this exercise, you should try to write code using that mode, since the book has just gone over it.

[andrew]

Code: Select all

  LDA #$05 ; these 4 lines put $0205 into p0
  STA $01
  LDA #$02
  STA $02

  LDY #05
lowList:     ; deal with 5 to 1
  TYA
  STA ($01),Y
  DEY
  BNE lowList

  LDX #15
  LDY #05
highList:     ; deal with 15 to 11
  TXA
  INY
  STA ($01),Y
  DEX
  CPX #10
  BCS highList ; if X > 10, loop

  LDY #10   ; reload Y
  LDA ($01),Y   ; Assume last is biggest
loop:
  CMP ($01), Y  ; compare with next
  BCS smaller
bigger:
  LDA ($01), Y  ; Bigger (or same) replace best
smaller:
  DEY  ; Reached the first?
  BPL loop
  STA $0220
  RTS  ; Done

So now, with this version, I have tried to do indirect indexed addressing to deal with some of the addresses. Also, during the first half of making my "list" I have tried to reduce it to only use 1 index for counting. I was not able to get it to work using only 1 index for the upper half however, I wasn't able to think of a way to make the numbers work.

Thanks again.