assembly - weird behaviour of code (corrupted draw) when using own keyboard interrupt `int 09h` handler

Question

I'm working on an assignment for the univesity, we need to create a simple breakout/arkanoid clone, it is going pretty well but I found a bug that would delete everything on the screen, this bug is random but I suspect its related to my DrawPaddle function. Maybe you can spot the bug or have the knowledge about why is the video memory doing that.

The game has to be done with 16 bit ms-dos assembly, I'm using NASM + VAL + Dosbox to create it, I compile it with:

nasm -f obj test.asm
val test.obj

The game justs moves the paddle in a fixed screen using the keyboard arrows, you can also quit the game by pressing escape.

This is the while everything is still alright: https://puu.sh/yeKtG/affc912d4b.png and it looks like this when the program overflows: http://puu.sh/yeKEy/caeef089d1.png or http://puu.sh/yeKJH/1106e1e823.png

I noticed that the weird behaviour only happens when I move the paddle and it will happen at random, for example now that I removed almost everything else from the program it can take a few tries to get the bug.

This is the DrawPaddle code:

DrawPaddle:
    push di
    mov di, [paddleposition]
    mov cx, 5 ;the paddle will be 5 pixels tall
.p0:
    push cx
    mov cx, paddlesize
.p1:
    mov byte [es:di], bl
    inc di
    loop .p1
    add di, screenweight - paddlesize
    pop cx
    loop .p0
    pop di
    ret

And this is the complete code, it uses a keyboard handler to read the input and will write directly into the video memory using 320x200x256.

BITS 16

stacksize       EQU 0200h

;Constantes
;Direccion de inicio de la memoria de video
videobase       EQU 0a000h

;Definicion de colores
black           EQU 0
green           EQU 00110000b

;Screen data
screenweight    EQU 320


;Paddle data
startx      EQU 140
starty      EQU 170
paddlesize      EQU 40
paddlecolor     EQU 00101010b 

;Paddle movement limits
leftlimit       EQU starty * screenweight + 1 + 10 + 1
rightlimit       EQU ((starty + 1) * screenweight) - paddlesize - 10 - 1

segment mystack stack
    resb stacksize
stacktop:   

segment mydata data

;Variables
escpressed  dw 0
leftpressed     dw 0
rightpressed    dw 0
oldintseg       resw 1
oldintoff       resw 1
originalVideoMode resb 1
paddleposition  resw 1

segment mycode code
;Subrutinas

KeybInt:
        push    ds ;guardamos ds:ax       
        push    ax              

        mov ax, mydata ;los re-inicializamos
        mov ds, ax          

        cli

.getstatus:
        in      al, 64h
        test    al, 02h
        loopnz  .getstatus ;esperando a que el puerto esté listo

        in      al,60h ;obtenemos el codigo make o break de la tecla leida

        cmp     al, 01h ;revisamos si es escape
        jne     .revEsc 
        mov     word [escpressed], 1
        jmp     .kbread
.revEsc:
        cmp     al, 81h ;revisamos si el escape fue soltado
        jne     .revIzq
        mov     word [escpressed], 0
        jmp     .kbread
.revIzq:
        cmp     al, 4bh ;revisamos si es la flecha izquierda
        jne     .revDer
        mov     word [leftpressed], 1
        jmp     .kbread
.revDer:
        cmp     al, 4dh ;revisamos si es la flecha derecha
        jne     .revIzq2
        mov     word [rightpressed], 1
        jmp     .kbread
.revIzq2:
        cmp     al, 0cbh ;si se solto la flecha izquierda
        jne     .revDer2
        mov     word [leftpressed], 0
        jmp     .kbread
.revDer2:
        cmp     al, 0cdh ;o la derecha
        jne     .kbread
        mov     word [rightpressed], 0
        jmp     .kbread
.kbread:
        in      al, 61h     
        or      al, 10000000b
        out     61h, al            
        and     al, 01111111b                     
        out     61h, al                      
        mov     al, 20h
        out     20h, al               

        sti 

        pop     ax ;recuperamos ds:ax       
        pop     ds
        iret

DrawStage:
    push di
    push bx
    ;movemos el cursor a la posicion 10,10
    ;que seria en realidad 10*320+10
    mov di, (10 * screenweight) + 10
    ;ahora repetiremos esto 320-20 veces
    mov cx, 300
.h1:
    mov byte [es:di], green
    inc di
    loop .h1

    mov di, (190 * screenweight) + 10
    ;ahora repetiremos esto 320-20 veces
    mov cx, 301
.h2:
    mov byte [es:di], green
    inc di
    loop .h2    

    ;ahora volveremos al primer punto
    ;y dibujaremos hacia abajo
    mov di, (10 * screenweight) + 10
    ;y lo repetiremos 200-20 veces
    mov cx, 180
.v1:
    mov byte [es:di], green
    add di, screenweight
    loop .v1

    mov di, (10 * screenweight) + 310
    mov cx, 180
.v2:
    mov byte [es:di], green
    add di, screenweight
    loop .v2

    pop bx
    pop di
    ret

;Rutina para dibujar el palo
;Recibe en bl el color del mismo
DrawPaddle:
    push di
    mov di, [paddleposition]
    mov cx, 5 ;the paddle will be 5 pixels tall
.p0:
    push cx
    mov cx, paddlesize
.p1:
    mov byte [es:di], bl
    inc di
    loop .p1
    add di, screenweight - paddlesize
    pop cx
    loop .p0
    pop di
    ret

