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

Implement new InterruptNumber trait from next cortex-m release #473

Merged
merged 1 commit into from
Nov 25, 2020

Conversation

therealprof
Copy link
Contributor

This eliminates the bare-metal and cortex-m-rt dependencies for Cortex-M
and thus changes CI and svd2rust-regress accordingly.

Replaces #455

Signed-off-by: Daniel Egger [email protected]

@rust-highfive
Copy link

r? @adamgreig

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-tools labels Sep 20, 2020
@adamgreig
Copy link
Member

This LGTM to me, except I don't understand why we were using cortex_m_rt::interrupt before anyway.

@therealprof
Copy link
Contributor Author

I have no idea why we're not getting random errors in for different vendors. Need to investigate after cooking...

@therealprof
Copy link
Contributor Author

@adamgreig If we're happy with this we'll only need the cortex-m release to finalize this PR.

@@ -112,7 +112,7 @@ pub fn render(
out.extend(quote! {
pub use cortex_m::peripheral::Peripherals as CorePeripherals;
#[cfg(feature = "rt")]
pub use cortex_m_rt::interrupt;
Copy link
Member

Choose a reason for hiding this comment

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

I think this was here to ensure the svd2rust generated PAC exports an interrupt macro, since the cortex-m-rt documentation says you should use interrupt from your PAC and not from c-m-rt. So, I think we should keep the pub use cortex_m_rt::interrupt, but we don't need to pub use cortex_m::interrupt::InterruptNumber at all, since the impl writes the full path anyway.

adamgreig
adamgreig previously approved these changes Oct 6, 2020
Copy link
Member

@adamgreig adamgreig left a comment

Choose a reason for hiding this comment

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

Looks good. I think we'll have to try it out a bit with cortex-m and PACs to be sure it's working as we expect but PR looks fine to me. Could you add a CHANGELOG entry?

This eliminates the bare-metal and cortex-m-rt dependencies for Cortex-M
and thus changes CI and svd2rust-regress accordingly.

Replaces #455

Signed-off-by: Daniel Egger <[email protected]>
@therealprof
Copy link
Contributor Author

@adamgreig I've updated the PR to use the release and added a CHANGELOG. I also ran svd2rust-regress locally but I haven't tried adapting a HAL impl yet.

Copy link
Member

@adamgreig adamgreig left a comment

Choose a reason for hiding this comment

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

bors merge

@bors bors bot merged commit c68b71c into master Nov 25, 2020
@bors bors bot deleted the cortex-m-InterruptNumber branch November 25, 2020 01:07
@@ -180,9 +180,9 @@ pub fn render(
#variants
}

unsafe impl bare_metal::Nr for Interrupt {
unsafe impl cortex_m::interrupt::InterruptNumber for Interrupt {
Copy link

Choose a reason for hiding this comment

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

This change breaks the build for avr-device. The problem is that you modified the else branch which is for everything but msp430 ...

I suppose this code needs to be refactored such that this line is only emitted for cortex-m. I would assume other platforms (like riscv) will also run into the same issue.

Cc: @trembel

@therealprof
Copy link
Contributor Author

therealprof commented Nov 26, 2020

@Rahix Thanks for noticing. Indeed we has some discussion in #455 about other architectures and the approach to be taken there.

Since this doesn't break RISC-V in CI, this would imply that either it doesn't break or that it is not exercised properly.

AVR is not even tested in CI which would be a good first thing to add.

@Rahix
Copy link

Rahix commented Nov 26, 2020

Since this doesn't break RISC-V in CI, this would imply that either it doesn't break or that it is not exercised properly.

Hmm, from reading the code in interrupt.rs I cannot tell how RISC-V manages to evade this problem (unless all tested SVD files contain no interrupts at all which I would assume is not the case). Would need another closer look ...

AVR is not even tested in CI which would be a good first thing to add.

I can look into adding that. For AVR, the vendor files are in a format called ATDF which we first convert to SVD using a small utility called atdf2svd. I suppose I should upload a pre-generated SVD somewhere and reference that for the CI tests?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants