Keyboard driver skips keystrokes

[osdever]

I rewrote my keyboard driver yesterday. Everything works perfectly except for keystroke skipping. I don't know what it caused by. Also maybe this is because of my delay in kybd_buffer_pop() because without it system gets stuck in infinite loop. Here's my code:

Code: Select all

#include "arch/i686/kbd.h"
#include "arch/i686/regs.h"
#include "debug.h"

typedef struct {
    uint8_t scancode;
    char chr; //Character it corresponds to.
} kybd_scancode;

static kybd_scancode regular_scancodes[] = {
  /* Numerical keys */
  {0x02, '1'}, {0x03, '2'}, {0x04, '3'}, {0x05, '4'}, {0x06, '5'}, {0x07, '6'}, {0x08, '7'}, {0x09, '8'}, {0x0A, '9'}, {0x0B, '0'}, 
  /* Some characters after numerical keys */
  {0x0C, '-'}, {0x0D, '='}, /*{0x0E, '\b'},*/ {0x0F, '\t'},
  /* Alphabet! */
  {0x10, 'q'}, {0x11, 'w'}, {0x12, 'e'}, {0x13, 'r'}, {0x14, 't'}, {0x15, 'y'}, {0x16, 'u'}, {0x17, 'i'}, {0x18, 'o'}, {0x19, 'p'}, {0x1A, '['}, {0x1B, ']'}, {0x1C, '\n'},
  {0x1E, 'a'}, {0x1F, 's'}, {0x20, 'd'}, {0x21, 'f'}, {0x22, 'g'}, {0x23, 'h'}, {0x24, 'j'}, {0x25, 'k'}, {0x26, 'l'}, {0x27, ';'}, {0x28, '\''}, {0x29, '`'}, 
  {0x2B, '\\'}, {0x2C, 'z'}, {0x2D, 'x'}, {0x2E, 'c'}, {0x2F, 'v'}, {0x30, 'b'}, {0x31, 'n'}, {0x32, 'm'}, {0x33, ','}, {0x34, '.'}, {0x35, '/'}, {0x37, '*'}, 
  {0x39, ' '}, {0x47, '7'}, {0x48, '8'}, {0x49, '9'}, {0x4A, '-'},
               {0x4B, '4'}, {0x4C, '5'}, {0x4D, '6'}, {0x4E, '+'},
               {0x4F, '1'}, {0x50, '2'}, {0x51, '3'},
               {0x52, '0'}, {0x53, '.'}, {0x00, '\0'}
};
static kybd_scancode uppercase_scancodes[] = {
  /* Numerical keys */
  {0x02, '1'}, {0x03, '2'}, {0x04, '3'}, {0x05, '4'}, {0x06, '5'}, {0x07, '6'}, {0x08, '7'}, {0x09, '8'}, {0x0A, '9'}, {0x0B, '0'}, 
  /* Some characters after numerical keys */
  {0x0C, '-'}, {0x0D, '='}, /*{0x0E, '\b'},*/ {0x0F, '\t'},
  /* Alphabet! */
  {0x10, 'Q'}, {0x11, 'W'}, {0x12, 'E'}, {0x13, 'R'}, {0x14, 'T'}, {0x15, 'Y'}, {0x16, 'U'}, {0x17, 'I'}, {0x18, 'O'}, {0x19, 'P'}, {0x1A, '['}, {0x1B, ']'}, {0x1C, '\n'},
  {0x1E, 'A'}, {0x1F, 'S'}, {0x20, 'D'}, {0x21, 'F'}, {0x22, 'G'}, {0x23, 'H'}, {0x24, 'J'}, {0x25, 'K'}, {0x26, 'L'}, {0x27, ';'}, {0x28, '\''}, {0x29, '`'}, 
  {0x2B, '\\'}, {0x2C, 'Z'}, {0x2D, 'X'}, {0x2E, 'C'}, {0x2F, 'V'}, {0x30, 'B'}, {0x31, 'N'}, {0x32, 'M'}, {0x33, ','}, {0x34, '.'}, {0x35, '/'}, {0x37, '*'}, 
  {0x39, ' '}, {0x47, '7'}, {0x48, '8'}, {0x49, '9'}, {0x4A, '-'},
               {0x4B, '4'}, {0x4C, '5'}, {0x4D, '6'}, {0x4E, '+'},
               {0x4F, '1'}, {0x50, '2'}, {0x51, '3'},
               {0x52, '0'}, {0x53, '.'}, {0x00, '\0'}
};
static kybd_scancode shift_modified_scancodes[] = {
  /* Numerical keys */
  {0x02, '!'}, {0x03, '@'}, {0x04, '#'}, {0x05, '$'}, {0x06, '%'}, {0x07, '^'}, {0x08, '&'}, {0x09, '*'}, {0x0A, '('}, {0x0B, ')'}, 
  /* Some characters after numerical keys */
  {0x0C, '_'}, {0x0D, '+'}, /*{0x0E, '\b'},*/ {0x0F, '\t'},
  /* Alphabet! */
  {0x10, 'Q'}, {0x11, 'W'}, {0x12, 'E'}, {0x13, 'R'}, {0x14, 'T'}, {0x15, 'Y'}, {0x16, 'U'}, {0x17, 'I'}, {0x18, 'O'}, {0x19, 'P'}, {0x1A, '{'}, {0x1B, '}'}, {0x1C, '\n'},
  {0x1E, 'A'}, {0x1F, 'S'}, {0x20, 'D'}, {0x21, 'F'}, {0x22, 'G'}, {0x23, 'H'}, {0x24, 'J'}, {0x25, 'K'}, {0x26, 'L'}, {0x27, ':'}, {0x28, '"'}, {0x29, '~'}, 
  {0x2B, '\\'}, {0x2C, 'Z'}, {0x2D, 'X'}, {0x2E, 'C'}, {0x2F, 'V'}, {0x30, 'B'}, {0x31, 'N'}, {0x32, 'M'}, {0x33, '<'}, {0x34, '>'}, {0x35, '/'}, {0x37, '*'}, 
  {0x39, ' '}, {0x47, '7'}, {0x48, '8'}, {0x49, '9'}, {0x4A, '-'},
               {0x4B, '4'}, {0x4C, '5'}, {0x4D, '6'}, {0x4E, '+'},
               {0x4F, '1'}, {0x50, '2'}, {0x51, '3'},
               {0x52, '0'}, {0x53, '.'}, {0x00, '\0'}
};

typedef struct {
    int lshift : 1;
    int rshift : 1;
    int lctrl  : 1;
    int rctrl  : 1;
    int numlk  : 1;
    int capslk : 1;
    int scrllk : 1;
} kybd_modifier_key_states;
static kybd_modifier_key_states modifier_keys;

static char keyboard_buffer[256] = {};
static uint8_t kybd_buffer_current_position = 0; //0 to 255
static void kybd_buffer_push(char value)
{
    if(kybd_buffer_current_position < 255)
        keyboard_buffer[kybd_buffer_current_position++] = value;
    else
    {
        printk("Keyboard buffer overflow! Keypresses will NOT be processed before buffer will be freed. User hit the keys randomly in Flash speed, system is too slow, or BPS monkeys just mistaken?");
        return;
    }
}
static char kybd_buffer_pop()
{
    if(kybd_buffer_current_position > 0)
        return keyboard_buffer[--kybd_buffer_current_position];
    else
    {
        inb(0x60); //Note for OSDev.org users: without any kind of delay my code infinitely loops. O_O
        return 0; //we have nothing in the buffer
    }
}

//Interrupt part of the keyboard driver.
volatile bool kbd_irq_fired=false;
void kbd_wait_irq()
{
    //if(kbd_irq_fired){while(kbd_irq_fired);return;}
    while(!kbd_irq_fired);
    kbd_irq_fired=false;
}

char kbd_scancode_convert(uint8_t scancode)
{
    if(modifier_keys.capslk)
    {
        for(int i=0; uppercase_scancodes[i].scancode != 0x00; i++)
            if(uppercase_scancodes[i].scancode == scancode)
              return uppercase_scancodes[i].chr;
        return 0;
    }
    else if(modifier_keys.lshift || modifier_keys.rshift)
    {
        for(int i=0; shift_modified_scancodes[i].scancode != 0x00; i++)
            if(shift_modified_scancodes[i].scancode == scancode)
              return shift_modified_scancodes[i].chr;
        return 0;
    }
    else {
        for(int i=0; regular_scancodes[i].scancode != 0x00; i++)
            if(regular_scancodes[i].scancode == scancode)
              return regular_scancodes[i].chr;
        return 0;
    }
}

void kbd_handler(struct regs *UNUSED(r))
{
    kbd_irq_fired=true;
    //We need to put every pressed printable key to the buffer.
    uint8_t scancode = inb(KBD_DATA);
    switch(scancode)
    {
        case KEY_LSHIFT_P: modifier_keys.lshift = 1; break;
        case KEY_RSHIFT_P: modifier_keys.rshift = 1; break;
        case KEY_LSHIFT_R: modifier_keys.lshift = 0; break;
        case KEY_RSHIFT_R: modifier_keys.rshift = 0; break;
        default: kybd_buffer_push(kbd_scancode_convert(scancode)); break;
    }
}

uint8_t kbd_getchar()
{
    char ret=0;
    while(!ret)
    {
        kbd_wait_irq();
        ret = kybd_buffer_pop();
    }
    return ret;
}

size_t kbd_gets(char *s)
{
    uint8_t c=0;
    int i=0;
    while(true)
    {
        c=kbd_getchar();
        if(c>0)
        {
           if(c=='\n')
           {
               s[i]='\0';
               return i;
           }
           if(c=='\b')
           {
              if(i>0)
              {
                tty_putchar('\b');
                s[--i]=0;
              }
           }
           tty_putchar(c);
           s[i++]=c;
        }
    }
}
void kbd_scancodes_setup()
{
	//IMPORTANT: Remove that function as soon as possible, it does nothing.
}
//Keyboard-powered CPU reset.
void kbd_reset_cpu()
{
    asm("cli");
    uint8_t good = 0x02;
    while (good & 0x02)
        good = inb(0x64);
    outb(0x64, 0xFE);
    tty_wrstr("Keyboard CPU reset failed.");
    asm("hlt");
}

[iansjack]

If you expect people to take any notice of questions like this you're probably going to have to act a little more grown up. Resurrecting ancient threads doesn't fit within that category any more than giving yourself extra stars does. You'll have to decide whether this is a serious site or a bit of social media where you can play juvenile games.

[osdever]

Thanks for advice. I solved that problem, this thread can be locked.

[Octocontrabass]

What was your solution? Was it adding the volatile keyword to variables that need it but don't have it? Did you replace your stack (which will scramble the order of key presses) with a ring buffer?

[osdever]

No. I just removed a delay from kybd_buffer_pop() and added sleep(1) to getchar. Then everything started to work.

[Octocontrabass]

In other words, you still have no idea what's wrong with it.

By the way, neither of my suggestions were hypothetical. Those are both bugs I saw in your code.

[osdever]

Yes. Unfortunately, but yes. Anyway it works now

[MichaelFarthing]

Yes. Unfortunately, but yes. Anyway it works now

Catnikita, it works now, but tomorrow?
Don't ignore the advice. A delay is not a solution - it puts things right by chance. Tomorrow you might only need 0.5 second - or 17 seconds. You haven't found the real solution. (Neither have I, but I know the problem is still there). Carry on looking. Carry on listening.

[Ch4ozz]

Everytime I had to put a Sleep(0) into my code to make something work I knew it was some stupid problem with my scheduler.
Solving something with a Sleep didnt satisfy me in any way and I kep searching for the bugs (sometimes 2-3 days) and when I found the root of the problem suddenly everything worked as expected and made sense now.
Please only use Sleep as bugfix when you know the exact problem and this is the only fix

[Boris]

Sleeps/yields are evil.
They may make your synchronisation work for now but you will realise later that you had to pay a heavy price : the purity of your code . What you will have left is a corrupt world where things works holistically.
Don't let thee Eden get wasted and create event based synchronisation right away.
You should be able to execute a small driver code in your IRQ handler and wake up a thread.

[osdever]

I know that, but I don't have multitasking/multithreading and won't have it long time.

[max]

What is this supposed to be?

[matt11235]

What is this supposed to be?

In the default forum theme it's supposed to look like he has 5 extra stars.

[max]

In the default forum theme it's supposed to look like he has 5 extra stars.

lol