Development #11
Development #11
Conversation
WalkthroughThis update introduces a driver management subsystem to the kernel, including a new driver interface, a PS/2 keyboard driver, and registration/initialization logic. The kernel now initializes drivers and enables keyboard interrupts. Fast print functions are made non-inline and exposed via the header. The bootloader and GRUB config are updated for debug output and text mode, respectively. Changes
Sequence Diagram(s)sequenceDiagram
participant Bootloader
participant Kernel
participant DriverManager
participant KeyboardDriver
participant InterruptHandler
Bootloader->>Kernel: Transfer control (after VGA "BOOT" output)
Kernel->>DriverManager: DriverRegister(KeyboardDriver)
Kernel->>DriverManager: DriverInit()
DriverManager->>KeyboardDriver: init()
Kernel->>Hardware: Enable IRQ0 (timer) and IRQ1 (keyboard)
Hardware->>InterruptHandler: Keyboard interrupt (IRQ1)
InterruptHandler->>DriverManager: DriverGet(DRIVER_KEYBOARD)
DriverManager-->>InterruptHandler: KeyboardDriver
InterruptHandler->>KeyboardDriver: handle_interrupt(1)
KeyboardDriver->>InterruptHandler: (process scancode, buffer key)
InterruptHandler->>Hardware: Send EOI
Possibly related PRs
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
Boot/boot.asm (1)
160-166: LGTM! Clean debug output implementation.The VGA text buffer debug output is correctly implemented. The color attribute 0x04 (red foreground) and character positioning are appropriate for early boot debugging.
Consider defining constants for the color attributes to improve readability:
+; Color attributes +VGA_RED equ 0x04 + ; DEBUG: Write 'BOOT' to VGA before calling C code mov rax, 0xb8000 -mov word [rax], 0x0442 ; Red 'B' -mov word [rax+2], 0x044F ; Red 'O' -mov word [rax+4], 0x044F ; Red 'O' -mov word [rax+6], 0x0454 ; Red 'T' +mov word [rax], (VGA_RED << 8) | 'B' +mov word [rax+2], (VGA_RED << 8) | 'O' +mov word [rax+4], (VGA_RED << 8) | 'O' +mov word [rax+6], (VGA_RED << 8) | 'T'Kernel/Drivers/Keyboard.c (1)
17-19: Buffer implementation is correct but lacks overflow protection.The circular buffer implementation is sound, but there's no mechanism to handle or report buffer overflow conditions to the caller.
Consider adding overflow detection:
static char key_buffer[256]; static int buffer_head = 0; static int buffer_tail = 0; +static int buffer_overflows = 0;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
Boot/boot.asm(1 hunks)Kernel/Core/Kernel.c(6 hunks)Kernel/Core/Kernel.h(1 hunks)Kernel/Drivers/Driver.c(1 hunks)Kernel/Drivers/Driver.h(1 hunks)Kernel/Drivers/Interrupts.c(3 hunks)Kernel/Drivers/Keyboard.c(1 hunks)grub.cfg(1 hunks)meson.build(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
Kernel/Drivers/Interrupts.c (1)
Learnt from: assembler-0
PR: assembler-0/VoidFrame#2
File: Kernel/Syscall.c:44-44
Timestamp: 2025-07-08T00:36:50.290Z
Learning: In IdtSetGate function calls, the parameters are: (interrupt_vector, handler_address, selector, flags). The interrupt vector (like 0x80) is the first argument, and the selector (like 0x08 for kernel code segment) is the third argument.
🧬 Code Graph Analysis (2)
Kernel/Drivers/Driver.c (2)
Kernel/Drivers/Driver.h (3)
DriverRegister(26-26)DriverInit(27-27)DriverGet(28-28)Kernel/Core/Panic.h (1)
Panic(7-7)
Kernel/Drivers/Interrupts.c (3)
Kernel/Drivers/Driver.h (1)
DriverGet(28-28)Kernel/Drivers/Driver.c (1)
DriverGet(25-32)Kernel/Drivers/Io.h (2)
outb(6-8)outb(6-6)
🔇 Additional comments (25)
grub.cfg (1)
5-5: LGTM! Proper graphics payload configuration.Setting
gfxpayload=textcorrectly aligns with the VGA text buffer approach used in the bootloader debug output.meson.build (1)
66-67: LGTM! Proper integration of driver subsystem into build.The new driver source files are correctly added to the build configuration with appropriate file paths.
Kernel/Core/Kernel.h (1)
10-12: LGTM! Well-designed function declarations.The new fast print function declarations follow consistent naming conventions and provide explicit positioning parameters for direct screen output.
Kernel/Drivers/Driver.c (4)
4-4: LGTM! Reasonable driver capacity limit.The
MAX_DRIVERSlimit of 16 is appropriate for a kernel subsystem and provides clear bounds checking.
9-15: LGTM! Robust driver registration with proper error handling.The function correctly validates input parameters and array bounds, using panic for critical failures which is appropriate in kernel context.
17-23: LGTM! Safe driver initialization implementation.The function properly iterates through registered drivers and safely checks for the existence of initialization functions before calling them.
25-32: LGTM! Efficient driver retrieval by type.The linear search approach is appropriate for the expected number of drivers, and returning null for not-found cases follows standard kernel conventions.
Kernel/Drivers/Keyboard.c (3)
48-58: Read function implementation is correct.The read function properly handles the circular buffer, respects the size limit, and returns the actual number of bytes read.
61-68: Driver structure is well-implemented.The driver structure correctly implements the interface defined in Driver.h, with appropriate function pointers and a null write function for input-only devices.
71-73: Registration function is correctly implemented.The registration function properly calls DriverRegister with the driver instance.
Kernel/Drivers/Interrupts.c (5)
5-5: Proper integration with the driver subsystem.The inclusion of "Driver.h" correctly integrates the interrupt handler with the new driver management system.
35-35: Function signature change is appropriate.Changing FastDisplayTicks from
static inlinetostaticis correct given its usage pattern and the need to expose it through the header.
70-78: Keyboard interrupt handling is well-implemented.The keyboard interrupt handling correctly:
- Checks for IRQ1 (remapped to interrupt 33)
- Retrieves the keyboard driver using DriverGet
- Validates the driver and handler existence
- Calls the handler with the correct IRQ number
- Sends EOI to the PIC
92-94: Scheduling call remains unchanged.The scheduling call on every timer interrupt maintains system responsiveness despite the reduced tick display frequency.
84-86: Confirm tick display frequency aligns with debugging needsNo code or comments reference the previous 0x3F (64-tick) display rate—only the updated
(tick_count & 0x3FF) == 0remains in Kernel/Drivers/Interrupts.c (lines 84–86).• Kernel/Drivers/Interrupts.c:84–86 – display now every 1024 ticks (0x3FF)
• No other occurrences of “0x3F”, “64 ticks” or related tick-display documentation foundPlease verify that reducing display frequency 16× still meets your debugging and user-experience requirements, and update any external docs or tests accordingly.
Kernel/Drivers/Driver.h (3)
7-13: Driver type enumeration is well-designed.The enumeration provides clear categorization of driver types and includes a DRIVER_MAX sentinel value for bounds checking and iteration.
16-23: Driver interface structure is comprehensive.The Driver struct provides a clean interface with:
- Type identification
- Human-readable name
- Initialization function
- Interrupt handling
- Read/write operations
This covers the essential operations for most device drivers.
26-28: Driver management functions are appropriately declared.The function declarations provide the necessary operations for driver lifecycle management:
- Registration for adding drivers
- Initialization for setup
- Retrieval for access by type
Kernel/Core/Kernel.c (7)
13-13: Proper driver subsystem integration.The inclusion of "Driver.h" correctly integrates the kernel with the new driver management system.
16-17: External function declaration is appropriate.The declaration of KeyboardRegister() as an external function is correct for the modular driver architecture.
131-131: Function signature changes are correct.Converting the fast print functions from
static inlineto regular functions is appropriate given their usage patterns and the need to expose them through the header.Also applies to: 141-141, 147-147
176-176: Task loop reduction improves scheduling responsiveness.Reducing the busy-wait loops from larger values to 100 iterations improves task switching responsiveness, which complements the increased timer frequency.
Also applies to: 183-183, 208-208
227-229: Driver initialization is properly integrated.The driver initialization sequence is correctly placed:
- Register the keyboard driver
- Initialize all registered drivers
This ensures proper driver system setup.
240-241: Interrupt masking update is correct.The interrupt masking correctly enables both timer (IRQ0) and keyboard (IRQ1) interrupts by clearing bits 0 and 1 in the PIC mask register.
235-238: Validate high-frequency PIT timer configurationRunning the Programmable Interval Timer at ~4000 Hz (divisor 299) is well above traditional kernel tick rates (100–1000 Hz) and may introduce:
- Elevated CPU overhead from handling 4000 interrupts/sec
- Increased power draw on battery-powered or low-end systems
- Risk of scheduler or driver instability under high interrupt load
Before merging, please verify that:
- 4 kHz is strictly required for “maximum concurrency” (e.g., link to design doc or benchmark)
- The scheduler and all interrupt handlers remain stable and performant at this rate
- Power and real-world performance impacts have been measured on target hardware
Relevant locations:
- Kernel/Core/Kernel.c (lines ~235–238): PIT setup
- Kernel/Drivers/Interrupts.c: scheduling on every IRQ
| static void KeyboardInterrupt(uint8_t irq) { | ||
| if (irq != 1) return; // IRQ1 = keyboard | ||
|
|
||
| uint8_t scancode = inb(KEYBOARD_DATA_PORT); | ||
|
|
||
| // Only handle key press (not release) | ||
| if (scancode & 0x80) return; | ||
|
|
||
| char key = keymap[scancode]; | ||
| if (key) { | ||
| // Add to buffer | ||
| int next_head = (buffer_head + 1) % 256; | ||
| if (next_head != buffer_tail) { | ||
| key_buffer[buffer_head] = key; | ||
| buffer_head = next_head; | ||
|
|
||
| // Echo key to screen | ||
| FastPrintChar(key, 16, (buffer_head % 80)); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential screen bounds issue in echo functionality.
The echo position calculation (buffer_head % 80) may overwrite other screen content and doesn't account for different screen regions or line wrapping.
Consider implementing proper echo positioning:
// Echo key to screen
- FastPrintChar(key, 16, (buffer_head % 80));
+ static int echo_col = 0;
+ FastPrintChar(key, 16, echo_col);
+ echo_col = (echo_col + 1) % 80;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static void KeyboardInterrupt(uint8_t irq) { | |
| if (irq != 1) return; // IRQ1 = keyboard | |
| uint8_t scancode = inb(KEYBOARD_DATA_PORT); | |
| // Only handle key press (not release) | |
| if (scancode & 0x80) return; | |
| char key = keymap[scancode]; | |
| if (key) { | |
| // Add to buffer | |
| int next_head = (buffer_head + 1) % 256; | |
| if (next_head != buffer_tail) { | |
| key_buffer[buffer_head] = key; | |
| buffer_head = next_head; | |
| // Echo key to screen | |
| FastPrintChar(key, 16, (buffer_head % 80)); | |
| } | |
| } | |
| } | |
| static void KeyboardInterrupt(uint8_t irq) { | |
| if (irq != 1) return; // IRQ1 = keyboard | |
| uint8_t scancode = inb(KEYBOARD_DATA_PORT); | |
| // Only handle key press (not release) | |
| if (scancode & 0x80) return; | |
| char key = keymap[scancode]; | |
| if (key) { | |
| // Add to buffer | |
| int next_head = (buffer_head + 1) % 256; | |
| if (next_head != buffer_tail) { | |
| key_buffer[buffer_head] = key; | |
| buffer_head = next_head; | |
| // Echo key to screen | |
| static int echo_col = 0; | |
| FastPrintChar(key, 16, echo_col); | |
| echo_col = (echo_col + 1) % 80; | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In Kernel/Drivers/Keyboard.c between lines 26 and 46, the echo position for
printing the key uses (buffer_head % 80), which can cause overwriting and does
not handle line wrapping or screen regions. To fix this, implement a proper
cursor tracking mechanism that maintains current row and column positions,
updates them correctly on each character printed, and wraps to the next line
when reaching the screen width, ensuring the echo respects screen boundaries and
avoids overwriting unrelated content.
| static char keymap[128] = { | ||
| 0, 27, '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '-', '=', '\b', | ||
| '\t', 'q', 'w', 'e', 'r', 't', 'y', 'u', 'i', 'o', 'p', '[', ']', '\n', | ||
| 0, 'a', 's', 'd', 'f', 'g', 'h', 'j', 'k', 'l', ';', '\'', '`', | ||
| 0, '\\', 'z', 'x', 'c', 'v', 'b', 'n', 'm', ',', '.', '/', 0, | ||
| '*', 0, ' ' | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Incomplete keymap may cause issues with extended keys.
The keymap only covers basic keys up to index 57 (space) but the array is sized for 128 entries. Extended keys, function keys, and other special keys will not be handled properly.
Consider extending the keymap to include commonly used keys:
static char keymap[128] = {
0, 27, '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '-', '=', '\b',
'\t', 'q', 'w', 'e', 'r', 't', 'y', 'u', 'i', 'o', 'p', '[', ']', '\n',
0, 'a', 's', 'd', 'f', 'g', 'h', 'j', 'k', 'l', ';', '\'', '`',
0, '\\', 'z', 'x', 'c', 'v', 'b', 'n', 'm', ',', '.', '/', 0,
- '*', 0, ' '
+ '*', 0, ' ', 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, // F1-F10 (scancodes 59-68)
+ 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, // Additional keys
+ // ... continue for remaining entries
};Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In Kernel/Drivers/Keyboard.c around lines 9 to 15, the keymap array is sized for
128 entries but only partially initialized, missing extended keys, function
keys, and special keys. To fix this, extend the keymap array to include mappings
for all 128 possible scan codes, covering function keys (F1-F12), arrow keys,
and other special keys, ensuring proper handling of all keyboard inputs.
1 participant
Add a bit faster timer and now you can type in the termial
Summary by CodeRabbit
New Features
Improvements
Bug Fixes