What's wrong with this code?

Index > DOS > What's wrong with this code?

Author

Thread

Ben321

Joined: 07 Dec 2017
Posts: 70

Ben321 01 Oct 2018, 07:54

My code is intended to do some graphics operations. But first it needs to switch to 256color VGA mode and set the data segment to equal to the VGA video memory. So far my program doesn't set any pixels, but even what it does do (prepare the software for graphics operations) is not working.

My code is:

Code:

format mz
start:
        ;set graphics mode to 320x200 with 256 colors VGA mode
        mov ah,0
        mov al,0x13
        int 0x10


        ;set DS to start of VGA video RAM
        push 0xA000 ;push the one argument (desired DS segment address) for the function
        call SetDS ;call the SetDS function

        ;later code is going to go in here to set pixel data

        ;exit to DOS
        mov ax,0x4C00
        int 0x21


SetDS:
        mov ax,bx ;make sure to backup the BX register
        mov bx,sp ;set the BX register to the current stack pointer
        mov ds,[bx+2] ;set the DS register to the value passed to the function in the stack
        mov bx,ax ;restore the BX register
        ret 2 ;return from the SetDS function

This is fully documented to help you debug it.

Here's a screenshot to show just what is not working. I'm using Insight as my debugger. DOS 6.22 is my OS, and it's running in VirtualBox (not real hardware), if that might have any impact on my program's operation.

Notice that the DS register is NOT being set to 0xA000 as it should be, but rather is being set to 0x0000. Why is the DS register ignoring the value that I'm trying to set it to?

01 Oct 2018, 07:54

Tomasz Grysztar

Joined: 16 Jun 2003
Posts: 8351
Location: Kraków, Poland

Tomasz Grysztar 01 Oct 2018, 08:16

You need to use SS prefix when accessing stack:

Code:

        mov ds,[ss:bx+2] ;set the DS register to the value passed to the function in the stack

Alternatively, a better correction would be to use BP register instead of BX, because it is the actual register intended for stack addressing and it uses SS addressing by default:

Code:

SetDS:
        mov ax,bp ;make sure to backup the BP register
        mov bp,sp ;set the BX register to the current stack pointer
        mov ds,[bp+2] ;set the DS register to the value passed to the function in the stack
        mov bp,ax ;restore the BP register
        ret 2 ;return from the SetDS funct

To summarize: [BX] accesses memory in DS segment while [BP] accessed memory in SS segment. To change the default behavior you need to use prefix, [SS:BX] to access stack with BX address, [DS:BP] to access data segment with BP address.

01 Oct 2018, 08:16

Ben321

Joined: 07 Dec 2017
Posts: 70

Ben321 01 Oct 2018, 08:26

DOH! Why didn't I see that?

01 Oct 2018, 08:26

DimonSoft

Joined: 03 Mar 2010
Posts: 1228
Location: Belarus

DimonSoft 01 Oct 2018, 11:59

Well, in fact, a better solution would be to pop ds without this bloated procedure.

01 Oct 2018, 11:59

Ben321

Joined: 07 Dec 2017
Posts: 70

Ben321 01 Oct 2018, 18:46

DimonSoft wrote:

Well, in fact, a better solution would be to pop ds without this bloated procedure.

Popping can only come from the top of the stack. I'd have to pop twice, once to get the return address and again to get DS. Then I'd have to push the return address back on the stack before calling RET. It would end up having at least the same number of lines of code as the original.

01 Oct 2018, 18:46

DimonSoft

Joined: 03 Mar 2010
Posts: 1228
Location: Belarus

DimonSoft 01 Oct 2018, 19:38

Ben321 wrote:

DimonSoft wrote:
Well, in fact, a better solution would be to pop ds without this bloated procedure.

Popping can only come from the top of the stack. I'd have to pop twice, once to get the return address and again to get DS. Then I'd have to push the return address back on the stack before calling RET. It would end up having at least the same number of lines of code as the original.

Without the procedure. There’s no need to write a procedure for something that is perfectly done with a single instruction.

01 Oct 2018, 19:38

Ben321

Joined: 07 Dec 2017
Posts: 70

Ben321 01 Oct 2018, 22:00

DimonSoft wrote:

Ben321 wrote:

DimonSoft wrote:
Well, in fact, a better solution would be to pop ds without this bloated procedure.

Popping can only come from the top of the stack. I'd have to pop twice, once to get the return address and again to get DS. Then I'd have to push the return address back on the stack before calling RET. It would end up having at least the same number of lines of code as the original.

Without the procedure. There’s no need to write a procedure for something that is perfectly done with a single instruction.

you mean like:

Code:

push 0xA000
pop ds

I created a separate function as a helper function, because calling a function tells me exactly what's happening when going back to debug the source code. Seeing the function name PopDS tells me exactly what the code does.

01 Oct 2018, 22:00

Ben321

Joined: 07 Dec 2017
Posts: 70

Ben321 01 Oct 2018, 22:35

I ended up using a similar approach to the one you mentioned, but instead of push 0xA000 and pop ds, I used this:
mov ax,0xA000
mov es,ax

I changed ds to es as well, because es is the destination segment for the STOSB operation (a very fast way of transferring whatever is held in the AL register to whatever memory address is pointed to by ES:DI)

My full FASM code (with most lines commented) is below, and now it does something with the pixels. It draws a horizontal gradient of all 256 colors in the default VGA palette.

Code:

format mz
start:
        ;set graphics mode to 320x200 with 256 colors VGA mode
        mov ah,0
        mov al,0x13
        int 0x10

        ;set ES to start of VGA video RAM
        push es; backup ES register
        mov ax, 0xA000 ;set AX register to value for ES register
        mov es,ax ;set the ES register with the value stored in AX

        ;draw horizontal gradient of all VGA colors
        push di ;backup DI register by pushing to stack
        xor di,di ;clear DI register to make it point to the first pixel
        .loop:
                movzx eax,di ;copy DI register to lower 16bits of EAX and clear upper 16bits
                             ;this prepares EAX for 32bit operations as 32bit operations will be needed here
                             ;this is needed because the largest intermediate value encountered in this pixel math is > 65535
                             ;though the final output pixel value will be no larger than 255
                mov ecx,320 ;set ECX to image width
                xor edx,edx ;clear EDX to make sure dividend upper 32 bits is 0
                div ecx ;calculate x position (goes into EDX) and y position (goes into EAX)
                mov eax,edx ;set EAX to x position of next pixel to draw
                mov ecx,256 ;set ECX the number of colors in VGA palette
                mul ecx ;multiply x position by number of colors
                mov ecx,320 ;set ECX to image width
                ;no need to clear EDX in preparation for division, as EDX is 0, due to MUL output < 4 billion
                div ecx ;divide EAX by image width
                ;the previous operations are equivalent to the following floating point pseudocode
                ;PixelValue = IntegerPart(x*ColorCount/ImageWidth)
                ;I was able to avoid the use of floating point math, by making sure to multiply before dividing
                stosb ;set pixel pointed to by DI to the desired pixel value stored in AL
                      ;and increment DI by 1.
                cmp di,0xFA00 ;compare DI to the total number of pixels in the image
        jb .loop ;loop until the image is finished drawing
        pop di ;restore original DI register by popping from stack

        ;restore original ES register
        pop es

        ;exit to DOS
        mov ax,0x4C00
        int 0x21

01 Oct 2018, 22:35

rugxulo

Joined: 09 Aug 2005
Posts: 2341
Location: Usono (aka, USA)

rugxulo 02 Oct 2018, 07:05

Try looking at PSR Invaders. Granted, it's 8086 only code (although you're free to modify it with 386 instructions), but it does use 320x200x256 (VGA, 0xA000:0) and PC speaker sound (via IN, OUT) in pure DOS. It should work in Win9x, too.

02 Oct 2018, 07:05

DimonSoft

Joined: 03 Mar 2010
Posts: 1228
Location: Belarus

DimonSoft 02 Oct 2018, 08:33

Ben321 wrote:

I created a separate function as a helper function, because calling a function tells me exactly what's happening when going back to debug the source code. Seeing the function name PopDS tells me exactly what the code does.

I don’t see how debugging a procedure may be easier than debugging a single instruction. You can clearly see it yourself: your procedure turned out to be buggy, while the instruction just works.

