OSDev.org

Posted: **Wed Sep 10, 2008 8:29 pm**

I've got an issue with my keyboard driver in 32-bit assembly. kbd_readchar reads one character from the keyboard using i/o ports 0x64 and 0x60, echos the character, and returns it in al. So far the kernel is responsible for the loop of getting the input over and over again. But after around 170-ish (I don't feel like counting exactly) characters are inputted, the cursor goes back to the top and keeps moving, but the inputted characters still go to the correct place. At 257, it starts overwriting the previous input.

Help!

Code: Select all

kbd_readchar:

	; OK, I'm going to note a few things here before you read
	; any more of the routine. VGA is a slow beast sometimes,
	; but the constant calling to move_cursor is a permanent
	; fixture. Also, no longer does this thing take characters
	; until infinity. It returns one character printed in ah.

	in al,64h		; read status byte
	and al,01h		; wait for output buffer to be full
	jz kbd_readchar

	in al,60h		; read scancode byte

	cmp al,02h
	je near .q
	cmp al,03h
	je near .w
	cmp al,04h
	je near .e
	cmp al,05h
	je near .r
	cmp al,06h
	je near .t
	cmp al,07h
	je near .y
	cmp al,08h
	je near .u
	cmp al,09h
	je near .i
	cmp al,0ah
	je near .o
	cmp al,0bh
	je near .p

	cmp al,10h
	je near .q
	cmp al,11h
	je near .w
	cmp al,12h
	je near .e
	cmp al,13h
	je near .r
	cmp al,14h
	je near .t
	cmp al,15h
	je near .y
	cmp al,16h
	je near .u
	cmp al,17h
	je near .i
	cmp al,18h
	je near .o
	cmp al,19h
	je near .p

	cmp al,1eh
	je near .a
	cmp al,1fh
	je near .s
	cmp al,20h
	je near .d
	cmp al,21h
	je near .f
	cmp al,22h
	je near .g
	cmp al,23h
	je near .h
	cmp al,24h
	je near .j
	cmp al,25h
	je near .k
	cmp al,26h
	je near .l

	cmp al,2ch
	je near .z
	cmp al,2dh
	je near .x
	cmp al,2eh
	je near .c
	cmp al,2fh
	je near .v
	cmp al,30h
	je near .b
	cmp al,31h
	je near .n
	cmp al,32h
	je near .m

	cmp al,0eh
	je near .backspace

	cmp al,39h
	je near .space

	cmp al,1ch
	je near .enter

	jmp kbd_readchar

.q:

	mov bl,'q'
	mov al,'q'
	call putch32
	mov	bh, byte [_CurY]	; get current position
	mov	bl, byte [_CurX]
	call	move_cursor		; update cursor
	ret

.w:
	
	mov bl,'w'
	mov al,'w'
	call putch32
	mov	bh, byte [_CurY]
	mov	bl, byte [_CurX]
	call	move_cursor
	ret

.e:
	
	mov bl,'e'
	mov al,'e'
	call putch32
	mov	bh, byte [_CurY]
	mov	bl, byte [_CurX]
	call	move_cursor
	ret

.r:
	
	mov bl,'r'
	mov al,'r'
	call putch32
	mov	bh, byte [_CurY]
	mov	bl, byte [_CurX]
	call	move_cursor
	ret

.t:
	
	mov bl,'t'
	mov al,'t'
	call putch32
	mov	bh, byte [_CurY]
	mov	bl, byte [_CurX]
	call	move_cursor
	ret

.y:
	
	mov bl,'y'
	mov al,'y'
	call putch32
	mov	bh, byte [_CurY]
	mov	bl, byte [_CurX]
	call	move_cursor
	ret

.u:
	
	mov bl,'u'
	mov al,'u'
	call putch32
	mov	bh, byte [_CurY]
	mov	bl, byte [_CurX]
	call	move_cursor
	ret