Delay1:
    mov dx, 4
    sub dx, 3
.pause1:
    mov cx, 6000
.pause2:
    dec cx
    jne .pause2
    dec dx
    jne .pause1
    ret

..start:
    mov ax, mydata
    mov ds, ax
    mov ax, mystack
    mov ss, ax
    mov sp, stacktop

    ;guardando el manejador actual
    mov ah, 35h
    mov al, 9h
    int 21h
    mov [oldintseg], es
    mov [oldintoff], bx

    ;instalando el manejador nuevo
    mov ax, mycode
    mov es, ax
    mov dx, KeybInt
    mov ax, cs
    mov ds, ax
    mov ah, 25h
    mov al, 9h
    int 21h

    ;restaurando el segmento de datos
    mov ax, mydata
    mov ds, ax

    ;guardando el modo de video y aplicando el nuevo
    xor ax, ax
    mov ah, 0fh
    int 10h
    mov [originalVideoMode], al
    mov ah, 00h
    mov al, 13h
    int 10h
    ;coordenada de inicio para el palo
    mov ax, (screenweight * starty) + startx
    mov word [paddleposition], ax
    mov ax, videobase
    mov es, ax

    call DrawStage
    mov bl, paddlecolor
    call DrawPaddle
    jmp .main

.main:
    call Delay1

    ;leemos las entradas
    cmp word [escpressed], 1
    je .dosexit
    cmp word [rightpressed], 1
    je .movRight
    cmp word [leftpressed], 1
    je .movLeft
    jmp .main
.movRight:
    mov bl, black
    call DrawPaddle
    cmp word [paddleposition], rightlimit
    je .ending
    inc word [paddleposition]
    jmp .ending
.movLeft:
    mov bl, black
    call DrawPaddle
    cmp word [paddleposition], leftlimit
    je .ending
    dec word [paddleposition]
    jmp .ending
.ending:    
    mov bl, paddlecolor
    call DrawPaddle
    jmp .main

.dosexit:
    ;restaurando el modo de video original
    mov ah, 00h
    mov byte al, [originalVideoMode]
    int 10h

    ;restaurando el manejador de teclado original
    mov dx, [oldintoff]
    mov ax, [oldintseg]
    mov ds, ax
    mov ah, 25h
    mov al, 9h
    int 21h
    mov al, 0
    mov ah, 4ch
    int 21h

Thanks for reading!

Answer 1

You modify cx in your keyboard interrupt without preserving it.

_{^^^ this is the ANSWER (what is causing your bug), not just some advice}

---

_{Here some advice is following:}

Also it feels wrong to have any loop (dynamic delay) in interrupt, interrupts should proceed as fast as possible.

I can't recall from head what is correct way to read 0x6X ports of keyboard (I just recall it is a bit tricky, to have it fully correct), so I'm not going to check particular in/out sequence and its correctness.

But if you will set XXXpressed in interrupt by actual current state, and the main loop will be too slow, it may not see very short key presses (as the input is not buffered). For a simple game as arkanoid clone this is OK, and I wouldn't be bothered by this at all, sounds to me as correct behaviour (you would need to be actually incredibly fast to hold the key so short).

Also you can avoid ds setup in interrupt by reserving some data space near the interrupt code handler (moving escpressed dw 0 into the code part after iret), then using that everywhere as mov word [cs:escpressed], 1, etc. The total penalty for using cs: addressing inside interrupt would be lower than the ds setup, if you would actually set the memory flags in more efficient manner and short interrupt code (can be simplified a lot).

And it's sort of funny how extensively you use slow loop instruction for all main loops, but then in delay subroutine you do the faster dec cx jnz ... alternative.

---

And I did check in the end how to write DOS keyboard handler, so this is my suggestion (unfortunately I didn't test it, if it works):

segment mycode code

escpressed      db 0
leftpressed     db 0
rightpressed    db 0

KeybInt:
        cli
        push    ax      ;guardamos ax

        ; when IRQ1 is fired, int 9 is called to handle it and the input
        ; already waits on port 0x60, no need to validate IBF flag on 0x64

        in      al,60h ;obtenemos el codigo make o break de la tecla leida
        mov     ah,al
        and     al,0x7F ; AL = scan code without pressed/released flag
        shr     ah,7
        xor     ah,1    ; AH = 1/0 pressed/released

        cmp     al, 01h ;revisamos si es escape
        jne     .checkLeft
        mov     [cs:escpressed], ah
        jmp     .kbread
.checkLeft:
        cmp     al, 4bh ;revisamos si es la flecha izquierda
        jne     .checkRight
        mov     [cs:leftpressed], ah
        jmp     .kbread
.checkRight:
        cmp     al, 4dh ;revisamos si es la flecha derecha
        jne     .kbread
        mov     [cs:rightpressed], ah
.kbread:

        in      al, 61h
        mov     ah, al          ; store original value
        or      al, 10000000b
        out     61h, al         ; set "enable kbd" bit
        mov     al, ah
        out     61h, al         ; set original value back

        mov     al, 20h
        out     20h, al         ; send end-of-interrupt signal to 8259 IC

        pop     ax ;recuperamos ax
        sti        ; not needed in real x86 real mode, IRET restores flags
        iret       ; but explicit STI paired with CLI may help some VMs

... then in game code, to check state of key, you must use cs too:

        ...
        cmp     byte [cs:escpressed], 1
        ...