[WIP] use modular_bitfield crate

[Robbepop]

This experimental PR simply replaced the bitfield! macro from the bitfield crate with the #[bitfield] attribute macro of the modular_bitfield crate. I am the author of the latter crate and wanted to know if you are interested in using it. Some benchmarks I have conducted earlier shows that both crates perform pretty much equally. There was just one benchmark where apparently modular_bitfield performed roughly 25% better for a setter case.

Generally the modular_bitfield crate is actively maintained and just received lots of new features. Also it is probably a bit more convenient to use as you might notice from the diff. Finally, by introducing the modular_bitfield to the cmse.rs file I noticed that the mregion and mvalid fields are unused. Is this intentional?

warning: associated function is never used: `mregion`
  --> src/cmse.rs:73:5
   |
73 |     mregion: u8,
   |     ^^^^^^^
   |
   = note: `#[warn(dead_code)]` on by default

warning: associated function is never used: `mrvalid`
  --> src/cmse.rs:75:5
   |
75 |     mrvalid: bool,
   |     ^^^^^^^

If you like the changes done in this PR I can also convert the other usage sites of bitfield! to use the modular_bitfield crate instead. So far I just replaced one usage site so that the diff is clearer to you.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @jonas-schievink (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.

[Robbepop]

CI for Rust 1.38 seems to fail because modular_bitfield's macro hygiene expands to these re-exports that have been introduced in Rust 1.43. Please tell me how big of an issue this is and we might find a solution for you.

[jonas-schievink]

I'm fine with this in principle, but we might want to remove the bitfields-generated API entirely (otherwise we export an API generated by a crate that might change incompatibly)

[Robbepop]

@jonas-schievink thanks for the update and info.

I am going to update the modular_bitfield crate and implement some further improvements to better support the other use cases in your crate.

[...] we might want to remove the bitfields-generated API entirely (otherwise we export an API generated by a crate that might change incompatibly)

Can you elaborate on that point? So are you thinking about removing the bitfield crate as dependency and instead manually program the stuff to prevent exposing users with a generated API? In this case there are 2 alternative solutions forward:

1. Wrap the generated bitfields for the public API items with a stable API so even if bitfield or modular_bitfield crates change their generated API this wrapper stays the same (unlike its implementation ofc).
2. Wait for 1.0 release of modular_bitfield. I am already thinking about turning it into 1.0 once I am fully satisfied with its design (some open issues still). Then staying on 1.0 would also provide you with the guarantees of a stable generated API of course.

[jonas-schievink]

Yeah exactly. We could still use this crate internally, but would write our own API to wrap it.

If modular_bitfield goes 1.0 that could also work here, as long as everyone is happy with the API.

(and this is a somewhat long-term plan for cortex-m 1.0, which isn't very close yet)

[Robbepop]

Okay cool, I will keep you updated on the modular_bitfield updates.
This is the current list: https://github.com/Robbepop/modular-bitfield/issues
Feel free to add anything (feature) to the list.

[hug-dev]

Hey!

Definitely agree to use something more up-to-date and currently maintained! Maybe the author of bitfield would be happy to incorporate your changes/ideas? As in I think one safe and canonical bitfield implementation in Rust is definitely useful and needed and I am sure there are good things to pick in both projects 😃

Finally, by introducing the modular_bitfield to the cmse.rs file I noticed that the mregion and mvalid fields are unused. Is this intentional?

Those fields are currently unused but they do representation something in the payload. Maybe they will be for some use in the future!
I guess it makes sense to either ignore the warning, or maybe it's possible to prefix them with an underscore like we do for unused parameters?

One generic question about your crate: how do your represent unused/reserved fields in a payload? With bitfield because you specify positions you don't have to define those unused fields but with your implemention you would have to add them anyway with like reserved or unused field names?

[Robbepop]

Some really good questions, @hug-dev !

Maybe the author of bitfield would be happy to incorporate your changes/ideas?

I like your vision about the bigger picture for the Rust ecosystem. Both crates are vastly different though. Whereas bitfield is a declarative macro with its unique set of syntax and features, modular_bitfield is a proc. macro and tries to cover everything bitfield provides while doing a lot more on top.

Performance wise both crates are more or less the same from what I can tell.
The problem with bitfield is that it has not received updates since approx. a year now. So my way of unifying the Rust ecosystem is to wait until I am fine with a 1.0 release of the modular_bitfield crate and then take over bitfield at crates.io with the accordance of its author. Does not really make sense to incorporate modular_bitfield into bitfield since that would imply a 100% rewrite anyways.

I guess it makes sense to either ignore the warning, or maybe it's possible to prefix them with an underscore like we do for unused parameters?

Yes I already noticed that this feature is needed so I already created some issues for that:

• Re-expand attributes on bitfields for expanded getters and setters Robbepop/modular-bitfield#45
• Add #[skip] attribute for the fields of the #[bitfield] macro Robbepop/modular-bitfield#42

One generic question about your crate: how do your represent unused/reserved fields in a payload?

You are right that both crates work differently in that regard. It might be nice to be able to specify _ as identifier for fields to basically imply that you are not at all interested in the field at all and its acts as a mere placeholder which is probably the most lightweight solution and simple to implement. I will just write that down as yet another issue for modular_bitfield.

Note that the modular_bitfield crate currently is heavily in flux to find a decent overall design that fits all realworld use cases neatly which is why I opened this PR - to answer this question and also to find out if such a crate is actually needed which fortunately is true. :)

[hug-dev]

Ah yes, I can understand that both crates are very different in how they are used. I guess I was hoping that the two implementations could co-exist in some way in the same crate but maybe it's not possible that easily and would not make sense.

In the defense of bitfield, although a new version has not been published in a long time, new commits were pushed and the author definitely seems to answer to issues. Maybe it's just the case of no-one asking for a release for one year 😄 (which also can mean that the API is good and stable)!

Thanks for linking with the corresponding issues, nice that you thought about those.

In summary, I am happy to switch modular_bitfield as I do think that the proc. macro interface makes it clean and easy to understand. To be fair, most of the bitfields I've come accross in the embedded space do not need positions as they just come one after the other. It makes defining bitfields with modular_bitfield much easier!

[jonas-schievink]

Do we still need the as u8 in mpu_region with this?

[Robbepop]

Ah yes, I can understand that both crates are very different in how they are used. I guess I was hoping that the two implementations could co-exist in some way in the same crate but maybe it's not possible that easily and would not make sense.

In the defense of bitfield, although a new version has not been published in a long time, new commits were pushed and the author definitely seems to answer to issues. Maybe it's just the case of no-one asking for a release for one year smile (which also can mean that the API is good and stable)!

Thanks for linking with the corresponding issues, nice that you thought about those.

In summary, I am happy to switch modular_bitfield as I do think that the proc. macro interface makes it clean and easy to understand. To be fair, most of the bitfields I've come accross in the embedded space do not need positions as they just come one after the other. It makes defining bitfields with modular_bitfield much easier!

Cool! I will work on those open issues and keep you updated in this PR once I am done and fine with it. :) Will then also update the PR to cover the other usage sites.

[Robbepop]

Do we still need the as u8 in mpu_region with this?

Can you point me to the specific location of code in question?

[jonas-schievink]

https://github.com/rust-embedded/cortex-m/pull/282/files#diff-d5ffe78e03fd8e79912976fdab01a464d0fc32533f56907a2cb156d7ccad372bR181

[Robbepop]

https://github.com/rust-embedded/cortex-m/pull/282/files#diff-d5ffe78e03fd8e79912976fdab01a464d0fc32533f56907a2cb156d7ccad372bR181

I guess you mean the

    #[inline]
    pub fn mpu_region(self) -> Option<u8> {
        if self.tt_resp.srvalid() {
            // Cast is safe as SREGION field is defined on 8 bits.
            Some(self.tt_resp.sregion() as u8)
        } else {
            None
        }
    }

For which I can say: Yes it looks like with modular_bitfield you should be able to drop that as u8 since the return type should already be a u8.

Also this gives me the ideas about implementing Specifier for Option<T: Specifier> so that in this case you could have a field of type s_region: Option<u8> instead of two fields srvalid: bool and sregion: u8. I am not sure though if this would be a good idea in general. 😅 But technically possible with modular_bitfield.

[Robbepop]

I am not yet done with the updates on the design of modular_bitfield, however, I am at a point where I can already inform you about how the remaining bitfields here might eventually look like using the future modular_bitfield crate.

Showcase

Please tell me what you like and especially what you dislike about this current direction.
The #[skip] attribute has already been implemented. It allows to either skip code generation for getters only #[skip(getters)], skip setters only #[skip(setters)] or skip both #[skip]. The #[repr(u32)] is also already present as well as filled = false. The only feature that is currently missing is the bits = N attribute but I will work on that next.

The cryptic looking #[skip] __: B5 simply means that we are skipping over 5 bits at this point. The double wildcard just allows us to avoid having to choose a name for this field. Not particular a hack but I found out it just works out of the box.

For bitfields such as Type that are just 8 bits while having a #[repr(u32)]. How is the conversion between u32 and the bitfield going to work in your opinion in the case that the u32 defines bits that are undefined for the bitfield? In my current design I would not generate a From impl but a TryFrom impl in these cases in order to guard against undefined bits.

#[bitfield(bits = 32, filled = false)]
#[derive(Copy, Clone)]
#[repr(u32)]
/// Control Register description.
pub struct Ctrl {
    enable: bool,
    allns: bool,
}

#[bitfield(bits = 32, filled = false)]
#[derive(Copy, Clone)]
#[repr(C, u32)]
/// Type Register description.
pub struct Type {
    sregion: u8,
}

#[bitfield(bits = 32, filled = false)]
#[derive(Copy, Clone)]
#[repr(C, u32)]
/// Region Number Register description.
pub struct Rnr {
    region: u8,
}

#[bitfield]
#[derive(Copy, Clone)]
#[repr(C, u32)]
/// Region Base Address Register description.
pub struct Rbar {
    #[skip] __: B5,
    baddr: B27,
}

#[bitfield]
#[derive(Copy, Clone)]
#[repr(C, u32)]
/// Region Limit Address Register description.
pub struct Rlar {
    enable: bool,
    nsc: bool,
    #[skip] __: B3,
    laddr: B27,
}

#[bitfield]
#[derive(Copy, Clone)]
#[repr(C, u32)]
/// Secure Fault Status Register description.
pub struct Sfsr {
    invep: bool,
    invis: bool,
    inver: bool,
    auviol: bool,
    invtran: bool,
    lsperr: bool,
    sfarvalid: bool,
    lserr: bool,
}

#[bitfield]
#[derive(Copy, Clone)]
#[repr(C, u32)]
/// Secure Fault Address Register description.
pub struct Sfar {
    address: u32, // Why is this a bitfield? Could be a simple `u32`.
}

[Robbepop]

I just finalized version 0.11 of the modular_bitfield crate and would be happy about feedback of my above proposals.
The next step is to integrate modular_bitfield version 0.11 into cortex-m, replace the bitfield! macro with it and see how well it adjust into the cortex-m crate.
I will update this PR once I am done with that.

[Robbepop]

I am done with replacing the old bitfield! definitions with the new modular-bitfield types.
The cargo test --workspace command seems to be running through nicely. Also I fixed the spammy const point mutation warning.
Are there some extended test suites? How can I make sure that everything is still working as previously?

---

This was the warnings I have also fixed in this PR:
(Not caused by the changes introduced here!)

warning: taking a mutable reference to a `const` item
   --> src/peripheral/mod.rs:432:18
    |
432 |         unsafe { &mut *Self::PTR }
    |                  ^^^^^^^^^^^^^^^
    |
    = note: `#[warn(const_item_mutation)]` on by default
    = note: each usage of a `const` item creates a new temporary
    = note: the mutable reference will refer to this temporary, not the original `const` item
note: `const` item defined here
   --> src/peripheral/mod.rs:411:5
    |
411 |     pub const PTR: *mut icb::RegisterBlock = 0xE000_E004 as *mut _;
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Robbepop]

FYI: I do not proposed, yet, to merge this PR before I am sure that the modular-bitfield version used is really production ready.

[jonas-schievink]

The compiler warnings are incorrect. The code in cortex-m should be correct. I've filed rust-lang/rust#78819 for this.

[Robbepop]

The compiler warnings are incorrect. The code in cortex-m should be correct. I've filed rust-lang/rust#78819 for this.

Thanks for the info. However, I think using NonNull here might even be an improvement anyways.

[Robbepop]

@jonas-schievink I can of course remove the warning fixes if they are unwanted.

[Robbepop]

Btw.: I have only used features of the modular-bitfield crate that I plan to keep in any case for an eventual 1.0 release so that you very likely will not suffer from breaking future updates. :)

[jonas-schievink]

rust-lang/rust#78819 has been fixed, please undo the NonNull workaround since it is an unrelated change (and it isn't immediately obvious to me that we want it)

[Robbepop]

Is there still interest in this? I'd be willing to update to recent main or master branch.

[therealprof]

We just discussed this in our meeting and we're inclined to go for it. We'll need to figure out whether this is a breaking change, i.e. which target release to bring it in.

@Robbepop Would you mind cleaning it up and update the title?

[Robbepop]

We just discussed this in our meeting and we're inclined to go for it. We'll need to figure out whether this is a breaking change, i.e. which target release to bring it in.

@Robbepop Would you mind cleaning it up and update the title?

I will next weekend. Looking forward. :)

[Robbepop]

@github-actions
CI / ci-linux (1.38.0) (pull_request) Failing after 1m — ci-linux (1.38.0)

Being based on Rust version 1.38 this might be problematic, I will have to check. What is the reason for this specific version for cortex-m?

[adamgreig]

Being based on Rust version 1.38 this might be problematic, I will have to check. What is the reason for this specific version for cortex-m?

Right now, cortex-m's minimum supported Rust version (MSRV) is 1.38, hence the CI check. It's possible to bump it if necessary though, do you know what the MSRV modular_bitfield would require is / what features it needs?

[Robbepop]

I will check which version is needed at minimum. However, looking at the CI output it could very well be that Rust 1.38 also works if I change code a little bit.

[Robbepop]

Closing this due to my own inactivity. If the maintainers still want this we can try to tackle this again.