don't generate core peripherals if SVD is explicitly not Cortex M

[ahixon]

Although the SVD spec states:

This section is mandatory if the SVD file is used to generate the device header file.

a bunch of SVDs from vendors (eg Fujistu) don't define a cpu element.

So, to avoid any regression, svd2rust will continue to generate core peripherals even if it isn't specified. It will only skip generation if the cpu name tag is a non-Cortex M CPU.

Relies on rust-embedded/svd#34

[pftbest]

Perhaps this can be made even more generic, by adding a --target=xxx command line flag for example.

When your CPU is not cortex-m some may also want to use another base crate instead of cortex-m, like the one I'm working on for msp430:

https://github.com/pftbest/msp430

[kjetilkjeka]

@pftbest

I do agree that the dependency on cortex-m is probably less general than it need to (should) be. How do you think this can be solved?

Would all general embedded features (like Peripheral) have to be moved out of the cortex-m crate and use this new crate as a base?

Or rather implement these general embedded features in the file generated from svd2rust and make it self contained?

Would it be worth doing since "most" MCUs with svd files are ARM cores?

[japaric]

This seems OK as a first step but the generated file will still contain a dependency on the cortex-m crate (extern crate cortex_m) which is probably not what you want.

@ahixon Could you tell us more about your target device? Cortex A? non ARM device?

[ahixon]

Yeah, the end-goal was basically to remove the hard dependency on the cortex-m crate on devices that don't need it.

as @kjetilkjeka mentions, that probably means Peripheral needs to get taken out of cortex-m and either put into a new crate or have it put into the generated Rust library. But, that's a pretty breaking change for anyone who uses cortex-m, so I was going to open an issue separately to discuss that. I've done that now: rust-embedded/cortex-m#50

I think the former makes the most sense anyway.

@japaric The target device is a Rockchip RK3399. It has a dual-core Cortex-A72 and quad-core Cortex-A53, as well as two separate Cortex-M0 cores.

Currently, svd2rust sticks in all the Cortex peripherals in the Cortex A library; which isn't a problem as long as you make sure your code doesn't use them, but they don't really belong there. Also interestingly, the Cortex-M0's peripheral blocks are just memory mapped to the AP's peripheral set, so the ones svd2rust generates aren't actually valid there either (i.e. they are not the standard ones provided by the ARMv7 spec).

[japaric]

I'm in favor of sharing abstractions like Peripheral between all the different svd2rust users: msp430, cortex-m, etc. I would prefer having both the msp430 and the cortex-m crate depend on a common crate rather than use duck typing (i.e. let svd2rust assume that msp430::Peripheral and cortex-m::Peripheral have the same API / semantics).

Looking just at svd2rust generated code these are the cortex-m things it depends on:

• Peripheral
• Context
• Nr

Looking at the msp430 and cortex-m crates these are the things they share in common:

• Peripheral
• Context
• CriticalSection
• Mutex
• Nr. I think this is unused in the msp430 crate, but the SVD spec mandates that <interrupt> entries have a number associated to it.

So it probably makes sense to put all those in common crate, and have svd2rust generated code directly depend on the common crate rather than on cortex-m or msp430. The big question is how do we call that common crate?

@ahixon We may be able to get away with doing this without breaking the cortex-m crate if the cortex-m crate re-exports all the common crate API exactly where it currently provides e.g. Peripheral.

[pftbest]

svd2rust generated code also depends on cortex-m::Reserved but it has different repr size in this crates. Not sure if it can be moved inside.

Also there is still one open question, do we go with cpu section in SVD file, like is suggested by this patch, or do we have a command line switch. I don't have a strong preference here, but it has to be decided at some point.

[japaric]

I took a stab at extracting the common bits. The common crate contains the bits that both cortex-m and msp430 can reuse.

@pftbest Could you test if you can implement your msp430 crate on top of the common crate?

@ahixon How much of the common crate can you use for your use case? Note that the common crate is missing an implementation of critical sections so you would have to create your own. I suppose that the criticical section in the cortex-m crate (interrupt::free) is applicable for your use case but you probably don't want to depend on the cortex-m crate.

I also made a branch where I have changed the cortex-m crate to re-export all the stuff in common rather than re-implement it. I believe the public interface hasn't changed with this refactor.

Finally I tried to make svd2rust generated code not depend on the cortex-m crate. I found two problems:

• The first one is the core peripherals re-exports which this PR would solve.
• The other problem which I didn't notice earlier is that svd2rust generates a interrupt::Handlers struct and a interrupt::DEFAULT_HANDLERS constant which are used to register interrupts / exceptions in application code. The problem is that DEFAULT_HANDLERS currently depends on cortex_m::exception::default_handler which can't be moved into the common crate because it contains ARM specific code.

I have a revamp of the interrupt/exception registration mechanism in the works which might make the second problem disappear but I have yet to post a RFC on the topic.

[japaric]

do we go with cpu section in SVD file, like is suggested by this patch, or do we have a command line switch.

My preference would be a command line switch instead of trying to make svd2rust "too smart". cpu.name is an enumeration as per the SVD spec and there's no MSP430 variant in it so we probably can't do option 1 in any case.

[pftbest]

Could you test if you can implement your msp430 crate on top of the common crate?

@japaric I've merged your patch in branch common, and it looks good. The one example project based on msp430g2553 crate that I use for testing didn't show any regressions.

[ahixon]

Yeah, probably makes sense then we just go the command line flag route, if there's also the msp430 base crate.

Possibly the default interrupt handler could then be sourced from whatever crate is specified via the command line?

@japaric all the Cortex-A stuff I've been doing so far runs with interrupts disabled, so I haven't written any interrupt handling support yet (I'm not sure if I'll even end up doing that for my project). However, if I do, it will require some special thought since I need to worry about multiple exception tables for each Exception Level, and configuring the GIC.

The M0, will, yeah, probably have the same interrupt::free implementation as cortex-m. Though, this M0 core has some multiplexed IRQ weirdness that isn't SVD related-information.

Because of that, I'll probably be supplementing the interrupt stuff from a separate tool and simply rely on svd2rust for peripheral generation.

[japaric]

@pftbest Thanks for testing. BTW, could you open an issue about the svd2rust changes you had to do to get it working with msp430?

@ahixon I hope that cortex_m::exception::default_handler won't be required once I refactor the exception registration stuff.

I suppose that for your use case you want a --arch none mode where the svd2rust generated code contains no references to the cortex-m or msp430 crate but only to the common crate?

We need a better name for the common crate. Any suggestion?

[japaric]

I hope that cortex_m::exception::default_handler won't be required once I refactor the exception registration stuff.

I have opened an RFC about the exception registration refactor in rust-embedded/cortex-m#51. The required svd2rust changes are in #118. With those changes the generated crate will no longer depend on cortex-m's Reserved, Context or default_handler as all those will be deleted from cortex-m.

[homunkulus]

☔ The latest upstream changes (presumably 5e2da93) made this pull request unmergeable. Please resolve the merge conflicts.

[japaric]

#120 subsumes this PR.

@ahixon thanks for starting the discussion on this topic. 👍