Keyboard driver in bran's tutorials

Question about which tools to use, bugs, the best way to implement a function, etc should go here. Don't forget to see if your question is answered in the wiki first! When in doubt post here.
User avatar
Schol-R-LEA
Member
Member
Posts: 1925
Joined: Fri Oct 27, 2006 9:42 am
Location: Athens, GA, USA

Re: Keyboard driver in bran's tutorials

Post by Schol-R-LEA »

On a small note, I would recommend that, for functions that are related to or mirror the behavior of standard C library functions, you prefix the letter 'k' in front of the function name, to prevent confusion and ensure that no linker conflicts exist. That is to say, you would name you kernel version of gets() as kgets(), printf() as kprintf(), and so forth.

On a larger note, the design you have chosen is, to my mind, problematic.
  • You never declare the sizes of _kdata[] and i[]. AFAIK, this shouldn't even compile as it is; while an implicit array size can be used when using an initialization, when declaring an extern variable, or when passing an array as a function argument, it shouldn't work as the actual declaration of a static array. The only thing I can think of is that it is automatically devolving to a pointer, but if so, what is it pointing at?
  • You never check whether you have overrun the buffers, either. The conventional implementation of a keyboard buffer is as a ring buffer; this avoids the question of what to do if the keyboard buffer is overrun, at the slight risk of losing some typed characters. Since you do need to check the input buffer to make sure that you don't scribble on some other data, adding the code to wrap around to the head of the buffer is a minor addition.
  • The tests in the implementation of kflush() are redundant; they are more costly than the unnecessary assignment operations you are trying to avoid. Just set the values regardless of what they are now, and don't worry about it. The same applies to the test inside the while() loop in gets(). Furthermore, if you change _kdata so that it is handled as a ring buffer, then you wouldn't need to reset _TMP_ at all; you would simply keep reading into the buffer and letting it wrap around. OTOH, you want to assign the current _kdata[_TMP_] to a newline (since that is what you are testing for in gets()), not zero.
  • The return value for gets() makes no sense; it is returning the address of the _kdata array (which is a constant) after casting it to an int. What is this supposed to accomplish?
  • Your gets() works with only a single implicit input buffer and a single implicit destination buffer. You may find that it makes more sense to handle the buffers explicitly, more like fgets() does, with a buffer pointer and size for each. While you may not expect to need multiple keyboard buffers for the kernel, you may find that you will want them or even need them at some point. More important is the destination buffer. You presumably want to be able to read from the keyboard buffer into different C-strings, correct? If so, then what you are doing now makes little sense, as a) you are copying into a fixed buffer, which you then would have to copy from again to put it into your working strings, and b) you are overwriting it on every gets() call, making it, in effect, the same as the keyboard buffer. This doesn't make sense to me.
Perhaps I'm simply misunderstanding the code, but if so, I'd appreciate it if you could explain it a bit more.
Rev. First Speaker Schol-R-LEA;2 LCF ELF JAM POEE KoR KCO PPWMTF
Ordo OS Project
Lisp programmers tend to seem very odd to outsiders, just like anyone else who has had a religious experience they can't quite explain to others.
snasim2002
Member
Member
Posts: 37
Joined: Sat Apr 11, 2015 9:37 am

Re: Keyboard driver in bran's tutorials

Post by snasim2002 »

Oh yeah !! I re wrote the code a with a relativly simple aproach and now it is working !! Alejandro Bezdjian (https://github.com/alebian) helped me a lot.

Code: Select all

#include "../includes/kernel.h"
#include "../includes/screen.h"
#include "../includes/irq.h"
#include "../includes/kb.h"

int STATUS = 0;
static char scanCodeToAsciiTable[] = 
{
0, ESC, '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '-', '=', BACKSPACE, TAB,
'q', 'w', 'e', 'r', 't', 'y', 'u', 'i', 'o', 'p', '[', ']', ENTER, 0, 'a', 's',
'd', 'f', 'g', 'h', 'j', 'k', 'l', ';', '\'', '`', 0, '\\', 'z', 'x', 'c', 'v',
'b', 'n', 'm', ',', '.', '/', 0, '*', 0, ' ', 0, KF1, KF2, KF3, KF4, KF5,
KF6, KF7, KF8, KF9, KF10, 0, 0, KHOME, KUP, KPGUP, '-', KLEFT, 0, KRIGHT, '+', KEND,
KDOWN, KPGDN, KINS, KDEL, 0, 0, 0, KF11, KF12 
};

/* Handles the keyboard interrupt */
void keyboard_handler(struct regs *r)
{
  STATUS = 1;
}
unsigned char kgetc()
{
  while (STATUS == 0){}
  unsigned char scn = inportb(0x60);
      if (scn & 0x80)
    {
	return;
    }
    else
    {
	return scanCodeToAsciiTable[scn];
    }
}
void turn_on_leds(){
    outportb(0x60, SET_LED);
    outportb(0x60, 0x07);
    return;
}

void turn_off_leds(){
    outportb(0x60, SET_LED);
    outportb(0x60, 0x00);
    return;
}

void turn_on_capsLock(){
    outportb(0x60, SET_LED);
    outportb(0x60, 0x04);
    return;
}

void turn_on_numLock(){
    outportb(0x60, SET_LED);
    outportb(0x60, 0x02);
    return;
}

void turn_on_scrollLock(){
    outportb(0x60, SET_LED);
    outportb(0x60, 0x01);
    return;
}
/* Installs the keyboard handler into IRQ1 */
void keyboard_install()
{
    irq_install_handler(1, keyboard_handler);
}
Post Reply