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

Feature: Add defmt println! macro #569

Merged
merged 17 commits into from
Sep 30, 2021
Merged

Conversation

justahero
Copy link
Contributor

@justahero justahero commented Sep 6, 2021

This PR addresses #541, adding a defmt::println! macro to always log content irrespective of a log level.

This changes the level property in Frame struct to be of Option type, which requires a number of refactorings to handle both ways to log, either using the log macros (e.g. defmt::info!) or using the defmt::println! macro.
The addition of the defmt::println macro should not break existing functionality.

parser/src/lib.rs Outdated Show resolved Hide resolved
parser/src/lib.rs Outdated Show resolved Hide resolved
@justahero justahero force-pushed the feature/add-defmt-println-macro branch 4 times, most recently from 0e4ac44 to ba979b5 Compare September 9, 2021 14:51
@justahero justahero marked this pull request as ready for review September 10, 2021 12:08
Copy link
Contributor

@jonas-schievink jonas-schievink left a comment

Choose a reason for hiding this comment

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

Looks generally good, but I'm still not sure what our strategy with log should be. Perhaps the best way forward is to not funnel defmt logs through log and remove that code?

decoder/src/lib.rs Outdated Show resolved Hide resolved
decoder/src/frame.rs Outdated Show resolved Hide resolved
#[proc_macro]
#[proc_macro_error]
pub fn println(args: TokenStream) -> TokenStream {
function_like::println::expand(args)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason you couldn't reuse log::expand like the other logging macros? (if so, please add a comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I based it on function write::expand which is simlar, println::expand does not check feature flags and sets a different interned string. But I think both versions could be made work in log::expand.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, that makes sense. Yeah, would be good to merge the logic together.

src/lib.rs Outdated Show resolved Hide resolved
.module_path(module_path)
.file(file)
.line(line)
.build(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you don't call .level(...) here, which level will end up in the Record?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The alternative is to be more transparent, for example:

        log::logger().log(
            &Record::builder()
                .args(format_args!("{}", display))
                .level(level.unwrap_or(Level::Trace))
                .target(&target)
                .module_path(module_path)
                .file(file)
                .line(line)
                .build(),
        );

and remove the if else block completely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's better to be consistent with the underlying default level in log, therefore the call is now .level(level.unwrap_or(Level::Info)

@justahero
Copy link
Contributor Author

This feature still requires a bit of writing in the defmt book so that users know about how & when to use this.

Copy link
Contributor

@jonas-schievink jonas-schievink left a comment

Choose a reason for hiding this comment

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

Code LGTM

Still not sure what the best way to proceed with the log interop code is. I'm thinking that deleting it (and handling defmt frames manually) might be the best way forward, otherwise downstream consumers will have println! converted to INFO-level logs, which is kinda wrong.

crate::Level::Info => Some(Level::Info),
crate::Level::Warn => Some(Level::Warn),
crate::Level::Error => Some(Level::Error),
crate::Level::Println => None,
Copy link
Contributor

Choose a reason for hiding this comment

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

If we keep the fallback to Info, this branch could just be => Level::Info and we'd remove the Option

@japaric
Copy link
Member

japaric commented Sep 23, 2021

I have not looked at the PR but wanted to comment on these:

otherwise downstream consumers will have println! converted to INFO-level logs, which is kinda wrong.

I agree with this. defmt::println! should not go through log's machinery (including its filter mechanisms). Those messages should be directly printed to stdout.

Still not sure what the best way to proceed with the log interop code is.

The log adapter is not part of stable decoder library API (there's no stable decoder API) so I'd be OK with removing it.

I think a "stable" log adapter would be useful in the future when there's more of a proper defmt-decoder library.
In my mind, that adapter should only expose defmt's log statements and not defmt::println. The reason for that is that we kind of (already) have semihosting functionality in defmt so defmt::println should use the host's stdout, defmt::eprintln (does not exist) should use the host's stderr, defmt::exit (this is not in defmt proper) should exit the "host process", defmt::info should use the host's logging infrastructure (log crate), and so on and so forth.
(whether we should more clearly separate semihosting functionality from deferred formatting is a separate discussion)

@justahero justahero force-pushed the feature/add-defmt-println-macro branch 2 times, most recently from fb80fec to 2e04bab Compare September 27, 2021 14:08
* fix a few lint issues
* export `println` macro
* add `Tag` variant `Println`
* add `expand` function for `println` macro
* rename log level `None` to `Println`
* refactor `println` call to use `log::expand` logic
* refactor `if else` block to log text
@justahero justahero marked this pull request as draft September 27, 2021 14:13
@japaric japaric self-assigned this Sep 28, 2021
@@ -75,8 +75,7 @@ impl<'a> DefmtRecord<'a> {
}

let timestamp = &target[DEFMT_TARGET_MARKER.len()..];

Some(Self {
(Self{
Copy link
Member

Choose a reason for hiding this comment

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

does this compile? the return type is an option but this no longer has the Some constructor

@@ -210,7 +210,7 @@ fn tests_impl(args: TokenStream, input: TokenStream) -> parse::Result<TokenStrea
test.func.sig.ident
);
quote_spanned! {
test.func.sig.ident.span() => defmt::info!(#message, __defmt_test_number, __DEFMT_TEST_COUNT);
test.func.sig.ident.span() => defmt::println!(#message, __defmt_test_number, __DEFMT_TEST_COUNT);
Copy link
Member

Choose a reason for hiding this comment

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

👍

quote!({
match (#(&(#formatting_exprs)),*) {
(#(#patterns),*) => {
defmt::export::istr(&#format_tag);
Copy link
Member

Choose a reason for hiding this comment

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

this should follow the acquire - release pattern used in the log macros, including the header call. the main difference between the two will be that tag used to constructor the header (there's a make_istr call in the expansion I believe)

@justahero justahero force-pushed the feature/add-defmt-println-macro branch from 2e04bab to 049e93c Compare September 28, 2021 10:13
This is similar to the `function_like::log::expand` function now.
The `println` macro uses similar logic as the logging macros. When
creating a new log frame the log level is optional now, adjusting for
the case when a `Frame` is generated from a `println` call.
@justahero justahero marked this pull request as ready for review September 28, 2021 13:04
Copy link
Member

@japaric japaric left a comment

Choose a reason for hiding this comment

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

Changes look good to me. It's not clear to me, though, what's the output when timestamps are enabled. Could you extend the timestamp snapshot test to include a defmt::println! call?

The other thing that's not obvious from here is whether probe-run will also show the source code location for defmt::println! statements. Including the location would not be standard but certainly useful. I think either way is fine for now personally; same thing for the timestamps.

Copy link
Member

@japaric japaric left a comment

Choose a reason for hiding this comment

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

looks great! feel free to send it to bors with the inline nit addressed 🚀

decoder/src/log.rs Outdated Show resolved Hide resolved
@justahero
Copy link
Contributor Author

bors r+

@bors
Copy link
Contributor

bors bot commented Sep 30, 2021

Build succeeded:

@bors bors bot merged commit 8a6e8ee into main Sep 30, 2021
@bors bors bot deleted the feature/add-defmt-println-macro branch September 30, 2021 12:52
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.

4 participants