-
Notifications
You must be signed in to change notification settings - Fork 156
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
base: master
Are you sure you want to change the base?
Conversation
If you @ me when the prior pr is in I can take a look |
There was a problem hiding this 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.
Thanks @thejpster, previous PR now merged so this one can be reviewed in isolation. |
There was a problem hiding this 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
Do we need to discuss this today? Is it ready to go? |
For the |
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. |
Yep, my plan is to create a new |
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). |
Since #241 we've had an
InterruptNumber
trait incortex-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 ofcortex-m
, because it's only allowed to have onecortex-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.