A Virtual Piano

Question

I have created a virtual piano that turns the home row (excluding 'g' and 'h') and the top row (excluding 'y' and 'u') of the user's keyboard in a piano's keyboard.

The home row is used for white keys and the top row is used for black keys.

The keyboard's keys bind to piano's as shown below:

a - C
w - C#
s - D
e - D#
d - E
r - E# (F)
f - F
t - F#
j - G
i - G#
k - A
o - A#
l - B
p - B# (C)
; - C

To change octaves, you use 'z' to go up one octave and 'x' to down one octave.

The code

virtual_piano.asm

;===============================================================================
; Virtual Piano -- a virtual and playable piano
; By SirPython of Code Review and GitHub
;
; virtual_piano.asm
;===============================================================================

%define MIDI_CONTROL_PORT 0331h
%define MIDI_DATA_PORT 0330h
%define MIDI_UART_MODE 3Fh
%define MIDI_PIANO_INSTRUMENT 93h

start:
    call setup_midi
    mov ch, 60;             default octave(0)
    mov cl, 5;              used for making sure that the user does not go too low or too high with the octaves
.loop:
    call read_character
    call process_input

    cmp bh, 0;              if bad input OR octave change goes out of range
    je .loop

    call get_pitch

    cmp bh, 2;              if shouldn't play note (was an octave switch)
    je .loop

    call play_note

    jmp .loop

;--------------------------------------------------
; Plays a note
;
; IN: AL, CH = pitch, (octave * 12) + 60
; OUT: NONE
; ERR: NONE
; REG: AL

play_note:
    add al, ch;             apply the octave
    out dx, al;             DX will already contain MIDI_DATA_PORT from the setup_midi function

    mov al, 7Fh;            note duration
    out dx, al

    ret

;--------------------------------------------------
; Based on input, returns a pitch to be played
;
; IN: AL = key code
; OUT: AL, BH, CH = pitch, 2 if no pitch to be played, (octave * 12) + 60
; ERR: NONE
; REG: preserved

get_pitch:
    cmp al, 'a'
    je .a
    cmp al, 's'
    je .s
    cmp al, 'd'
    je .d
    cmp al, 'f'
    je .f
    cmp al, 'j'
    je .j
    cmp al, 'k'
    je .k
    cmp al, 'l'
    je .l
    cmp al, ';'
    je .sc

    cmp al, 'w'
    je .w
    cmp al, 'e'
    je .e
    cmp al, 'r'
    je .r
    cmp al, 't'
    je .t
    cmp al, 'i'
    je .i
    cmp al, 'o'
    je .o
    cmp al, 'p'
    je .p

    cmp al, 'z'
    je .z
    cmp al, 'x'
    je .x

.a: mov al, 0
    jmp .end
.s: mov al, 2
    jmp .end
.d: mov al, 4
    jmp .end
.f: mov al, 5
    jmp .end
.j: mov al, 7
    jmp .end
.k: mov al, 9
    jmp .end
.l: mov al, 11
    jmp .end
.sc: mov al, 12
    jmp .end

.w: mov al, 1
    jmp .end
.e: mov al, 3
    jmp .end
.r: jmp .f
    jmp .end
.t: mov al, 6
    jmp .end
.i: mov al, 8
    jmp .end
.o: mov al, 10
    jmp .end
.p: jmp .l
    jmp .end

.z: add ch, 12
    add cl, 1
    mov bh, 2
    jmp .end
.x: sub ch, 12
    sub cl, 1
    mov bh, 2
    jmp .end


.end:
    ret

;--------------------------------------------------
; Set's up the MIDI ports for use
;
; IN: NONE
; OUT: NONE
; ERR: NONE
; REG: DX

setup_midi:
    push ax

    mov dx, MIDI_CONTROL_PORT
    mov al, MIDI_UART_MODE; play notes as soon as they are recieved
    out dx, al

    mov dx, MIDI_DATA_PORT
    mov al, MIDI_PIANO_INSTRUMENT
    out dx, al

    pop ax
    ret

;--------------------------------------------------
; Checks to make sure that input is acceptable
;
; IN: AL = key code
; OUT: BH = 1 (accpetable) or 0 (not acceptable, or octave is trying to change too far)
; ERR: NONE
; REG: preserved

process_input:

.check_key_code:
    cmp al, 'a'
    je .safe
    cmp al, 's'
    je .safe
    cmp al, 'd'
    je .safe
    cmp al, 'f'
    je .safe
    cmp al, 'j'
    je .safe
    cmp al, 'k'
    je .safe
    cmp al, 'l'
    je .safe
    cmp al, ';'
    je .safe

    cmp al, 'w'
    je .safe
    cmp al, 'e'
    je .safe
    cmp al, 'r'
    je .safe
    cmp al, 't'
    je .safe
    cmp al, 'i'
    je .safe
    cmp al, 'o'
    je .safe
    cmp al, 'p'
    je .safe

.check_octave_code:
    cmp al, 'z'
    je .z
    cmp al, 'x'
    je .x

    jmp .err;               none of the keys pressed were valid keys

.z:
    cmp cl, 10;             if user is about to go out of octave range, then drop down to error
    jne .safe

.x:
    cmp cl, 1
    jne .safe

.err:
    xor bh, bh
    ret

.safe:
    mov bh, 1
    ret


;--------------------------------------------------
; Reads a single character from the user
;
; IN: NONE
; OUT: AL = key code
; ERR: NONE
; REG: preserved

read_character:
    xor ah, ah
    int 16h;                BIOS 16h 00h
    ret

Build

This was written in x86 NASM Assembler and run in the DOSBOX.

You can use the Makefile below for building and running the code.

NAME=virtual_piano
ASM=nasm
ASMFLAGS=-o $(NAME).com

DIR=$(shell pwd)

DB=dosbox
DBFLAGS=-c "mount c $(DIR)" -c "C:" -c "VIRTUA~2.COM"

all:
    $(ASM) $(NAME).asm $(ASMFLAGS)

run:
    $(DB) $(DBFLAGS)

clean:
    rm *.com

Documentation syntax

For each subroutine in the code, I created a short documentation following this syntax:

;--------------------------------------------------
; DESCRIPTION
;
; IN: reg1, reg2, reg3 = desc1, desc2, desc3
; OUT: reg1, reg2, reg3 = desc1, desc2, desc3
; ERR: any errors that could possibly be produced and how the output will change if they are produced
; REG: any registers that will not be preserved (excluding ones holding the returns). Or, if all are being preserved (excluding the
ones holding the returns), just "preserved"

Concerns:

1. Those 'switch statements' in the subroutines get_pitch and process_input. It is very, very ugly. And, with all the conditional checks, I'm assuming it's also very, very inefficient. Is there a better way I could do this?

2. My register use for input and output of the subroutines. I know that it doesn't really matter whether or not I'm using AX or BX in this specific code but, for example, is it okay that I stuff a return code in BH? Or that I use CH to hold octave values, rather than another register?

3. Are the subroutines doing their job and only their job? I'm especially concerned about get_pitch and process_input; they both do the same kind of thing, but they both return completely separate values. Is one doing another's job? Or, how about combining them. Would that be a good idea?

4. Is my code understandable? Is it easy to follow what is happening and why it is happening?

5. How is my documentation? I know many of you hate having lines going across the screen like that. If you are on of those people, how else do you recommend doing it? Or, if you aren't one of those people, is there anything differently I should do with the documentation.

6. All that MIDI port stuff and instrument stuff came from various documentations unofficial documentations and codes that I read from the internet. These numbers came pieced from those. Should I, for the goal of my code, be using different values/ports for MIDI/sound?

Any other recommendations are encouraged.

Answer 1

I see a number of things that may help you improve your program.

Commenting

Your per-routine header comments are excellent. They provide exactly the right information that a programmer would want to know about the program. Namely, what registers are expected, which are returned and which are trashed. I have not continued that style within the code I show here, but that's only for the sake of brevity. Please do continue with that style -- it's a very good habit! For a slightly different approach here is an example showing my usual commenting style.

Think of the user

There are a few user-oriented things I think should be improved. First, I see that you have the notes for the keys a through ; arranged from left to right and from low to high, just like a real piano. That's good because it is intuitive to the user. However, in the same way, I would expect z to cause the octave to be lower and x to make it go higher, but it's the other way around in this program. Also, there really ought to be a way to allow the user to exit the program! I would suggest something obvious like the Esc key.

Carefully consider what the program must do

As you have suspected, there are indeed better ways to handle the very large "switch statement" currently in the program. The way I typically write assembly language programs like this is to start with what we have as input (in this case the keycode in the AL register), and what we ultimately need to output, which is a pitch, also in AL. So in pseudocode it might look like this:

1. get keycode
2. translate AL = code to AL = pitch
3. play sound

However, some keys, namely z and x don't emit a sound. They just change the octave. Also, as I've mentioned in the previous item, there should be a way to exit the program. So our modified psuedocode might look like this:

1. get keycode
2. if it's Esc, quit the program
3. if it's z, make the octave lower
4. if it's x, make the octave higher
5. if it's any other valid key, translate to pitch and play sound
6. goto step 1

It's still a bit messy. Another way to construct things would be like this:

1. get keycode
2. lookup keycode action
3. if no action, goto 1
4. otherwise execute action

Now that's looking fairly neat, so let's see how to do that.

Use a data structure

We want to map a keycode to a particular action, so we could have a list of tuples: keycode, subroutine, but the only difference between the action for a and the action for s is a different value for the pitch. Likewise the difference between z and x is only the direction of the octave change. This suggests that we can very efficiently handle all of these with tuples like this: keycode, value, subroutine. In NASM assembly:

; this is the structure used for key handling
struc        keyhandle
        .keycode            resb 1
        .value              resb 1
        .process            resw 1
endstruc

Then we can use the data structure to define all key actions:

keys    db 'a',0
        dw play_note
        db 's',2
        dw play_note
        db 'd',4
        dw play_note
        db 'f',5
        dw play_note
        db 'j',7
        dw play_note
        db 'k',9
        dw play_note
        db 'l',11
        dw play_note
        db ';',12
        dw play_note
        db 'w',1
        dw play_note
        db 'e',3
        dw play_note
        db 'r',5
        dw play_note
        db 't',6
        dw play_note
        db 'i',8
        dw play_note
        db 'o',10
        dw play_note
        db 'p',11
        dw play_note
        db 'z',-12
        dw change_octave
        db 'x',+12
        dw change_octave
        db 1bh,0
        dw quit
; end of list marker
        db 0,0
        dw 0

Then all that's left is to define play_note, change_octave and quit routines. Since quit is simplest, I show it here:

quit:
    mov ah,4ch
    int 21h

Streamline range checking

Currently two 8-bit registers are being used to represent octaves. The ch register holds the actual octave value that is used to calculate the pitch, and the cl register holds the value representing the octave number. However, both are not really needed. The minimum value is 12, and the maximum usable value is probably 108. The program currently only effectively limits the low end of the range, but fails on the high end. I would propose instead using only ch and having a change_octave routine that accepts the step (+12 or -12) in al:

change_octave:
    add ch, al
    cmp ch, 108
    jbe .ok
    mov ch, 108
.ok:
    cmp ch, 12
    jae .done
    mov ch, 12
.done:
    ret

Note that this has the attribute of pegging the value so that if, due to some kind of error, the octave value were out of range, it will always return to being in range after this call.

Avoid overrunning slower hardware

The interface you are using was popularized many years ago by a dominant manufacturer of sound cards then. Documentation for that interface is still available if you search for it. The values you are sending to the port are correct, but in order to properly send them, the code must poll and wait until the sound hardware is ready to accept another input value. This is done by reading the control port and waiting until bit 6 is clear. So I'd suggest rewriting the play_note routine as follows:

play_note:
    add al, ch;             apply the octave
    out dx, al;             DX = MIDI_DATA_PORT
    inc dx
.busy:    
    in  al, dx
    test al,40h ; ready for output?
    jnz .busy
    dec dx
    mov al, 7Fh;            note duration
    out dx, al
    inc dx
.notready:
    in   al,dx
    test al,40h     ; ready for output?
    jnz .notready
    dec  dx
    ret

Use generic code to drive the interface

With the data structure introduced earlier, the code for handling input become much shorter and easier to follow. Given a keycode in the al register, this routine scans the keys table and executes the appropriate action, if any:

find_action:
    mov si,keys
.next:
    cmp al,[si]
    je .found
    add si, keyhandle_size
    cmp byte [si],0
    jnz .next
    ret
.found:
    mov al,[si + keyhandle.value]
    jmp [si + keyhandle.process]

This code simply linearly searches through the keys table until either a matching value is found or until the end of the table is reached. If the end of the table is reached, the code simply returns. Otherwise, it loads al with the value from the table and then jmps to the routine. Because we're already in a subroutine, this has the effect that when the called routine is done, it will ret to the main loop from within which this code is called. That code is shown in the next suggestion.

Use org to tell the assembler important things

Because your code is being linked as a DOS .com program, its code and data are in the same segment and the starting offset is 100h. This is important to tell the assembler so that it can correctly generate offsets. The main routine now looks like this:

org 100h
start:
    call setup_midi
    mov ch, 60;             default octave value
.loop:
    call read_character
    call find_action
    jmp .loop