Development #167

assembler-0 · 2025-10-26T11:04:29Z

Chardevice and DEVFS

Summary by CodeRabbit

New Features
- Added cryptographic functionality: CRC32 checksums and SHA‑256 hashing
- Added random number generation with hardware acceleration support
- Added device filesystem for unified device file access (/Devices)
- Added character device registry and management system
- Integrated serial device into the device subsystem and registry
Documentation
- Removed development roadmap (ROADMAP.md)

coderabbitai · 2025-10-26T11:04:39Z

Caution

Review failed

The pull request is closed.

Walkthrough

This PR adds a crypto module (xoroshiro128+ RNG, CRC32, SHA‑256), new headers, and CMake updates to include crypto and fs/devfs. It introduces a CharDevice registry and DevFS implementation mounted at /Devices, and wires the Serial driver to register as a char device. CRC32Init is invoked in kernel bootstrap; A20Test and its calls were removed. docs/ROADMAP.md was deleted, and a README link line and toolchain path were adjusted.

Poem

🐇 I dug a hole and found a key of bits,
RNG hums, CRC checks every little wit,
SHA‑256 paints a tidy, stubborn trace,
DevFS lines up devices in their place,
I twitch my nose — the kernel wakes, all lit! ✨

Pre-merge checks

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 5.71% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.
Title Check	❓ Inconclusive	The pull request title is "Development," which is an extremely generic and non-descriptive term that fails to convey meaningful information about the actual changeset. While the PR description indicates "Chardevice and DEVFS," the title itself provides no insight into what was changed. The changeset is substantial, including a new character device subsystem, a virtual DevFS filesystem, cryptographic functions (CRC32, RNG, SHA256), removal of the roadmap documentation, and integration of these components into the kernel and existing drivers. The title should reflect these primary contributions but instead uses a vague placeholder term that could apply to any development work.	Revise the PR title to be specific and descriptive of the actual changes. A more appropriate title might be something like "Add character device subsystem and DevFS virtual filesystem" or "Implement CharDevice registry and DevFS with crypto utilities" to clearly indicate the main contributions of this changeset and help reviewers understand the scope at a glance.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0b4e2c4 and 1e3a6f2.

📒 Files selected for processing (6)

README.md (3 hunks)
crypto/RNG.c (1 hunks)
drivers/Serial.c (2 hunks)
fs/CharDevice.c (1 hunks)
fs/VFS.c (6 hunks)
kernel/core/Kernel.c (3 hunks)

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 27

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

kernel/core/Kernel.c (1)
383-397: Move CharDeviceInit() to PXS1 before SerialInit(), and remove duplicate initialization from VFS.c.

The verification confirms the issue: SerialInit() (called in PXS1 at line 386) registers the serial device via CharDeviceRegister() at drivers/Serial.c:75, but CharDeviceInit() is currently only called later in VfsInit() (fs/VFS.c:69), clearing the registry after registration. Initialize the registry first in PXS1, then remove it from VFS.

