Machine code comment syntax recommendations

[Hedge1970]

As I progress with my game I am finding that with machine code reusing the same registers there are few indicators to really catch your eye and let you know what’s going on. I wonder what hints and tips there are for really good code commenting. I find as I return to my code each day that it takes me a while to “get back into it” and start to progress. Currently I am using so much commenting that it almost detracts from being able to quickly assess what the code is doing. So I would love to hear from you any tips you find helps.

Many Thanks

Paul

[berarma]

Thinking in functional blocks that do one thing is always useful. You can comment what each block does and which registers hold what at the block entry. Same for output registers if there are any. This way you think in abstractions, you don't need to remember every instruction and what every register holds every time, only the blocks.

[Hedge1970]

Thinking in functional blocks that do one thing is always useful. You can comment what each block does and which registers hold what at the block entry. Same for output registers if there are any. This way you think in abstractions, you don't need to remember every instruction and what every register holds every time, only the blocks.

Interesting I’ve been writing what each line does but actually the block is much better. Do you have any rules around the size of a block?

[AndyC]

Interesting I’ve been writing what each line does but actually the block is much better. Do you have any rules around the size of a block?

Lots of people start out by commenting what literally every line does. And it's the worst thing ever. The code very quickly becomes swamped by comments and, because you haven't learnt the opcodes, you start reading the comments rather than the code - which makes debugging harder if the comment is actually wrong.

As to the size of a block of code, really it should encapsulate a single small task. A good rule of thumb is that if you can't see all the code on screen at once, it's definitely too large. As you get better and start using optimization techniques like loop unrolling that might change, but when starting out keep things small, simple and well contained. Don't overthink things, it's fine to write the results to memory and then read them back a little while later.

[catmeows]

Interesting I’ve been writing what each line does but actually the block is much better. Do you have any rules around the size of a block?

Single block does single thing

Code: Select all

clearScreen
    ; CLEAR SCREEN
    ; A = attribute
    ld hl, 16384
    ld de, 16385
    ld bc, 6144
    ld (hl), 0
    ldir                    ;clear pixels
    ld bc, 767
    ld (hl), a
    ldir                    ;set attributes
    ret

Do not assume content of any register except the case you pass argument through register or return result through register.
The only exception are IX, IY registers when used as pointers to a structure.

When you have working code, you may try to break rules if optimalization is needed.

[Hedge1970]

Single block does single thing

Code: Select all

clearScreen
    ; CLEAR SCREEN
    ; A = attribute
    ld hl, 16384
    ld de, 16385
    ld bc, 6143
    ld (hl), 0
    ldir                    ;clear pixels
    ld (hl), a
    ldir                    ;set attributes
    ret

Do not assume content of any register except the case you pass argument through register or return result through register.
The only exception are IX, IY registers when used as pointers to a structure.

When you have working code, you may try to break rules if optimalization is needed.

Thank you for the example I’ve started just commenting lines as opposed to commenting a line of code, just like you showed here with
; Clear Screen
; A = attribute

Good to see, thank you.

[Hedge1970]

Lots of people start out by commenting what literally every line does. And it's the worst thing ever. The code very quickly becomes swamped by comments and, because you haven't learnt the opcodes, you start reading the comments rather than the code - which makes debugging harder if the comment is actually wrong.

As to the size of a block of code, really it should encapsulate a single small task. A good rule of thumb is that if you can't see all the code on screen at once, it's definitely too large. As you get better and start using optimization techniques like loop unrolling that might change, but when starting out keep things small, simple and well contained. Don't overthink things, it's fine to write the results to memory and then read them back a little while later.

I’ll take that onboard thank you

[ParadigmShifter]

A block doesn't have to be a separate function though you can just put whitespace and a comment before a "block" if you like, especially if the block is only used in one place. You can always move it to a function or macro later if you need it doing in more than one place.

Sensible label names helps a lot to (especially local labels if your assembler supports that e.g. sjasmplus, dunno about pasmo). That means you can have more than 1 label called .loop for example.

If you keep pushing and popping registers and forget what they contain at any particular point your code is probably doing too much in that section.

If you do need to push and pop registers a fair bit it helps to comment what has been pushed onto the stack (e.g. stack = data pointer, loop counter, i.e. descriptive names for what is on the stack instead of just registers) at that point during a routine.

Here's some code which has more comments than I usually have... it logical ORs an 8x8 graphic onto the screen aligned to a character cell

Code: Select all