.i:
	
	mov bl,'i'
	mov al,'i'
	call putch32
	mov	bh, byte [_CurY]
	mov	bl, byte [_CurX]
	call	move_cursor
	ret

.o:
	
	mov bl,'o'
	mov al,'o'
	call putch32
	mov	bh, byte [_CurY]
	mov	bl, byte [_CurX]
	call	move_cursor
	ret

.p:
	
	mov bl,'p'
	mov al,'p'
	call putch32
	mov	bh, byte [_CurY]
	mov	bl, byte [_CurX]
	call	move_cursor
	ret

.a:
	
	mov bl,'a'
	mov al,'a'
	call putch32
	mov	bh, byte [_CurY]
	mov	bl, byte [_CurX]
	call	move_cursor
	ret

.s:
	
	mov bl,'s'
	mov al,'s'
	call putch32
	mov	bh, byte [_CurY]
	mov	bl, byte [_CurX]
	call	move_cursor
	ret

.d:
	
	mov bl,'d'
	mov al,'d'
	call putch32
	mov	bh, byte [_CurY]
	mov	bl, byte [_CurX]
	call	move_cursor
	ret

.f:
	
	mov bl,'f'
	mov al,'f'
	call putch32
	mov	bh, byte [_CurY]
	mov	bl, byte [_CurX]
	call	move_cursor
	ret

.g:
	
	mov bl,'g'
	mov al,'g'
	call putch32
	mov	bh, byte [_CurY]
	mov	bl, byte [_CurX]
	call	move_cursor
	ret

.h:
	
	mov bl,'h'
	mov al,'h'
	call putch32
	mov	bh, byte [_CurY]
	mov	bl, byte [_CurX]
	call	move_cursor
	ret

.j:
	
	mov bl,'j'
	mov al,'j'
	call putch32
	mov	bh, byte [_CurY]
	mov	bl, byte [_CurX]
	call	move_cursor
	ret

.k:
	
	mov bl,'k'
	mov al,'k'
	call putch32
	mov	bh, byte [_CurY]
	mov	bl, byte [_CurX]
	call	move_cursor
	ret

.l:
	
	mov bl,'l'
	mov al,'l'
	call putch32
	mov	bh, byte [_CurY]
	mov	bl, byte [_CurX]
	call	move_cursor
	ret

.z:
	
	mov bl,'z'
	mov al,'z'
	call putch32
	mov	bh, byte [_CurY]
	mov	bl, byte [_CurX]
	call	move_cursor
	ret

.x:
	
	mov bl,'x'
	mov al,'x'
	call putch32
	mov	bh, byte [_CurY]
	mov	bl, byte [_CurX]
	call	move_cursor
	ret

.c:
	
	mov bl,'c'
	mov al,'c'
	call putch32
	mov	bh, byte [_CurY]
	mov	bl, byte [_CurX]
	call	move_cursor
	ret

.v:
	
	mov bl,'v'
	mov al,'v'
	call putch32
	mov	bh, byte [_CurY]
	mov	bl, byte [_CurX]
	call	move_cursor
	ret

.b:
	
	mov bl,'b'
	mov al,'b'
	call putch32
	mov	bh, byte [_CurY]
	mov	bl, byte [_CurX]
	call	move_cursor
	ret

.n:
	
	mov bl,'n'
	mov al,'n'
	call putch32
	mov	bh, byte [_CurY]
	mov	bl, byte [_CurX]
	call	move_cursor
	ret

.m:
	
	mov bl,'m'
	mov al,'m'
	call putch32
	mov	bh, byte [_CurY]
	mov	bl, byte [_CurX]
	call	move_cursor
	ret

.space:
	
	mov bl,' '
	mov al,' '
	call putch32
	mov	bh, byte [_CurY]
	mov	bl, byte [_CurX]
	call	move_cursor
	ret

.enter:
	
	mov bl,0ah
	mov bl,0ah
	call putch32
	mov	bh, byte [_CurY]
	mov	bl, byte [_CurX]
	call	move_cursor
	ret

