Skip to content

Conversation

romancardenas
Copy link
Contributor

An alternative to #345

It presents way fewer changes, but now we expect caller crates of the pac_enum macro to include two features:

  • rt
  • v-trap

I personally prefer this approach, but let me know your opinion.

Related issues: rust-embedded/svd2rust#948

@romancardenas romancardenas requested a review from a team as a code owner September 22, 2025 14:20
@romancardenas
Copy link
Contributor Author

romancardenas commented Sep 22, 2025

The issue with this approach, as you can see in CI, is that we need to include rt features everywhere, even if we don't use it.

I can move the riscv-rt to a tests-riscv-rt directory to avoid including the rt feature and improve the CI to check more variants, but I prefer to get your input before working.

@romancardenas
Copy link
Contributor Author

There is a drawback with this approach: PACs must include both rt and v-trap features, even if the target does not support vectored mode at all.

@rmsyn
Copy link
Contributor

rmsyn commented Sep 24, 2025

There is a drawback with this approach: PACs must include both rt and v-trap features, even if the target does not support vectored mode at all.

I think your approach in #345 is probably preferable. Especially since the macros live in that crate, it makes sense to add the features there, rather than in the generated code.

It will probably also be easier for downstream users to understand, rather than magic new features that show up in their code, and having to trace the source back to the proc macro.

The issue with this approach, as you can see in CI, is that we need to include rt features everywhere, even if we don't use it.

This issue also appears in #345, there are CI jobs that have been continually hung for over 24 hours 😱

@romancardenas
Copy link
Contributor Author

I agree with you @rmsyn . Adding magical feature gates in proc macros does not sound like a good practice. Closing in favor of #345

@romancardenas romancardenas deleted the riscv-pac-enum-fix2 branch September 26, 2025 07:53
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.

2 participants