This page is a mirror of Tepples' nesdev forum mirror (URL TBD).
Last updated on Oct-18-2019 Download

Need to get a code reviewed

Need to get a code reviewed
by on (#182659)
Hi guys, I'm a new NES developer. Still trying to figure out things, especially the asm 6502. Currently I'm using the cc65 and tried to change a simple data copying method from C into asm. I'm using Shiru's example as the base. So anyway, here's my code.

Code:
.segment "CODE"

.importzp  _temp_x, _p_x
.importzp _temp_y, _temp_pos, _temp_moving, _temp_type, _temp_health, _temp_sprite_type
.importzp _temp_dx, _temp_dy
.importzp _p_y, _p_pos,  _p_moving,  _p_type,  _p_health, _p_active, _p_sprite_type
.importzp _p_dx, _p_dy
.importzp _temp_team


.export _CopyToTemp, _CopyFromTemp


;void __fastcall__ CopyToTemp(u8 id);

_CopyToTemp:
; temp_team = id < 6 ? 0:1;
    ldx     #$00
   lda     sp
   cmp    #$06
   bcc     @lessThanSix
   ldx     #$01
@lessThanSix:
    stx  _temp_team
   tay
   
; temp_y = p_y[id]; unsigned 8 bit
    lda    (_p_y), y
   sta    _temp_y
   
; temp_dx = p_dx[id]; signed 8 bit
    lda    (_p_dx),y
   sta    _temp_dx
   
; temp_dy = p_dy[id]; signed 8 bit
    lda    (_p_dy),y
   sta    _temp_dy
   
; temp_moving = p_moving[id]; unsigned 8 bit
    lda    (_p_moving),y
   sta    _temp_moving
   
; temp_type = p_type[id]; unsigned 8 bit
    lda    (_p_type),y
   sta    _temp_type

; temp_pos = p_pos[id]; unsigned 8 bit
   lda   (_p_pos),y
   sta    _temp_pos
   
; temp_x = p_x[id]; unsigned 16 bit. should we shift the index value to the left by one?]
    lda    sp
    asl    a
    tay   
    lda    (_p_x), y
    sta    _temp_x
    iny
    lda    (_p_x), y
    sta    _temp_x+1

;if ( id < 3)
;   temp_health = p_health[id];
;else if (id >5 && id < 9)
;   temp_health = p_health[id-3];
    lda    sp
    cmp   #$03     
    bcc    @copyP1Health
    cmp    #$05
    bcc    @doneCopyToTemp
    cmp    #$09
    bcc    @copyP2Health
    jmp @doneCopyToTemp
   
@copyP1Health:
    tay
    lda    (_p_health), y
    jmp    @storeCopyHealth
   
@copyP2Health:
    clc
    sbc    #$03
    tay
    lda    (_p_health), y
   
@storeCopyHealth:
    sta    _temp_health
@doneCopyToTemp:
    rts
   
   
;void __fastcall__ CopyToTemp(u8 id);

_CopyFromTemp:
   
;p_y[id] = temp_y;unsigned 8 bit
    ldy    sp
    lda    _temp_y
    sta   (_p_y), y
   
;p_dx[id] = temp_dx;signed 8 bit
    lda    _temp_dx
    sta   (_p_dx), y
   
;p_dy[id] = temp_dy; signed 8 bit
    lda    _temp_dy
    sta   (_p_dy), y
   
;p_moving[id] = temp_moving; unsigned 8 bit
    lda    _temp_moving
    sta   (_p_moving), y
   
;p_sprite_type[id] = temp_sprite_type; unsigned 8 bit
    lda    _temp_sprite_type
    sta   (_p_sprite_type), y
   
;p_x[id] = temp_x; unsigned 16 bit. should we shift the index value to the left by one?
    lda    sp
    asl    a
    tay
    lda    _temp_x
    sta    (_p_x), y
    iny
    lda    _temp_x + 1
    sta    (_p_x), y
   
;TODO: Add health?
    rts


I've added this file in the crt0.s like so:
Code:
   .include "main.sinc"


Can you please check if this code works as intended and give comment about it please? I don't know that much about asm and when I use the pure C codes, it has some flickering. So I though I'll try getting some of my codes changed to asm. Thanks in advance.
Re: Need to get a code reviewed
by on (#182660)
Code:
; temp_y = p_y[id]; unsigned 8 bit
lda    (_p_y), y
sta    _temp_y

I think you mean this:

Code:
lda    _p_y, y
sta    _temp_y

Brackets mean the address of the address.
Re: Need to get a code reviewed
by on (#182661)
bawenang wrote:
Code:
;if ( id < 3)
;   temp_health = p_health[id];
;else if (id >5 && id < 9)
;   temp_health = p_health[id-3];
    lda    sp
    cmp   #$03     
    bcc    @copyP1Health
    cmp    #$05
    bcc    @doneCopyToTemp
    cmp    #$09
    bcc    @copyP2Health
    jmp @doneCopyToTemp


CMP puts the result of < or >= in the carry flag, not < or >, so here you're testing id >= 5, not id > 5.
You could either CMP #6 instead or add BEQ @doneCopyToTemp before the CMP #9 line.
Re: Need to get a code reviewed
by on (#182665)
I think "lda sp" is supposed to mean "pla" or "pull accumulator from stack".
Re: Need to get a code reviewed
by on (#182667)
Oh, I didn't catch that. Actually it's not supposed to be PLA, but rather it's the index to CC65's "C stack".

To fetch from the top of the C stack:
Code:
LDY #0
LDA (sp), Y


However, the functions are defined "fastcall" which means that the parameter is not on the stack, it is in the A register.

More info here: http://cc65.github.io/doc/cc65-intern.html
Re: Need to get a code reviewed
by on (#182680)
psycopathicteen wrote:
Code:
; temp_y = p_y[id]; unsigned 8 bit
lda    (_p_y), y
sta    _temp_y

I think you mean this:

Code:
lda    _p_y, y
sta    _temp_y

Brackets mean the address of the address.

I suspect the "p_" is a hungarian prefix for "pointer type", and with what looks like an attempt to put the "id" parameter into Y as an index, I think it's a pointer to an array, in which case indirect indexed lookup "(pointer), Y" was the correct thing to do.

However, it is probably good advice to suggest using static arrays instead of pointers to arrays (in which case you would use an absolute indexed lookup "array, Y"), unless those pointers really do need to point at other locations in memory. CC65 tends to have performance problems associated with excessive use of pointers.
Re: Need to get a code reviewed
by on (#182756)
I see. Thanks for the answers guys. But what about the unsigned 16 bit one / this one?

Code:
; temp_x = p_x[id]; unsigned 16 bit. should we shift the index value to the left by one?]
    lda    sp
    asl    a
    tay   
    lda    (_p_x), y
    sta    _temp_x
    iny
    lda    (_p_x), y
    sta    _temp_x+1


Is that actually correct?
Re: Need to get a code reviewed
by on (#182757)
It's correct except for "sp" not being the parameter to the function. The parameter (id) is passed in the A register, not in "sp". See the page I linked about CC65 calling conventions.
Re: Need to get a code reviewed
by on (#183104)
Ah yeah. My bad. And I actually copied the accumulator to the y register. To store the parameter. So, after I changed the "lda sp" to "tya", it worked like a charm. Thanks so much.