.backspace:			; this is a fun little thing

	cmp byte [_CurX],0	; beginning of line?
	je .ignbksp		; if yes, ignore backspace

	mov ah,[_CurX]
	dec ah
	mov [_CurX],ah
	
	mov	bh, byte [_CurY]
	mov	bl, byte [_CurX]
	call	move_cursor
	
	mov bl,20h
	call putch32
	
	mov ah,[_CurX]
	dec ah
	mov [_CurX],ah
	
	mov	bh, byte [_CurY]
	mov	bl, byte [_CurX]
	call	move_cursor
	
	mov al,08h
	ret

.ignbksp:

	ret

Posted: **Wed Sep 10, 2008 11:01 pm**

Hi,

Troy Martin wrote:I've got an issue with my keyboard driver in 32-bit assembly. kbd_readchar reads one character from the keyboard using i/o ports 0x64 and 0x60, echos the character, and returns it in al. So far the kernel is responsible for the loop of getting the input over and over again. But after around 170-ish (I don't feel like counting exactly) characters are inputted, the cursor goes back to the top and keeps moving, but the inputted characters still go to the correct place. At 257, it starts overwriting the previous input.

This is going to hurt, but you'll thank me eventually...

Did you have a stroke or some other medical condition that prevented you from thinking of using a table instead of repeating "cmp al,<scancode>; je <code_to_handle_one_specific_case>" (and each piece of code these jump to) heaps of times?

At which point during development did you decide it'd be a good idea to combine the keyboard driver with the video driver and some console code?

