Re: [OpenBIOS] [PATCH] get-key-map implementation

[Programmingkid]

On Dec 20, 2012, at 8:40 PM, Amadeusz Sławiński wrote:
> On Thu, Dec 13, 2012 at 03:57:54PM -0500, Programmingkid wrote:
> > This patch is an improvement upon the last get-key-map patch. This patch makes the code more 64bit friendly. It also fixes some whitespace damage.
> >
> > The get-key-map word is used by bootx to see if any keys are being held down.
> >
> > signed-off-by: John Arbuckle
>
> This patch caused some issues while I was doing some tests. It seemed to
> go in infinte loop.
>
> Anyway, do we even need to remap control as command?
> From what I tested it boots fine in verbose when I pass it '-v' in boot-args
> -boot order=d -prom-env 'boot-args=-v'
> and I'm pretty sure other arguments can also be passed this way.
>
> Still because we need get-key-map, some comments inline.
> > Index: drivers/adb_kbd.c
> >
> > --- drivers/adb_kbd.c	(revision 1078)
> > +++ drivers/adb_kbd.c	(working copy)
> > @@ -24,7 +24,7 @@
> > #include "libc/byteorder.h"
> > #include "libc/vsprintf.h"
> > #include "kbd.h"
> >
> > +#include <math.h>
> > #include "adb_bus.h"
> > #include "adb_kbd.h"
> >
> > @@ -42,11 +42,13 @@
> > }
> >
> > static void keyboard_read(void);
> > +static void getKeyMap(void);
> >
> > NODE_METHODS( keyboard ) = {
> >    { "open",		keyboard_open		},
> >    { "close",		keyboard_close		},
> >    { "read",               keyboard_read		},
> > { "get-key-map",        getKeyMap		},
> >
> > };
> >
> > /* VT100 escape sequences */
> > @@ -566,3 +568,65 @@
> >    }
> >    PUSH(i);
> > }
> >
> > +// The implementation of the get-key-map word.
> > +// Returns an array of 32 bytes. Certain bits 
> > +// are used to determine which keys are being
> > +// held down.
> >
> > +static void getKeyMap(void)
> > +{
> > #define sizeOfArray 8   // 8 bytes (32 bits) in size
> > uint32_t keyPushed, *keyMapArray, offset;
> > const uint32_t modifierKeyIndex = 7;
> > const uint32_t commandKeyValue = pow(2,28);
> > const uint32_t shiftKeyValue = pow(2,30);
> > const uint32_t theAKeyValue = pow(2,27);
>
> pow() needs another header, can't you just give it numbers it wants 
> 0x10000000;
> 0x40000000;
> 0x08000000;
> or at least
> 1<<28;
> 1<<30;
> 1<<27;
> both seem clearer to me seeing as it is encoded value
> > keyMapArray = (uint32_t *) malloc(sizeOfArray);
>
> malloc() ... is it freed anywhere?
> I'm pretty sure it just eats memory on each call, there may be not much
> calls, but still
> > if(!keyMapArray)        // if failed to allocate memory
> > {
> >   printk("Failed to allocate memory for keyMapArray!\n");
> >   PUSH(0);
> >   return;
> > }
> > // Set all the elements to zero
> > int index;
> > for(index = 0; index < sizeOfArray; index++)
> > {
> >  keyMapArray[index] = 0;
> > }
> > feval("key?");
> > if(POP() != -1)         // if no key was pushed
> > {
> >   PUSH((uint32_t)keyMapArray);  // returns the address of keyMapArray
> >   return;
> > }
> > feval("key");
> > keyPushed = POP();
> > if(keyPushed > 0 && keyPushed < 27) // control key combination
> > {
> >  offset = 1;
> >  keyMapArray[modifierKeyIndex] = commandKeyValue;
> > }
>
> I think that pretending that some keys are others is really bad idea
> and when you boot emulated OS you will be still missing key, it should
> just be fixed in emulator.
> Also I haven't tested this but there is chance that sdl backend works
> properly (ie it uses meta keys which from what I read are mapped to
> command on Macs).
> > else if(keyPushed > 64 && keyPushed < 91)    // shift key combination
> > {
> >  offset = 65;
> >  keyMapArray[modifierKeyIndex] = shiftKeyValue;
> > }
> > else     // just a letter is being held down
> > {
> >  offset = 97;
> > }
> > // Determines the value and location of a bit in the array.
> > *keyMapArray = theAKeyValue >> (keyPushed - offset);
> > PUSH((uint32_t) keyMapArray);
> >
> > +}

Pretty much every body hates my get-key-map patch. I think I will just give up on it. After some recent test, I'm not even sure we need a get-key-map word. Still, it would be nice to have.

I also want the command key sent to the emulator, but it doesn't look like any one is going to step forward and make this happen. :(

Let me know if the SDL backend works better than with the cocoa option.