Development #109
Development #109
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds an in-kernel window compositor and window model, moves console font data into a C object, introduces console Snooze/Unsnooze routing, extends PS/2 input hooks and accessors, integrates the compositor into boot/runtime, and updates bootstrap (identity map, earlier higher‑half switch) and build flags. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Boot as Kernel Boot (PXS)
participant Console as Console
participant Compositor as VFCompositor / WindowManager
participant VBE as VBE / Framebuffer
rect rgb(250,250,255)
Boot->>Console: If VF_CONFIG_SNOOZE_ON_BOOT -> Snooze()
Boot->>Boot: PXS1: allocate identity PML4, identity-map range, map kernel/stack, SwitchToHigherHalf()
Boot->>Boot: PXS2: VMem/Heap/Pools, GDT/IDT, devices, PrintBootstrapSummary
Boot->>Compositor: If VBE initialized -> WindowManagerInit()
end
rect rgb(240,255,240)
Boot->>Console: If VF_CONFIG_SNOOZE_ON_BOOT -> Unsnooze()
Boot->>Boot: sti() (enable interrupts)
end
sequenceDiagram
autonumber
participant HW as PS2 IRQ
participant PS2 as drivers/PS2
participant Compositor as WindowManager
participant Scheduler as MLFQ
participant VBE as VBE/Framebuffer
HW->>PS2: Keyboard/Mouse packet
PS2->>PS2: parse
alt key != 0
PS2->>Compositor: OnKeyPress(char)
PS2->>Scheduler: enqueue char
end
PS2->>Compositor: OnMouseMove(x,y,dx,dy)
PS2->>Compositor: OnMouseButtonDown/Up(...) on change
Compositor->>Compositor: mark redraw / move windows
Compositor->>VBE: CompositeAndDraw() -> memcpy to framebuffer
note right of PS2: If console snoozed, PrintKernel routes to VFShell/Serial
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (2)
✨ 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/Issue comments)Type 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
kernel/etc/Console.c (2)
166-184: Lock imbalance when snoozed: SpinUnlock called without prior SpinLock.if (snooze) goto serial; jumps past the lock acquisition, but still calls SpinUnlock(&lock). This is unsafe and can deadlock/corrupt the lock state.
Apply this fix to early-return on snooze and avoid unlocking an unlocked lock:
void PrintKernel(const char* str) { if (!str) return; - if (snooze) goto serial; + if (snooze) { + SerialWrite(str); + SystemLog(str); + return; + } SpinLock(&lock); if (use_vbe) { VBEConsolePrint(str); } else { const uint8_t original_color = console.color; for (const char* p = str; *p; p++) { ConsolePutchar(*p); } console.color = original_color; } -serial: SpinUnlock(&lock); - SerialWrite(str); - SystemLog(str); + SerialWrite(str); + SystemLog(str); }
325-333: Honor snooze in PrintKernelAt to keep the screen quiet.Right now it always writes to serial and still draws on-screen. Return early when snoozed.
void PrintKernelAt(const char* str, uint32_t line, uint32_t col) { if (!str) return; - SerialWrite(str); - SerialWrite("\n"); - // if (snooze) return; + if (snooze) { + SerialWrite(str); + SerialWrite("\n"); + SystemLog(str); + return; + } if (use_vbe) { VBEConsoleSetCursor(col, line); VBEConsolePrint(str);
🧹 Nitpick comments (4)
meson.build (1)
243-244: Make KVM accel portable/fallback-safe.-hardcoding -enable-kvm fails on hosts without KVM (CI, macOS). Prefer an accel with fallback.
Apply one of these:
Option A (portable fallback to TCG):
- '-enable-kvm', + '-accel', 'kvm:tcg',Option B (env-gated):
- '-enable-kvm', + ${VF_QEMU_KVM:-'-accel kvm:tcg'},kernel/etc/Console.h (1)
31-33: STATUS_LABEL_ROW exceeds VGA text height. Clarify scope.VGA_HEIGHT is 25; STATUS_LABEL_ROW is 29. If used in VGA mode, PrintKernelAt will early-return and nothing renders. Add a comment that these coords are VBE-only or clamp at callsites.
-#define STATUS_LABEL_ROW 29 +#define STATUS_LABEL_ROW 29 // VBE-only; out of range for VGA text (25 rows)kernel/etc/Console.c (2)
15-16: Ensure snooze flag visibility across CPUs/ISRs.Mark as volatile to avoid compiler reordering. For stronger guarantees you could later switch to atomic load/store, but volatile is sufficient here.
-static uint8_t snooze = 0; +static volatile uint8_t snooze = 0;
166-184: Avoid double-emitting to serial unless intentional.PrintKernel now always writes to serial even when not snoozed. If the intent was “route to serial when snoozed,” consider gating SerialWrite with snooze or a config macro to prevent duplicate output.
- SpinUnlock(&lock); - SerialWrite(str); - SystemLog(str); + SpinUnlock(&lock); + SystemLog(str); + #ifdef VF_CONFIG_LOG_TO_SERIAL_ALWAYS + SerialWrite(str); + #endif
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
kernel/core/Kernel.c(2 hunks)kernel/etc/Console.c(6 hunks)kernel/etc/Console.h(2 hunks)kernel/sched/MLFQ.c(1 hunks)meson.build(2 hunks)
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-08-13T03:11:50.346Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#77
File: drivers/USB/xHCI.c:117-122
Timestamp: 2025-08-13T03:11:50.346Z
Learning: In VoidFrame's DynamoX frequency controller (kernel/process/Process.c), the ENHANCED_HZ_PER_PROCESS parameter controls per-process frequency scaling. Reducing it from 80Hz to 55Hz per process achieves approximately 500Hz idle frequency instead of 700Hz+ while maintaining the same scaling logic.
Applied to files:
kernel/sched/MLFQ.c
📚 Learning: 2025-08-23T10:06:02.997Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#95
File: kernel/core/Panic.c:65-67
Timestamp: 2025-08-23T10:06:02.997Z
Learning: In VoidFrame kernel's panic handler, the VBE graphics path intentionally mixes VBEShowPanic() with console text operations (PrintKernel*, PrintKernelAt) to overlay stack traces and debugging information on top of the panic image. This overlap is deliberate to ensure critical diagnostic information remains visible during kernel panics, prioritizing debugging capability over visual presentation.
Applied to files:
kernel/core/Kernel.ckernel/etc/Console.c
📚 Learning: 2025-08-10T06:03:22.076Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#62
File: kernel/core/Panic.c:69-79
Timestamp: 2025-08-10T06:03:22.076Z
Learning: In VoidFrame kernel, after VBEShowPanic() draws a panic image to the framebuffer, continuing to use text console operations like PrintKernelAt() causes display corruption because it tries to render text on top of the raw pixel data. The panic handler must choose either pure VBE graphics mode (show image only) or pure text console mode, but not mix both.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-10T06:22:28.816Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#62
File: kernel/core/Panic.c:69-79
Timestamp: 2025-08-10T06:22:28.816Z
Learning: In VoidFrame kernel, panic functions must call "asm volatile("cli")" as the very first instruction to disable interrupts immediately. Without this, if interrupts are enabled (after "sti"), an interrupt can fire during panic handling causing the panic to fail, hang, or behave unpredictably. This must be done in ALL panic entry points: Panic(), PanicWithCode(), PanicWithContext(), PanicFromInterrupt(), and at the start of KernelPanicHandler().
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-10T06:22:28.816Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#62
File: kernel/core/Panic.c:69-79
Timestamp: 2025-08-10T06:22:28.816Z
Learning: In VoidFrame kernel, panic functions must disable interrupts with "cli" as the very first operation, before any other code executes. If interrupts are enabled (after "sti"), an interrupt can fire between the panic call and the cli instruction in KernelPanicHandler, causing the panic to fail or behave unpredictably.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-10T02:19:46.948Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#61
File: include/Font.h:36-46
Timestamp: 2025-08-10T02:19:46.948Z
Learning: In VoidFrame kernel, the PS/2 keyboard controller needs proper initialization including buffer flushing and error handling. Without flushing the controller buffer in KeyboardInit() and handling overflow errors (status bits 6-7) in KeyboardHandler(), the keyboard can stop working if keys are pressed rapidly during boot due to controller buffer overflow.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-10T06:03:22.076Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#62
File: kernel/core/Panic.c:69-79
Timestamp: 2025-08-10T06:03:22.076Z
Learning: In VoidFrame kernel, the console color state (current_color) needs to be explicitly preserved and restored when switching between display modes or clearing the screen. ConsoleSetColor() should update both text mode and VBE mode color states when vbe_enabled is true.
Applied to files:
kernel/etc/Console.h
🧬 Code graph analysis (4)
kernel/sched/MLFQ.c (1)
kernel/etc/Console.c (1)
PrintKernelSuccess(198-203)
kernel/core/Kernel.c (1)
kernel/etc/Console.c (3)
Snooze(39-42)Unsnooze(44-46)ClearScreen(72-95)
kernel/etc/Console.h (1)
kernel/etc/Console.c (2)
Snooze(39-42)Unsnooze(44-46)
kernel/etc/Console.c (1)
kernel/sched/MLFQ.c (1)
MLFQGetCurrentProcess(1233-1238)
🔇 Additional comments (5)
meson.build (1)
153-155: Config flag wiring looks good.The build-time VF_CONFIG_SNOOZE_ON_BOOT define is correctly propagated via cfg_flags.
kernel/etc/Console.h (1)
66-67: Header API additions are fine.Snooze/Unsnooze declarations align with usage in Kernel.c and Console.c.
kernel/core/Kernel.c (1)
765-768: Good placement of Unsnooze/ClearScreen before sti().Restores display state prior to enabling interrupts; ordering is correct.
kernel/etc/Console.c (1)
4-4: Including MLFQ only in the .c avoids header cycles.No issues; keeps dependency local to implementation.
kernel/sched/MLFQ.c (1)
1255-1255: Logging level tweak is fine.Using PrintKernelSuccess for DynamoX startup aligns with the new messaging style.
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
drivers/PS2.c (1)
276-286: Bug: Mouse delta functions don't update the stored deltas.The
GetMouseDeltaX()andGetMouseDeltaY()functions resetmouse.delta_xandmouse.delta_yto 0, but these fields are never actually set in theMouseHandler. The functions will always return 0.Update the MouseHandler to store deltas:
// Update state + mouse.delta_x = delta_x; + mouse.delta_y = delta_y; mouse.x += delta_x; mouse.y += delta_y; mouse.buttons = flags & 0x07;
♻️ Duplicate comments (1)
kernel/core/Kernel.c (1)
442-444: Snooze-at-boot still won't take effect due to MLFQ privilege check.This is the same issue identified in the previous review. The Snooze() call happens before MLFQ initialization, so MLFQGetCurrentProcess() in the Snooze function will fail, preventing the snooze from engaging.
🧹 Nitpick comments (8)
include/Font.h (1)
12-14: Good extern; align declaration with macro and avoid magic 16.Use the defined glyph-size macro to keep header and data in sync.
-extern const unsigned char console_font[256][16]; +extern const unsigned char console_font[256][FONT_BYTES_PER_GLYPH];include/Font.c (1)
1-1: Tie data to header and enforce font metrics at compile time.Include the header, use the glyph-size macro, and add a static-assert to catch mismatches early.
+ #include "Font.h" - const unsigned char console_font[256][16] = { + const unsigned char console_font[256][FONT_BYTES_PER_GLYPH] = { + #if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 201112L + _Static_assert(FONT_WIDTH == 8 && FONT_HEIGHT == 16, "Unexpected font dimensions"); + _Static_assert(FONT_BYTES_PER_GLYPH == (FONT_HEIGHT * ((FONT_WIDTH + 7) / 8)), + "Glyph size must equal rows * bytes_per_row"); + #endifinclude/Window.h (2)
16-31: Solid minimal window struct; consider adding stride for back_buffer.A per-window stride (pitch) avoids assuming tightly packed rows and helps future composition/copy optimizations.
struct Window { Rect rect; const char* title; - uint32_t* back_buffer; // Off-screen buffer for window content + uint32_t* back_buffer; // Off-screen buffer for window content + int back_buffer_stride; // pixels per row in back_buffer bool needs_redraw;
4-5: Use system headers for std types.Angle brackets prevent accidental shadowing by project files.
-#include "stdbool.h" -#include "stdint.h" +#include <stdbool.h> +#include <stdint.h>drivers/PS2.h (1)
1-1: Prefer angle brackets for stdint.-#include "stdint.h" +#include <stdint.h>kernel/core/Compositor.h (1)
5-6: Consider using angle brackets for standard headers.For better consistency with C conventions, standard library headers should use angle brackets:
-#include "stdbool.h" -#include "stdint.h" +#include <stdbool.h> +#include <stdint.h>kernel/core/Compositor.c (2)
46-53: Optimize window compositing with bounds checking.The current pixel-by-pixel compositing can be optimized significantly, especially since bounds checking is done for every pixel.
Apply this diff to improve performance:
- for (int y = 0; y < win->rect.height; y++) { - if (win->rect.y + y >= g_vbe_info->height) continue; - for (int x = 0; x < win->rect.width; x++) { - if (win->rect.x + x >= g_vbe_info->width) continue; - uint32_t pixel = win->back_buffer[y * win->rect.width + x]; - g_compositor_buffer[(win->rect.y + y) * g_vbe_info->width + (win->rect.x + x)] = pixel; - } - } + // Calculate clipping bounds once + int clip_x = win->rect.x < 0 ? -win->rect.x : 0; + int clip_y = win->rect.y < 0 ? -win->rect.y : 0; + int clip_width = (win->rect.x + win->rect.width > g_vbe_info->width) ? + g_vbe_info->width - win->rect.x : win->rect.width; + int clip_height = (win->rect.y + win->rect.height > g_vbe_info->height) ? + g_vbe_info->height - win->rect.y : win->rect.height; + + if (clip_width <= clip_x || clip_height <= clip_y) continue; + + // Copy row by row for better cache performance + for (int y = clip_y; y < clip_height; y++) { + uint32_t* dst = &g_compositor_buffer[(win->rect.y + y) * g_vbe_info->width + (win->rect.x + clip_x)]; + uint32_t* src = &win->back_buffer[y * win->rect.width + clip_x]; + FastMemcpy(dst, src, (clip_width - clip_x) * sizeof(uint32_t)); + }
120-125: Use FastMemset for better performance in WindowFill.The current pixel-by-pixel filling can be optimized using the existing FastMemset function, but it needs to handle 32-bit values correctly.
Apply this diff to optimize the fill operation:
-void WindowFill(Window* window, uint32_t color) { - if (!window) return; - for (int i = 0; i < window->rect.width * window->rect.height; i++) { - window->back_buffer[i] = color; - } -} +void WindowFill(Window* window, uint32_t color) { + if (!window) return; + int pixel_count = window->rect.width * window->rect.height; + for (int i = 0; i < pixel_count; i++) { + window->back_buffer[i] = color; + } +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
drivers/PS2.c(3 hunks)drivers/PS2.h(1 hunks)drivers/Vesa.c(1 hunks)include/Font.c(1 hunks)include/Font.h(1 hunks)include/Window.h(1 hunks)kernel/core/Compositor.c(1 hunks)kernel/core/Compositor.h(1 hunks)kernel/core/Kernel.c(4 hunks)kernel/etc/Console.c(6 hunks)meson.build(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- meson.build
- kernel/etc/Console.c
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: assembler-0
PR: assembler-0/VoidFrame#73
File: kernel/elf/ELFloader.c:0-0
Timestamp: 2025-08-12T03:19:52.873Z
Learning: VoidFrame kernel now has complete UEFI boot support with hybrid BIOS/UEFI ISO creation capability. The existing VBE framebuffer implementation requires no changes as it uses Multiboot2 framebuffer tags and direct memory-mapped access, making it inherently UEFI-compatible. Build process: ninja + ./scripts/build_hybrid_iso.sh creates dual-platform bootable ISO.
📚 Learning: 2025-08-10T02:19:46.948Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#61
File: include/Font.h:36-46
Timestamp: 2025-08-10T02:19:46.948Z
Learning: In VoidFrame kernel, the PS/2 keyboard controller needs proper initialization including buffer flushing and error handling. Without flushing the controller buffer in KeyboardInit() and handling overflow errors (status bits 6-7) in KeyboardHandler(), the keyboard can stop working if keys are pressed rapidly during boot due to controller buffer overflow.
Applied to files:
drivers/PS2.c
📚 Learning: 2025-08-13T07:26:56.892Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#79
File: drivers/PCI/PCI.c:154-154
Timestamp: 2025-08-13T07:26:56.892Z
Learning: VoidFrame kernel uses direct interrupt dispatch in arch/x86_64/interrupts/Interrupts.c rather than RegisterInterruptHandler pattern. Hardware interrupts are handled in switch cases by interrupt number (32+ for IRQs), with proper PIC EOI handling required.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-11T09:51:32.713Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#70
File: kernel/process/Process.c:1224-1228
Timestamp: 2025-08-11T09:51:32.713Z
Learning: In the VoidFrame kernel (kernel/process/Process.c), avoid using floating-point arithmetic in kernel space. Use fixed-point arithmetic instead with integer scaling (e.g., multiplying by 1000 for 3 decimal places) to avoid FPU state management overhead and potential exceptions.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-23T10:06:02.997Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#95
File: kernel/core/Panic.c:65-67
Timestamp: 2025-08-23T10:06:02.997Z
Learning: In VoidFrame kernel's panic handler, the VBE graphics path intentionally mixes VBEShowPanic() with console text operations (PrintKernel*, PrintKernelAt) to overlay stack traces and debugging information on top of the panic image. This overlap is deliberate to ensure critical diagnostic information remains visible during kernel panics, prioritizing debugging capability over visual presentation.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-11T09:51:51.217Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#70
File: kernel/process/Process.c:1224-1228
Timestamp: 2025-08-11T09:51:51.217Z
Learning: In VoidFrame kernel, avoid using floating-point arithmetic in kernel space functions like SystemService. Use fixed-point arithmetic instead (e.g., 16.16 format with integer operations) to prevent FPU state corruption, improve performance, and ensure predictable behavior. This is especially important for system-level processes running with PROC_PRIV_SYSTEM privileges.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-12T03:19:52.873Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#73
File: kernel/elf/ELFloader.c:0-0
Timestamp: 2025-08-12T03:19:52.873Z
Learning: User assembler-0 has successfully implemented UEFI boot support for VoidFrame kernel. The VBE framebuffer implementation is fully UEFI-compatible using Multiboot2 framebuffer tags and direct memory-mapped access, requiring no kernel code changes - only bootloader configuration updates.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-12T03:19:52.873Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#73
File: kernel/elf/ELFloader.c:0-0
Timestamp: 2025-08-12T03:19:52.873Z
Learning: VoidFrame kernel now has complete UEFI boot support with hybrid BIOS/UEFI ISO creation capability. The existing VBE framebuffer implementation requires no changes as it uses Multiboot2 framebuffer tags and direct memory-mapped access, making it inherently UEFI-compatible. Build process: ninja + ./scripts/build_hybrid_iso.sh creates dual-platform bootable ISO.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-31T14:19:11.338Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#103
File: docs/ARCHITECHTURE.md:1-3
Timestamp: 2025-08-31T14:19:11.338Z
Learning: assembler-0 wants to implement deep fault tracing for VoidFrame kernel exceptions (Page Fault, General Protection Fault, Invalid Opcode) instead of just panicking. The current interrupt handler has complete register state available in the Registers struct (defined in arch/x86_64/cpu/Cpu.h) including RBP for stack walking, RIP for fault location, and error codes for analysis.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-09-01T01:10:22.088Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#103
File: README.md:9-9
Timestamp: 2025-09-01T01:10:22.088Z
Learning: The current VoidFrame Registers struct in arch/x86_64/cpu/Cpu.h only saves basic GPRs, segment registers, and exception context (interrupt_number, error_code, rip, cs, rflags, rsp, ss). It's missing critical registers for deep fault tracing: SIMD registers (XMM/YMM/ZMM), control registers (CR0/CR2/CR3/CR4), debug registers (DR0-DR7), and extended FPU state. The struct has a "DO NOT TOUCH" comment indicating it must match interrupt ASM stack layout.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-10T06:22:28.816Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#62
File: kernel/core/Panic.c:69-79
Timestamp: 2025-08-10T06:22:28.816Z
Learning: In VoidFrame kernel, panic functions must call "asm volatile("cli")" as the very first instruction to disable interrupts immediately. Without this, if interrupts are enabled (after "sti"), an interrupt can fire during panic handling causing the panic to fail, hang, or behave unpredictably. This must be done in ALL panic entry points: Panic(), PanicWithCode(), PanicWithContext(), PanicFromInterrupt(), and at the start of KernelPanicHandler().
Applied to files:
kernel/core/Kernel.c
🧬 Code graph analysis (3)
kernel/core/Compositor.h (1)
kernel/core/Compositor.c (7)
WindowManagerInit(62-76)WindowManagerRun(78-80)CreateWindow(82-108)DestroyWindow(110-118)WindowFill(120-125)WindowDrawRect(127-136)WindowDrawString(138-157)
kernel/core/Compositor.c (3)
mm/MemOps.c (2)
FastMemset(10-73)FastMemcpy(75-162)kernel/etc/Console.c (4)
PrintKernelError(209-214)Snooze(39-47)PrintKernelSuccess(202-207)SerialWriteF(294-301)mm/KernelHeap.c (2)
KernelMemoryAlloc(272-330)KernelFree(377-422)
kernel/core/Kernel.c (3)
kernel/etc/Console.c (2)
Snooze(39-47)Unsnooze(48-50)kernel/core/Compositor.c (3)
WindowManagerInit(62-76)CreateWindow(82-108)WindowManagerRun(78-80)kernel/sched/MLFQ.c (1)
MLFQYield(994-1003)
🔇 Additional comments (12)
drivers/Vesa.c (1)
2-2: LGTM: early Font.h include resolves extern symbol cleanly.drivers/PS2.h (1)
6-12: Strong overrides for PS2 hooks exist in kernel/core/Compositor.c; no default weak stubs required.drivers/PS2.c (4)
164-173: Good implementation of keyboard event hook mechanism.The addition of the
OnKeyPresscallback before buffering allows the window manager to intercept keyboard input while preserving the existing buffering mechanism. The weak symbol pattern enables clean optional override.
219-226: Good addition of screen boundary clamping.The mouse position clamping to screen dimensions prevents cursor from going out of bounds. The NULL check on VBE info is appropriate.
234-246: Well-structured button event dispatching.The XOR-based change detection and iterative dispatch for button events is clean and efficient. The button numbering (1-3) in the callbacks is intuitive.
212-217: Preserve or document Y-axis convention change
I found no internal uses of mouse.y/GetMouseY besides its PS2.h declaration; verify any external consumers of GetMouseY to ensure they expect positive-downward coordinates. Either restore themouse.y -= delta_y;inversion for backward compatibility or clearly document this breaking coordinate-system change.kernel/core/Compositor.h (1)
1-32: Clean and well-structured window manager API.The header provides a clear interface for window management with appropriate separation of initialization, lifecycle management, and drawing operations. The documentation about operations targeting backbuffers is helpful.
kernel/core/Kernel.c (2)
589-604: Good addition of memory pool initialization and statistics.The early initialization of memory pools and detailed memory statistics display provides valuable debugging information and should improve small allocation performance.
775-777: Good placement of Unsnooze before enabling interrupts.The Unsnooze() call is correctly positioned just before
sti()to restore display output at the right moment in the boot sequence.kernel/core/Compositor.c (3)
62-76: LGTM! Well-structured initialization with proper error handling.The initialization properly checks for VBE availability, allocates the compositor buffer with appropriate error handling, and provides clear success/failure feedback. The use of
Snooze()aligns with the kernel's snooze functionality mentioned in the retrieved learnings.
110-118: LGTM! Proper doubly-linked list management.The window destruction correctly handles all edge cases for doubly-linked list removal and properly frees allocated memory.
138-157: LGTM! Robust text rendering with proper bounds checking.The text rendering implementation correctly handles font bitmap data, includes proper bounds checking for pixel placement, and efficiently processes each character glyph.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
drivers/PS2.c (1)
119-128: Handle 8042 parity/timeout errors to avoid keyboard lockups.Per prior kernel learnings, drop bytes on status bits 6/7 to prevent buffer poisoning during boot-time bursts.
void KeyboardHandler(void) { uint8_t status = inb(KEYBOARD_STATUS_PORT); if (!(status & 0x01)) return; + // Parity (bit 7) or timeout (bit 6) error: read and discard + if (status & 0xC0) { inb(KEYBOARD_DATA_PORT); return; }
♻️ Duplicate comments (1)
kernel/core/Kernel.c (1)
442-444: Re-check Snooze-at-boot effectiveness.Past reports indicated privilege checks could neuter Snooze during early boot. Verify IF state at this call site so Snooze actually engages pre-MLFQ.
Would you like a quick probe patch to log Snooze state transitions during PXS1?
🧹 Nitpick comments (9)
kernel/sched/MLFQ.h (1)
216-216: Expose compositor init behind a feature guardHeader exposure looks fine. Consider guarding this API so non-compositor builds don’t break.
-void VFCompositorRequestInit(const char * str); +#ifdef VF_CONFIG_USE_COMPOSITOR +void VFCompositorRequestInit(const char * str); +#endifkernel/etc/Shell.c (1)
976-976: Wire-up is correct; add help entry for discoverabilityCommand registration is good. Add a help line so users can find it.
static void HelpHandler(const char * args) { @@ PrintKernel(" lscpu - List cpu features (CPUID)\n"); + PrintKernel(" vfc - Start the VFCompositor\n"); }kernel/sched/MLFQ.c (1)
4-4: Guard Compositor include to avoid build breaks in non-compositor configsIf Compositor isn’t compiled in, this unconditional include will fail.
-#include "Compositor.h" +#ifdef VF_CONFIG_USE_COMPOSITOR +#include "Compositor.h" +#endifdrivers/PS2.c (2)
196-209: Simplify PS/2 sign/overflow handling; remove bogus manual sign-extension.Casting to int8_t already sign-extends on promotion. The OR with 0xFFFFFF00 is unnecessary and confusing. If overflow flags set, saturate based on sign bit.
- // Handle X overflow - if (flags & 0x40) { - delta_x = (flags & 0x10) ? -256 : 255; - } else if (flags & 0x10) { - delta_x = (delta_x == 0) ? 0 : (delta_x | 0xFFFFFF00); - } + // Handle X overflow: saturate + if (flags & 0x40) { + delta_x = (flags & 0x10) ? -127 : 127; + } @@ - // Handle Y overflow - if (flags & 0x80) { - delta_y = (flags & 0x20) ? -256 : 255; - } else if (flags & 0x20) { - delta_y = (delta_y == 0) ? 0 : (delta_y | 0xFFFFFF00); - } + // Handle Y overflow: saturate + if (flags & 0x80) { + delta_y = (flags & 0x20) ? -127 : 127; + }
53-61: Add ACK/resend handling for mouse commands.0xFE requires resend; lack of retries can leave the mouse disabled on flaky controllers.
-void send_mouse_command(uint8_t cmd) { - wait_for_input_buffer_empty(); - outb(KEYBOARD_STATUS_PORT, PS2_CMD_WRITE_AUX); - wait_for_input_buffer_empty(); - outb(KEYBOARD_DATA_PORT, cmd); - wait_for_output_buffer_full(); - uint8_t response = inb(KEYBOARD_DATA_PORT); - // Should be 0xFA (ACK) for most commands -} +void send_mouse_command(uint8_t cmd) { + for (int tries = 0; tries < 3; tries++) { + wait_for_input_buffer_empty(); + outb(KEYBOARD_STATUS_PORT, PS2_CMD_WRITE_AUX); + wait_for_input_buffer_empty(); + outb(KEYBOARD_DATA_PORT, cmd); + wait_for_output_buffer_full(); + uint8_t response = inb(KEYBOARD_DATA_PORT); + if (response == 0xFA) return; // ACK + if (response != 0xFE) break; // not RESEND, give up + } + // Optionally: PrintKernelWarning("PS/2 mouse cmd failed\n"); +}kernel/core/Compositor.h (1)
4-7: Use system headers for stdbool/stdint.Prefer angle brackets to avoid accidental shadowing by local files.
-#include "stdbool.h" -#include "stdint.h" +#include <stdbool.h> +#include <stdint.h>kernel/core/Kernel.c (2)
94-101: Don’t silently re-align non-page-aligned map requests.Masking vaddr/paddr hides bugs. Either assert/alarm or return error to callers.
- if (vaddr & 0xFFF || paddr & 0xFFF) { - vaddr &= ~0xFFF; - paddr &= ~0xFFF; - } + if ((vaddr | paddr) & 0xFFF) { + PANIC("BootstrapMapPage: unaligned vaddr/paddr"); + }
301-334: CET enable path may #GP on emulators/older CPUs.Even with CPUID checks, some environments (e.g., QEMU without CET) #GP on CET MSRs/CR4.CET. Consider making CET gating a build-time option or add runtime MSR write guarding.
kernel/core/Compositor.c (1)
57-85: Consider partial redraws to reduce full-screen clears/blits.Track dirty regions per window and only CompositeAndDraw those rects; then FastMemcpy only those spans (scanline granularity).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
drivers/PS2.c(3 hunks)drivers/PS2.h(1 hunks)include/Font.c(1 hunks)kernel/core/Compositor.c(1 hunks)kernel/core/Compositor.h(1 hunks)kernel/core/Kernel.c(4 hunks)kernel/etc/Shell.c(1 hunks)kernel/sched/MLFQ.c(4 hunks)kernel/sched/MLFQ.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- include/Font.c
- drivers/PS2.h
🧰 Additional context used
🧠 Learnings (20)
📚 Learning: 2025-09-02T00:55:58.212Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#106
File: kernel/sched/MLFQ.c:691-694
Timestamp: 2025-09-02T00:55:58.212Z
Learning: The VoidFrame MLFQ scheduler has a critical bug where ProcessRuntimePath uses FormatS() which returns a static buffer, causing all processes to share the same path string. This results in process directory cleanup operations affecting the wrong directories - killing one process can delete another process's /Runtime/Processes directory because they all point to the same FormatS static buffer.
Applied to files:
kernel/sched/MLFQ.h
📚 Learning: 2025-09-03T12:55:44.794Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#109
File: kernel/core/Kernel.c:736-740
Timestamp: 2025-09-03T12:55:44.794Z
Learning: In VoidFrame kernel's window manager initialization, the Snooze() call in WindowManagerInit() suppresses display output by redirecting console operations to serial-only mode, causing the GUI to render but remain invisible to the user.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-23T10:06:02.997Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#95
File: kernel/core/Panic.c:65-67
Timestamp: 2025-08-23T10:06:02.997Z
Learning: In VoidFrame kernel's panic handler, the VBE graphics path intentionally mixes VBEShowPanic() with console text operations (PrintKernel*, PrintKernelAt) to overlay stack traces and debugging information on top of the panic image. This overlap is deliberate to ensure critical diagnostic information remains visible during kernel panics, prioritizing debugging capability over visual presentation.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-10T06:03:22.076Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#62
File: kernel/core/Panic.c:69-79
Timestamp: 2025-08-10T06:03:22.076Z
Learning: In VoidFrame kernel, mixing VBE graphics mode operations (like VBEShowPanic drawing to framebuffer) with text console operations causes display corruption with random pixels. The panic handler must choose either pure VBE graphics mode or pure text mode, not both simultaneously.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-10T06:03:22.076Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#62
File: kernel/core/Panic.c:69-79
Timestamp: 2025-08-10T06:03:22.076Z
Learning: In VoidFrame kernel, after VBEShowPanic() draws a panic image to the framebuffer, continuing to use text console operations like PrintKernelAt() causes display corruption because it tries to render text on top of the raw pixel data. The panic handler must choose either pure VBE graphics mode (show image only) or pure text console mode, but not mix both.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-10T06:31:43.779Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#62
File: kernel/core/Panic.c:69-79
Timestamp: 2025-08-10T06:31:43.779Z
Learning: In VoidFrame kernel's VBEConsolePutChar() function, the color_buffer must be updated with the current attribute byte whenever a character is stored in the buffer. Without this, characters inherit stale colors from VBEConsoleClear() causing all text to appear in the wrong color (gray/0x08) after refresh operations.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-10T06:03:22.076Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#62
File: kernel/core/Panic.c:69-79
Timestamp: 2025-08-10T06:03:22.076Z
Learning: In VoidFrame kernel's VBEConsoleClear() function, the color is hardcoded to 0x08 (gray on black) instead of using the current console color state, causing color washing issues. The function should use console.color or the current color state to maintain color consistency across screen clears.
Applied to files:
kernel/core/Kernel.ckernel/core/Compositor.c
📚 Learning: 2025-08-13T07:26:56.892Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#79
File: drivers/PCI/PCI.c:154-154
Timestamp: 2025-08-13T07:26:56.892Z
Learning: VoidFrame kernel uses direct interrupt dispatch in arch/x86_64/interrupts/Interrupts.c rather than RegisterInterruptHandler pattern. Hardware interrupts are handled in switch cases by interrupt number (32+ for IRQs), with proper PIC EOI handling required.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-11T09:51:32.713Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#70
File: kernel/process/Process.c:1224-1228
Timestamp: 2025-08-11T09:51:32.713Z
Learning: In the VoidFrame kernel (kernel/process/Process.c), avoid using floating-point arithmetic in kernel space. Use fixed-point arithmetic instead with integer scaling (e.g., multiplying by 1000 for 3 decimal places) to avoid FPU state management overhead and potential exceptions.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-11T09:51:51.217Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#70
File: kernel/process/Process.c:1224-1228
Timestamp: 2025-08-11T09:51:51.217Z
Learning: In VoidFrame kernel, avoid using floating-point arithmetic in kernel space functions like SystemService. Use fixed-point arithmetic instead (e.g., 16.16 format with integer operations) to prevent FPU state corruption, improve performance, and ensure predictable behavior. This is especially important for system-level processes running with PROC_PRIV_SYSTEM privileges.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-12T03:19:52.873Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#73
File: kernel/elf/ELFloader.c:0-0
Timestamp: 2025-08-12T03:19:52.873Z
Learning: User assembler-0 has successfully implemented UEFI boot support for VoidFrame kernel. The VBE framebuffer implementation is fully UEFI-compatible using Multiboot2 framebuffer tags and direct memory-mapped access, requiring no kernel code changes - only bootloader configuration updates.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-12T03:19:52.873Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#73
File: kernel/elf/ELFloader.c:0-0
Timestamp: 2025-08-12T03:19:52.873Z
Learning: VoidFrame kernel now has complete UEFI boot support with hybrid BIOS/UEFI ISO creation capability. The existing VBE framebuffer implementation requires no changes as it uses Multiboot2 framebuffer tags and direct memory-mapped access, making it inherently UEFI-compatible. Build process: ninja + ./scripts/build_hybrid_iso.sh creates dual-platform bootable ISO.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-31T14:19:11.338Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#103
File: docs/ARCHITECHTURE.md:1-3
Timestamp: 2025-08-31T14:19:11.338Z
Learning: assembler-0 wants to implement deep fault tracing for VoidFrame kernel exceptions (Page Fault, General Protection Fault, Invalid Opcode) instead of just panicking. The current interrupt handler has complete register state available in the Registers struct (defined in arch/x86_64/cpu/Cpu.h) including RBP for stack walking, RIP for fault location, and error codes for analysis.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-09-01T01:10:22.088Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#103
File: README.md:9-9
Timestamp: 2025-09-01T01:10:22.088Z
Learning: The current VoidFrame Registers struct in arch/x86_64/cpu/Cpu.h only saves basic GPRs, segment registers, and exception context (interrupt_number, error_code, rip, cs, rflags, rsp, ss). It's missing critical registers for deep fault tracing: SIMD registers (XMM/YMM/ZMM), control registers (CR0/CR2/CR3/CR4), debug registers (DR0-DR7), and extended FPU state. The struct has a "DO NOT TOUCH" comment indicating it must match interrupt ASM stack layout.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-10T06:22:28.816Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#62
File: kernel/core/Panic.c:69-79
Timestamp: 2025-08-10T06:22:28.816Z
Learning: In VoidFrame kernel, panic functions must call "asm volatile("cli")" as the very first instruction to disable interrupts immediately. Without this, if interrupts are enabled (after "sti"), an interrupt can fire during panic handling causing the panic to fail, hang, or behave unpredictably. This must be done in ALL panic entry points: Panic(), PanicWithCode(), PanicWithContext(), PanicFromInterrupt(), and at the start of KernelPanicHandler().
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-10T02:19:46.948Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#61
File: include/Font.h:36-46
Timestamp: 2025-08-10T02:19:46.948Z
Learning: In VoidFrame kernel, the PS/2 keyboard controller needs proper initialization including buffer flushing and error handling. Without flushing the controller buffer in KeyboardInit() and handling overflow errors (status bits 6-7) in KeyboardHandler(), the keyboard can stop working if keys are pressed rapidly during boot due to controller buffer overflow.
Applied to files:
kernel/core/Kernel.cdrivers/PS2.c
📚 Learning: 2025-08-10T06:22:28.816Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#62
File: kernel/core/Panic.c:69-79
Timestamp: 2025-08-10T06:22:28.816Z
Learning: In VoidFrame kernel, panic functions must disable interrupts with "cli" as the very first operation, before any other code executes. If interrupts are enabled (after "sti"), an interrupt can fire between the panic call and the cli instruction in KernelPanicHandler, causing the panic to fail or behave unpredictably.
Applied to files:
kernel/core/Kernel.c
📚 Learning: 2025-08-10T09:50:21.823Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#63
File: kernel/etc/Shell.c:24-28
Timestamp: 2025-08-10T09:50:21.823Z
Learning: VoidFrame's VBE graphics can be optimized by using FastMemcpy for entire scanlines instead of pixel-by-pixel VBEPutPixel calls, and SSE2 instructions can fill/gradient 4 pixels at once for 4-8x performance gains.
Applied to files:
kernel/core/Compositor.c
📚 Learning: 2025-08-10T09:50:21.823Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#63
File: kernel/etc/Shell.c:24-28
Timestamp: 2025-08-10T09:50:21.823Z
Learning: VoidFrame's VBE graphics can be optimized by using FastMemcpy for entire scanlines instead of pixel-by-pixel VBEPutPixel calls, reducing function calls from millions to thousands for full-screen operations.
Applied to files:
kernel/core/Compositor.c
📚 Learning: 2025-08-13T03:11:50.346Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#77
File: drivers/USB/xHCI.c:117-122
Timestamp: 2025-08-13T03:11:50.346Z
Learning: In VoidFrame's DynamoX frequency controller (kernel/process/Process.c), the ENHANCED_HZ_PER_PROCESS parameter controls per-process frequency scaling. Reducing it from 80Hz to 55Hz per process achieves approximately 500Hz idle frequency instead of 700Hz+ while maintaining the same scaling logic.
Applied to files:
kernel/sched/MLFQ.c
🧬 Code graph analysis (7)
kernel/sched/MLFQ.h (1)
kernel/sched/MLFQ.c (1)
VFCompositorRequestInit(1807-1821)
kernel/core/Compositor.h (1)
kernel/core/Compositor.c (8)
WindowManagerInit(89-103)WindowManagerRun(105-107)CreateWindow(109-144)DestroyWindow(147-154)WindowFill(156-161)WindowDrawRect(163-172)WindowDrawString(174-193)VFCompositor(26-42)
kernel/etc/Shell.c (1)
kernel/sched/MLFQ.c (1)
VFCompositorRequestInit(1807-1821)
kernel/core/Kernel.c (4)
drivers/Vesa.c (2)
VBEShowSplash(287-300)VBEIsInitialized(325-327)kernel/etc/Console.c (3)
Snooze(39-47)Unsnooze(48-50)ClearScreen(76-99)kernel/core/Compositor.c (1)
WindowManagerRun(105-107)kernel/sched/MLFQ.c (1)
MLFQYield(998-1007)
kernel/core/Compositor.c (6)
kernel/etc/Console.c (5)
Snooze(39-47)Unsnooze(48-50)PrintKernelError(209-214)PrintKernelSuccess(202-207)SerialWriteF(294-301)drivers/Vesa.c (2)
VBEIsInitialized(325-327)VBEGetInfo(321-323)kernel/sched/MLFQ.c (1)
MLFQYield(998-1007)mm/MemOps.c (1)
FastMemcpy(75-162)mm/KernelHeap.c (2)
KernelMemoryAlloc(272-330)KernelFree(377-422)kernel/etc/StringOps.c (2)
StringLength(3-8)strcpy(66-97)
drivers/PS2.c (2)
kernel/core/Compositor.c (4)
OnKeyPress(197-201)OnMouseMove(203-224)OnMouseButtonDown(226-261)OnMouseButtonUp(263-267)drivers/Vesa.c (1)
VBEGetInfo(321-323)
kernel/sched/MLFQ.c (2)
kernel/etc/Console.c (4)
PrintKernelSuccess(202-207)PrintKernel(170-188)PrintKernelError(209-214)PrintKernelInt(303-327)kernel/core/Compositor.c (1)
VFCompositor(26-42)
🔇 Additional comments (6)
kernel/sched/MLFQ.c (1)
1259-1259: Minor: success badge for DynamoX startup is fineCosmetic log improvement. No action.
drivers/PS2.c (1)
164-172: Keypress callback before buffering looks good.Calling OnKeyPress (if set) and then appending to the ring buffer is reasonable and avoids latency.
kernel/core/Compositor.h (1)
8-32: API surface looks coherent.Init/run, window lifecycle, and basic drawing primitives form a minimal usable WM interface.
kernel/core/Kernel.c (1)
775-781: Good: compositor loop yields; avoids busy spin.WindowManagerRun() + MLFQYield() per iteration prevents pegging a CPU.
kernel/core/Compositor.c (2)
65-83: Draw order and focus logic are consistent.Head-to-tail blit with tail promotion on click yields correct z-order.
203-224: Clamping and movement bounds look sane.Title-bar-based dragging with visibility constraints prevents windows from getting lost.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
kernel/sched/MLFQ.c (1)
759-763: Likely build break: PrintKernelErrorF not defined.Console API exposes PrintKernel, PrintKernelError, PrintKernelSuccess, but not PrintKernelErrorF; this will not link.
Apply:
- PrintKernelErrorF("[AS-PREFLIGHT] ProcINFOPath tampering detected for PID: %d (%s)\n", proc->pid, proc->ProcessRuntimePath); + PrintKernelError("[AS-PREFLIGHT] ProcINFOPath tampering detected for PID: "); + PrintKernelInt(proc->pid); + PrintKernelError(" ("); + PrintKernelError(proc->ProcessRuntimePath); + PrintKernelError(")\n");Optional: add a small FormatF-style helper to Console.c later.
♻️ Duplicate comments (1)
kernel/core/Compositor.c (1)
21-36: Snooze is never Unsnooze’d (dead code after infinite loop). GUI remains “serial-only”.Unsnooze() after while(1) is unreachable, matching the known issue where the UI renders but stays invisible.
Apply one of:
void VFCompositor(void) { - Snooze(); + Snooze(); if (VBEIsInitialized()) { WindowManagerInit(); CreateWindow(50, 50, 400, 250, "VFShell"); } - while (1) { + Unsnooze(); // restore console after init + while (1) { if (VBEIsInitialized()) { WindowManagerRun(); MLFQYield(); } else { MLFQYield(); } } - Unsnooze(); }Or, if you prefer to control Snooze lifecycle elsewhere, remove Snooze entirely here.
🧹 Nitpick comments (5)
kernel/core/Pallete.h (3)
1-2: Typo in header guard and filename (“Pallete” → “Palette”).Recommend standardizing spelling to avoid future include/guard drift.
Apply:
-#ifndef VF_PALLETE_H -#define VF_PALLETE_H +#ifndef VF_PALETTE_H +#define VF_PALETTE_HFollow-up: if you opt to rename the file to Palette.h, update includes accordingly.
103-106: Make CURRENT_PALETTE override explicit and documented.Minor: add a short comment that board configs can -D CURRENT_PALETTE=N to switch themes at build time.
107-185: Reduce macro duplication with token-pasting helper.The 8-way #elif chain is verbose and error-prone. Consider a single APPLY_PALETTE(CURRENT_PALETTE) indirection.
Example:
+#define PVAL(n, kind) kind##_##n +#define APPLY_PALETTE(n) \ + enum { _p = n }; \ + enum { \ + _bg = PVAL(_p, TERMINAL_BG), \ + _tx = PVAL(_p, TERMINAL_TEXT),\ + _wb = PVAL(_p, WINDOW_BG), \ + _tb = PVAL(_p, TITLE_BAR), \ + _bd = PVAL(_p, BORDER), \ + _ac = PVAL(_p, ACCENT), \ + _er = PVAL(_p, ERROR), \ + _ok = PVAL(_p, SUCCESS) \ + }; \ + enum { \ + TERMINAL_BG = _bg, \ + TERMINAL_TEXT = _tx, \ + WINDOW_BG = _wb, \ + TITLE_BAR = _tb, \ + BORDER = _bd, \ + ACCENT = _ac, \ + ERROR_COLOR = _er, \ + SUCCESS_COLOR = _ok \ + } - -#if CURRENT_PALETTE == 1 - #define TERMINAL_BG TERMINAL_BG_1 - ... -#else - ... -#endif +APPLY_PALETTE((CURRENT_PALETTE>=1 && CURRENT_PALETTE<=8)?CURRENT_PALETTE:8);kernel/sched/MLFQ.c (1)
4-4: Guard compositor include behind feature macro.Avoid compile-time dependency when compositor is disabled.
-#include "Compositor.h" +#ifdef VF_CONFIG_USE_COMPOSITOR +#include "Compositor.h" +#endifkernel/core/Compositor.c (1)
83-97: Init: consider zeroing compositor buffer and checking mode.Add a clear on first init and assert 32 bpp mode to prevent garbage flash.
g_compositor_buffer = (uint32_t*)KernelMemoryAlloc(buffer_size); if (!g_compositor_buffer) { PrintKernelError("Window Manager: Failed to allocate compositor buffer!\n"); return; } - // Snooze(); + for (uint32_t i = 0; i < buffer_size / 4; i++) g_compositor_buffer[i] = TERMINAL_BG; + if (g_vbe_info->bpp != 32) { + PrintKernelError("Window Manager: Unsupported VBE bpp (need 32)\n"); + return; + } PrintKernelSuccess("Window Manager Initialized\n");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
drivers/PS2.c(3 hunks)kernel/core/Compositor.c(1 hunks)kernel/core/Pallete.h(1 hunks)kernel/sched/MLFQ.c(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- drivers/PS2.c
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: assembler-0
PR: assembler-0/VoidFrame#109
File: kernel/core/Kernel.c:736-740
Timestamp: 2025-09-03T12:55:44.794Z
Learning: In VoidFrame kernel's window manager initialization, the Snooze() call in WindowManagerInit() suppresses display output by redirecting console operations to serial-only mode, causing the GUI to render but remain invisible to the user.
📚 Learning: 2025-09-02T00:55:58.212Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#106
File: kernel/sched/MLFQ.c:691-694
Timestamp: 2025-09-02T00:55:58.212Z
Learning: The VoidFrame MLFQ scheduler has a critical bug where ProcessRuntimePath uses FormatS() which returns a static buffer, causing all processes to share the same path string. This results in process directory cleanup operations affecting the wrong directories - killing one process can delete another process's /Runtime/Processes directory because they all point to the same FormatS static buffer.
Applied to files:
kernel/sched/MLFQ.c
📚 Learning: 2025-09-02T00:55:58.212Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#106
File: kernel/sched/MLFQ.c:691-694
Timestamp: 2025-09-02T00:55:58.212Z
Learning: The VoidFrame MLFQ ProcessRuntimePath bug occurs because: 1) ProcessRuntimePath is defined as char* (not char array), 2) All processes get assigned the same FormatS static buffer pointer, 3) Directory cleanup via VfsDelete happens during TerminateProcess() not final cleanup, 4) When terminating a process, VfsDelete uses whatever path is currently in the shared static buffer, causing wrong directories to be deleted. Fix requires either changing ProcessRuntimePath to char array or using dynamic allocation.
Applied to files:
kernel/sched/MLFQ.c
📚 Learning: 2025-09-02T00:55:58.212Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#106
File: kernel/sched/MLFQ.c:691-694
Timestamp: 2025-09-02T00:55:58.212Z
Learning: In VoidFrame's MLFQ.c, line 1076 assigns ProcessRuntimePath using FormatS() which returns a static buffer, causing all processes to share the same path pointer. During cleanup, VfsDelete() can delete the wrong process directory because all ProcessRuntimePath fields point to the same static buffer containing the path of the most recently created process. This causes directory cleanup operations to affect unintended processes.
Applied to files:
kernel/sched/MLFQ.c
📚 Learning: 2025-09-01T05:15:45.841Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#104
File: kernel/process/Process.c:1074-1075
Timestamp: 2025-09-01T05:15:45.841Z
Learning: In the VoidFrame kernel Process.c, the ProcessRuntimePath field uses FormatS() which returns a static buffer. While technically this creates a race condition where all processes could share the same path memory, the user has tested alternatives and they cause system failures. The current implementation works reliably in practice because the path is used immediately after assignment before FormatS() is called again.
Applied to files:
kernel/sched/MLFQ.c
📚 Learning: 2025-08-13T03:11:50.346Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#77
File: drivers/USB/xHCI.c:117-122
Timestamp: 2025-08-13T03:11:50.346Z
Learning: In VoidFrame's DynamoX frequency controller (kernel/process/Process.c), the ENHANCED_HZ_PER_PROCESS parameter controls per-process frequency scaling. Reducing it from 80Hz to 55Hz per process achieves approximately 500Hz idle frequency instead of 700Hz+ while maintaining the same scaling logic.
Applied to files:
kernel/sched/MLFQ.c
📚 Learning: 2025-08-10T09:50:21.823Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#63
File: kernel/etc/Shell.c:24-28
Timestamp: 2025-08-10T09:50:21.823Z
Learning: VoidFrame's VBE graphics can be optimized by using FastMemcpy for entire scanlines instead of pixel-by-pixel VBEPutPixel calls, and SSE2 instructions can fill/gradient 4 pixels at once for 4-8x performance gains.
Applied to files:
kernel/core/Compositor.c
📚 Learning: 2025-08-10T09:50:21.823Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#63
File: kernel/etc/Shell.c:24-28
Timestamp: 2025-08-10T09:50:21.823Z
Learning: VoidFrame's VBE graphics can be optimized by using FastMemcpy for entire scanlines instead of pixel-by-pixel VBEPutPixel calls, reducing function calls from millions to thousands for full-screen operations.
Applied to files:
kernel/core/Compositor.c
📚 Learning: 2025-08-10T06:03:22.076Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#62
File: kernel/core/Panic.c:69-79
Timestamp: 2025-08-10T06:03:22.076Z
Learning: In VoidFrame kernel's VBEConsoleClear() function, the color is hardcoded to 0x08 (gray on black) instead of using the current console color state, causing color washing issues. The function should use console.color or the current color state to maintain color consistency across screen clears.
Applied to files:
kernel/core/Compositor.c
📚 Learning: 2025-09-03T12:55:44.794Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#109
File: kernel/core/Kernel.c:736-740
Timestamp: 2025-09-03T12:55:44.794Z
Learning: In VoidFrame kernel's window manager initialization, the Snooze() call in WindowManagerInit() suppresses display output by redirecting console operations to serial-only mode, causing the GUI to render but remain invisible to the user.
Applied to files:
kernel/core/Compositor.c
🧬 Code graph analysis (2)
kernel/sched/MLFQ.c (4)
kernel/etc/Format.c (1)
FormatA(280-286)kernel/etc/StringOps.c (1)
FastStrCmp(25-35)kernel/etc/Console.c (4)
PrintKernelSuccess(202-207)PrintKernel(170-188)PrintKernelError(209-214)PrintKernelInt(303-327)kernel/core/Compositor.c (1)
VFCompositor(21-36)
kernel/core/Compositor.c (5)
kernel/etc/Console.c (5)
Snooze(39-47)Unsnooze(48-50)PrintKernelError(209-214)PrintKernelSuccess(202-207)SerialWriteF(294-301)drivers/Vesa.c (2)
VBEIsInitialized(325-327)VBEGetInfo(321-323)mm/MemOps.c (1)
FastMemcpy(75-162)mm/KernelHeap.c (2)
KernelMemoryAlloc(272-330)KernelFree(377-422)kernel/etc/StringOps.c (2)
StringLength(3-8)strcpy(66-97)
🔇 Additional comments (4)
kernel/sched/MLFQ.c (4)
728-732: Good: fixes the FormatS static-buffer bug.Using a stack buffer with FormatA eliminates the shared static buffer hazard for ProcINFOPath validation.
903-907: Astra gating spot looks correct.Pre-schedule preflight with retry via select_next is a reasonable containment path.
1260-1260: Cosmetic: success badge for DynamoX startup is fine.No functional concerns here.
1717-1717: Switch FormatA to vsnprintf
It currently ignores the size parameter by calling vsprintf; using vsnprintf will enforce the buffer bound and NUL-terminate.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
kernel/etc/Console.c (1)
183-202: Potential spinlock issue with thegoto serialpattern.When snooze is active, the function calls
PrintToVFShell()then jumps to theseriallabel, but this bypasses theSpinLock(&lock)call. This meansSpinUnlock(&lock)at line 199 will be called without a corresponding lock, potentially corrupting the lock state.Apply this diff to fix the lock imbalance:
void PrintKernel(const char* str) { if (!str) return; if (snooze) { PrintToVFShell(str); - goto serial; + SerialWrite(str); + SystemLog(str); + return; } SpinLock(&lock); if (use_vbe) { VBEConsolePrint(str); } else { const uint8_t original_color = console.color; for (const char* p = str; *p; p++) { ConsolePutchar(*p); } console.color = original_color; } -serial: SpinUnlock(&lock); SerialWrite(str); SystemLog(str); }
♻️ Duplicate comments (9)
kernel/etc/Console.c (1)
50-58: LGTM! Proper privilege checking with early boot consideration.The function correctly handles early boot scenarios (when interrupts are disabled) by allowing snooze unconditionally, then restricts it to system processes once the scheduler is active. This implementation aligns with the learning from previous reviews about allowing snooze during early boot.
kernel/core/Compositor.c (8)
277-280: LGTM! Background clearing properly implemented.The background clearing now correctly uses a loop to set each 32-bit pixel value, addressing the previous issue with FastMemset.
289-298: Excellent implementation of window clipping.The window rendering correctly handles negative positions and partial off-screen windows by computing the overlapping region and using FastMemcpy for efficient scanline copying. This addresses the previous concerns about buffer underflow.
365-378: LGTM! Proper doubly-linked list management in DestroyWindow.The function correctly updates both prev/next pointers of neighboring windows before updating head/tail, preventing list corruption. The title string is also properly freed.
341-350: LGTM! Title string properly copied to avoid dangling pointers.The implementation correctly allocates and copies the title string, with proper rollback on allocation failure.
432-448: LGTM! Window movement properly constrained.The window movement logic correctly prevents windows from being moved completely off-screen while keeping at least the title bar visible.
454-479: LGTM! Z-order management properly implemented.The implementation correctly:
- Iterates from tail (topmost) to find the clicked window
- Breaks on first match to get the topmost window
- Moves the focused window to the tail for correct rendering order
206-254: Unsnooze() is unreachable after infinite loop.The Unsnooze() call at line 253 will never be executed because it comes after an infinite
while(1)loop. This means console output will remain suppressed indefinitely once the compositor starts.Based on the learnings, consider moving Unsnooze to the appropriate lifecycle point or removing the unreachable code:
void VFCompositor(void) { Snooze(); if (!VBEIsInitialized()) { PrintKernel("VFCompositor: VBE not initialized, waiting...\n"); while (!VBEIsInitialized()) { MLFQYield(); } } WindowManagerInit(); // Create VFShell window and cache reference g_vfshell_window = CreateWindow(50, 50, 640, 480, "VFShell"); if (g_vfshell_window) { WindowInitTextMode(g_vfshell_window); WindowPrintString(g_vfshell_window, "VFShell - Kernel Log\n"); WindowPrintString(g_vfshell_window, "===================\n\n"); } + Unsnooze(); // Re-enable console output after initialization + while (1) { // ... event loop ... } - - Unsnooze(); }
302-303: Framebuffer copy should respect VBE pitch.The direct memory copy assumes the framebuffer pitch equals width * 4, but VBE often adds padding at the end of each scanline. This could cause display corruption or memory access violations.
Apply this diff to handle VBE pitch correctly:
- FastMemcpy((void*)g_vbe_info->framebuffer, g_compositor_buffer, g_vbe_info->width * g_vbe_info->height * 4); + const uint32_t bpp = g_vbe_info->bpp; + const uint32_t pitch = g_vbe_info->pitch; + if (bpp != 32 || pitch == 0) { + // Unsupported pixel format + return; + } + uint8_t* dst = (uint8_t*)g_vbe_info->framebuffer; + uint8_t* src = (uint8_t*)g_compositor_buffer; + const uint32_t row_bytes = g_vbe_info->width * 4; + for (uint32_t row = 0; row < g_vbe_info->height; row++) { + FastMemcpy(dst + row * pitch, src + row * row_bytes, row_bytes); + }
🧹 Nitpick comments (3)
kernel/etc/Console.c (1)
347-347: Consider implementing the snooze check in PrintKernelAt.The commented-out snooze check suggests this was considered but not implemented. If PrintKernelAt should respect snooze mode like PrintKernel does, either implement the check or remove the comment to avoid confusion.
Consider either implementing:
- // if (snooze) return; + if (snooze) return;Or removing the comment entirely if this function should always print regardless of snooze state.
kernel/core/Compositor.h (1)
14-20: Consider documenting the WindowTextState structure.The
cursor_rowfield is being repurposed as a window identifier in the implementation (line 64-65 of Compositor.c), which is non-obvious from the structure definition. Consider adding a comment or using a union to make this dual-purpose clear.Consider documenting the overloaded usage:
typedef struct { char buffer[WINDOW_TEXT_ROWS][WINDOW_TEXT_COLS]; - int cursor_row; + int cursor_row; // Also used as window ID in GetWindowTextState int cursor_col; int scroll_offset; bool needs_refresh; } WindowTextState;kernel/core/Compositor.c (1)
232-241: Optimize text rendering to avoid character-by-character drawing.The current implementation calls WindowDrawString for each character individually, which is inefficient. Consider batching consecutive characters into strings.
Apply this optimization:
- for (int col = 0; col < WINDOW_TEXT_COLS && state->buffer[row][col] != 0; col++) { - char single_char[2] = {state->buffer[row][col], 0}; - WindowDrawString(g_vfshell_window, text_x, text_y, single_char, TERMINAL_TEXT); - text_x += FONT_WIDTH; - } + // Draw entire row as a string for efficiency + char row_buffer[WINDOW_TEXT_COLS + 1]; + int len = 0; + for (int col = 0; col < WINDOW_TEXT_COLS && state->buffer[row][col] != 0; col++) { + row_buffer[len++] = state->buffer[row][col]; + } + row_buffer[len] = '\0'; + if (len > 0) { + WindowDrawString(g_vfshell_window, text_x, text_y, row_buffer, TERMINAL_TEXT); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
kernel/core/Compositor.c(1 hunks)kernel/core/Compositor.h(1 hunks)kernel/etc/Console.c(6 hunks)kernel/sched/MLFQ.c(6 hunks)meson.build(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- kernel/sched/MLFQ.c
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: assembler-0
PR: assembler-0/VoidFrame#109
File: kernel/sched/MLFQ.c:1808-1822
Timestamp: 2025-09-04T12:22:28.115Z
Learning: The user is implementing a windowing compositor system in their kernel and wants to transition from console-based logging to window-based logging. They need functions to get window references by title and print characters/strings to windows with proper text management including scrolling and cursor positioning.
Learnt from: assembler-0
PR: assembler-0/VoidFrame#109
File: kernel/core/Kernel.c:736-740
Timestamp: 2025-09-03T12:55:44.794Z
Learning: In VoidFrame kernel's window manager initialization, the Snooze() call in WindowManagerInit() suppresses display output by redirecting console operations to serial-only mode, causing the GUI to render but remain invisible to the user.
📚 Learning: 2025-09-04T12:22:28.115Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#109
File: kernel/sched/MLFQ.c:1808-1822
Timestamp: 2025-09-04T12:22:28.115Z
Learning: The user is implementing a windowing compositor system in their kernel and wants to transition from console-based logging to window-based logging. They need functions to get window references by title and print characters/strings to windows with proper text management including scrolling and cursor positioning.
Applied to files:
kernel/core/Compositor.hkernel/core/Compositor.c
📚 Learning: 2025-09-03T12:55:44.794Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#109
File: kernel/core/Kernel.c:736-740
Timestamp: 2025-09-03T12:55:44.794Z
Learning: In VoidFrame kernel's window manager initialization, the Snooze() call in WindowManagerInit() suppresses display output by redirecting console operations to serial-only mode, causing the GUI to render but remain invisible to the user.
Applied to files:
kernel/etc/Console.ckernel/core/Compositor.c
📚 Learning: 2025-08-23T10:06:02.997Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#95
File: kernel/core/Panic.c:65-67
Timestamp: 2025-08-23T10:06:02.997Z
Learning: In VoidFrame kernel's panic handler, the VBE graphics path intentionally mixes VBEShowPanic() with console text operations (PrintKernel*, PrintKernelAt) to overlay stack traces and debugging information on top of the panic image. This overlap is deliberate to ensure critical diagnostic information remains visible during kernel panics, prioritizing debugging capability over visual presentation.
Applied to files:
kernel/etc/Console.c
📚 Learning: 2025-08-10T06:03:22.076Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#62
File: kernel/core/Panic.c:69-79
Timestamp: 2025-08-10T06:03:22.076Z
Learning: In VoidFrame kernel, after VBEShowPanic() draws a panic image to the framebuffer, continuing to use text console operations like PrintKernelAt() causes display corruption because it tries to render text on top of the raw pixel data. The panic handler must choose either pure VBE graphics mode (show image only) or pure text console mode, but not mix both.
Applied to files:
kernel/etc/Console.c
📚 Learning: 2025-08-10T09:50:21.823Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#63
File: kernel/etc/Shell.c:24-28
Timestamp: 2025-08-10T09:50:21.823Z
Learning: VoidFrame's VBE graphics can be optimized by using FastMemcpy for entire scanlines instead of pixel-by-pixel VBEPutPixel calls, and SSE2 instructions can fill/gradient 4 pixels at once for 4-8x performance gains.
Applied to files:
kernel/core/Compositor.c
📚 Learning: 2025-08-10T09:50:21.823Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#63
File: kernel/etc/Shell.c:24-28
Timestamp: 2025-08-10T09:50:21.823Z
Learning: VoidFrame's VBE graphics can be optimized by using FastMemcpy for entire scanlines instead of pixel-by-pixel VBEPutPixel calls, reducing function calls from millions to thousands for full-screen operations.
Applied to files:
kernel/core/Compositor.c
📚 Learning: 2025-08-10T06:03:22.076Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#62
File: kernel/core/Panic.c:69-79
Timestamp: 2025-08-10T06:03:22.076Z
Learning: In VoidFrame kernel's VBEConsoleClear() function, the color is hardcoded to 0x08 (gray on black) instead of using the current console color state, causing color washing issues. The function should use console.color or the current color state to maintain color consistency across screen clears.
Applied to files:
kernel/core/Compositor.c
📚 Learning: 2025-08-10T06:03:22.076Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#62
File: kernel/core/Panic.c:69-79
Timestamp: 2025-08-10T06:03:22.076Z
Learning: In VoidFrame kernel, mixing VBE graphics mode operations (like VBEShowPanic drawing to framebuffer) with text console operations causes display corruption with random pixels. The panic handler must choose either pure VBE graphics mode or pure text mode, not both simultaneously.
Applied to files:
kernel/core/Compositor.c
🧬 Code graph analysis (3)
kernel/core/Compositor.h (1)
kernel/core/Compositor.c (16)
GetWindowByTitle(27-43)WindowInitTextMode(73-94)WindowPrintChar(115-176)WindowPrintString(179-186)WindowScrollUp(97-112)WindowClearText(189-202)GetWindowTextState(46-70)WindowManagerInit(307-321)WindowManagerRun(323-325)CreateWindow(327-362)DestroyWindow(365-378)WindowFill(380-385)WindowDrawRect(387-396)WindowDrawString(398-417)VFCompositor(205-254)GetVFShellWindow(257-259)
kernel/etc/Console.c (3)
kernel/core/Compositor.c (2)
GetVFShellWindow(257-259)WindowPrintString(179-186)drivers/Serial.c (1)
SerialWrite(129-138)kernel/sched/MLFQ.c (1)
MLFQGetCurrentProcess(1238-1243)
kernel/core/Compositor.c (7)
kernel/atomic/Spinlock.h (2)
SpinLockIrqSave(124-129)SpinUnlockIrqRestore(131-134)kernel/etc/StringOps.c (3)
FastStrCmp(25-35)StringLength(3-8)strcpy(66-97)mm/MemOps.c (2)
FastMemset(10-73)FastMemcpy(75-162)kernel/etc/Console.c (5)
Snooze(50-58)PrintKernel(181-202)Unsnooze(59-61)PrintKernelError(223-228)PrintKernelSuccess(216-221)drivers/Vesa.c (2)
VBEIsInitialized(325-327)VBEGetInfo(321-323)kernel/sched/MLFQ.c (1)
MLFQYield(999-1008)mm/KernelHeap.c (2)
KernelMemoryAlloc(272-330)KernelFree(377-422)
🔇 Additional comments (4)
kernel/etc/Console.c (1)
41-48: LGTM! Smart fallback logic for VFShell output.The function correctly handles the case where the VFShell window isn't available yet by falling back to serial output, ensuring logs aren't lost during initialization.
meson.build (2)
93-93: LGTM! Compositor and font integration properly added to build.The new compositor and font source files are correctly added to the kernel build inputs.
Also applies to: 131-131
152-152: Build configuration correctly enables compositor and snooze features.The VF_CONFIG_ENABLE_VFCOMPOSITOR and VF_CONFIG_SNOOZE_ON_BOOT flags are properly configured to enable the new windowing system.
Also applies to: 156-156
kernel/core/Compositor.h (1)
1-40: Well-structured compositor API with comprehensive window management.The header provides a clean and complete API for window management, text rendering, and compositor control. The constants for text dimensions and font metrics are appropriate, and the function declarations follow a logical organization.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
kernel/etc/Console.c (2)
181-199: Bug: unlocking spinlock without locking when snoozed.
if (snooze) goto serial;jumps pastSpinLockand then executesSpinUnlock(&lock), causing undefined behavior.Apply this fix to early-return on snooze and avoid unlocking an unheld lock. Also routes to VFS shell window when available:
void PrintKernel(const char* str) { if (!str) return; - if (snooze) goto serial; + if (snooze) { + // Prefer VFS shell window; falls back to serial inside. + PrintToVFShell(str); + SystemLog(str); + return; + } SpinLock(&lock); if (use_vbe) { VBEConsolePrint(str); } else { const uint8_t original_color = console.color; for (const char* p = str; *p; p++) { ConsolePutchar(*p); } console.color = original_color; } -serial: SpinUnlock(&lock); SerialWrite(str); SystemLog(str); }
340-366: Guard and serialize PrintKernelAt; align logging behavior.Currently it always writes to serial first, doesn’t honor
snooze, and updates VGA buffers without the console lock—risking interleaving withPrintKernel.Apply this refactor:
void PrintKernelAt(const char* str, uint32_t line, uint32_t col) { if (!str) return; - SerialWrite(str); - SerialWrite("\n"); - // if (snooze) return; + if (snooze) { + PrintToVFShell(str); + SystemLog(str); + return; + } + SpinLock(&lock); if (use_vbe) { VBEConsoleSetCursor(col, line); VBEConsolePrint(str); } else { if (line >= VGA_HEIGHT || col >= VGA_WIDTH) return; const uint32_t saved_line = console.line; const uint32_t saved_col = console.column; console.line = line; console.column = col; for (const char* p = str; *p && console.column < VGA_WIDTH; p++) { if (*p == '\n') break; ConsolePutchar(*p); } console.line = saved_line; console.column = saved_col; } + SpinUnlock(&lock); + SerialWrite(str); + SerialWrite("\n"); + SystemLog(str); }
🧹 Nitpick comments (9)
.gitignore (1)
11-11: Avoid over-broad*cache*; it may hide legit sources.
*cache*matches any path containing “cache” (e.g., headers like include/cacheline.h). Replace with targeted cache patterns.Apply:
-.venv -*cache* +.venv +# Cache directories +__pycache__/ +.pytest_cache/ +.cache/ +cache/ +**/cache/**If you also have build caches under build/, add:
build/**/.cache/..github/workflows/main.yaml (5)
4-11: Add manual trigger and cancel in-progress runs for the same ref.
This improves operability and CI cost.on: push: branches: [ main, dev ] pull_request: branches: [ main ] release: types: [created] + workflow_dispatch: + +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true
35-39: Harden build step and surface build config flags explicitly.
Use strict shell options and set build type. If compositor/Snooze are Meson options, pass them here.- - name: Build - run: | - meson setup build - ninja -C build + - name: Build + run: | + set -euxo pipefail + meson setup build --buildtype=release ${MESON_ARGS:-} + ninja -C buildIf applicable, set in workflow/env or job.env:
MESON_ARGS: "-Dvf_config_enable_vfcompositor=true -Dvf_config_snooze_on_boot=true"(adjust to your Meson option names).Confirm your Meson option names for these features.
40-56: Check artifacts before running QEMU; simplify headless flags.
Fail fast if ISO is missing and prefer-nographicfor CI.- - name: Basic smoke test + - name: Basic smoke test run: | - timeout 30s qemu-system-x86_64 \ - -cdrom VoidFrame.iso \ - -m 256M \ - -display none \ - -serial stdio \ - -no-reboot \ - -no-shutdown || true - - ls -la voidframe.krnl - test -s voidframe.krnl - - # Check if ISO was created - ls -la VoidFrame.iso - test -s VoidFrame.iso + set -euxo pipefail + # Verify outputs exist + test -s voidframe.krnl + test -s VoidFrame.iso + # Boot ISO headlessly; allow failure but capture output + timeout 30s qemu-system-x86_64 \ + -cdrom VoidFrame.iso \ + -m 256M \ + -nographic \ + -no-reboot \ + -no-shutdown || trueIf Meson places outputs under build/, switch to
build/voidframe.krnlandbuild/VoidFrame.iso.Please confirm artifact paths (root vs build/).
57-64: Always upload artifacts; consider including logs.
Ensure artifacts are uploaded even if smoke test fails.- - name: Upload kernel artifacts + - name: Upload kernel artifacts + if: always() uses: actions/upload-artifact@v4 with: name: voidframe-build-${{ github.sha }} path: | voidframe.krnl VoidFrame.iso retention-days: 30Optionally add
build/meson-logs/and the QEMU console log for debugging.
15-15: Fix YAML lint issues (trailing spaces and EOF newline).
Removes trailing spaces on blank lines and adds a newline at EOF.- runs-on: ubuntu-latest - + runs-on: ubuntu-latest @@ - + @@ - + @@ - retention-days: 30 + retention-days: 30 +Also applies to: 49-49, 52-52, 64-64
kernel/etc/Console.c (3)
17-18: Make snooze flag concurrency-safe.
snoozeis read/written from different contexts without ordering. Use READ_ONCE/WRITE_ONCE helpers (if available) or mark itvolatileas a minimal step.Apply this minimal change if you don’t have READ_ONCE/WRITE_ONCE yet:
-static uint8_t snooze = 0; +static volatile uint8_t snooze = 0;
41-48: Wire this into the snooze path or drop it.
PrintToVFShellis currently unused (static). Either call it from the snooze path inPrintKernel/PrintKernelAt(preferred) or remove it to avoid dead code.
50-61: Privilege gating LGTM; consider visibility guarantees.Logic for early-boot (IF=0) and post-boot privilege check is good. Pair with READ/WRITE_ONCE on
snoozeto ensure prompt visibility on SMP.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
.github/workflows/main.yaml(1 hunks).gitignore(1 hunks)kernel/core/Compositor.c(1 hunks)kernel/core/Kernel.h(1 hunks)kernel/etc/Console.c(6 hunks)meson.build(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- meson.build
- kernel/core/Compositor.c
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: assembler-0
PR: assembler-0/VoidFrame#109
File: kernel/sched/MLFQ.c:1808-1822
Timestamp: 2025-09-04T12:22:28.115Z
Learning: The user is implementing a windowing compositor system in their kernel and wants to transition from console-based logging to window-based logging. They need functions to get window references by title and print characters/strings to windows with proper text management including scrolling and cursor positioning.
Learnt from: assembler-0
PR: assembler-0/VoidFrame#109
File: kernel/core/Kernel.c:736-740
Timestamp: 2025-09-03T12:55:44.794Z
Learning: In VoidFrame kernel's window manager initialization, the Snooze() call in WindowManagerInit() suppresses display output by redirecting console operations to serial-only mode, causing the GUI to render but remain invisible to the user.
📚 Learning: 2025-09-03T12:55:44.794Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#109
File: kernel/core/Kernel.c:736-740
Timestamp: 2025-09-03T12:55:44.794Z
Learning: In VoidFrame kernel's window manager initialization, the Snooze() call in WindowManagerInit() suppresses display output by redirecting console operations to serial-only mode, causing the GUI to render but remain invisible to the user.
Applied to files:
kernel/etc/Console.c
📚 Learning: 2025-08-23T10:06:02.997Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#95
File: kernel/core/Panic.c:65-67
Timestamp: 2025-08-23T10:06:02.997Z
Learning: In VoidFrame kernel's panic handler, the VBE graphics path intentionally mixes VBEShowPanic() with console text operations (PrintKernel*, PrintKernelAt) to overlay stack traces and debugging information on top of the panic image. This overlap is deliberate to ensure critical diagnostic information remains visible during kernel panics, prioritizing debugging capability over visual presentation.
Applied to files:
kernel/etc/Console.c
🧬 Code graph analysis (1)
kernel/etc/Console.c (3)
kernel/core/Compositor.c (2)
GetVFShellWindow(256-258)WindowPrintString(178-185)drivers/Serial.c (1)
SerialWrite(129-138)kernel/sched/MLFQ.c (1)
MLFQGetCurrentProcess(1238-1243)
🪛 YAMLlint (1.37.1)
.github/workflows/main.yaml
[error] 15-15: trailing spaces
(trailing-spaces)
[error] 49-49: trailing spaces
(trailing-spaces)
[error] 52-52: trailing spaces
(trailing-spaces)
[error] 64-64: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (3)
.gitignore (1)
10-10: LGTM on .venv ignore.
This is a sensible addition for local Python tooling..github/workflows/main.yaml (1)
22-34: Verify QEMU binary availability.
You installqemu-system-x86but runqemu-system-x86_64. On Ubuntu this is usually fine (the package provides the 64-bit binary), but please confirm.Run
qemu-system-x86_64 --versionin the job to ensure it’s present.kernel/etc/Console.c (1)
3-6: Includes look correct.Compositor.h and MLFQ.h are needed for the new window routing and privilege check.
1 participant
Summary by CodeRabbit
New Features
Behavioral Changes
Chores