Is the kernel designed specifically for computers with broken PIC chips (it's a niche market!)?

Troy Martin wrote:Help!

There's no point fixing minor bugs when the design is so badly flawed... it's much better to fix the design instead (starting with deleting everything).

Cheers,

Brendan

Posted: **Wed Sep 10, 2008 11:32 pm**

I am sorry to say this, but I agree with the above poster! What are you using a ton of jumps and code for each and every character when you could very easily be using a table of codes? Also, using a table, you can consolidate a print character function rather than writing it out so many times, so fixing it in one place will fix it in all places! Also, if you decide to use another scan-code type (there are more than 1 scan code table for keyboards), you can simply replace the table rather than rewrite 80+ functions! The way my keyboard handler works (and all other input devices for that matter), is that they get a keystroke, and send the information to the kernel (key down or key-up). The kernel stores a linked list of inputs (from various sources, but we'll stick with the keyboard for now). So, it simply checks to make sure the buffer isn't full yet and adds the key down or up to the list. Then, during the application or whatever is reading the keyboards time, it checks to see if the input buffer has data, and reads it in and process' it. This way, my keyboard driver can be PS2, USB, AT, etc without my kernel or applications caring at all. The output goes through either my kernel debugger for testing, or my video driver for display (or my serial output for logging, whatever method of output I chose as my output since all outputs go to whatever is set in my kernel as the current output device). I seriously think you need to at least modify your design to use tables rather than individual functions for each key, and storing them in a buffer for later use will almost be a must in any OS (you could use a keydown/keyup callback function if it needs to respond immediately, but individual functions is not a good idea).

Posted: **Thu Sep 11, 2008 9:57 am**

This piece of code seems to have made it's way onto the Wiki. With all due respect, I must object to putting poorly designed code on the Wiki.

Posted: **Thu Sep 11, 2008 11:45 am**

Hi,

Stevo14 wrote:This piece of code seems to have made it's way onto the Wiki. With all due respect, I must object to putting poorly designed code on the Wiki.

Fixed.

Cheers,

Brendan

Posted: **Thu Sep 11, 2008 12:11 pm**

Hi,

here's the code from wiki

Code: Select all

kbd_readchar:
  ;Poll port 0x64 for a full output buffer
  in al, 0x64
  and al, 0x01
  jz kbd_readchar

  in al,60h		; read scancode byte

  cmp al,02h
  je near .q
  jmp al,03h
  je near .w
  cmp al,04h
  je near .e
  cmp al,05h
  je near .r
  cmp al,06h
  je near .t
  cmp al,07h
  je near .y
  cmp al,08h
  je near .u
  cmp al,09h
  je near .i
  cmp al,0ah
  je near .o
  cmp al,0bh
  je near .p

  ret

.q:
  mov al, 'q'
  call bios.write_char
  ret
.w:
  mov al, 'w'
  call bios.write_char
  ret
.e:
  mov al, 'e'
  call bios.write_char
  ret
.r:
  mov al, 'r'
  call bios.write_char
  ret
.t:
  mov al, 't'
  call bios.write_char
  ret
.y:
  mov al, 'y'
  call bios.write_char
  ret
.u:
  mov al, 'u'
  call bios.write_char
  ret
.i:
  mov al, 'i'
  call bios.write_char
  ret
.o:
  mov al, 'o'
  call bios.write_char
  ret
.p:
  mov al, 'p'
  call bios.write_char
  ret

I don't see any difference?
am I missing somthing here...

Thanx.

Posted: **Thu Sep 11, 2008 8:08 pm**

Posted that before it was labeled broken and stroke-induced...

I think I'll delete this topic after I get something that could be an answer..

Posted: **Thu Sep 11, 2008 9:30 pm**

Hi,

Troy Martin wrote:I think I'll delete this topic after I get something that could be an answer..

If you do get an answer, it'd probably come from someone who's seen the code for the "move_cursor" routine and the "putch32" routine. The code you posted has serious design flaws (especially if it's meant to be part of an actual OS) but otherwise it should work as intended.

Cheers,

Brendan

Posted: **Fri Sep 12, 2008 4:43 pm**

Brendan wrote:If you do get an answer, it'd probably come from someone who's seen the code for the "move_cursor" routine and the "putch32" routine. The code you posted has serious design flaws (especially if it's meant to be part of an actual OS) but otherwise it should work as intended.

I *gulp* agree, the code I have has some serious flaws in the designs. If some of the design flaws you mention are the fact that some things take the long route when it could take the short route (in a few syscalls, ignore the keyboard driver issues for a second here), it's for clarity to learning developers (and even anyone experienced who will revise the code.)

I'm definately going to reduce the size of the keyboard driver by at least setting up a call to a single piece of code that does the repeated putch32 and move_cursor work.

If there were to be one smiley that describes how I feel about my code right now it's this one:

Posted: **Fri Sep 12, 2008 5:42 pm**

Don't worry, looking back at every single line of code I've ever written makes me feel like that.

Posted: **Fri Sep 12, 2008 10:08 pm**

Hi,

Troy Martin wrote:If there were to be one smiley that describes how I feel about my code right now it's this one:

Welcome to OS Development...

I've been writing my OS for over 10 years. As a general rule of thumb (based on my experience), while writing "version x" of an OS the developer will either:

a) realize their current code can be improved using what they learnt writing it, and decide to start working on "version x+1"
b) decide that the current code is so good that they should increase the scope (add more features, etc), and start working on "version x+1"
c) give up, and stop writing OSs
d) give up, and accept code they know can be improved

If you never give up and never accept code that can be improved, then after an infinite number of versions you'll end up with a perfect OS...

Cheers,

Brendan

Posted: **Fri Sep 12, 2008 10:29 pm**

JackScott wrote:Don't worry, looking back at every single line of code I've ever written makes me feel like that.

Ah, someone who feels my pain.

Optimised, the kernel source code is now less than 10KB, and compiles to about 1,129 bytes. I would assume it's a bit faster too. Next plan is real input: get a character, compare it to backspace, if so decrement cx and go to beginning, compare it to enter, if so ret, else increment cx, add character to string and go back to beginning.
The above is a rough idea, if I missed something I'll probably catch if in mid-test.

Posted: **Fri Sep 12, 2008 11:32 pm**

Hi,

Troy Martin wrote:Next plan is real input: get a character, compare it to backspace, if so decrement cx and go to beginning, compare it to enter, if so ret, else increment cx, add character to string and go back to beginning.

Modern CPU's do something called "speculative execution" to improve performance. This means that for branches, they'll take a guess at which way the branch will go and start speculatively executing the code after the branch. If the CPU's guess is wrong (branch mis-prediction) then you usually end up with a full pipeline flush and the CPU has to start executing instructions that haven't been prefetched, decoded, etc. Basically a branch mis-prediction hurts performance.

To reduce the chance of branch mis-prediction the CPU has some complex stuff that keeps track of branch history. This is called the BTB (Branch Target Buffer). The BTB has a limited size, which means that using too many branches will consume too many "slots" in the BTB and increase the chance of branch mis-predictions everywhere else.

If you use a series of comparisons and branches, you'll get lots of branch mis-predictions and bad performance. Instead, try something like this:

Code: Select all

   section .data

%define KBD_SPECIAL_ignore     0x00
%define KBD_SPECIAL_backspace  0x01
%define KBD_SPECIAL_enter      0x02

	align 2
kbd_special_table:
   dw kbd_readchar             ;Ignore the scancode
   dw kbd_backspace            ;Handle backspace
   dw kbd_enter                ;Handle enter

kbd_scancode_table:
   db KBD_SPECIAL_ignore       ;0x00
   db KBD_SPECIAL_ignore       ;0x01 (escape)
   db '1'                      ;0x02
   db '2'                      ;0x03
   db '3'                      ;0x04
   db '4'                      ;0x05
   db '5'                      ;0x06
   db '6'                      ;0x07
   db '7'                      ;0x08
   db '8'                      ;0x09
   db '9'                      ;0x0A
   db '0'                      ;0x0B
   db '-'                      ;0x0C
   db '='                      ;0x0D
   db KBD_SPECIAL_backspace    ;0x0E
   db KBD_SPECIAL_ignore       ;0x0F (tab)

   db 'q'                      ;0x10
   db 'w'                      ;0x11
   db 'e'                      ;0x12
   db 'r'                      ;0x13
   db 't'                      ;0x14
   db 'y'                      ;0x15
   db 'u'                      ;0x16
   db 'i'                      ;0x17
   db 'o'                      ;0x18
   db 'p'                      ;0x19
   db '['                      ;0x1A
   db ']'                      ;0x1B
   db KBD_SPECIAL_enter        ;0x1C
   db KBD_SPECIAL_ignore       ;0x1D (control)
   db 'a'                      ;0x1E
   db 's'                      ;0x1F

   db 'd'                      ;0x20
   db 'f'                      ;0x21
   db 'g'                      ;0x22
   db 'h'                      ;0x23
   db 'j'                      ;0x24
   db 'k'                      ;0x25
   db 'l'                      ;0x26
   db ';'                      ;0x27
   db "'"                      ;0x28
   db '`'                      ;0x29
   db KBD_SPECIAL_ignore       ;0x2A (left shift)
   db '\'                      ;0x2B
   db 'z'                      ;0x2C
   db 'x'                      ;0x2D
   db 'c'                      ;0x2E
   db 'v'                      ;0x2F

   db 'b'                      ;0x30
   db 'n'                      ;0x31
   db 'm'                      ;0x32
   db ','                      ;0x33
   db '.'                      ;0x34
   db '/'                      ;0x35
   db KBD_SPECIAL_ignore       ;0x36 (right shift)
   db '*'                      ;0x37
   db KBD_SPECIAL_ignore       ;0x38 (alt)
   db ' '                      ;0x39
   db KBD_SPECIAL_ignore       ;0x3A (caps lock)
   db KBD_SPECIAL_ignore       ;0x3B (F1)
   db KBD_SPECIAL_ignore       ;0x3C (F2)
   db KBD_SPECIAL_ignore       ;0x3D (F3)
   db KBD_SPECIAL_ignore       ;0x3E (F4)
   db KBD_SPECIAL_ignore       ;0x3F (F5)
