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

feat: place __INTERRUPTS vector in .trap.rodata #119

Merged
merged 4 commits into from
May 22, 2023

Conversation

sethp
Copy link
Contributor

@sethp sethp commented May 19, 2023

This change preemptively adopts the change proposed to svd2rust to
expose the link section as a config parameter.

I split out the changes to the generator from the actual update of the generated files to make it a little easier to review, since the generated code changes are a little noisy.

It also addresses an unrelated issue I ran into with rust-analyzer (by way of vscode) in 64e16da . Just let me know if you hate it, and I'll pop it off this PR.

sethp added 3 commits May 18, 2023 22:09
This commit preemptively adopts the change proposed to [svd2rust] to
expose the link section as a config parameter.

[svd2rust]: rust-embedded/svd2rust#718
via:

```
(cd xtask; cargo run -- --generate-only)
```
Rust-analyzer's "all targets" default mode runs afoul of `#[no_std]`
crates that don't have a test harness. This change tells cargo (and
indirectly, rust-analyzer) that we have no expectation for that to work.

The CI changes don't have any effect currently, since none of these
crates have any `bin`s or `example`s, but it does check that the
rust-analzyzer complaints won't crop back up in the future.
Copy link
Member

@MabezDev MabezDev left a comment

Choose a reason for hiding this comment

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

This looks good from my perspective, I don't really see an issue depending on a git version of svd2rust.

I'll defer to @jessebraham regarding the rust-analyzer commit.

@MabezDev MabezDev requested a review from jessebraham May 22, 2023 11:32
@sethp
Copy link
Contributor Author

sethp commented May 22, 2023

Cool, it looks like rust-embedded/svd2rust#718 got merged with no changes, so I'll just re-point this to the "official" sha and then I'm happy with this one.

@sethp sethp marked this pull request as ready for review May 22, 2023 15:13
@@ -42,7 +42,7 @@ jobs:
- name: generate pac
run: cargo run --manifest-path=xtask/Cargo.toml -- --generate-only ${{ matrix.chip }}
- name: build pac
run: cd ${{ matrix.chip }} && cargo check
run: cd ${{ matrix.chip }} && cargo check --all-targets
Copy link
Member

Choose a reason for hiding this comment

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

Why is this option required? Just curious

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a good question! I'm fairly sure it doesn't do anything for these crates in the present: the idea is to line CI up with the default developer experience so someone like me who's just running git clone && code hopefully gets a project with no spurious rust-analyzer errors.

That's only as important as preserving that property, which it seems like we're at least loosely agreed about being worthwhile. It's also only as effective as the purpose of that --all-targets is clear, which is why I'm glad you asked!

Copy link
Member

@jessebraham jessebraham left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the PR!

@jessebraham jessebraham merged commit 0165453 into esp-rs:main May 22, 2023
@sethp sethp deleted the feat/riscv-isr-ram branch May 22, 2023 21:25
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.

3 participants