Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new cortex-m-interrupt-number crate #488

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add new cortex-m-interrupt-number crate #488

wants to merge 1 commit into from

Conversation

adamgreig
Copy link
Member

Since #241 we've had an InterruptNumber trait in cortex-m, which is intended to be used by PACs to mark their enum of interrupts in a way that cortex-m's NVIC driver can use. We moved this out of the bare-metal crate because the implementation is architecture-dependent (u16 for cortex-m, but u8 or u32 for various other platforms).

However, in doing so we end up with all PACs having a direct dependency on cortex-m only for this trait. This makes it harder to release a new major version of cortex-m, because it's only allowed to have one cortex-m version in a project and most projects will use a PAC that depends on an older version, possibly indirectly via a HAL. Users are then stuck until their PAC (and probably HAL) update. In the past we've used semver hacks to make it possible to run different cortex-m versions alongside each other, but it's a lot of bug-prone work to make each release that way.

Instead, this PR moves InterruptNumber into its own crate, so PACs can depend on that instead of cortex-m, allowing us to more easily make breaking cortex-m changes in the future. We can make a backported cortex-m 0.7 release that depends on the new crate and publically re-exports it, and in upcoming cortex-m 0.8 we use it without re-exporting it to ensure the new crate is depended upon directly.

Builds on #487 which should be merged first. Incidentally CI fails because it can't check cortex-m because of the new dependency until it's been released.

@adamgreig adamgreig requested a review from a team as a code owner October 16, 2023 01:02
@thejpster
Copy link
Contributor

If you @ me when the prior pr is in I can take a look

Copy link
Member

@newAM newAM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me! Added one small suggestion to make this more workspace friendly.

cortex-m/Cargo.toml Outdated Show resolved Hide resolved
@adamgreig
Copy link
Member Author

If you @ me when the prior pr is in I can take a look

Thanks @thejpster, previous PR now merged so this one can be reviewed in isolation.

Copy link
Contributor

@thejpster thejpster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One note about compatibility but otherwise ok

@@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
### Breaking changes

- `NVIC::request()` no longer requires `&mut self`.
- `InterruptNumber` is now provided by the `cortex-m-interrupt-number` trait
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a breaking change? What does cargo-semver-checks say? I'd have assumed it was the same trait exported with the same name, so it was OK?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question, I'll try and do some tests to check. The main scenarios I'm interested in are:

  • We release c-m-im and use/re-export it in cortex-m 0.7.8. A user updates to c-m 0.7.8 but keeps using their old PAC, which implements c-m's InterruptNumber. Does the PAC now actually implement c-m-im's trait (since it's re-exported) and therefore still work with c-m 0.7.8's NVIC?
  • We release c-m-im and a PAC updates to implement it directly, but a user's still using c-m 0.7.7 which requires the old trait, alongside the new version of the PAC. The PAC doesn't depend on a new c-m, so nothing would drive the user to update their lock file. Does the build now fail with the new PAC because the PAC's enum doesn't implement the trait that c-m wants for NVIC? Is a fix as simple as "cargo update", which might happen anyway when PAC is updated?

@thejpster
Copy link
Contributor

Do we need to discuss this today? Is it ready to go?

@romancardenas
Copy link

For the riscv environment, would it make sense to move the Exception and Interrupt enums to this crate? Or do you think it is better to have them duplicate in riscv and riscv-rt?

@jonathanpallant
Copy link
Contributor

I think it makes sense for the RISC-V embedded rust world to have their own crate with their own trait for Interrupt numbering. There doesn't seem to be a good reason for someone to be able to plug a Cortex-M PAC that implements the trait into the RISC-V runtime which is looking for a trait implementation.

@romancardenas
Copy link

Yep, my plan is to create a new riscv-interrupt-number crate (or perhaps a more generic name, as I also plan to move priority numbers or HART ID numbers) that follows this fashion. But I wonder if I could also move the enums to this new crate, not only the traits

@jonathanpallant
Copy link
Contributor

The number of interrupts varies depending on which Cortex-M based SoC you have, so it has to be defined in the PAC (or similar). But the runtime can't depend upon the PAC, and we don't want the PAC to depend upon the runtime, so now both depend on this shared trait. The PAC can create an enum to define what Interrupts are available and the runtime can deal with any enum as long as it implements this trait (which simply lets it convert the enum into an integer that the NVIC understands).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants