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 optional automatic defmt implementation #42

Merged
merged 4 commits into from
Jun 18, 2024
Merged

Conversation

agrif
Copy link
Contributor

@agrif agrif commented Jun 8, 2024

This PR adds a defmt option to the bitfield macro, which adds code to implement defmt::Format. It also adds info to the README and a few appropriate tests.

Some implementation notes you might want to know:

  • Defmt has built-in support for bitfields which leads to a very compact representation, however this method always outputs plain integers for the fields. I chose not to use this, and instead format each field separately, in order to take advantage of the defmt::Format implementations on any custom types. I feel like this better matches the default fmt::Debug implementation. I considered doing both, and making it configurable, but that seemed like too big a change.
  • This macro assumes any type named u8 is the primitive u8, etc. This is also what the official defmt derive macro does, to more efficiently encode these values. It looks like the bitfield macro already makes this assumption when figuring out default bit widths, so probably not a big deal.

  • The generated code uses defmt to refer to defmt, and not ::defmt. This is what the defmt crate itself does in its own macros, for some pretty subtle reasons. I've added a big comment in the code about this, to hopefully prevent somebody from 'fixing' it.

Please let me know if you would like any changes, or have any questions!

Copy link
Owner

@wrenger wrenger left a comment

Choose a reason for hiding this comment

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

Your changes are quite good. Still, I would change a few parts to make it easier to add new formatting/logging crates in the future, if the need arises.

src/lib.rs Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
tests/test.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@agrif
Copy link
Contributor Author

agrif commented Jun 11, 2024

I'll start on these changes, I just have a few clarifying questions first...

@agrif
Copy link
Contributor Author

agrif commented Jun 11, 2024

Ok, I removed the extra #[cfg(test)] and moved the debug implementations into implement_debug. I've left these as separate commits for the sake of discussion, but of course you can squash them (or not) as you please.

If you're keeping the separate boolean flags, some other projects use e.g. #[bitfield(u16, defmt = true)] to unconditionally implement defmt, and #[bitfield(u16, defmt = "flag")] to conditionally implement it behind a feature flag. I'd be happy to add that (for debug also??), but since it mixes argument types I know not everybody is happy about that.

@wrenger
Copy link
Owner

wrenger commented Jun 18, 2024

Ok, thanks for the changes. I merge this.

@wrenger
Copy link
Owner

wrenger commented Jun 18, 2024

If you're keeping the separate boolean flags, some other projects use e.g. #[bitfield(u16, defmt = true)] to unconditionally implement defmt, and #[bitfield(u16, defmt = "flag")] to conditionally implement it behind a feature flag. I'd be happy to add that (for debug also??), but since it mixes argument types I know not everybody is happy about that.

Let's do this in another PR, as accessing features inside proc macros is a bit tedious.

@wrenger wrenger merged commit 93dcfff into wrenger:main Jun 18, 2024
3 checks passed
@agrif
Copy link
Contributor Author

agrif commented Jun 18, 2024

Thanks!

Let's do this in another PR, as accessing features inside proc macros is a bit tedious.

Oh? It's more complicated than adding #[cfg(feature = #feature_name)] before the impl inside the quote?

@wrenger
Copy link
Owner

wrenger commented Jun 20, 2024

Ive extended this to support arbritrary cfg attributes in afa7fd8

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