Development #49
Development #49
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis update introduces a new serial driver for COM1, integrates serial output into kernel panic and console printing routines, and adds a Changes
Sequence Diagram(s)sequenceDiagram
participant Kernel
participant SerialDriver
participant Console
participant PanicHandler
Kernel->>SerialDriver: SerialInit()
Console->>SerialDriver: SerialWrite(str)
PanicHandler->>SerialDriver: SerialWrite(panic_message)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Possibly related PRs
Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
✨ Finishing Touches
🧪 Generate unit tests
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. 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: 7
🔭 Outside diff range comments (1)
kernel/core/Panic.c (1)
141-144: Avoid UTF-8 glyphs in VGA text mode; use code page 437 (0xDB) for '█'"█" is multi-byte UTF-8; your PanicPrint will emit raw bytes that won’t map to CP437, producing garbage on screen.
Use CP437 value 0xDB:
- PanicPrint(bar_x + j, current_y, "█", COLOR_YELLOW_ON_RED); + char block[2] = { (char)0xDB, '\0' }; // CP437 '█' + PanicPrint(bar_x + j, current_y, block, COLOR_YELLOW_ON_RED);
🧹 Nitpick comments (10)
kernel/core/Panic.c (1)
138-148: Condition is always true; clarify intent for progress barsThe comment implies skipping a bar for a final status, but
i < num_stepsis always true within the loop.If you intend to skip the last iteration’s bar, use
i < num_steps - 1; otherwise, update the comment to match current behavior.- // Don't draw a progress bar for the final "halt reached" message. - if (i < num_steps) { + // Draw a progress bar for every listed step. + if (i < num_steps) {meson.build (1)
75-86: Build inclusion looks correct; watch for command array flattening and tool discovery
- Adding Console.c and drivers/Serial.c is correct. Order in object lists generally doesn’t affect linking with object files, so you’re fine.
- Meson flattens nested arrays in
command, but usingmeson.get_compiler('c')andcc.cmd_array()is more portable than hardcoding 'clang'.If portability matters, consider:
- Replace
'clang'withmeson.get_compiler('c')andcc.get_id()/cc.get_exelist().- Use
compiler_args = cc.get_supported_arguments(c_flags)to filter flags per toolchain.drivers/Serial.h (1)
1-8: Add C++ linkage guards for compatibilityIf this header is ever used from C++ (tests, utilities), expose C linkage.
#ifndef VOIDFRAME_SERIAL_H #define VOIDFRAME_SERIAL_H +#ifdef __cplusplus +extern "C" { +#endif void SerialInit(void); void SerialPutChar(char c); void SerialWrite(const char* str); +#ifdef __cplusplus +} +#endif #endif // VOIDFRAME_SERIAL_Hkernel/etc/Console.c (1)
40-41: Stray semicolonMinor cleanup.
- SpinUnlock(&lock);; + SpinUnlock(&lock);arch/x86_64/interrupts/Interrupts.c (1)
49-56: Normalize PANIC message: drop leading space and embedded newlineKeep panic messages consistent with other cases and let the panic handler manage newlines.
- PANIC_CODE(" General Protection Fault\n", regs->error_code); + PANIC_CODE("General Protection Fault", regs->error_code);kernel/core/Kernel.c (2)
209-212: Initialize serial earlier to capture early boot logsSerialInit() this late means all prior PrintKernel output won’t mirror to serial. Initialize as early as possible (e.g., at the beginning of KernelMainHigherHalf, before ParseMultibootInfo/VMemInit), since COM1 I/O ports don’t require paging and you’re not enabling serial IRQs anyway.
I can propose exact placement changes if you confirm where you prefer serial logs to start (very early in KernelMain vs. early in KernelMainHigherHalf).
209-212: Minor wording: “driver management” → “driver”Log reads oddly. Recommend shortening.
- PrintKernel("[INFO] Initializing serial driver management...\n"); + PrintKernel("[INFO] Initializing serial driver...\n");drivers/Serial.c (3)
15-17: Scope helper and/or reuse it to avoid duplicate LSR polling codeis_transmit_empty is unused and globally visible. Either make it static and reuse in SerialWriteChar, or remove it.
-int is_transmit_empty(void) { - return inb(COM1 + 5) & 0x20; -} +static inline int serial_tx_empty(void) { + return inb(COM1 + 5) & 0x20; +}And in SerialWriteChar:
- while ((inb(COM1 + 5) & 0x20) == 0) {} + while (!serial_tx_empty()) {}
6-14: Optional: detect port presence during initA quick loopback or LSR read check can avoid hanging on non-existent COM1 ports. Not required if you control QEMU/Bochs config, but it improves robustness.
I can add a lightweight presence check (e.g., use the scratch register at COM1+7) if desired.
30-34: Guard against null pointers in SerialWriteTiny safety net for callers.
-void SerialWrite(const char* str) { - for (int i = 0; str[i] != '\0'; i++) { +void SerialWrite(const char* str) { + if (!str) return; + for (int i = 0; str[i] != '\0'; i++) { SerialWriteChar(str[i]); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.clang-format(1 hunks)arch/x86_64/interrupts/Interrupts.c(1 hunks)drivers/Serial.c(1 hunks)drivers/Serial.h(1 hunks)grub.cfg(0 hunks)kernel/core/Kernel.c(7 hunks)kernel/core/Panic.c(2 hunks)kernel/etc/Console.c(4 hunks)meson.build(1 hunks)
💤 Files with no reviewable changes (1)
- grub.cfg
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-16T01:04:23.790Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#47
File: kernel/core/Kernel.c:434-434
Timestamp: 2025-07-16T01:04:23.790Z
Learning: VMemAlloc() in VoidFrame kernel fails with error code -2 (VMEM_ERROR_INVALID_ADDR) due to virtual address space layout conflict where the heap start address (_bss_end + KERNEL_VIRTUAL_OFFSET + 1MB margin) exceeds VIRT_ADDR_SPACE_END (0xFFFFFFFF80000000ULL), causing VMemMap validation to fail.
Applied to files:
kernel/core/Kernel.carch/x86_64/interrupts/Interrupts.c
🔇 Additional comments (3)
.clang-format (1)
1-58: Verify clang-format keys & C-language coveragePlease confirm your installed clang-format version supports all of these keys (unsupported ones are ignored without warning) and decide if you need identical rules for C sources:
- Run locally:
to catch any unsupported options (e.g. InsertNewlineAtEOF, AfterCaseLabel).clang-format --version clang-format -style=file -dump-config >/dev/null 2>&1 || echo "Config error"- Your
.clang-formatblock targetsLanguage: Cpp; if you also want these rules for C files, either:
- Duplicate the block under
Language: C- Or confirm your clang-format treats C as Cpp in your version
kernel/core/Panic.c (1)
3-3: Ensure serial is safe to use in panic pathsIncluding Serial.h is fine, but verify that SerialInit has run on all code paths that can panic; otherwise add a best-effort init here or make SerialWrite resilient if uninitialized.
Do you guarantee CoreInit (or equivalent) runs before any possible panic? If not, consider calling SerialInit here (idempotent) or guarding SerialWrite.
kernel/etc/Console.c (1)
2-3: Include order change is fine; verify header availabilityIncluding Serial.h here is appropriate. Ensure drivers is in your include path (it is in meson inc_dirs).
| #include "Serial.h" | ||
| #include "Io.h" | ||
|
|
||
| #define COM1 0x3f8 // COM1 | ||
|
|
||
| void SerialInit(void) { | ||
| outb(COM1 + 1, 0x00); // Disable all interrupts | ||
| outb(COM1 + 3, 0x80); // Enable DLAB (set baud rate divisor) | ||
| outb(COM1 + 0, 0x03); // Set divisor to 3 (lo byte) -> 38400 baud | ||
| outb(COM1 + 1, 0x00); // (hi byte) | ||
| outb(COM1 + 3, 0x03); // 8 bits, no parity, one stop bit | ||
| outb(COM1 + 2, 0x01); // Enable FIFO with 1-byte threshold | ||
| outb(COM1 + 4, 0x0B); // IRQs enabled, RTS/DSR set | ||
| } | ||
| int is_transmit_empty(void) { | ||
| return inb(COM1 + 5) & 0x20; | ||
| } | ||
|
|
||
| void SerialWriteChar(const char a) { | ||
| while ((inb(COM1 + 5) & 0x20) == 0) {} | ||
| outb(COM1, a); | ||
|
|
||
| // If it's a newline, also send carriage return | ||
| if (a == '\n') { | ||
| while ((inb(COM1 + 5) & 0x20) == 0) {} | ||
| outb(COM1, '\r'); | ||
| } | ||
| } | ||
|
|
||
| void SerialWrite(const char* str) { | ||
| for (int i = 0; str[i] != '\0'; i++) { | ||
| SerialWriteChar(str[i]); | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify header function names match (SerialWriteChar vs SerialPutChar)
The AI summary mentions SerialPutChar in Serial.h, but this file implements SerialWriteChar. Ensure prototypes and callsites match to avoid link errors.
Run to verify across the repo:
🏁 Script executed:
#!/bin/bash
# Search for put-char like symbols and declarations
rg -n "SerialPutChar|SerialWriteChar|SerialWrite\(" -A2 -B2
# Locate Serial.h and inspect declarations
fd -a "Serial.h" | xargs -I{} sh -c 'echo "----- {} -----"; sed -n "1,200p" "{}"'Length of output: 2134
Align header and implementation names for single-char output
The header declares SerialPutChar, but the C file implements SerialWriteChar. This mismatch will lead to link errors and hidden API gaps. Please pick one of the options below and apply it consistently across the driver and all callsites:
• Option A: Rename the implementation and internal calls
In drivers/Serial.c
- void SerialWriteChar(const char a) {
+ void SerialPutChar(char c) {
while ((inb(COM1 + 5) & 0x20) == 0) {}
outb(COM1, c);
if (c == '\n') {
while ((inb(COM1 + 5) & 0x20) == 0) {}
outb(COM1, '\r');
}
}And update any SerialWriteChar(…) invocations to SerialPutChar(…).
• Option B: Change the header to match
In drivers/Serial.h
- void SerialPutChar(char c);
+ void SerialWriteChar(char c);Ensure that whichever name you choose is declared in Serial.h, defined in Serial.c, and used uniformly in all modules.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In drivers/Serial.c lines 1 to 34, the function SerialWriteChar is implemented
but the header declares SerialPutChar, causing a mismatch. To fix this, choose
either to rename SerialWriteChar to SerialPutChar in Serial.c and update all
calls accordingly, or change the declaration in Serial.h to SerialWriteChar.
Ensure the chosen function name is consistent in the header, implementation, and
all callsites.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Other