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.

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.

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.

[Rahix]

So this is huge chain of interdependencies that we can't easily resolve right now:

1. We can't upgrade the compiler right now because of some pending regression fixes. See Upgrade rust toolchain to nightly-2025-01-01 avr-hal#585.
2. As you probably noticed, we can't upgrade proc-macro2 because of the old compiler. Also see cargo build - error[E0658] - proc_macro::Literal::byte_character(byte) avr-hal#537.
3. So being stuck with the old proc-macro2 for now, we can't use prettyplease, right?
4. And because of the old compiler, we also can't use cargo::error=?

My take is this: Let's put as little effort as possible into anything relating to legacy versions of dependencies and the compiler. But I'd still like to see your changes happen soon regardless. So my suggestion would be that we stick to rustfmt invocation (if available) for now. And then also keep the panics in the build script as well.

Once we can move forward with the compiler, we do a big breaking change of upgrading svd2rust, proc-macro2, and at that point also introduce prettyplease and better build-script errors.

What do you think?

[tones111]

We can't upgrade the compiler right now because of some pending regression fixes.

If I'm reading that issue correctly it sounds like most of the regressions are fixed and backported into rustc. There are a few mcus failing to pass a test suite but most are working well enough with recent nightlies. Please correct me if that's not accurate.

If that's the case couldn't we land a commit that updates the compiler version and comments out the feature flags for a bad mcus? We can repeat once the new fixes make it into rustc. Obviously we wouldn't publish a new release until support can be restored for all the processors again but this would allow us to continue making infrastructure progress while we have some momentum (instead of grinding to a halt).

Alternatively we could do that on a different branch if you're concerned about breaking main and merge it in once everything's working again.

Any thoughts on my efforts to try and break this into smaller efforts?

[LuigiPiucco]

If I'm reading that issue correctly it sounds like most of the regressions are fixed and backported into rustc.

Some have been accepted by LLVM, and I think it's enough for the MCUs to work again, but that doesn't seem to have hit released rustc nightlies yet, because I've just tested and it still gives the out of range branch target error on the affected MCUs.

For now, I'll push my stripped down version for review, but bear in mind I choose to go with the full update, at least for this pass. This way we can see a more finalized state of the repo once the regression fixes hit rustc. After, I'll copy this version of the branch. 1) We can then try to remove the updates in this PR, and after we land this, 2) @tones111's PR comes for updating svd2rust. I think this order is better, because otherwise we'd need to update the Makefile system with the updated svd2rust, but that'd be removed immediately by this PR. 3) As a final step, we update the stuff we couldn't due to proc-macro, at which point the repo should look similar to the copied version from earlier. 1 and 2 may not need to wait for the compiler, as new svd2rust still works with old proc-macro, just the reverse doesn't. Also, I found version 0.1.22 of prettyplease that doesn't require very recent proc-macro, we can use that initially.

[tones111]

My PR made it into svdtools v0.4.1 which allows us to patch the SVD files without needing to remove/generate the _svd key in the patch file or copy them into a working directory. This significantly simplifies the patching logic.

My build.rs experiment shows how to use the new functionality.

[LuigiPiucco]

@Rahix Hmm, I downgraded versions locally, including the compiler (nightly-2023-12-27), but the faulty MCUs listed in Rahix/avr-hal#585 still don't build. Now I get this:

LLVM ERROR: cannot create long jump without FeatureJMPCALL

I've tested with atmega8, atmega88p, and attiny85. It seems the problem happens while building core. Can you confirm they were actually working before? For reference, here's rustc --version --verbose

rustc 1.77.0-nightly (89e2160c4 2023-12-27)
binary: rustc
commit-hash: 89e2160c4ca5808657ed55392620ed1dbbce78d1
commit-date: 2023-12-27
host: x86_64-unknown-linux-gnu
release: 1.77.0-nightly
LLVM version: 17.0.6

I also inform I intend to use svdtools's new process_reader, once it's published to crates.io. It should simplify things, as well as make the includer files unnecessary.

[Rahix]

Try nightly-2024-03-22, that's the currently recommended compiler.

I will get back to you in a few days, to take a closer look at the state of things - feel free to push the version with the downgrades applied here already.

[LuigiPiucco]

Try nightly-2024-03-22, that's the currently recommended compiler.

That one works.

I'll push the downgrade then, but leaving the compiler version as it was before (2023-12-17). It'll work for CI here because it only checks with ATmega328p's ISA, but we should keep the issue in mind for later.

One little note is that I couldn't keep proc-macro2 as it was due to some inter-dependencies, but it's still below the version that breaks svd2rust.

[Rahix]

The proc-macro2 vs nightly rustc story is also a whole other minefield on top, see #156 as well.