Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Multiple critical and moderate issues remain unresolved, including unsafe PS/2 waits, buffer overruns, and the queue linkage failure.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 7
Open (16)
What changed in this PR
Adds a modular PS/2 keyboard subsystem, string/stdio utilities, VGA enhancements, and unified type headers.
Changes:
- Adds keyboard layouts, scan-code handling, IRQ integration, and event queues.
- Adds PS/2 controller communication and initialization.
- Adds VGA scrolling/colors and formatting utilities.
| File | Reviewed changes and final findings |
|---|---|
src/libs/string.c |
String/integer conversion helpers. moderate (3 votes): int2str(INT_MIN) overflows when negated. |
src/libs/stdio.c |
Formatting and console output. critical (3 votes): zero-size buffers cause out-of-bounds writes. critical (3 votes): unbounded appends overflow the 1024-byte buffer. moderate (2 votes): trailing % advances past the terminator. |
src/kernel/kernel.c |
Initialization and keyboard event loop. moderate (3 votes): queue polling busy-spins and makes hlt unreachable, also appearing on line 64. moderate (3 votes): initialization reports success despite device failures. |
src/interrupt/pit.c |
Unified type usage. |
src/interrupt/irq/irq.c |
Keyboard IRQ dispatch. |
src/driver/vga.c |
Cursor controls and scrolling. critical (3 votes): full-screen writes can advance past the VGA buffer. |
src/driver/ps2.c |
PS/2 controller communication. critical (3 votes): unbounded waits can spin forever. moderate (1 vote): port 2 is enabled despite IRQ12 being disabled. moderate (3 votes): response reads can consume stale data without waiting for output. |
src/driver/pit.c |
PIT driver entry. |
src/driver/keyboard/layout/us.c |
US keyboard translation. moderate (3 votes): Caps Lock state is ignored for alphabetic input. |
src/driver/keyboard/keyboard.c |
Keyboard setup and commands. moderate (3 votes): invalid values greater than 3 still wait for an ACK and send raw data. |
src/driver/keyboard/handler.c |
Scan-code and key-state handling. critical (1 vote): the IRQ handler can spin forever waiting for device output. |
src/driver/keyboard/event_queue.c |
Input event ring buffer. moderate (3 votes): wrapped queue counts underflow and indexing starts from the wrong position. |
includes/utils/utils.h |
Unified type include. |
includes/utils/types.h |
Shared type definitions. |
includes/timer.h |
Unified type include. |
includes/lib/vga_lib.h |
Unified type include. |
includes/lib/string.h |
String API. |
includes/lib/stdio.h |
Formatting API. |
includes/interrupt/pit.h |
Unified type include. |
includes/interrupt/pic.h |
Unified type include. |
includes/interrupt/isr.h |
Unified type include. |
includes/interrupt/idt.h |
Unified type include. |
includes/driver/vga.h |
VGA constants and scrolling API. critical (3 votes): DARK_GRAY references undefined BLACk. |
includes/driver/ps2.h |
PS/2 API and structures. |
includes/driver/keyboard/keyboard.h |
Keyboard API. nit (2 votes): rename misspelled set_scaning to set_scanning. |
includes/driver/keyboard/keyboard_layout.h |
Layout definitions. |
includes/driver/keyboard/keyboard_keys.h |
Key and scan-code definitions. |
includes/driver/keyboard/handler.h |
Keyboard handler API. |
includes/driver/keyboard/event_queue.h |
Event queue API. critical (3 votes): declared keyboard_queue has no implementation definition, causing link failures for consumers. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| uint32_t tail; | ||
| } keyboard_event_queue_t; | ||
|
|
||
| extern keyboard_event_queue_t keyboard_queue; |
| #define BROWN 0x06 // YELLOW | ||
| #define LIGHT_GRAY 0x07 // WHITE | ||
|
|
||
| #define DARK_GRAY BLACk | LIGHT_FLAG |
Comment on lines
+16
to
+18
| if (!ps2_wait_output_full()) { | ||
| return; | ||
| } |
Comment on lines
+31
to
+34
| bool ps2_wait_output_full(void) { | ||
| while (!ps2_output_buffer_full()) | ||
| ; | ||
| return true; |
Comment on lines
+103
to
+105
| vga_buf[cursor_position] = ((uint16_t)current_color << 8) | (uint8_t)c; | ||
|
|
||
| vga_set_cursor_position(cursor_position + 1); |
Comment on lines
+31
to
+33
| init_function(); | ||
|
|
||
| print_at_end("... Ok\n", GREEN); |
Comment on lines
+63
to
+66
| while (1) { | ||
| input_event_t event; | ||
| input_queue_pop(&event); | ||
|
|
Comment on lines
+147
to
+149
| format++; | ||
|
|
||
| switch (*format) { |
Comment on lines
+23
to
+34
| bool negative = num < 0; | ||
|
|
||
| if (negative) | ||
| num = -num; | ||
|
|
||
| while (num > 0) { | ||
| *(--p) = (num % 10) + '0'; | ||
| num /= 10; | ||
| } | ||
|
|
||
| if (negative) | ||
| *(--p) = '-'; |
| void init_keyboard(void); | ||
|
|
||
| void reset_keyboard(void); | ||
| void set_scaning(bool state); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR introduces a modular keyboard driver subsystem and improves overall code consistency.
<stdint.h>usage with the project's unifiedtypes.hacross existing drivers and headers.stdio.hfor basic I/O/formatting andstring.hfor string utilities.