ZigEmbeddedGroup / microzig

Unified abstraction layer and HAL for several microcontrollers
zlib License
1.26k stars 102 forks source link

Build system rewrite #259

Closed tact1m4n3 closed 1 week ago

tact1m4n3 commented 1 month ago

Build system rewrite

If anyone has any other ideas of things to add to the new build system (even stuff that was there before and I missed it), please leave a comment in this PR and I will add it to the list.

Tasks to complete

tact1m4n3 commented 3 weeks ago

If each port is now a type, is it possible to add out of source ports? If we can't is this a direction we want to take the project?

Sure. This is how it would look if you followed the same design as the ports in the repo (though this is not required).

pub fn build(b: *std.Build) void {
    const optimize = b.standardOptimizeOption(.{});

    const mz_dep = b.dependency("microzig", .{});
    const mz = MicroZig.init(b, mz_dep);

    const port_dep = b.dependency("port", .{});
    const port = @import("port").init(port_dep);

    const fw = mz.add_firmware(.{
        .name = "test",
        .root_source_file = b.path("src/main.zig"),
        .target = port.whatever,
        .optimize = optimize,
    });
    mz.install_firmware(fw, .{});
}

I don't understand why altering the structure of the core package is useful to this task.

~~I did this because since the root build.zig contains the build system, it seemed logical to make core/microzig.zig a part of this package (since it is the one that merges all the moving parts we have to add all the necessary imports, which can be done even if the module belongs to a different package, but it makes sense that the package that creates the module also adds all the imports). The same goes for core/start.zig or any other modules (chip, hal, board). They are constructed by the build system. Another reason is that core can't be used on it's own (at least in it's current form), it would only ever be used by the microzig build system. There are some definitions that I think can be placed in another package (core) that contains Mmio, etc. This would allow other packages to access those definitions.~~

I guess you are right. I deleted a bunch of stuff (tests, etc) without realising it. I'll add back the core package as it was.

I apologize for the excessive amount of changes and for making you navigate the long long diff :). I guess my lack of experience with open source really shows here. I went through several iterations of the code (without starting fresh) and I changed a lot of things I didn't write in the process. If there are two many changes, I can make a cleaner pr.

tact1m4n3 commented 3 weeks ago

I agree that this is not a pr, but more of a playground of ideas and it contains stuff not necessarily related to the purpose of the original pr. If it's ok with you, I'll make a new pr with only the only changes being to the build system or force push in this one. I'll cleanup this pr as much as possible. When it's ready, I'll mark it as ready for review. There is also a bug in the stm32 chip generation (fixed here) for which I'll make a different pr.

mattnite commented 1 week ago

@tact1m4n3 Thank you for the example, I really like how this is turning out. No worries on being new to open source, you've done some great work here and I'm happy to help get this into the main branch. As a bit of extra information, we do squash commits here, so in the final history every PR will result in a single commit. This makes it so that one feature, bug fix, etc is reflected by every commit, so when you look at a patch, it's a meaningful addition to the codebase.

You're making some big changes to the build system so it makes sense that it'll touch a lot of files, but if you can please make those other, PRs that you mentioned!

tact1m4n3 commented 1 week ago

Just fixed the build error and merged the branch with upstream/main.

if you can please make those other, PRs that you mentioned!

Can you clarify please to what PRs are you referring to?

There is also a bug in the stm32 chip generation (fixed here) for which I'll make a different pr.

I've already submitted this one. #282

tact1m4n3 commented 1 week ago

I've already submitted this one. #282

I removed the changes addressed by #282 from this PR. I think it would be better to merge this one first (there are conflicts between them) because then it would be clearer what PR patched what.