kbd_scancode_table_end:

%define KBD_scancode_table_length (kbd_scancode_table_end - kbd_scancode_table)

   section .text



kbd_readchar:
   in al,64h                         ;Read status byte
   and al,01h                        ;Wait for output buffer to be full
   jz kbd_readchar

   in al,60h                         ;Read scancode byte
   cmp al,KBD_scancode_table_length  ;Is the scancode in our scancode table?
   jae kbd_readchar                  ; no, ignore the scancode and try again

   movzx eax,al                      ;eax = scancode
   mov al,[kbd_scancode_table + eax] ;al = ASCII character or special code
   cmp al,32                         ;Is it an ASCII character?
   jae .gotASCII                     ; yes, send it to the console to be displayed
   jmp [kbd_special_table + eax * 2] ; no, jump to special handler

.gotASCII:
   mov bl,al
   call putch32
   mov   bh, byte [_CurY]
   mov   bl, byte [_CurX]
   call   move_cursor
   jmp kbd_readchar


kbd_enter:
   mov bl,0ah
   mov bl,0ah
   call putch32
   mov   bh, byte [_CurY]
   mov   bl, byte [_CurX]
   call   move_cursor
   ret                        ;This is the only return from "kbd_readchar"....


kbd_backspace:
   cmp byte [_CurX],0   ; beginning of line?
   je kbd_readchar      ; if yes, ignore backspace

   dec byte [_CurX]
   mov   bh, byte [_CurY]
   mov   bl, byte [_CurX]
   call   move_cursor

   mov bl,20h
   call putch32

   dec byte [_CurX]
   mov   bh, byte [_CurY]
   mov   bl, byte [_CurX]
   call   move_cursor

   jmp kbd_readchar

For 64 scancodes you'd use 64 conditional branches. The code above uses 3 conditional branches instead (and is probably smaller).

Cheers,

Brendan

Posted: **Sat Sep 13, 2008 10:36 am**

My god.... Do you want any credit for that?

If it was a bit smaller I could fit that entire kernel into the bootsector: current size: 643 bytes! It shaved off AT LEAST half a kilobyte from my code.

EDIT: Triple fault.
EDIT2: Ran through bochs debugger. Got a general protection fault after typing any ignored key, enter, or backspace. GPF's on bootup with VPC.

Posted: **Sat Sep 13, 2008 12:07 pm**

Hi,

Troy Martin wrote:My god.... Do you want any credit for that?

Heh - no thanks...

Troy Martin wrote:EDIT: Triple fault.
EDIT2: Ran through bochs debugger. Got a general protection fault after typing any ignored key, enter, or backspace. GPF's on bootup with VPC.

That's my fault - try changing the corresponding lines to this:

Code: Select all

   cmp al,32                         ;Is it an ASCII character?
   jae .gotASCII                     ; yes, send it to the console to be displayed
   jmp word [kbd_special_table + eax * 2] ; no, jump to special handler

Cheers,

Brendan

OSDev.org

Keyboard Driver Craziness.

Keyboard Driver Craziness.

Re: Keyboard Driver Craziness.

Re: Keyboard Driver Craziness.

Re: Keyboard Driver Craziness.

Re: Keyboard Driver Craziness.

Re: Keyboard Driver Craziness.

Re: Keyboard Driver Craziness.

Re: Keyboard Driver Craziness.

Re: Keyboard Driver Craziness.

Re: Keyboard Driver Craziness.

Re: Keyboard Driver Craziness.

Re: Keyboard Driver Craziness.

Re: Keyboard Driver Craziness.

Re: Keyboard Driver Craziness.

Re: Keyboard Driver Craziness.