1
\$\begingroup\$

This code is intended to be included with X86-16 writing ASCIIZ strings directly to video and is dependent upon some of the declarations in that code. When combined with that code a string declared @ 1000:0 in the following manner would;

AscTxt:  db  01, 84H                           ; View page 0 output to 4
         db  'This is an example', 01, 40H     ; View page 4 output to 0
         db  'This is replacing whatever is on top line of page 0'
         db  0, 01, 0FH, 0, 0   ; Waits for response then view page 0

As an example, let's start with this.

Page 0

This is the next thing you would see after entering your .COM filename @ command prompt.

enter image description here

Notice how the cursor has not moved to EOS. That is by design. Then after pressing any key except ESC, we'll revert back to page 0 with top line being replaced.

enter image description here

; =============================================================================================
;    Change display and/or destination page

;   ENTER:  00  -     7 = Set do not change current page being displayed
;                   6-4 = Display page. Ignored if equal to active or but 7 set
;                     3 = Set, do not change display page
;                   2-0 = Destination page. Ignored if already pointing there or bit 3 set.

;   LEAVE:   DI = New pointer when applicable
;            DX = volatile, all others unchanged.

;   FLAGS:  Undefined
; ---------------------------------------------------------------------------------------------

    F01:    cmp     al, 1
            jnz     F02
            lodsb                       ; Read only parameter for this function

    ; Real mode has a few limitations in addressing far data, so this just seems to be the
    ; most practical way of addressing data in BDA

            push    bx

    ; Test bit 7 to determine if display page should be changed. If so, then bits 6-4 are
    ; ignored.

            test    al, 10000000B       ; Is bit 7 on
            jnz     .cDest              ; if so we are not changing display page

            push    ax                  ; Still going to need low nibble
            shr     ax, 4               ; Shift page number into low nibble
            cmp     al, [fs:DispPg]     ; Check BDA if anything actually needs done
            jz      .cDest - 1          ; Already on that page

    ; The intent of procedurd e as a whole is to avoid BIOS as much as possible, but I
    ; did not want to implement code to manipulate controller.

            mov     ah, SAP
            int     VIDEO
            pop     ax                  ; Restore value in low nibble

   .cDest:  test    al, 1000B           ; Is bit 3 on
            jnz     .exit               ; if so we are not changing destination page            ; Return to instruction just before label .next

    ; Check if there is any need to do anything by determining if destination video segment
    ; is already being pointed too.

            and     al, 7
            mov     dx, es              ; Get current video segment
            mov     dl, al
            add     dl, 0B8H
            cmp     dh, dl              ; ZF will be set if same segments
            jz      .exit

    ; Set new segment and then determine offset based on that pages cursor position from
    ; x/y coordinates specified in BDA.

            shl     dx, 8
            mov     es, dx              ; Set new segment

    ; Caret position of new page or even one that has been written to before is assumed
    ; to be the starting point of next write.

            mov     bx, Cursors         ; Point to beginning of array of vectors
            shl     ax, 1
            add     bl, al
            mov     dx, [bx]                ; Points into arrary of vectors in BDA

            mov     di, dx              ; Just in case we are already at 0,0.
            or      dx, dx
            jz      .exit

    ; If position is top/left, not much point multiplying by zero

            mov     al, dh
            imul    ax, 80              ; 80 x 25 x 16 color assumed
            and     di, 0FFH
            add     di, ax
            shl     di, 1               ; ES:DI set to new page & offset

    ; Restore non-volatile

    .exit:  pop     bx
            ret

      F02:  ret
\$\endgroup\$
-1
\$\begingroup\$

