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 defmt logging support #455

Merged
merged 3 commits into from
Apr 1, 2021
Merged

Add defmt logging support #455

merged 3 commits into from
Apr 1, 2021

Conversation

Dirbaio
Copy link
Member

@Dirbaio Dirbaio commented Mar 31, 2021

This adds basic support for optionally logging with defmt instead of log, controlled via Cargo features.

Any struct that can implement defmt::Format now does so. There's a few left, due to:

  • Missing impls for Cell, RefCell that should be added in defmt.
  • Missing impls for managed types.

The left structs are not used in log statements anyway, and are usually "big" structs, like sockets. Since this is already fully functional for integration in a firmware that uses defmt I would merge this now, and add the missing impls later when they're unblocked.

Fixes #376

Comment on lines +50 to +56

defmt-trace = []
defmt-debug = []
defmt-info = []
defmt-warn = []
defmt-error = []

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, what's the point of these, are they needed by defmt?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the defmt macros only emit code if the corresponding log level (or lower) is enabled. The end-user is supposed to depend on smoltcp with defmt, defmt-debug features to see up to debug level, for example. This is so log level is configurable per-crate.

Copy link
Contributor

Choose a reason for hiding this comment

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

But don't you need to forward the features then? I'm confused about how this would work as-is in this PR.

Copy link
Contributor

@therealprof therealprof left a comment

Choose a reason for hiding this comment

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

Sweet! Any comparison available between typical binary size and defmtenabled?

@Dirbaio Dirbaio mentioned this pull request Apr 1, 2021
@Dirbaio
Copy link
Member Author

Dirbaio commented Apr 1, 2021

defmt won't build with 1.40, which is our MSRV. The lowest I can get this to build is with 1.46, see knurling-rs/defmt#443

I guess the best option is to declare our MSRV is 1.40 except if you're using defmt, in which case the limiting factor is defmt's MSRV.

@Dirbaio
Copy link
Member Author

Dirbaio commented Apr 1, 2021

Any comparison available between typical binary size and defmtenabled?

Unfortunately no :( I've been using this internally for a while already and no longer have a firmware that uses log handy...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Support for defmt logging
2 participants