at_keyboard and checkkey()

at_keyboard and checkkey()

[Robert Millan]

[-- Attachment #1: Type: text/plain, Size: 1271 bytes --]


Hi,

at_keyboard's checkkey() implementation is buggy.  It will flush the
controller's input buffer, but checkkey()'s semantics require that it's
not flushed.  This results in some keys being lost when using at_keyboard.

A proper fix that adjusts to this API is complicated.  I implemented one [1],
only to realize afterwards that maybe this mess isn't really necessary.

A quick check at grub_checkkey() users reveals that none of them care about
the actual key, even if grub_checkkey returns it.  They just want to know
whether there's a key available for read at all.  If we removed this
constraint from the checkkey() semantics, perhaps at_keyboard can get a
checkkey() with less hassle.

However, when at_keyboard reads an event, it needs to determine whether it's
a MAKE (key hit) or BREAK (key unhit) event.  Which AFAICT can't be done
without reading KEYBOARD_REG_DATA.  But maybe I'm missing something.

Any bright ideas?

[1] See attachment.  This made me implement queues as well, perhaps some of
    this can be reused.

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

[-- Attachment #2: at_keyboard_queue.diff --]
[-- Type: text/x-diff, Size: 6763 bytes --]

Index: conf/i386.rmk
===================================================================
--- conf/i386.rmk	(revision 2607)
+++ conf/i386.rmk	(working copy)
@@ -6,7 +6,7 @@ cpuid_mod_CFLAGS = $(COMMON_CFLAGS)
 cpuid_mod_LDFLAGS = $(COMMON_LDFLAGS)
 
 pkglib_MODULES += at_keyboard.mod
-at_keyboard_mod_SOURCES = term/i386/pc/at_keyboard.c
+at_keyboard_mod_SOURCES = term/i386/pc/at_keyboard.c kern/queue.c
 at_keyboard_mod_CFLAGS = $(COMMON_CFLAGS)
 at_keyboard_mod_LDFLAGS = $(COMMON_LDFLAGS)
 
Index: kern/queue.c
===================================================================
--- kern/queue.c	(revision 0)
+++ kern/queue.c	(revision 0)
@@ -0,0 +1,53 @@
+/* queue.c - grub queue functions */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2009  Free Software Foundation, Inc.
+ *
+ *  GRUB is free software: you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  GRUB is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <grub/queue.h>
+#include <grub/misc.h>
+#include <grub/mm.h>
+
+void
+grub_queue_push (grub_queue_t queue, grub_queue_item_t item)
+{
+  item->next = queue->head;
+  item->prev = NULL;
+  if (queue->head)
+    queue->head->prev = item;
+  queue->head = item;
+
+  if (! queue->tail)
+    /* Empty queue becomes non-empty.  */
+    queue->tail = item;
+}
+
+void *
+grub_queue_pop (grub_queue_t queue)
+{
+  grub_queue_item_t item;
+
+  item = queue->tail;
+  if (queue->tail)
+    queue->tail = item->prev;
+  if (queue->tail)
+    queue->tail->next = NULL;
+  else
+    /* Non-empty queue becomes empty.  */
+    queue->head = NULL;
+
+  return item;
+}
Index: include/grub/queue.h
===================================================================
--- include/grub/queue.h	(revision 0)
+++ include/grub/queue.h	(revision 0)
@@ -0,0 +1,52 @@
+/* queue.h - header for grub queue */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2009  Free Software Foundation, Inc.
+ *
+ *  GRUB is free software: you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  GRUB is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef GRUB_QUEUE_HEADER
+#define GRUB_QUEUE_HEADER 1
+
+#include <grub/symbol.h>
+#include <grub/types.h>
+#include <grub/list.h>		/* GRUB_FIELD_MATCH */
+
+struct grub_queue_item
+{
+  struct grub_queue_item *next, *prev;
+};
+typedef struct grub_queue_item *grub_queue_item_t;
+
+struct grub_queue
+{
+  grub_queue_item_t head, tail;
+};
+typedef struct grub_queue *grub_queue_t;
+
+void EXPORT_FUNC(grub_queue_push) (grub_queue_t queue, grub_queue_item_t item);
+void * EXPORT_FUNC(grub_queue_pop) (grub_queue_t queue);
+
+extern void* grub_assert_fail (void);
+
+#define GRUB_AS_QUEUE_ITEM(ptr) \
+  (GRUB_FIELD_MATCH (ptr, grub_queue_item_t, next) && GRUB_FIELD_MATCH (ptr, grub_queue_item_t, prev) ? \
+   (grub_queue_item_t) ptr : grub_assert_fail ())
+
+#define GRUB_AS_QUEUE_ITEM_P(pptr) \
+  (GRUB_FIELD_MATCH (*pptr, grub_queue_item_t, next) && GRUB_FIELD_MATCH (*pptr, grub_queue_item_t, prev) ? \
+   (grub_queue_item_t *) (void *) pptr : grub_assert_fail ())
+
+#endif /* ! GRUB_QUEUE_HEADER */
Index: term/i386/pc/at_keyboard.c
===================================================================
--- term/i386/pc/at_keyboard.c	(revision 2607)
+++ term/i386/pc/at_keyboard.c	(working copy)
@@ -1,6 +1,6 @@
 /*
  *  GRUB  --  GRand Unified Bootloader
- *  Copyright (C) 2007,2008  Free Software Foundation, Inc.
+ *  Copyright (C) 2007,2008,2009  Free Software Foundation, Inc.
  *
  *  GRUB is free software: you can redistribute it and/or modify
  *  it under the terms of the GNU General Public License as published by
@@ -22,7 +22,52 @@
 #include <grub/i386/io.h>
 #include <grub/misc.h>
 #include <grub/term.h>
+#include <grub/mm.h>
+#include <grub/queue.h>
 
+struct grub_at_keyboard_queue_item
+{
+  struct grub_at_keyboard_queue_item *next, *prev;
+  int key;
+};
+typedef struct grub_at_keyboard_queue_item *grub_at_keyboard_queue_item_t;
+
+static struct grub_queue grub_at_keyboard_queue;
+
+static void
+grub_at_keyboard_queue_push (int key)
+{
+  grub_at_keyboard_queue_item_t item;
+  item = grub_malloc (sizeof (*item));
+  item->key = key;
+  grub_queue_push (&grub_at_keyboard_queue, GRUB_AS_QUEUE_ITEM (item));
+}
+
+static int
+grub_at_keyboard_queue_pop (void)
+{
+  grub_at_keyboard_queue_item_t item;
+  int key;
+  item = grub_queue_pop (&grub_at_keyboard_queue);
+  if (item)
+    {
+      key = item->key;
+      grub_free (item);
+    }
+  else
+    key = -1;
+  return key;
+}
+
+static int
+grub_at_keyboard_queue_last (void)
+{
+  if (grub_at_keyboard_queue.tail)
+    return ((grub_at_keyboard_queue_item_t) grub_at_keyboard_queue.tail)->key;
+  else
+    return -1;
+}
+
 static short at_keyboard_status = 0;
 
 #define KEYBOARD_STATUS_SHIFT_L		(1 << 0)
@@ -146,7 +191,7 @@ grub_keyboard_getkey (void)
 
 /* If there is a character pending, return it; otherwise return -1.  */
 static int
-grub_at_keyboard_checkkey (void)
+grub_at_keyboard_getkey_noblock (void)
 {
   int code, key;
   code = grub_keyboard_getkey ();
@@ -186,10 +231,24 @@ static int
 	      key += 'a' - 'A';
 	  }
     }
-  return (int) key;
+  return key;
 }
 
 static int
+grub_at_keyboard_checkkey (void)
+{
+  int key;
+  key = grub_at_keyboard_getkey_noblock ();
+
+  /* If there was a key, queue it.  */
+  if (key != -1)
+    grub_at_keyboard_queue_push (key);
+
+  /* Return last key from queue.  */
+  return grub_at_keyboard_queue_last ();
+}
+
+static int
 grub_at_keyboard_getkey (void)
 {
   int key;
@@ -197,6 +256,10 @@ grub_at_keyboard_getkey (void)
     {
       key = grub_at_keyboard_checkkey ();
     } while (key == -1);
+
+  /* Remove it from the queue.  */
+  (void) grub_at_keyboard_queue_pop ();
+  
   return key;
 }
 

Re: at_keyboard and checkkey()

[Robert Millan]

I opted for committing a smaller fix.  It's not completely satisfactory, but
it's less intrusive and hence more suitable for the 1.97 release.

After 1.97 is out, this will probably need some restructuring (e.g.
implementing queues and/or an interrupt handler).

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."