The initial comment: not clear what is entering (deducting it's about byte [ds:si]) and I would rather use "arguments" or "input" word. Not clear the description is about bits. Not all arguments are described (the code does use also ds:si address for lodsb). Typos. Modified registers are incomplete too (again si). Etc..


F01:    cmp     al, 1
        jnz     F02

Why name like F01? Was it just for this review, or is it actual label? Use rather something more descriptive.


Why does the function end when al is not equal to 1? That should be mentioned in description, that al must be 1 to make it work.


I would move push bx lot more closer to the area where it's modified, so it can be easily seen with eye, where the whole push/pop block is, and span over fewer branching points (which is always error prone, to keep correct stack across some branching). You can actually surround just few instructions with push/pop bx, not having any branching at all.


        push    ax                  ; Still going to need low nibble
        shr     ax, 4               ; Shift page number into low nibble
        cmp     al, [fs:DispPg]     ; Check BDA if anything actually needs done
        jz      .cDest - 1          ; Already on that page

Bug: if ah contains some low bits, you will get wrong display page value. Should be shr al,4 instead.

Why .cDest-1? First why cDest, that's not English, and why -1, you are on x86 powerful machine, there absolutely no reason to save bytes on symbol names or amount of symbols. 1988 called, they want your habits back.


Some more typos... don't you have spellcheck in your editor? :-o (I'm using simple kate for NASM sources, and Shift+Ctrl+O will switch spellchecking on/off, so it doesn't bother me on instructions, but I can use it to review comments from time to time).


        mov     ah, SAP
        int     VIDEO

This will not compile, you should rather provide working source for review. By building it yourself before posting you will prevent some kind of accident, like cutting out more than you did want. Also I have no idea what is SAP or VIDEO, so no way to review it and put any advice on that. Except stop using cryptic short symbol names which can't be comprehended by reading like plain English.


        mov     bx, Cursors         ; Point to beginning of array of vectors
        shl     ax, 1
        add     bl, al

Do just shl al, 1 if you are using only al later. This at first did look as bug to me, because you have anything in ah, then I realized you will avoid that by using al only.

Actually this will fail if Cursors array spans over 256B boundary, then the add bl,al will produce wrong address.

But you didn't show how you define Cursors, maybe you are sure it is well aligned and will always fit into 256B "page".

Still maybe consider more robust version doing whole and ax,7 add ax,ax add bx,ax to calculate with full 16b values.


And the whole set cursor code is a bit fishy, I mean the calculation of di based on [x, y], don't you have such code already somewhere?

Are you optimizing for speed that you can't afford to call that? (probably not, as you are still in real mode)

Also imul ax, 80 is again risky as you don't specify ah content (would work with my modification above to and ax,7).

I would probably write separate subroutine to calculate di based on [x, y] coordinates, like for example:

; input: dh:dl = y:x of cursor
; output: di = text mode offset for cursor position
    getDiForCursorPosition:
            push    dx
            movzx   di, dh          ; di = y
            imul    di, 80          ; di = 80 * y
            movzx   dx, dl          ; dx = x
            add     di, dx          ; di = 80 * y + x
            add     di, di          ; di = (80 * y + x) * 2
            pop     dx
            ret

And use that from everywhere (after you will check it works as expected, I didn't debug it :) ).

\$\endgroup\$
  • \$\begingroup\$ I had hoped the first sentence was informative enough that the code from that link needed to be included in order this snippet be fully functional. Labels are not meant to be human readable. They are simply a means by which the assembler can calculate offsets and addresses and it would be nice if NASM incorporated something like @@: as in MASM. Case in point, in STDLIB you'll see atoi not ascii_to_unsigned_or_signed_integer. \$\endgroup\$ – Shift_Left Feb 7 '18 at 23:56
  • \$\begingroup\$ @Shift_Left: if you don't want them read by human, why do you post on code review site? atoi is from different century. Literally. \$\endgroup\$ – Ped7g Feb 8 '18 at 0:06
  • \$\begingroup\$ The operative word in programming is CODE defined as; a system used for brevity or secrecy of communication, in which arbitrarily chosen words, letters, or symbols are assigned definite meanings. To that end, the comments you made about code are relevant and directly related to the process of reviewing code, and its functionality. Your personal idiosyncrasy's though as to how labels should be named are not. That being said, the comments you made about my comments is valid, as those are intended to be human readable and understandable. \$\endgroup\$ – Shift_Left Feb 8 '18 at 0:31
  • \$\begingroup\$ Look, whatever. We did use labels a, b, c, d, ..., aa, ab, ac, ... on ZX Spectrum to make it fit into ~40kB RAM (with huge 5-10kB source), and I had sheet of paper next to computer with the meanings of labels. Then with TASM I had 360kB ASM file, with every label being readable and comprehended without any guessing. Why? Because it did compile, and it made debugging and reviewing much easier, so it did save me time (although letting that part of project to grow to 300+kB ASM was huge mistake). I'm now offering you this advice, how to write asm code faster... apply as you wish. :) \$\endgroup\$ – Ped7g Feb 8 '18 at 0:39
  • 1
    \$\begingroup\$ @Shift_Left "messy" may be editor, plus the 256B effort really doesn't help to write code in simple way (but at least it is short). My non-size code is usually using simpler/more-straightforward constructs. Anyway, I wrote this answer to help you, and it is perfectly fine if you cherry pick, what is truly helping you, and reject the rest, sorry for me being a bit cocky, in the end I'm happy if you are happy with your code, so take my comments like some sort of a bit heated discussion, but done in good faith, and enjoy your project. :) Good luck. \$\endgroup\$ – Ped7g Feb 8 '18 at 9:31

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.