Ben321 wrote:

I ended up using a similar approach to the one you mentioned, but instead of push 0xA000 and pop ds, I used this:
mov ax,0xA000
mov es,ax

I changed ds to es as well, because es is the destination segment for the STOSB operation (a very fast way of transferring whatever is held in the AL register to whatever memory address is pointed to by ES:DI)

While replacing push/pop sequence with two moves may be a good idea, mixing 16-bit and 32-bit code without really needing it is kind of mauvais ton.

02 Oct 2018, 08:33

Ben321

Joined: 07 Dec 2017
Posts: 70

Ben321 02 Oct 2018, 20:02

DimonSoft wrote:

Ben321 wrote:
I created a separate function as a helper function, because calling a function tells me exactly what's happening when going back to debug the source code. Seeing the function name PopDS tells me exactly what the code does.

I don’t see how debugging a procedure may be easier than debugging a single instruction. You can clearly see it yourself: your procedure turned out to be buggy, while the instruction just works.

Ben321 wrote:
I ended up using a similar approach to the one you mentioned, but instead of push 0xA000 and pop ds, I used this:
mov ax,0xA000
mov es,ax

I changed ds to es as well, because es is the destination segment for the STOSB operation (a very fast way of transferring whatever is held in the AL register to whatever memory address is pointed to by ES:DI)

While replacing push/pop sequence with two moves may be a good idea, mixing 16-bit and 32-bit code without really needing it is kind of mauvais ton.

The intermediate values in the pixel math will produce values that are larger than 0xFFFF. Therefore 32bits are needed during the intermediate calculations. The final result will be 1 byte in size though.

02 Oct 2018, 20:02

DimonSoft

Joined: 03 Mar 2010
Posts: 1228
Location: Belarus

DimonSoft 03 Oct 2018, 00:13

Ben321 wrote:

The intermediate values in the pixel math will produce values that are larger than 0xFFFF. Therefore 32bits are needed during the intermediate calculations. The final result will be 1 byte in size though.

For your particular task multiplication and division are not necessary at all. Exactly the case when thinking a little more saves you a lot of trouble, general-purpose registers, code size and support requirements. Dirty piece of code drawing exactly (pixel-by-pixel exactly) the same image as your program:

Code:

        org 100h

EntryPoint:
        mov     ax, $0013
        int     10h

        push    $A000
        pop     es

        xor     di, di
        xor     ax, ax
        mov     bl, 1
        mov     cx, 320 * 200
.DrawLoop:
        stosb
        dec     bx
        jnz     @F
        dec     al
        mov     bl, 5
@@:
        inc     al
        loop    .DrawLoop

        xor     ax, ax
        int     16h
        ret

35 bytes COM file, waits for keypress, no 32-bit maths, only 4 general-purpose registers involved. In fact you can even avoid using BX at all:

Code:

        org 100h

EntryPoint:
        mov     ax, $0013
        int     10h

        push    $A000
        pop     es

        xor     di, di
        xor     ax, ax
        mov     ah, 51
        mov     cx, 320 * 200
.DrawLoop:
        stosb
        sub     ah, 51
        jnz     @F
        dec     al
        dec     ah
@@:
        inc     al
        loop    .DrawLoop

        xor     ax, ax
        int     16h
        ret

36 bytes COM file, waits for keypress, no 32-bit maths, only 3 general-purpose registers involved.

Could be even smaller and faster if I didn’t try to follow the pattern of your code, i.e. if I didn’t care which colours become stretched and which are exactly 1 pixel wide.

Not to mention that in both cases the code is likely to be even faster than 64000 of multiplications and 128000 of divisions.

In fact, in assembly, if you need multiplication and/or division by a constant, you’d better think a few more times if you’re doing the right thing. Generally, if you think you need multiplication or division, there’re three cases:
* you need floating-point calculations;
* you perform integer multiplication/division by an integer that is determined at runtime;
* you can avoid using integer multiplication/division completely.

If the first two are not your cases, you’re most likely in the third case.

03 Oct 2018, 00:13

< Last Thread | Next Thread >

Forum Rules:

You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot vote in polls in this forum
You cannot attach files in this forum
You can download files in this forum