; B: row (in character cells, so [0-23])
; C: column
; DE: 8 rows 8x1 sprite data (8 byte aligned)
sprite8x8a:
	ld a, b
	add b ; A = B*2
	ld h, tbl_rows/256 ; high byte of screen address lookup table. Aligned 256 so low byte will be just row*2
	ld l, a ; index into table 
	ld a, (hl) ; low byte of screen address
	inc l ; point HL to high byte of screen address
	ld h, (hl) ; read high byte of screen address
	add c ; add on column to low byte of screen address
	ld l, a ; and write it back. HL now holds correct screen address
	; so we now know the address...
; HL: screen address
; DE: 8 rows 8x1 sprite data (8 byte aligned)
sprite8x8aKnowAddr:
	IF UNROLL_Sprite8x8a
		REPT 7
			ld a, (de)
			inc e ; ok because gfx data is 8 byte aligned

			or (hl)

			ld (hl), a
			inc h ; next row of pixels down
		ENDR
	ELSE
		ld b, 7 ; draw 7 rows and increment gfx data pointer, screen row
.loop
		ld a, (de)
		inc e ; ok because gfx data is 8 byte aligned

		or (hl)

		ld (hl), a
		inc h ; next row of pixels down
		djnz .loop
	ENDIF

	; last row, don't need to increment gfx data pointer or screen row
	ld a, (de)

	or (hl)

	ld (hl), a
	ret

Which describes what lines are doing, not just what their effect is - e.g. inc h ; add 1 to h is NOT a good comment

[Hedge1970]

Hey Paradigm, The syntax you have there does not look like anything I’ve seen before in a z80 assembler, there is an If Then Else and what looks like a classname, is that something you’ve written?

[ParadigmShifter]

No it's sjasmplus.

IF/ELSE/ENDIF is conditional compilation like #if, #else, #endif in C if you know that.

It only assembles an IF x section if x is non-zero (otherwise it leaves it out and does the ELSE section).

So if I have

UNROLL_Sprite8x8a EQU 1

then it unrolls the loop but I can set it EQU to 0 to remove the loop unrolling.

REPT 7
; some code
ENDR

just pastes in ; some code 7 times.

.loop is a local label so I can have another .loop in the code if needed (rather than having to have a unique label for everything)

[Hedge1970]

Maybe one day, for now I’ve got enough problem writing/reading/debugging without the need for conditional blocks thrown in

[ParadigmShifter]

It means you can keep already working code in the listing while you optimise it, to check it does the same thing, otherwise you can go back.

Or I can remove the unrolling if speed doesn't matter and code size is more important.

IF DEBUG is very useful as well you can do extra checks only if DEBUG is non zero and remove them when your code works (or put it back if it breaks again).

or you can just comment out a big section of code with

IF 0
; code to comment out
ENDIF

[Ralf]

You probably know it but maybe someone will find this beginner advice useful:

- Dont make comments like "Decrease A register by one". It's a useless comment. If you have a DEC A command then it's obvious.
But a comment like "Decrease number of lives" is very useful. So describe game logic, not operations or registers

- If you have something like a procedure then use consistent labels through it - DrawMenu1, DrawMenu2, DrawMenu3... Make obvious that
it's the same procedure...

-Describe used registers working as procedure parameters in the procedure header, have a look at my example below

- Comment a lot. If you have some experience with PHP or Java or any modern language then assembler needs more commenting then them.
It needs comments, believe me. Forget about this: https://en.wikipedia.org/wiki/Self-documenting_code It's bulls..t

- Make you code looking neat. If you have comments, use tab key to keep them all at equal position like here:

Have a look at some example

Code: Select all

DrawTile	
	;HL: upper left tile corner size	
	;BC: set to tile size 
	;DE: tile memory address	
	;if first byte in a char is 16, program will interpret it as an empty square
		
	DI
	LD A,(DE)				;load tile size to BC
	LD B,A 					;x-width
	INC DE
	LD A,(DE)
	LD C,A 					;y-height	
	INC DE			
				
	PUSH HL					;store position
	PUSH BC					;store size
			
	LD(StoreSP),SP				
	EX HL,DE				;stack points to tile graphics
	LD SP,HL

[berarma]

Interesting I’ve been writing what each line does but actually the block is much better. Do you have any rules around the size of a block?

