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

[Amadeusz Sławiński]

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);
>
> +}