Repository navigation
Update serial device to support reading functionality - #44
Merged
automas-dev merged 3 commits intoSep 12, 2026
Merged
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Critical write-buffering and blocking-read issues remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Enhances the serial device driver with buffered reads, per-port state, and updated I/O APIs.
Changes:
- Adds bulk and single-character read support.
- Stores the selected serial port in the device structure.
- Updates write functions to return byte counts.
- Updates API documentation and signatures.
File summaries
| File | Summary | Findings |
|---|---|---|
src/kernel/src/kernel/device/serial.c |
Adds port-aware device callbacks and cleanup. | Critical: blocking reads can spin in kernel mode without yielding. |
src/kernel/src/drivers/serial.c |
Implements buffered reads and count-returning writes. | Critical: writes may lose bytes despite reporting success. Nit: add serial-driver tests. |
src/kernel/include/kernel/device/serial.h |
Updates the device-open signature. | None. |
src/kernel/include/drivers/serial.h |
Documents the updated serial APIs. | None. |
Review details
Suppressed comments (1)
src/kernel/src/drivers/serial.c:46
- This adds a blocking public read path without corresponding serial-driver tests. The repository already has driver tests and port-I/O fakes, so please cover null/zero-length buffers, the requested byte count, and the port-specific polling/read behavior to prevent regressions that can otherwise hang the test target or read from the wrong UART.
size_t serial_read(uint16_t port, char * buff, size_t count) {
if (!buff) {
return 0;
}
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
automas-dev
deleted the
agents/update-serial-device-read-support-68b305b0
branch
September 12, 2026 20:46
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.
This pull request enhances the serial device driver to support reading operations, along with several structural improvements. The key changes include:
serial_readfunction that allows reading a specified number of bytes into a buffer. The previous single-character read function was renamed toserial_read_char.serial_device_tstruct to hold the port information, following the pattern used in other device implementations.serial_writeandserial_write_strfunctions to return the number of bytes written, improving error handling.io_device_serial_openfunction to accept a port parameter, which is stored in the new device struct.These changes improve the functionality and maintainability of the serial device driver, aligning it with existing patterns in the codebase.