Skip to content

Conversation

newAM
Copy link

@newAM newAM commented Aug 9, 2021

This pull-request adds optional support for defmt.

defmt, like this crate aims to solve a problem in the core library, namely the code-size of the formatting logic.

@PTaylor-us
Copy link
Member

That you for the PR. I'm not yet familiar with defmt, but will definitely take a look. @korken89, thoughts?

@korken89
Copy link
Collaborator

From my side I think it's a good addition.
I use defmt in my projects and is a real space saver.

@newAM
Copy link
Author

newAM commented Aug 13, 2021

That you for the PR. I'm not yet familiar with defmt, but will definitely take a look. @korken89, thoughts?

For more context defmt is developed under the kurling-rs project by ferrous systems.

Advantages

  • Enables logging embedded-time types on targets with limited flash. Formatting a simple u8 with core::fmt takes >2k of code size on its own, on targets with 16k of flash that simply means logging is not realistic without defmt.

Disadvantages/Risks

  • defmt becomes unmaintained. I think given the backers and continual development since its inception in November 2020 that this is extremely unlikely.
  • Additional maintenance cost. The maintenance cost is very low for defmt; there has only ever been one release with breaking changes (0.1.0 -> 0.2.0) in the almost 2 years it has been around.

Copy link

@Sh3Rm4n Sh3Rm4n left a comment

Choose a reason for hiding this comment

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

Ha, I made a draft for implementing defmt as well, but did not had the time to make a proper PR out of it. 😀

So I took the opportunity to look over your implantation :)

LGTM

@PTaylor-us PTaylor-us added the enhancement New feature or request label Aug 15, 2021
@PTaylor-us
Copy link
Member

Tests are failing. Looks like some undefined references in the defmt. Are we missing some dependencies?

@Sh3Rm4n
Copy link

Sh3Rm4n commented Aug 15, 2021

It seems like only windows is failing and the linux build is successful. This might be because link.exe does not support the linkage symbols, which defmt is injecting, which it is using to store all the information / data to defer the strings.

It might just be that defmt does not support windows.

@newAM
Copy link
Author

newAM commented Aug 15, 2021

It does support Windows, just not non-embedded targets (knurling-rs/defmt#463). I am surprised the unit tests ran on Linux at all! I pushed a change to skip the defmt feature for the unit tests.

@PTaylor-us PTaylor-us added the good first issue Good for newcomers label Aug 29, 2021
@PTaylor-us PTaylor-us self-assigned this Aug 29, 2021
@PTaylor-us PTaylor-us added this to the v0.13.0 milestone Aug 29, 2021
@PTaylor-us PTaylor-us changed the base branch from master to release/v0.13.0 September 12, 2021 20:46
@PTaylor-us PTaylor-us merged commit ee0c883 into FluenTech:release/v0.13.0 Sep 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants