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

esp-println: compile time module level log filtering #1578

Closed
rhomber opened this issue Apr 16, 2024 · 5 comments · Fixed by #2291
Closed

esp-println: compile time module level log filtering #1578

rhomber opened this issue Apr 16, 2024 · 5 comments · Fixed by #2291
Labels
package:esp-println Issues related to the esp-println package

Comments

@rhomber
Copy link

rhomber commented Apr 16, 2024

Why not just model it from env_logger and send the output to the UART? The filtering that is provided by the standard log crates is really good. I'm just not sure why there's custom handling here for all the unrelated stuff vs. just working on channeling the output of how logging normally works? I'd just honor the regular env variable names too and set any internal ones you need based on them. Consistency would be awesome. Just a thought.

@bjoernQ
Copy link
Contributor

bjoernQ commented Apr 16, 2024

Not really sure I get what you are proposing. Mind expanding on that?

Adopting the naming and format of the env-variable from env-logger and re-implementing it?

@rhomber
Copy link
Author

rhomber commented Apr 16, 2024

Hey, I mean that RUST_LOG is the standard we use in rust. See: https://rust-lang-nursery.github.io/rust-cookbook/development_tools/debugging/config_log.html

Within that single variable you can define a wealth of filtering, it's very useful. I'm not entirely sure if the code that handles it is in env-logger or somewhere in the log crate. However what I am suggesting is you leverage the stuff that does all of that and simply setup a hook to channel the log output to the UART (which you can reference within env-logger as well). I did this for another project I worked on, and it turned out really well.

But my contention is that I don't want to learn how to use the logging here, I already know how to use RUST_LOG. I am not sure if your crate can filter the same way I am use to, but also don't have time to figure it out right now. I'm sure others may experience the same frustrations. I like standards, they avoid this drama.

Thanks for your cool project though :)

@MabezDev
Copy link
Member

These are some good points, thanks for bringing it to our attention @rhomber. I believe the original reasoning behind a different variable is because of confusion between host tooling and the target. For instance, specifying RUST_LOG=info and then using some host tooling to flash the chip would result in both the tooling and the embedded application printing info logs. I don't know if this is a big deal, or if it can be worked around, but I believe that's why we did it original. The defmt project also has its own variable for this reason.

However what I am suggesting is you leverage the stuff that does all of that and simply setup a hook to channel the log output to the UART (which you can reference within env-logger as well). I did this for another project I worked on, and it turned out really well.

Sounds interesting, would you be able to share the project, or share a minimal demonstration of what you mean?

@rhomber
Copy link
Author

rhomber commented Apr 18, 2024

I mean, the variable isn't really my issue. It's more the contents of it. if it worked like RUST_LOG it would be very powerful :)

The code I wrote was for a company I no longer work for sadly. But if you check out the code here: https://github.com/rust-cli/env_logger/blob/main/src/logger.rs#L623

I think I may have made a custom Formatter? Either that or a custom Logger like they have. It was something like that anyway. Later I needed more complexity for like structured logging, but to get the stuff you need working it would have been fine. I'd look at custom Formatter and see if that's enough, then bundle it up and include env-logger (I think it's no-std?).

@MabezDev MabezDev transferred this issue from esp-rs/esp-println May 23, 2024
@MabezDev MabezDev changed the title Why is this doing it's own thing? esp-println: compile time module level log filtering May 23, 2024
@jessebraham jessebraham added package:esp-println Issues related to the esp-println package and removed enhancement labels May 23, 2024
@lure23
Copy link
Contributor

lure23 commented Jun 15, 2024

@rhomber Would you be interested in collab on the esp-println design?

I had a look at the code, due to a separate issue, and think basing it more on what logger provides would be worth a go!

Note: I am still new to Rust on embedded, and this would make a good exercise on something real.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:esp-println Issues related to the esp-println package
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants