Viridis-0.5-Keyboard -- 12/21/03

[stonedzealot]

No disk image, because in reality, I think I know it works (very limited capability) but I'm more interested in the validity of the comments inside the source .gz and the quality of explanation within these comments.

Anyway, give it a download http://sourceforge.net/project/showfiles.php?group_id=91080 and tell me what you think of the code and comments, if you will.

[libber]

Nice work, your printf is really nice. a few things:

1) the .tar.gz itself seems to be just a tar archive (tar -cvf) instead of tar.gz, this could easily be my own fault and i accidentially renamed it.

2) in include/consts.h you define WORD as shortint, but you don't define shortint anywhere, mabey you meant just short?

3) for your outportb function you might want to make the port variable 16 bits instead of a byte because you can have addresses like 0x3d5 and such which would not fit in a byte.

4) in io/i386/io.asm:24 the comment reads "remember that even though the c function has two bytes as arguments, the size of all arguments pushed to the stack is integer sized (8 bytes, dword for the i386)." which is incorrect, the 8 bytes should be 4 bytes.

anyhow good work, i am making an OS that is well commented just like yours and we are at about the same stage so it is neat to see how you did things compared to me.

[Therx]

Code looks nice. Keep up the work. Pete

BTW signed, unsigned, short and long are all modifiers and if you omit the type it defaults to int. eg
signed -> signed int
unsigned -> unsigned int
short -> short int
long -> long int
unsigned long -> unsigned long int

[Tim]

I think your boolean algebra in mask_irq is wrong:

Code: Select all

/*
???Just a note, 2^0 (for example) = 00000001, 2^1 = 00000010, so you can see where
???the 2 ^ (interrupt - 8) and 2 ^ (interrupt) statements come from.
*/

Yes, these two are true. But what if you try to mask interrupt 2? 2^2 = 0, so no interrupt will be affected. Or interrupt 3? 2^3 = 1, so interrupt 1 will be affected, not 3.

I think you want:

Code: Select all

void mask_irq(int interrupt)
{
    if (interrupt == 16)????????????//Code to unmask all
    {
        slave_pic = 0;
        master_pic = 0;???
    }
    else if((interrupt > 7) && (interrupt < 16))???//If it's an interrupt handled by slave PIC
        slave_pic |= 1 << (interrupt - 8);
    else if((interrupt > -1) && (interrupt < 8))   //If it's an interrupt handled by master PIC
        master_pic |= 1 << interrupt;
    outportb(0x21, master_pic);
    outportb(0xA1, slave_pic);
}

Same for unmask_irq. This technique is really common in C: to go from a bit index N to a mask with only bit N set, use mask = 1 << N.

--------

Does kbd.c compile? Cos it's not valid C -- at least, not valid C89. You're declaring map[] and shiftmap[] in the middle of keymap_us, which is a C++/C99 feature. In any case, the compiler is probably generating code which reinitialises these two arrays every time the keymap_us function is called.

What I'd suggest you do is move these two declarations outside of the function and have them global, or if you don't like too many global variables, move them to the top of the functions and declare them static, which will generate the same code. You could make them const while you're at it, because they presumably never need to be modified. The compiler will put the definitions for these two variables into the kernel itself, instead of generating code to regenerate these variables each time keymap_us is entered.

[Tim]

I got Viridis to compile on Cygwin! OK, I cheated and used the NewOS version of gcc/ld (available from http://newos.sourceforge.net/), but at least it works.

Here are the commands I used:

Code: Select all

Tim Robinson@tim /ebin/Operating Systems/Viridis-0.5-Keyboard
$ make kernel.bin
nasm -f elf init/i386/kstart.asm -o kstart.o
/usr/i386-newos/bin/i386-newos-gcc -ffreestanding -nostdlib -fno-leading-underscore -Wall -Iinclude -c kernel/kernel.c
/usr/i386-newos/bin/i386-newos-gcc -ffreestanding -nostdlib -fno-leading-underscore -Wall -Iinclude -c drivers/video.c
/usr/i386-newos/bin/i386-newos-gcc -ffreestanding -nostdlib -fno-leading-underscore -Wall -Iinclude -c math/convert.c
nasm -f elf init/i386/idt.asm -o idt.o
/usr/i386-newos/bin/i386-newos-gcc -ffreestanding -nostdlib -fno-leading-underscore -Wall -Iinclude -c kernel/interrupt.c
nasm -f elf io/i386/io.asm -o io.o
/usr/i386-newos/bin/i386-newos-gcc -ffreestanding -nostdlib -fno-leading-underscore -Wall -Iinclude -c kernel/irq.c
/usr/i386-newos/bin/i386-newos-gcc -ffreestanding -nostdlib -fno-leading-underscore -Wall -Iinclude -c drivers/kbd.c
nasm -f elf drivers/kbd.asm -o kbdasm.o
/usr/i386-newos/bin/i386-newos-ld -T linker.ld -o kernel.bin kstart.o kernel.o video.o convert.o idt.o interrupt.o io.o irq.o kbd.o
kbdasm.o

Tim Robinson@tim /ebin/Operating Systems/Viridis-0.5-Keyboard
$ make boot.bin
nasm -f bin init/i386/boot.asm -o boot.bin

Attached is the disk image I created. After you download it, you'll need to rename it from .img.gz.png to just .img.gz. The board doesn't like .gz files . NOTE: THIS FILE IS NOT ACTUALLY A PICTURE.

Edit: Just remembered, I had to modify the linker.ld script to the file below, otherwise you end up with a > 1MB file which starts with strings, not code. And I changed the NASM output format from aout to elf. Not surprisingly, the version of ld I used doesn't support a.out any more.

Code: Select all

OUTPUT_FORMAT("binary")
ENTRY(start)
SECTIONS
{
  .text 0x100000 : {
    *(.text)
    *(.rodata)
  }
  .data  : {
    *(.data)
  }
  .bss  :
  {
    *(.bss)
  }
}

[attachment deleted by admin]

[Pype.Clicker]

http://www.mega-tokyo.com/forum/index.p ... eadid=5277

here comes a post you might wish to read.

[Ytinasni]

Tim's comment above "I think your boolean algebra in mask_irq is wrong"
stems from the fact that ^ in C is the XOR operator. Not exponents, as you seem to be using them as.

His solution "to go from a bit index N to a mask with only bit N set, use mask = 1 << N"
will work, i just thought explaining WHY your code is wrong would help you
a lot more than just giving you a fix for something you dont understand properly.

Hopefully, this will stop you from making the same mistake twice

[stonedzealot]

Thanks alot everyone. I've been pretty busy doing service lately (behaviorally disturbed kids are exhausting, take my word for it). I'll make these changes! Thanks alot.

As for ^ being XOR...whoa. That's pretty weird. The first language I learned was Pascal (in the form of Delphi) and I just sorta...well...woops.

Perhaps these errors are the source of those randoms failings I can only get on two/five test machines! I can only pray.

EDIT: Because updating so many packages is a pain, I'm going to have an informal rerelease of Keyboard while I update all of the other packages with valid/working information.

ANOTHER EDIT: The files are out, took much less time than expected (for a change).

[stonedzealot]

One last note about it: The problem that was causing it to screw up on certain computers has been fixed and suffice it to say that I now have renewed respect for Intel chipset computers. This is why:

As far as I can tell, when I accidentally told the Master PIC and the Slave PIC two different numbers for ICW3 in the PIC initialization code, the Intel chipset said: "Hey...these numbers are supposed to be identical" and changed them to be so. Other chipsets had no such nice sanity checks and just decided to fail on me. Anyway....it's fixed and the new release is out on the site.

For the people that don't want to spend the massive bandwidth downloading 22k files: just change "outportb(0x21, 0x03)" to "outportb(0x20, 0x04)" in the kernel/irq.c code.