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 Debug and defmt::Format impls for fieldsets and interrupt enums. #9

Merged
merged 1 commit into from
Jan 2, 2025

Conversation

de-vri-es
Copy link
Contributor

This PR regenerates the PAC with the latest chiptool and adds a defmt feature.

Follow up for embassy-rs/chiptool#44

Some numbers in the documentation changed apart from the new impls. I think this is due to chiptool switching to a BTreeMap instead of a HashMap, to ensure reproducible builds. So I guess this shouldn't happen anymore in the future.

#[repr(transparent)]
#[derive(Copy, Clone, Eq, PartialEq)]
pub struct Perfctr(pub u32);
impl Perfctr {
#[doc = "Busfabric saturating performance counter 2 Count some event signal from the busfabric arbiters. Write any value to clear. Select an event to count using PERFSEL2"]
#[doc = "Busfabric saturating performance counter 0 Count some event signal from the busfabric arbiters. Write any value to clear. Select an event to count using PERFSEL0"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For example: Here 2 was replaced with 0.

Copy link
Member

Choose a reason for hiding this comment

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

yep, this is expected, it's ok

@Dirbaio
Copy link
Member

Dirbaio commented Jan 2, 2025

tested build times (ryzen 9 7950x):

  • current main: 0.5s
  • this PR: 0.7s
  • this PR + enabling defmt: 1.4s

tested with cargo build --timings --release --features rp2040 then looking at the build time for just the rp-pac crate.

defmt doubles the build time, wow! Shows how slow proc macros are, derive(Debug) is built into the compiler so it's more optimized.

I think it's OK since it's optional, but I'm not sure if enabling embassy-rp/defmt should autoenable rp-pac/defmt though. Most people will have defmt enabled, but few people will be printing raw register values.

thanks!

@Dirbaio Dirbaio merged commit 57be4d3 into embassy-rs:main Jan 2, 2025
@de-vri-es
Copy link
Contributor Author

@Dirbaio: Hmm good point.

My initial goal with this was to get the Interrupt enum to impl defmt::Debug, which is re-exported by the high level embassy crates, so it would be nice if that one is covered by the defmt feature of the high level crate.

What do you think about adding a defmt-interrupt feature to the PAC crates? It's a bit fine grained for my taste, but its the only enum (I think) that is publicly re-exported. I assume most extra compile time comes from the fieldsets and value enums, which are much more and sometimes much bigger.

Then the high-level crate can do:

[features]
defmt = ["dep:defmt", "pac/defmt-interrupt"]

@Dirbaio
Copy link
Member

Dirbaio commented Jan 3, 2025

i'm not sure if it's worth splitting like that. Someone else will want defmt for spi enums, someone else for gpio enums, we're not going to add defmt-gpio, defmt-spi etc.

maybe the HALs could have a unstable-pac-defmt feature instead.

@de-vri-es
Copy link
Contributor Author

I kinda agree, but the reason I think Interrupt is special is because the HAL crates expose it directly, even without the unstable-pac feature. As a user, you don't even know it's a re-export (even looking at the source it's quite hard to find):

https://github.com/embassy-rs/embassy/blob/e68efc2d7cdea195aec112ecb61231e148a282c2/embassy-hal-internal/src/interrupt.rs#L17-L20

Since the Interrupt type (as far as the user knows) lives inside the HAL crate, it also makes sense to have it be covered by the defmt feature of the HAL crate. I was quite surprised to find out that it wasn't.

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