HiveBrain v1.2.0
Get Started
← Back to all entries
patternMinor

A Virtual Piano

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
virtualpianostackoverflow

Problem

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:
cm

Solution

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:

  • get keycode



  • translate AL = code to AL = pitch



  • 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:

  • get keycode



  • if it's Esc, quit the program



  • if it's z, make the octave lower



  • if it's x, make the octave higher



  • if it's any other valid key, translate to pitch and play sound



  • goto step 1



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

  • get keycode



  • lookup keycode action



  • if no action, goto 1



  • 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 inte

Code Snippets

; this is the structure used for key handling
struc        keyhandle
        .keycode            resb 1
        .value              resb 1
        .process            resw 1
endstruc
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
quit:
    mov ah,4ch
    int 21h
change_octave:
    add ch, al
    cmp ch, 108
    jbe .ok
    mov ch, 108
.ok:
    cmp ch, 12
    jae .done
    mov ch, 12
.done:
    ret
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

Context

StackExchange Code Review Q#87322, answer score: 7

Revisions (0)

No revisions yet.