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 Debug and defmt::Format for fieldsets, enums and the interrupt enum. #44

Merged
merged 5 commits into from
Jan 2, 2025

Conversation

de-vri-es
Copy link
Contributor

@de-vri-es de-vri-es commented Jan 1, 2025

This PR:

  • Implements the Debug and defmt::Format traits for generated fieldsets, value enums and interrupt enums.
    • Where possible the traits are simply derived.
    • The defmt::Format impls are disabled by default and must be enabled with a "defmt" feature.
  • Re-adds the expand_extends transform which I think was accidentally removed during a refactor of the transform system (couldn't test this with the stm32-metapac crate otherwise).
  • Fixes some clippy warnings.

I think the Debug and defmt::Format impls can help tremendously when debugging embedded applications :)

I've tested this by using it to regenerate the stm32-metapac crate, which added the trait impls as expected. I've also tested that the generated code compiles and looks as as expected.

@Dirbaio
Copy link
Member

Dirbaio commented Jan 1, 2025

The defmt::Format impls are disabled by default and must be enabled with a "defmt" feature.

could you make defmt optional at generation time?

Currently the generated code will always contain the #[cfg(feature="defmt")] somethingsomething. If someone wants their PAC crate to not depend on defmt at all and not have a defmt feature they'll get "unexpected cfg" warnings.

@de-vri-es
Copy link
Contributor Author

de-vri-es commented Jan 2, 2025

I was thinking about that and I actually initially did that.

However, adding a knob for this is a breaking change, and I wasn't sure how much freedom to allow: should the user be able to choose their own feature name?

That, and the fact that other feature flags are not configurable at generation time, I figured it might be fine like this.

I also thought, if this tool is basically used for embassy only, getting those warnings might be nice to ensure that the embassy PAC crates don't forget to add the feature to their Cargo.toml.

But, I can add a generation time configuration option :)

@de-vri-es
Copy link
Contributor Author

Added an option for it to generate::Options, and exposed it on the command line as well.

I also modified the way that generate::Options is constructed, so that it's possible to add more options without breaking backwards compatibility in the future. If this is undesired, let me know, I can revert it to a plain struct with public members.

Note that I defaulted to generating defmt support, as I think that it's a good thing if the ecosystem all provides easy debugging out of the box. If you prefer a more conservative approach, I can also adjust this.

Copy link
Member

@Dirbaio Dirbaio left a comment

Choose a reason for hiding this comment

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

thanks!

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