You've already got a lot of good answers, I'll just add mine. I wouldn't worry about the size of the block as long as you can think of it as a task without the need to know the implementation details of that task. Some tasks might take a few lines, some might take hundreds of lines, what's important is that the task can be easily described with a text and its input and output registers without worrying about the code details. And don't forget tasks can use other tasks.

Programming consists in splitting a problem recursively into smaller ones until decomposed into single tasks ready to be solved. You go down implementing the smallest tasks, then go up implementing the bigger tasks using the smaller ones. As new tasks are implemented you can forget about the details and just use them to implement the more complex ones.

Since you don't need to know the implementation details of other blocks to understand the implementation of any block, it's easy to review any block of code. Although as long as the block is solving the task correctly you won't need to keep reviewing it. You can also do more complex tasks more easily by using the already implemented ones as they will consist basically of calls to other blocks. You will forget soon about the implementation details of the simpler tasks and think more about the more complex ones in terms of sequences of blocks, unless you need to optimize something or fix a bug.

[ParadigmShifter]

Since the Z80 is kinda asymmetric though it does help to know about the implementation details.

HL can do more stuff than DE and BC is best used as a loop counter for example, so you try and write your routines to take advantage of that.

It's not like these days where every register is the same.

HL - primary data pointer
DE - secondary data pointer - EX DE, HL is good to swap though, and LDI/LDI uses DE as destination which is opposite to what I normally do
BC - other data or loop variables

[Morkin]

- Dont make comments like "Decrease A register by one". It's a useless comment. If you have a DEC A command then it's obvious.

Haha, when I first started learning z80 my code was littered with comments like this, before I realised it was all totally useless

For writing z80 code, my preference now would be similar to Ralf's example, maybe a brief description with register inputs, and one or two line comments in a block. I guess when you're writing z80 code the comments are really for yourself rather than others as it's normally a 1-person job.

Having attempted a disassembly of a game, that's a bit different - I discovered the comments were for me while I was trying to work out what the hell was going on with the code. But then I had to change most of them as the published commentary is ultimately for others to read.

