Add cfg to Peripheral fields

[hug-dev]

The cfg conditional compilation attribute was only set on impl blocks of peripherals. This commit also sets it on the fields themselves to be more consistent.

Also adds Armv8-M Baseline to the blacklist of the ITM peripheral (cf rule FMQF of the Armv8-M ARM).

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @korken89 (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[hug-dev]

I tested by compiling the crate for all thumb* targets (except the neon ones) successfully.

[hug-dev]

The commented line (28) in build.rs creates the clippy error:

//println!("cargo:rustc-cfg=armv7em");

It was added by @adamgreig here.

Should we remove the comment, merge the two blocks, ignore the clippy lint?

[thejpster]

The commented line (28) in build.rs creates the clippy error:

//println!("cargo:rustc-cfg=armv7em");

It was added by @adamgreig here.

Should we remove the comment, merge the two blocks, ignore the clippy lint?

I'd remove the commented out code, replacing with a note as to why armv7em isn't there (if appropriate).

Edit: We should merge the two identical if statement arms as well.

[hug-dev]

The commented line (28) in build.rs creates the clippy error:

//println!("cargo:rustc-cfg=armv7em");

It was added by @adamgreig here.
Should we remove the comment, merge the two blocks, ignore the clippy lint?

I'd remove the commented out code, replacing with a note as to why armv7em isn't there (if appropriate).

Edit: We should merge the two identical if statement arms as well.

Done! The "e" in armv7em means that the DSP extension is included, but the cortex-m crate does not seen to use it at the moment.

[thejpster]

Looking good. Just a couple of questions about v6m vs v8m.base. I'm sure what you've done is right, but I felt it was worth double checking.

[hug-dev]

Looking good. Just a couple of questions about v6m vs v8m.base. I'm sure what you've done is right, but I felt it was worth double checking.

Thanks! I can not see the questions that you have 👀

[thejpster]

They're inline comments that should appear in the source code diff view.

[thejpster]

Hah. And I need to click 'Submit'...

[japaric]

Adding conditional compilation to the fields of this structure is a breaking change. For the same reason as https://github.com/rust-embedded/cortex-m/pull/180/files#r360940824

The reason these fields are not conditionally compiled is because destructuring this structure or referencing its fields would otherwise become inconvenient (e.g. see cortex-m-rtfm where the fields are referenced but never used). As cfg(armv6m) and the others are not built-in conditions every crate that needs to access the fields of this structure needs to pretty much replicate verbatim the build script used in this crate (see the cargo:rustc-cfg= print statements in the build script).

[hug-dev]

The references are taken from the Armv8-M ARM.
Here is an image from the reference manual presenting the availability of the debug features in Mainline or Baseline:

[hug-dev]

Adding conditional compilation to the fields of this structure is a breaking change. For the same reason as https://github.com/rust-embedded/cortex-m/pull/180/files#r360940824

You are right and I did not think about mentioning the fact that this is indeed a breaking change! Does it have any consequences on the changes themselves? I guess because of that it will have to be tagged as a new major version?

The reason these fields are not conditionally compiled is because destructuring this structure or referencing its fields would otherwise become inconvenient (e.g. see cortex-m-rtfm where the fields are referenced but never used). As cfg(armv6m) and the others are not built-in conditions every crate that needs to access the fields of this structure needs to pretty much replicate verbatim the build script used in this crate (see the cargo:rustc-cfg= print statements in the build script).

Makes sense! Would you say it is better to leave it as it was before then? I think that having cfg on the Peripheral fields could bring some frustration when trying to destructure this struct for all targets (as one would need to copy the build.rs println!s and the same cfg than in the definition of the Peripheral struct). It might be hard to know how to fix the compiler errors when facing that. Having cfg on the impl will only bring errors when actually trying to use the field, which might happen less often.

But as it was said in the other PR, it also makes more sense to have cfg everywhere for completeness.

[thejpster]

I'm starting to wonder if inferring the Processor model (e.g. Cortex-M4), and its associated peripherals, from the architecture/instruction-set (e.g. ARMv7E-M), which is given by the target (e g. thumbv7em-none-eabi) is actually a good idea.

As the instruction sets are backwards compatible, what if I wanted to write a portable binary that ran on a Cortex-M3, M4 and M33, and used runtime feature detection to determine what peripherals were available? I would want ARMv7-M instructions, but acess to the M33's peripherals. Or what if some future peripheral is added to some mythical Cortex-M5, but which otherwise implements ARMv7E-M? Or, what if some CPU doesn't have the features outlined in the relevant ARM as Optional?

The LLVM backend generally has a distinction between TARGET and CPU. I wonder if we can make use of that, and if we can, whether we should.

[hug-dev]

Is the main goal of those cfg is to save memory? I guess you could have runtime feature detection and remove all the cfg at the expense of increasing memory.

It is true that some Arm C compilers (the ones that I know) give you the option of choosing either the architecture or the CPU but in Rust there is only the target switch. Having the possibility to choose a specific CPU could help having more precise cfg but there still would be optional features. For example for the Cortex-M33, the presence of a FPU, DSP, Security Extension and more is optional.

The way I think about those cfg is that they only remove peripherals we are sure they are not implemented but leave the implemented and optional ones, just in case.

[hug-dev]

I guess I need to rebase because of 6ac19a5.
About the changes: should I remove the cfg to the Peripheral structure definition and only keep them on the impl (as it was before, but updated)?

[thejpster]

So #193 is in now, so we're fine with #[cfg] on individual fields, right?

[hug-dev]

So #193 is in now, so we're fine with #[cfg] on individual fields, right?

I would say so! It would also mean that if a consumer of this crate wants to target multiple architectures, he would have to somehow use the same build configs:

    if target.starts_with("thumbv6m-") {
        println!("cargo:rustc-cfg=cortex_m");
        println!("cargo:rustc-cfg=armv6m");
    } else if target.starts_with("thumbv7m-") {
        println!("cargo:rustc-cfg=cortex_m");
        println!("cargo:rustc-cfg=armv7m");
    } else if target.starts_with("thumbv7em-") {
        println!("cargo:rustc-cfg=cortex_m");
        println!("cargo:rustc-cfg=armv7m");
        println!("cargo:rustc-cfg=armv7em");  // (not currently used)
    } else if target.starts_with("thumbv8m.base") {
        println!("cargo:rustc-cfg=cortex_m");
        println!("cargo:rustc-cfg=armv8m");
        println!("cargo:rustc-cfg=armv8m_base");
    } else if target.starts_with("thumbv8m.main") {
        println!("cargo:rustc-cfg=cortex_m");
        println!("cargo:rustc-cfg=armv8m");
        println!("cargo:rustc-cfg=armv8m_main");
    }

Those would become more "standard".

[thejpster]

bors r+

[bors]

👎 Rejected by label

[thejpster]

I think needs the clippy allow from #189 to land first, then it will build.

[thejpster]

bors r+

[bors]

👎 Rejected by label

[thejpster]

@jonas-schievink - you added the needs-decision label. Does this still need a decision?

[jonas-schievink]

Yeah, #181 (comment) has not been addressed yet.

[thejpster]

But didn't #193 fix that?

[jonas-schievink]

No, that future-proofed cortex-m for when new fields get added, but libraries can still move all current fields out of Peripherals. If we cfg-gate some of them then these libraries will have to basically copy the entire build.rs from cortex-m, I believe.

That doesn't mean we shouldn't do this, since IMO it does make things more clear, but this is a pretty big drawback. Maybe we can find a better solution that directly addresses what RTFM wants to do (which is use certain core peripherals for itself and only hand the remaining ones to the user), but I don't know how that would look.

Sounds like a good thing to bring up at the next meeting though.

[thejpster]

Ok, the outcome of the meeting was:

1. In the normal case, people shouldn't need to have a build.rs file, and cfg gating structure fields means you do need a build.rs file if you want to destructure the struct (as you need to know when fields are and are not present).
2. defining CPU features based on LLVM architecture target is not valid - running thumbv6m-none-eabi code on a Cortex-M7 is perfectly valid, especially if you want a portable binary.
3. If we do want to stop people using features they don't have on the particular Cortex-M they are aiming at / are running on, we need a better system than cfg gates based on LLVM architecture. This includes sorting out eabi and eabifp (which doesn't indicate the presence or not of an FPU, only the passing of float arguments in FPU registers).

So, I think we agreed we'd close this.

[adamgreig]

On the whole I agree with #181 (comment) but one alternative idea: we could instead add some cortex-m features such as cortex-m3, cortex-m4, cortex-m7 which we use to cfg-gate the relevant entries. They would make sense for the peripheral entries, since they relate to actual CPU rather than target architecture, and they don't need a build file in cortex-m or in other crates. Users building (admittedly very rare) portable binaries can enable multiple features to have access to all peripherals. Crates wanting to destructure Peripherals like cortex-m-rtfm could replicate these features (there's not many and they make sense for a cortex-m-specific crate; they could even be useful to gate other rtfm features?) or could enable all the CPU features (perhaps all are even enabled by default) which would ensure that the Peripherals struct is "full".

That way, most users can have a smaller Peripheral struct that relates to just their CPU without the overhead of loads of unused/inaccessible fields, we can simplify cortex-m's build.rs as well as having it make more sense than the current not-quite-right "thumbv6m == cortex-m0" logic, and crates wanting to destructure Peripherals wouldn't need to add their own build.rs.

[hug-dev]

Thanks for the update! The main reason for this PR was to address one comment of my other PR adding the SAU peripheral that is still pending.

I think the idea around adding cortex-m features per CPU should probably be discussed in another issue.

I have updated this one, removing the cfg gate on the peripherals but keeping the updates on the comments and existing cfg. I believe those changes are still valid and correct.

[thejpster]

bors r+

[bors]

👎 Rejected by label

[hug-dev]

@jonas-schievink I modified this PR to only be an update of code documentation and add armv8m_base where it was only before armv6m.
It's still a breaking change but only for Armv8-M Baseline developpers who were trying to use the ITM but maybe that's fine because the ITM is not available on Armv8-M Baseline?

[jonas-schievink]

bors r=thejpster

[bors]

Build succeeded:

• continuous-integration/travis-ci/push