Page 1 of 1
Viridis-0.5-Keyboard -- 12/21/03
Posted: Sun Dec 21, 2003 1:07 pm
by 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.
Re:Viridis-0.5-Keyboard -- 12/21/03
Posted: Sat Jan 03, 2004 3:05 pm
by 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.
Re:Viridis-0.5-Keyboard -- 12/21/03
Posted: Sun Jan 04, 2004 1:10 pm
by 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
Re:Viridis-0.5-Keyboard -- 12/21/03
Posted: Tue Jan 06, 2004 1:17 pm
by 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.
Re:Viridis-0.5-Keyboard -- 12/21/03
Posted: Tue Jan 06, 2004 1:50 pm
by 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]
Re:Viridis-0.5-Keyboard -- 12/21/03
Posted: Mon Jan 12, 2004 2:44 am
by Pype.Clicker
Re:Viridis-0.5-Keyboard -- 12/21/03
Posted: Mon Jan 12, 2004 6:52 pm
by 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
Re:Viridis-0.5-Keyboard -- 12/21/03
Posted: Fri Jan 16, 2004 11:56 pm
by 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).
Re:Viridis-0.5-Keyboard -- 12/21/03
Posted: Sat Jan 24, 2004 2:48 pm
by 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.