Apply the diffs as proposed:
--- a/kernel/core/Kernel.c
+++ b/kernel/core/Kernel.c
@@ -383,6 +383,8 @@
 void PXS1(const uint32_t info) {
     PICMaskAll();
+    // Initialize char device registry before drivers register
+    CharDeviceInit();
     int sret = SerialInit();
--- a/fs/VFS.c
+++ b/fs/VFS.c
@@ -69 +69 @@
-    CharDeviceInit();
-    PrintKernel("VFS: Char device subsystem initialized\n");
fs/VFS.c (1)
359-369: Add Fat1xSetActive() call and replace size check with proper file-type validation.

Lines 361 missing the required Fat1xSetActive(mount->device) call (present in all other FAT1x operations: lines 162, 195, 226, 253, 281, 306, 333, 386). Additionally, Fat1xGetFileSize(local_path) > 0 fails for zero-length files—VfsCopyFile (line 448) creates them, so they're valid. Since Fat1xIsFile() doesn't exist, either implement it following Fat1xIsDirectory() pattern or check file type directly, matching EXT2/NTFS behavior.
    if (mount->fs_driver) {
        if (FastStrCmp(mount->fs_driver->name, "FAT1x") == 0) {
            Fat1xSetActive(mount->device);
            return Fat1xIsFile(local_path);  // Use proper file check, not size > 0
        } else if (FastStrCmp(mount->fs_driver->name, "EXT2") == 0) {

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6edc6d3 and 0b4e2c4.

📒 Files selected for processing (16)

README.md (0 hunks)
cmake/source.cmake (5 hunks)
crypto/CRC32.c (1 hunks)
crypto/CRC32.h (1 hunks)
crypto/RNG.c (1 hunks)
crypto/RNG.h (1 hunks)
crypto/SHA256.c (1 hunks)
crypto/SHA256.h (1 hunks)
docs/ROADMAP.md (0 hunks)
drivers/Serial.c (2 hunks)
fs/CharDevice.c (1 hunks)
fs/CharDevice.h (1 hunks)
fs/VFS.c (7 hunks)
fs/devfs/DevFS.c (1 hunks)
fs/devfs/DevFS.h (1 hunks)
kernel/core/Kernel.c (2 hunks)

💤 Files with no reviewable changes (2)

README.md
docs/ROADMAP.md

🧰 Additional context used

🧠 Learnings (1)

📚 Learning: 2025-09-01T02:35:28.353Z

Learnt from: assembler-0
PR: assembler-0/VoidFrame#103
File: fs/VFS.c:331-335
Timestamp: 2025-09-01T02:35:28.353Z
Learning: VfsIsFile function was initially implemented as a placeholder that only checked for mount existence. The proper implementation should follow the same pattern as VfsIsDir: find mount, strip mount path, then use filesystem-specific functions to check if the path points to a file (FS_FILE for RAMFS, Fat12 functions for FAT12).

Applied to files:

fs/VFS.c

🧬 Code graph analysis (12)

fs/CharDevice.c (1)

kernel/etc/StringOps.c (1)

FastStrCmp (39-49)

drivers/Serial.c (1)

fs/CharDevice.c (1)

CharDeviceRegister (14-20)

fs/devfs/DevFS.c (2)

fs/CharDevice.c (3)

CharDeviceFind (22-29)

CharDeviceGetCount (38-40)

CharDeviceGet (31-36)

kernel/etc/StringOps.c (1)

FastStrCmp (39-49)

fs/VFS.c (6)

fs/CharDevice.c (1)

CharDeviceInit (7-12)

kernel/etc/Console.c (1)

PrintKernel (198-211)

fs/devfs/DevFS.c (5)

DevfsMount (6-11)

DevfsReadFile (13-22)

DevfsWriteFile (24-31)

DevfsListDir (33-48)

DevfsIsDir (50-56)

fs/FileSystem.c (1)

FileSystemRegister (13-22)

drivers/Serial.c (1)

SerialWrite (165-174)

kernel/etc/StringOps.c (1)

FastStrCmp (39-49)

crypto/CRC32.h (1)

crypto/CRC32.c (2)

CRC32 (20-35)

CRC32Init (5-18)

fs/devfs/DevFS.h (1)

fs/devfs/DevFS.c (5)

DevfsMount (6-11)

DevfsReadFile (13-22)

DevfsWriteFile (24-31)

DevfsListDir (33-48)

DevfsIsDir (50-56)

crypto/RNG.c (1)

include/Io.c (1)

cpuid (25-29)

crypto/SHA256.c (1)

mm/MemOps.c (1)

FastMemset (34-49)

fs/CharDevice.h (1)

fs/CharDevice.c (5)

CharDeviceInit (7-12)

CharDeviceRegister (14-20)

CharDeviceFind (22-29)

CharDeviceGet (31-36)

CharDeviceGetCount (38-40)

crypto/SHA256.h (1)

crypto/SHA256.c (2)

SHA256Update (78-88)

SHA256Final (90-124)

crypto/RNG.h (1)

crypto/RNG.c (6)

rng_seed (20-23)

xoroshiro128plus (10-18)

rdrand_supported (25-29)

rdrand16 (31-35)

rdrand32 (37-41)

rdrand64 (43-47)

kernel/core/Kernel.c (2)

kernel/etc/Console.c (2)

PrintKernel (198-211)

PrintKernelSuccess (225-230)

crypto/CRC32.c (1)

CRC32Init (5-18)

🪛 Clang (14.0.6)

crypto/CRC32.c

[warning] 3-3: variable 'crc32_table' is non-const and globally accessible, consider making it const

(cppcoreguidelines-avoid-non-const-global-variables)

[warning] 20-20: parameter 'data' is unused

(misc-unused-parameters)

[warning] 20-20: parameter 'length' is unused

(misc-unused-parameters)

[warning] 28-28: variable 'p' is not initialized

(cppcoreguidelines-init-variables)

[warning] 28-28: variable name 'p' is too short, expected at least 3 characters

(readability-identifier-length)

fs/CharDevice.c

[warning] 4-4: variable 'g_char_devices' is non-const and globally accessible, consider making it const

(cppcoreguidelines-avoid-non-const-global-variables)

[warning] 5-5: variable 'g_num_char_devices' is non-const and globally accessible, consider making it const

(cppcoreguidelines-avoid-non-const-global-variables)

drivers/Serial.c

[warning] 41-41: parameter 'dev' is unused

(misc-unused-parameters)

[warning] 41-41: parameter 'size' is unused

(misc-unused-parameters)

[warning] 42-42: statement should be inside braces

(readability-braces-around-statements)

[warning] 46-46: variable name 'c' is too short, expected at least 3 characters

(readability-identifier-length)

[warning] 55-55: parameter 'dev' is unused

(misc-unused-parameters)

[warning] 55-55: parameter 'size' is unused

(misc-unused-parameters)

[warning] 56-56: statement should be inside braces

(readability-braces-around-statements)

[warning] 66-66: variable 'g_serial_device' is non-const and globally accessible, consider making it const

(cppcoreguidelines-avoid-non-const-global-variables)

[warning] 73-73: variable 'result' is not initialized

(cppcoreguidelines-init-variables)

fs/devfs/DevFS.c

[warning] 13-13: parameter 'max_size' is unused

(misc-unused-parameters)

[warning] 24-24: parameter 'size' is unused

(misc-unused-parameters)

fs/VFS.c

[warning] 86-86: variable 'result' is not initialized

(cppcoreguidelines-init-variables)

crypto/CRC32.h

[error] 4-4: 'stdint.h' file not found

(clang-diagnostic-error)

crypto/RNG.c

[warning] 4-4: variable 's' is non-const and globally accessible, consider making it const

(cppcoreguidelines-avoid-non-const-global-variables)

[warning] 4-4: variable name 's' is too short, expected at least 3 characters

(readability-identifier-length)

[warning] 20-20: 2 adjacent parameters of 'rng_seed' of similar type ('int') are easily swapped by mistake

(bugprone-easily-swappable-parameters)

[note] 20-20: the first parameter in the range is 'a'

(clang)

[note] 20-20: the last parameter in the range is 'b'

(clang)

[warning] 20-20: parameter name 'a' is too short, expected at least 3 characters

(readability-identifier-length)

[warning] 20-20: parameter name 'b' is too short, expected at least 3 characters

(readability-identifier-length)

[warning] 26-26: multiple declarations in a single statement reduces readability

(readability-isolate-declaration)

[warning] 26-26: variable 'eax' is not initialized

(cppcoreguidelines-init-variables)

[warning] 26-26: variable 'ebx' is not initialized

(cppcoreguidelines-init-variables)

[warning] 26-26: variable 'ecx' is not initialized

(cppcoreguidelines-init-variables)

[warning] 26-26: variable 'edx' is not initialized

(cppcoreguidelines-init-variables)

[warning] 28-28: narrowing conversion from 'unsigned int' to signed type 'int' is implementation-defined

(bugprone-narrowing-conversions,cppcoreguidelines-narrowing-conversions)

[warning] 28-28: 30 is a magic number; consider replacing it with a named constant