Unify interface, add memory mapping #704

Grazfather · 2025-10-05T16:43:07Z

Unify interface to read/writes in IO and RAM
Add a memory space that also conforms to this interface
Store these interface objects directly in the CPU
mcu.zig that contains configs for different MCUs
Add build_spaces helper that uses an mcu config to create data, io, and eeprom data spaces

ikskuh · 2025-10-05T18:20:19Z

sim/aviron/build.zig

+    // Run unit tests for low-level library components (e.g., memory.zig)
+    const memory_tests = b.addTest(.{
+        .root_module = b.createModule(.{
+            .root_source_file = b.path("src/lib/memory.zig"),
+            .target = host_target,
+            .optimize = optimize,
+        }),
+        .use_llvm = true,
+    });
+    test_step.dependOn(&b.addRunArtifact(memory_tests).step);
+


Why do you add a separate executable? You can just pull in memory.zig in main.zig:

test { _ = @import("lib/memory.zig"); }

This will compile much much faster than using a separate unit test per file and won't make problems in the future

When I tried this it complained that the file was already imported in aviron.zig

sim/aviron/src/lib/Cpu.zig

sim/aviron/src/lib/io.zig

ikskuh · 2025-10-05T18:25:04Z

sim/aviron/src/lib/io.zig

+    pub const VTable = struct {
+        read8: *const fn (ctx: *anyopaque, idx: usize) u8,
+        write8: *const fn (ctx: *anyopaque, idx: usize, v: u8) void,
+        write_masked: *const fn (ctx: *anyopaque, idx: usize, mask: u8, v: u8) void,


write_masked can have a default impl which does a read-modify-write cycle by default.
If an implementation requires true atomics, they can implement it manually

sim/aviron/src/lib/io.zig

sim/aviron/src/lib/memory.zig

sim/aviron/src/main.zig

ikskuh

I really like the overall changes, but i want to make this incredibly good so we can build a nice and well suited test infrastructure on top of this

ikskuh · 2025-10-06T07:41:46Z

sim/aviron/src/lib/bus.zig

+    pub fn read(mem: Flash, addr: Address) u16 {
+        std.debug.assert(addr < mem.size);
+        return mem.vtable.readFn(mem.ctx, addr);
+    }


We could add a usage_hint: UsageHint here where UsageHint = enum { code, data}.

This would allow us logging memory access patterns in traces, which is sometimes incredibly useful to debug hard to find bugs or optimize algorithms to go as fast as possible

ikskuh · 2025-10-06T07:43:27Z

sim/aviron/src/lib/bus.zig

+
+pub const IO = struct {
+    // Some AVR families (e.g., XMEGA) expose extended I/O up to 0xFFF (12 bits).
+    pub const Address = u12;


I would still use u6 here as that's the width of the IO bus. If we actually need the "extended I/O", it would go through the Bus class anyways, so we have to expose two interfaces (and it may be interesting for logging to see how I/O is performed)

ikskuh · 2025-10-06T07:45:20Z

sim/aviron/src/lib/bus.zig

+pub const Bus = struct {
+    pub const Address = u24;


I guess we should rename this into Data, as bus.Data reads really fine

Otherwise we could also use Flash_Bus, IO_Bus and Data_Bus

ikskuh · 2025-10-06T07:45:40Z

sim/aviron/src/lib/bus.zig

+    pub fn write_masked(self: *const Bus, addr: Address, mask: u8, v: u8) void {
+        self.vtable.write_masked(self.ctx, addr, mask, v);
+    }


A data bus member doesn't need this function, as we can't have masked writes on the data bus, right?

Sure, but as it stands the Cpu doesn't access IO through anything but the IO bus, so e.g. sbi wants this method on the interface.

ikskuh · 2025-10-06T07:49:21Z

sim/aviron/src/lib/bus.zig

+/// A mapping entry within a MemoryMapping.
+pub const Segment = struct {
+    /// Base address within the enclosing memory space.
+    at: Bus.Address,
+    /// Size in bytes of the mapped range.
+    size: Bus.Address,
+    /// Backend handling the mapped range starting at index 0.
+    backend: Bus,
+};
+
+/// A logical memory space composed of non-overlapping segments.
+pub const MemoryMapping = struct {
+    const Self = @This();
+
+    segments: []const Segment, // sorted by .at, owned by this struct
+
+    pub const InitError = error{OverlappingSegments} || std.mem.Allocator.Error;
+    pub const AccessError = error{ Unmapped, ReadOnly, OutOfRange };
+
+    pub fn init(alloc: std.mem.Allocator, segs: []const Segment) InitError!Self {
+        // Copy segments to owned memory
+        const owned = try alloc.alloc(Segment, segs.len);
+        @memcpy(owned, segs);
+
+        // Sort by base address
+        std.sort.block(Segment, owned, {}, struct {
+            fn lessThan(_: void, a: Segment, b: Segment) bool {
+                return a.at < b.at;
+            }
+        }.lessThan);
+
+        // Validate non-overlap
+        if (owned.len > 1) {
+            var i: usize = 0;
+            while (i + 1 < owned.len) : (i += 1) {
+                const cur_end = owned[i].at + owned[i].size;
+                if (cur_end > owned[i + 1].at) {
+                    alloc.free(owned);
+                    return error.OverlappingSegments;
+                }
+            }
+        }
+
+        return .{ .segments = owned };
+    }
+
+    pub fn deinit(self: *const Self, alloc: std.mem.Allocator) void {
+        alloc.free(self.segments);
+    }
+
+    pub fn read(self: *const Self, addr: Bus.Address) AccessError!u8 {
+        const seg = self.find(addr) orelse return error.Unmapped;
+        const idx = addr - seg.at;
+        if (idx >= seg.size) return error.OutOfRange;
+        return seg.backend.read(idx);
+    }
+
+    pub fn write(self: *const Self, addr: Bus.Address, v: u8) AccessError!void {
+        const seg = self.find(addr) orelse return error.Unmapped;
+        const idx = addr - seg.at;
+        if (idx >= seg.size) return error.OutOfRange;
+        seg.backend.write(idx, v);
+        return;
+    }
+
+    pub fn write_masked(self: *const Self, addr: Bus.Address, mask: u8, v: u8) AccessError!void {
+        const seg = self.find(addr) orelse return error.Unmapped;
+        const idx = addr - seg.at;
+        if (idx >= seg.size) return error.OutOfRange;
+        seg.backend.write_masked(idx, mask, v);
+        return;
+    }
+
+    fn find(self: *const Self, addr: Bus.Address) ?*const Segment {
+        // Linear scan is fine initially; segments are sorted.
+        for (self.segments) |*s| {
+            if (addr >= s.at and addr < s.at + s.size) return s;
+        }
+        return null;
+    }
+
+    /// Returns a Bus interface for this MemoryMapping.
+    /// The returned Bus uses absolute addressing within this space (not segment-relative).
+    pub fn bus(self: *Self) Bus {
+        return .{
+            .ctx = @as(*anyopaque, @ptrCast(self)),
+            .vtable = &bus_vtable,
+        };
+    }
+
+    const bus_vtable = Bus.VTable{
+        .read = bus_read,
+        .write = bus_write,
+        .write_masked = bus_write_masked,
+        .check_exit = bus_check_exit,
+    };
+
+    fn bus_read(ctx: *anyopaque, addr: Bus.Address) u8 {
+        const self: *const Self = @ptrCast(@alignCast(ctx));
+        return self.read(addr) catch |e| switch (e) {
+            error.Unmapped => @panic("Read from unmapped memory address"),
+            error.OutOfRange => @panic("Read out of range"),
+            error.ReadOnly => @panic("ReadOnly error on read"),
+        };
+    }
+
+    fn bus_write(ctx: *anyopaque, addr: Bus.Address, v: u8) void {
+        const self: *const Self = @ptrCast(@alignCast(ctx));
+        self.write(addr, v) catch |e| switch (e) {
+            error.Unmapped => @panic("Write to unmapped memory address"),
+            error.OutOfRange => @panic("Write out of range"),
+            error.ReadOnly => @panic("Write to read-only memory"),
+        };
+    }
+
+    fn bus_write_masked(ctx: *anyopaque, addr: Bus.Address, mask: u8, v: u8) void {
+        const self: *const Self = @ptrCast(@alignCast(ctx));
+        self.write_masked(addr, mask, v) catch |e| switch (e) {
+            error.Unmapped => @panic("Masked write to unmapped memory address"),
+            error.OutOfRange => @panic("Masked write out of range"),
+            error.ReadOnly => @panic("Masked write to read-only memory"),
+        };
+    }
+
+    fn bus_check_exit(ctx: *anyopaque) ?u8 {
+        const self: *const Self = @ptrCast(@alignCast(ctx));
+        // Check all segments for exit condition
+        for (self.segments) |*seg| {
+            if (seg.backend.check_exit()) |code| {
+                return code;
+            }
+        }
+        return null;
+    }
+};


We can make the MemoryMapping generic over a bus type, this way it keeps the right properties:

pub fn MemoryMapping(comptime BusType: type) type { return struct { pub const Segment = struct { … }; … }; }

This way we would keep type confusion at a minimum while also keeping the amount of implementation at a minimum.

We can even safe-guard the implementation by only allowing Data_Bus, Flash_Bus and IO_Bus as the parameter (or we could pass an enum {flash,data,io})

sim/aviron/src/lib/Cpu.zig

ikskuh · 2025-10-06T07:52:08Z

sim/aviron/src/main.zig

+    var flash_storage = aviron.Flash.Static(mcu_config.flash_size){};
+    var sram = aviron.FixedSizedMemory(mcu_config.sram_size){};
+    var eeprom = aviron.FixedSizedMemory(mcu_config.eeprom_size){};


We could make FixedSizedMemory also expose a .flash() interface where the function call invokes a @compileError if the size isn't divisible by 2.

This would remove redundant code

But the interface would have to be expanded for returning u16.

ikskuh · 2025-10-06T07:54:08Z

sim/aviron/src/main.zig

+                        const segment_buf = try allocator.alloc(u8, phdr.p_filesz);
+                        defer allocator.free(segment_buf);


That buffer can be deleted and we can read byte-by-byte from the reader interface as it already performs buffering internally and we have a 4096 byte large buffer above, so it's already quite optimal and the additional copy would just take time and could fail

what method do you use to read a byte from a reader?

wip: Unify interface, add memory mapping

6247bc5

Grazfather added the aviron label Oct 5, 2025

Grazfather requested a review from ikskuh October 5, 2025 16:43

Grazfather added 3 commits October 5, 2025 12:45

rename io_space to just io

88ceec8

Remove data r/w helpers

32ae257

Use optional exit code in testrunner like we do in main

8ef6757

ikskuh requested changes Oct 5, 2025

View reviewed changes

Grazfather added 11 commits October 5, 2025 15:19

Use specific bit with ints for addresses and bases

c0e1f28

Rename read8/write8 to read/write in device interface

aa2251d

cleanup unused

d48258d

remove unnecessary int casts

9ca6c8c

Remove RAM in favour of FixedSizeMemory

a197d9e

Rename Device interface to Bus

c4899d7

combine io and memory into a single 'bus' module

fb32f8c

Use the bus interface when loading elf files

99444af

Rename MemorySpace to MemoryMapping

e256738

Use same SIO in mcu as Cpu

7907064

Cleanup name

cdfaae8

ikskuh requested changes Oct 6, 2025

View reviewed changes

Grazfather added 4 commits October 6, 2025 10:17

Don't initialize eeprom - We don't have a way to use it yet

ebf23a5

Don't alloc while loading elf

a7f881f

wip: Make Bus, Segment, MemoryMapping take config

aaa60c8

Separate interfaces for IO and data bus

43d8844

		const segment_buf = try allocator.alloc(u8, phdr.p_filesz);
		defer allocator.free(segment_buf);

Unify interface, add memory mapping #704

Are you sure you want to change the base?

Unify interface, add memory mapping #704

Uh oh!

Conversation

Grazfather commented Oct 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ikskuh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants