Move PAM generation to build time, optimizing download size

[LuigiPiucco]

Although from the titles alone it may seem unrelated to the issue, the original goal was to fix #59. The approach taken, as suggested in that thread, is to generate the Peripheral Access Modules (PAMs) after the user has downloaded the package, in an automated fashion. So, instead of shipping the modules generated "by hand" (with semi-automation actually, but still), we ship the ATDF sources from which they are generated. When running build either in the crate root or as a dependency, the build.rs script outputs a module for the selected MCU into a known path. The generation steps are described in the README.

The result of this is that the packaged .crate file does become much smaller, just short of 5 times (4.875...). Compilation time increases, of course, in particular as a fault of the build script. This could be improved by using the executable versions of svd2rust, svdtools and atdf2svd, but that would greatly reduce reproducibility. rustfmt is used via the executable, since it does not provide a library API, but in that case it should be fine, since anyone with a toolchain capable of compiling to AVR probably also has access to rustfmt.
We now use prettyplease, which does have library functionality.

The commit changing the example from a crate to an actual Cargo example, while technically not necessary, makes testing the use of the crate as a dependency more convenient.
This has been removed.

For a reference of crate size, see #59 (comment).

[LuigiPiucco]

@Rahix just a reminder about this.

[LuigiPiucco]

Is there still interest in this PR? Not to rush, but I'd like to know, since if there's not or not yet, I'll hold off on fixing CI. It's failing because this automated approach doesn't need the Makefile, and the current CI code expects it to be there.

[tones111]

Looking over the patch series it looks like it's trying to accomplish multiple independent efforts. As a diff increases in size or impact it becomes much harder to review.

If the primary motivation here is to transition from the makefile to build.rs then I would suggest trying to move anything unrelated into separate pull requests. For example, the first commit in the series ("refactor patches ...") doesn't appear to support the build.rs migration and likely adds unrelated complexity. I have the same concern for the second and third patches in the series too.

[LuigiPiucco]

The focus was on the build.rs transition, and the earlier commits help with that, but I could split them out. Here's my reasoning for the current series, and how I plan to shorten the diff:

• The first one is leftover from updating svd2rust and svdtools while doing this. I thought it would be less work to update them before implementing build.rs, because updating afterward would require revisiting the build logic too. An old commit message referenced this, I think. Regardless, I can probably remove it with some changes to commit 4;
• The second one is really independent, it makes testing easier but can be removed with a minor change to commit 5;
• The third one is relevant because the implementation prior to it required a vector module for the interrupt macro, which I had trouble generating from the build script, and it's also needed to allow more than 1 MCU to be built. The module requirement has since been removed in the cortex-m crate, which the implementation is based on, so I pulled that in here. Removing this one would enlarge commit 4 with extra code to generate the module plus something other solution to allow multi-MCU builds, which doesn't seem beneficial. I think the commit message could use a better explanation though;
• The fourth one is the main bit of this PR, I don't think I can remove anything without breaking it. Maybe this piece could be hoisted to here though, it may shorten the diff a little;
• The fifth one already seems OK in size.

I'll wait to hear your opinions on what I propose above before changing anything though.

[Rahix]

From my side, I wouldn't worry too much about the total diff of the PR as long as the commits stay clean and logical. IIRC you're one to take this serious @LuigiPiucco, so I have no concerns :)

To give a few short comments on the approach:

• Your first commit, dd32b3b, is then mostly related to Update to svd2rust 0.33.1 #155. I guess we can do the upgrade as layed out in that PR but I am not sure if we should also pull in the naming changes from svd2rust just yet. I'd for sure like to keep this separate to the topic here, though. So my main question would be: Do you need Update to svd2rust 0.33.1 #155 for the newer svd2rust version in any case or can we stick to the current one for the time being?
• I would push your second commit, ff31dc0, to the end of the series and then actually have the example use the in-repository version of avr-device directly via path dependency. The rather ugly current solution of the example relying on the latest published version from crates.io was only put in place due to the complicated codegen setup.
• The third commit, 8a588f8, is very important to discuss. I took a look a the changes and right now I don't quite understand how you do the translation from interrupt name to vector name? The reason we needed the vector module so far is that we have to translate the interrupt names into their corresponding vectors. I couldn't find where this translation is happening in your code.
• Remainder looks fine at a quick glance, let's do a more in-depth review when the time comes.

To be completely honest, I am still on the edge about this build-time approach. I do see the benefits but I also think there is a good reason why most other embedded Rust platforms have opted to publish pre-generated crates as we currently do.

That said, I am not opposed to going this route. Mainly because the current setup is very much hitting its limits. We'll be forced to split avr-device into multiple crates soon if we continue pre-generating the code. Build-time generation would solve this, at least until the amount of vendor-ATDFs explodes dramatically :)

The biggest fear I have is build-time. For downstream users, I assume the change to be neglible because they will only incur the additional time on first build. But where it will really hurt is in CI environments like the one avr-hal currently uses. I assume we will need to start caching build-artifacts there to cope with build-time codegen of avr-device. Maybe as an experiment, you could set up an avr-hal PR which uses your version of build-time generated avr-device as a dependency so we can then compare the CI times?

That's my two cents on the topic, I am very sorry it took this much time for me to get back to you. In any case, this is impressive work, thanks a lot for working on this and demonstrating that build-time codegen isn't as unrealistic as I had imagined so far!

[Rahix]

A few more comments:

This could be improved by using the executable versions of svd2rust, svdtools and atdf2svd, but that would greatly reduce reproducibility

I am strongly against using executables here. The current way of interfacing via the tools as pure-rust dependencies is what we need to be doing.

since anyone with a toolchain capable of compiling to AVR probably also has access to rustfmt

rustfmt is an optional toolchain component, we cannot rely on it being available. We have to decide between

1. Using rusfmt optionally when available and skipping formatting when it is missing.
2. Finding another way to interact with rustfmt or another rust formatter as a rust dependency. I'm sure we are not the first to hit this problem, maybe there is some alternative available

[tones111]

I started experimenting with a parallel implementation today and (like you) ran into some accidental complexity patching the svd file. I created svdtools #265 which, if implemented, could simplify our implementation by not having to re-write the top level patch files in a temp directory.

I'm still pretty new to SVD patching, but I think it should be doable.

[LuigiPiucco]

The third commit, 8a588f8, is very important to discuss. I took a look a the changes and right now I don't quite understand how you do the translation from interrupt name to vector name? The reason we needed the vector module so far is that we have to translate the interrupt names into their corresponding vectors. I couldn't find where this translation is happening in your code.

I revisited the cortex-m-rt crate, and it has more than just the the macro to implement its interrupts. This was indeed an oversight on my part. Their approach is that the macro doesn't convert the names to libc's __vector_N symbols, it just generates code to guarantee the interrupt exists and renames it to be reachable only from the linker script. This is what my commit does, but it doesn't bring in a linker script and a vector table like it would need. Doing it like this could allow us to fix #76, but that's a separate issue for later. For now, generating the vector module like before is not viable with this approach, and the only thing I can think of is to generate a linker script with PROVIDE statements from the build.rs. Would that be OK?

The issue with the module is that it needs to be present when the macros compile, and they compile before anything is generated by build.rs. We'd need to replicate a build.rs inside the macros crate, which would get messy really quick.

About the other comments, I'll reply later, I already spent quite a while trying to find an alternative to the above and it's getting late.

[Rahix]

Custom linker scripts will be a part of tackling #76, I don't think we should separate these two. The linker scripts need to be synchronized with the libc runtime, I'd fear breaking things if we just blindly substitute them.

I agree that generating the vectors from build.rs is tricky... The only idea I have left is generating a hidden macro in the main avr-device crate that does the translation. The #[interrupt] macro then only generates an invocation of that hidden macro. Like this:

     quote!(
         #(#cfgs)*
         #(#attrs)*
-        #[allow(static_mut_refs)]
-        #[doc(hidden)]
-        #[export_name = #ident_s]
-        pub unsafe extern "avr-interrupt" fn #tramp_ident() {
-            #ident(
-                #(#resource_args),*
-            )
-        }
+        avr_device::__avr_interrupt_trampoline!{
+            #ident_s, #ident, #(#resource_args),*
+        }
 
         #[doc(hidden)]
         #f
     )

[LuigiPiucco]

The only idea I have left is generating a hidden macro in the main avr-device crate that does the translation. The #[interrupt] macro then only generates an invocation of that hidden macro.

That does work, nice catch! I'll push my implementation after I clean it up, but I confirmed the ELF for the example includes a defined __vector_N symbol now, and the jump table changes to call it instead of __bad_interrupt.

rustfmt is an optional toolchain component, we cannot rely on it being available. We have to decide between

1. Using rusfmt optionally when available and skipping formatting when it is missing.

2. Finding another way to interact with rustfmt or another rust formatter as a rust dependency.  I'm sure we are not the first to hit this problem, maybe there is some alternative available

I don't think 2 can work due to rust-lang/rustfmt#5955. 1 seems reasonable, and another option is prettyplease, which seems even better than rustfmt for our use-case. The only issue I can see is that svd2rust only outputs a string, so we'd have to parse it with syn which may increase compile time, but that assumption needs testing.

The biggest fear I have is build-time.

In my quick testing, building with --all-features takes almost exactly 1 minute. On CI, the current run took 2 minutes on compiling (that includes everything down from core, though). I think that is OK for us, and the end-user won't see more than a couple of seconds with just 1 MCU enabled.

[Rahix]

Neat, prettyplease looks perfect for what we are doing here. Would be my favorite, then :)

[LuigiPiucco]

About updating svd2rust, I need at least this commit, which is already version 0.31.3 (the option got renamed before publishing, it's actually skip_crate_attrs), so we will have to pull in the field -> method changes in this PR anyway. I think I can leave out the upstream casing-related changes, though we might as well get that done too.

[tones111]

The first commit in the series (dd32b3b) generates an error for me. My tool versions match what's specified in the readme.

[2025-01-11T20:48:25Z ERROR svdtools::cli] by svdtools (0.4.0)

    Caused by:
        0: Processing device `ATmega8`
        1: According to `TC0`
        2: Processing peripheral `TC0`
        3: According to `TCCR0`
        4: Processing register `TCCR0`
        5: Modifying fields matched to `CS0`
        6: Could not find `TC0.TCCR0:CS0. Present fields: CS00, CS01, CS02.`

Update:
The SVD output of this commit seems questionable. For example, it looks like a lot of mcus (ex: atmega328p) no longer have an SPI_MASTER enumeral for the UMSEL field, but I see it listed in the datasheets. These discrepancies are still present when building from the head of this branch.

[LuigiPiucco]

I had forgotten to update the README requirements, and there was a change that sneaked into a later commit while rebasing. Now it should work, though I haven't thoroughly tested building in between each commit.

Edit: I also forgot to push the changes. I will investigate the missing elements in the SVD as well.

[Rahix]

Alright, managed to dig through all the changes in this PR now. Please check my comments below, there are a few topics I'd still like to discuss.

Thank you so much for all your efforts here, I really like how it is coming together now.

[LuigiPiucco]

I'm in the process of stripping the PR to the bare minimum, but I've met some issues with versioning.

1. Unsupported proc macro punctuation character while rendering device from svd rust-embedded/svd2rust#863 is only fixed in the latest version of svd2rust, and using recent proc-macro2 (which prettyplease requires) breaks the generation altogether on any version of svd2rust prior.
2. cargo::error= requires a very recent compiler, 1.84 and upwards, so I'll need to drop it if keeping the compiler requirement is a goal.

@Rahix How do you prefer to proceed? If we keep the old version of svd2rust, prettyplease cannot be used to format the code, and we lock our users into also using old versions of proc-macro2 (plus I think also an old compiler). If we update it, it's likely we'll have to update the compiler, plus use the field/method changes and most liked the casing changes as well. Preventing panics in the build script is also only possible if we update de compiler.

[tones111]

LGTM

[LuigiPiucco]

Sorry, I forgot the -Zbuild-std flag + rust-src component. It should be ready now. CI build on my fork is here: https://github.com/LuigiPiucco/avr-device/actions/runs/14283358043/job/40035486892

[LuigiPiucco]

@Rahix I'm working on refactoring the error handling of this PR, would you prefer I amend it here once finished, or that I open a separate one after merge? It should be ready by tomorrow at most.

[Rahix]

I'm working on refactoring the error handling of this PR, would you prefer I amend it here once finished, or that I open a separate one after merge? It should be ready by tomorrow at most.

Amend it here :)

[LuigiPiucco]

The build script grew a bit, but now the error messages are better in my opinion, we stop generation immediately on error (before it would got through and fail after due to the messages) and it seems easier to extend for other purposes later, like linker script generation.

[LuigiPiucco]

Just pushed a fix to the CI issue, and some minor improvements as well.

[Rahix]

Very nice, thanks a lot!!

I will run a lot more tests myself in the coming days to ensure everything is ready to ship this new approach. If anything comes up, I will let you know. I do have two things that I stumbled over during review, please see below.

[Rahix]

Alright, very very nice. I know it has been a slow process getting here, but thanks so much for sticking through it and keeping up the amazing work. I do appreciate your attention to detail, this feature has come along so well. Remember, I was very sceptical in the beginning, whether this will work well at all.

I am going to merge this PR as it stands, as it is ready for use in my eyes. I do have a few things I want to see done before we release this new version of avr-device, but I think it is better to take care of them afterwards. This allows unblocking other PRs and also helps getting the code out for others to test.

[LuigiPiucco]

Oh, one more thing: I didn't update the README "Internals" section with the new function names. I guess the links can be just removed though, as they are probably only a burden.

[Rahix]

@LuigiPiucco heads-up on #187 which is a follow-up topic for this PR