[RST#08]

For writing z80 code, my preference now would be similar to Ralf's example, maybe a brief description with register inputs, and one or two line comments in a block. I guess when you're writing z80 code the comments are really for yourself rather than others as it's normally a 1-person job.

^This.

It depends on what your annotation is for. If it's for your use only, or perhaps for sharing with experienced programmers, then what @Morkin, @Ralf and others suggest above is good advice. But if it's for teaching, then more generous step by step annotation might be useful. For example, the classic disassembly, primarily an instructional and reference text, has generous and mostly line-by-line annotation.

[berarma]

Since the Z80 is kinda asymmetric though it does help to know about the implementation details.

HL can do more stuff than DE and BC is best used as a loop counter for example, so you try and write your routines to take advantage of that.

It's not like these days where every register is the same.

HL - primary data pointer
DE - secondary data pointer - EX DE, HL is good to swap though, and LDI/LDI uses DE as destination which is opposite to what I normally do
BC - other data or loop variables

This doesn't apply to what I said. What I meant is that once a block is implemented you only need to know how to set registers on entry and what it will do, not how it will do it.

[ParadigmShifter]

I just meant you should pick your registers to pass parameters in and out of routines carefully if you can, since not all registers are created equal.

It's also worth passing related stuff like row and column in a register pair rather than registers that are not paired since you probably want to save and restore those on the stack.

[Bedazzle]

Code: Select all

    ld (hl), 0

it is two bytes, 10 Ts
replace it with

ld (hl),l

one byte, 7 Ts

[ParadigmShifter]

Larger but faster CLS. HL contains the attribs to set (so you can use it to stripe the attribs if H != L).

Code: Select all

	; HL -> attrib to set when clearing screen
	; at exit
	; B = 0
	; HL = 0
cls_fast:
	di                  ;disable interrupt
	ld (.stack+1), sp	;store current stack pointer
	ld sp, 16384 + 6144 + 768
	ld b, 128			; clear attribs in 128 * 3 pushes

.attribloop
	push hl
	push hl
	push hl
	djnz .attribloop

	;ld hl, 0
	ld h, b
	ld l, b
.loop1
	push hl             
	push hl             
	push hl             
	push hl
	push hl             
	push hl             
	push hl             
	push hl
	push hl             
	push hl             
	push hl             
	push hl
	djnz .loop1			
.stack
	ld sp, 0            ;parameter will be overwritten
	ei
	ret

Usage:

Code: Select all

	ld hl, #0f0f ; attribs (blue paper white ink)

	call cls_fast

EDIT: Also used idea from previous post to replace LD HL, 0 with LD H, B / LD L, B since B=0 at the end of .attribloop (2 T faster and 1 byte smaller). You can also unroll .attrib loop more to speed it up a bit more, every time you double the unroll amount half the initial value of B. Every doubling of the loop size saves (B-1)*5 T.

EDIT2: The other version can probably be made smaller and faster by going in reverse as well (using lddr instead of lddi) and taking advantage of B being 0 at the end of the first LDDR loop.

[ParadigmShifter]

Doing it backwards is not better since you need to do additional dec hl, dec de.

It does set the attribs first though which might be useful I suppose

Code: Select all

	; A -> attrib to set when clearing screen
cls:
	; set attribs
	ld hl, #5800+767
	ld de, #5800+766
	ld bc, 768-1
	ld (hl), a
	lddr

	; clear pixels
	dec hl
	dec de
	ld (hl), b ; since B=0 here
	ld bc, 6144-1
	lddr
	ret

EDIT: You can remove the dec hl and dec de by doing 1 extra iteration for the first loop though

Code: Select all

cls:
	; set attribs
	ld hl, #5800+767
	ld de, #5800+766
	ld bc, 768
	ld (hl), a
	lddr

	; clear pixels
	ld (hl), b ; since B=0 here
	ld bc, 6144-1
	lddr
	ret

so it's just the same as the forwards version in size and speed I believe.

EDIT2: I like to set the attribs first anyway since I currently clear with same ink/paper and then copy the proper attribs after from a shadow attribute buffer. (I use a backbuffer for attribs but not for the pixels, those are drawn direct to screen.

The fast stack version is recommended if you use a backbuffer for screen pixels and copy that across (so double buffer the screen) since it is quicker (it's still quite slow though). The LDDI/LDDR versions takes more than a frame, stack version does not.

EDIT3: Oh and here is the stack version where you can pick how many times you double the unrolling in the attrib loop. NUM_ATTRIBUNROLLS = 1 for no extra unrolling (3 push/djnz as before).

Code: Select all

	; HL -> attrib to set when clearing screen
	; at exit
	; B = 0
NUM_ATTRIBUNROLLS EQU 1

cls:
	di                  ;disable interrupt
	ld (.stack+1), sp	;store current stack pointer
	ld sp, 16384 + 6144 + 768
	ld b, (256>>NUM_ATTRIBUNROLLS)

.attribloop
	REPT 1<<(NUM_ATTRIBUNROLLS-1)
	push hl
	push hl
	push hl
	ENDR
	djnz .attribloop

	ld h, b
	ld l, b
.loop1
	push hl             
	push hl             
	push hl             
	push hl
	push hl             
	push hl             
	push hl             
	push hl
	push hl             
	push hl             
	push hl             
	push hl
	djnz .loop1			
.stack
	ld sp, 0            ;parameter will be overwritten
	ei
	ret

[Bedazzle]

If you are going to smallest clear screen routine, then

Code: Select all

clear_screen:
	ld hl, $5B00	; 3   23296
	xor a		; 1
loop_cls:
	dec hl		; 1
	ld (hl), a	; 1
	cp (hl)		; 1
	jr z, loop_cls	; 2	loop if ROM is not hit

	ret		; 1
			; TOTAL 10

It clears backwards, and assumes that ROM area is read only.

[Einar Saukas]

My advice:

• Good comments are about quality, not quantity. Don't feel obliged to fill your code with obvious unnecessary comments everywhere, this won't help anyone. You will waste time writing them, waste time again later reading through them all when looking for useful information... And most important of all, when you change your code later, you won't have time (or patience) to revise tons of frivolous comments thus they wouldn't even match the code anymore! Instead use your time wisely to document what really matters.
  
  Here's a practical example from Complica DX:
  

  Code: Select all

  ; -----------------------------------------------------------------------------
  ; Set border color and clear screen.
  ;
  ; Parameters:
  ;   A: BORDER color
  ;
  ; Destroys:
  ;   AF, DE
  ; -----------------------------------------------------------------------------
  cls_scr:
          halt
          out     (254), a                ; set BORDER
          ld      de, 23295
          xor     a
  cls_loop:
          ld      (de), a
          dec     de
          bit     3, d
          jr      nz, cls_loop            ; clear screen sttributes
          ret
  

  It may seem this routine doesn't have enough comments, but it has exactly the right ammount. There's no need to explain each line, it would just make you spend more time to read this code without making it any easier to understand.

• Carefully choosing good names for labels and constants is very important (just like properly naming variables, functions, classes and methods in high-level programming languages).
  
  For instance, consider this code:
  

  Code: Select all

  ld a,2          ; color red
  ld (23624), a   ; set default border color

  Proper labels and constants would make this code self-explanatory even without comments:
  

  Code: Select all

  ld a,COLOR_RED
  ld (BORDCR), a

  Actually we could do better:
  

  Code: Select all

  ld a,MENU_BORDER_COLOR
  ld (BORDCR), a

  Now it's clear the reason this code is modifying the border color (for the menu). Also if we later decide to change the menu border, we will simply need to modify a single constant instead of checking the entire code whenever it changes the border.

• Routine header documentation (to explain WHAT the routine does) is even more important than routine line comments (to explain HOW it does it). It means describing the routine purpose (it can be a very brief explanation like my first example above), input parameters, output results, affected registers and modified memory areas. With this information, you won't need to re-read the entire routine code whenever you want to reuse it (to find out all registers that need to be saved before invoking this routine, for instance).
  
  Here's another practical example from Complica DX:
  

  Code: Select all

  ; -----------------------------------------------------------------------------
  ; Specialized algorithm to choose defeated player's last move.
  ;
  ; When it's the player's last move, and there's no way to prevent opponent's
  ; victory immediately afterwards, then normally player's last move would be
  ; chosen randomly, since all column choices would provide the same heuristic
  ; evaluation result anyway. Although this behavior is strictly correct (notice
  ; that player's choice is irrelevant here since defeat is inevitable), in
  ; practice it would make BRAINIAC look bad. It would seem that opponent's
  ; victory was too easy and current player wasn't even trying hard enough.
  ; This specialized algorithm simulates a more "human" reaction, by "trying" to
  ; block the next opponent's move (albeit knowing this attempt will be futile).
  ;
  ; Parameters:
  ;   B: must be zero
  ;   E: "original" selected column (0-3)
  ;   HL: ai_depth address
  ;
  ; Returns:
  ;   E: "improved" selected column (0-3)
  ;
  ; Uses:
  ;   ai_depth: current recursion depth
  ;   ai_player: current player
  ;
  ; Destroys:
  ;   AF, BC, D, HL, AF'
  ; -----------------------------------------------------------------------------
          ld      (hl), b                 ; depth = 0
          ld      d, e
  ai_last_loop:                           ; for each column except bestColumn {
          ld      a, d
          inc     a
          and     3                       ;     column (0-3)
          cp      e
          ret     z                       ;     if (checked all columns) return bestColumn
          ld      d, a
          ld      a, (ai_player)
          xor     3
          ld      c, a                    ;     C = opponent
          ld      a, d
          push    de
          call    BRAINIAC_play           ;     insert opponent piece at column
          push    hl
          push    af
          call    ai_eval_board           ;     A = eval_board
          ld      c, a
          pop     af
          pop     hl
          call    BRAINIAC_undo           ;     remove opponent piece from column
          ld      a, c
          pop     de
          cp      128+96
          jr      nz, ai_last_loop        ;     if (opponent won't win) continue
          ex      af, af'
          ld      c, a                    ;     C = player
          ld      a, d                    ;     column (0-3)
          push    de
          call    BRAINIAC_play           ;     insert player piece at column
          push    hl
          push    af
          call    ai_eval_board           ;     A = eval_board
          ld      c, a
          pop     af
          pop     hl
          call    BRAINIAC_undo           ;     remove player piece from column
          ld      a, c
          pop     de
          cp      128-96
          jr      z, ai_last_loop         ;     if (player won't loose) return column
          ld      e, d                    ; }
          ret

The entire Complica DX documented source code is available for download, if anyone's interested.

[ParadigmShifter]

Anything that is 2 opcodes like your border colour example and is used more than once is probably better as a macro.

Sometimes it's worth commenting stuff that isn't obvious on a first scan through the code e.g.

inc a ; does not affect carry flag!

inc hl ; does not affect flags!

Both of those can be useful because they don't set flags though of course, you can do them just before a conditional jump for condition you tested earlier on.

Similarly add hl,bc does not set Z, C or P/V but you normally don't need a comment for that since it usually results in a bug if you thought it would set them. (You have to clear carry and use adc hl,bc if you want to set flags). (Been burned one too many times by this to forget about it now I hope).

EDIT: add hl, bc does set C flag, oops