-
Notifications
You must be signed in to change notification settings - Fork 119
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
Print esp-println generated defmt messages #466
Conversation
0fc516b
to
090c2d5
Compare
b816ac1
to
9162ef6
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Oh come on... @jessebraham do you think we can bump MSRV for defmt, either selectively or otherwise? (defmt has at least one dependency that is 1.67+. I'm not sure if we can use an earlier parser to work around it) |
Dependency hell is real :D. Are these not breaking changes from the library(s)? Surely bumping |
In this case, defmt-parser is a new dependency so it's probably not a case of a patch-with-breaking-MSRV-bump. CI just wasn't passing for the last few weeks and I didn't notice this PR requires a bump. |
Strictly speaking, I don't think this situation is technically covered by SemVer itself. There is, however, some documentation in The Cargo Book which states that this is allowed if we bump the We would need to do a I have no issue bumping the MSRV as long as it's not the current Edit: don't know why I didn't check the CI logs 😂 |
Oh yes, of course. In that case we either bump the MSRV and be done with it, or we say we don't make any MSRV guarentees behind a defmt feature (that we'd need to introduce). Imo bumping MSRV is okay, it realistically only affects library users which (publically) is just me: https://crates.io/crates/espflash/reverse_dependencies :D. |
Oh hell please do not start following this madness. While not covered by Semver, an MSRV change should absolutely be treated as major breaking. For an application it doesn't matter, but a library bumping MSRV with a minor version is absolutely inconsiderate and espflash is a library 🥺 |
Okay, well we can't merge this then and can't update a number of dependencies ever. So I'm not sure that's preferable. |
Not sure I follow. Sorry for being way more emotional here than probably warranted. An MSRV bump being a major change doesn't mean we can't update dependencies, the release just needs to be a major version after that, in my opinion. SemVer considers minor bumps with |
Have you guys a recommendation how I could featurize defmt? I think it would be nice to ship the binaries with it enabled, but I also don't want to force defmt on libraries. I believe I can extract the code itself to make parsing in monitor opt-in, hide that behind a feature flag and call the library part done. But for the binaries, I think it would be better to have the feature default on, as opposed to passing a flag in CI. I also don't know if the feature should be controllable in the binaries, via a --defmt switch or something similar. I tend to think it'd just complicate the implementation without much value, but I'd love to hear your take. |
I'd consider this done now, although there are a few open questions. I've been using this PR for the last few days/weeks without any issue, though I'm a bit unsure if defmt-by-default is the polite way to handle the issue. Because of the MSRV bump I'd suggest a major release (3.0.0) at least for the library, but considering the low number of public dependent maybe not doing this isn't the end of the world. For binaries, semver is a lot less relevant. The defmt feature is enabled by default mainly because I wanted to avoid editing the CI files for this. Let me know if you'd like to instead keep defmt off by default. Doing that would allow keeping MSRV low, which would avoid the semver-related headaches. From-source installations could still enable the feature, however one downside is that it's not very feasible to install from source on a Raspberry Pi. |
Co-authored-by: Sergio Gasquez Arcos <[email protected]>
Just tried testing your branch and couldnt monitor a
Also tried updating the But always getting the following:
Not sure if its failing, or I am doing something wrong? |
Interesting find. I have experienced a few panics myself, but so far the impl has been stable enough for me. The last commit has resolved a similar issue for me but maybe newlines weren't the root cause. I'll investigate. |
Okay so the panic happened like this:
|
I did some further testing and seems to be working fine! Just one question/discussion, shall we have the |
I guess it shouldn't be a default. While it's not very likely that someone tries to use the framing marker by accident it's not impossible. Additionally, I think that a user who wants to use defmt is probably a more advanced user and it should be okay if they need some extra steps in enabling it. But maybe I'm thinking too cautiously |
I agree with @bjoernQ's points. I think that if we get a lot of requests to enable it by default then we can re-assess later. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just did some testing and everything was working! Thanks for the contribution!
* Print defmt frames output by esp-println * Add changelog entry * Display log level * Bump MSRV to 1.70 * Refactor for opt-in defmt * Make defmt a default feature * Update CHANGELOG.md Co-authored-by: Sergio Gasquez Arcos <[email protected]> * Fix panic on frame containing multiple lines * Fix panic due to newline normalization * Don't enable defmt by default --------- Co-authored-by: Sergio Gasquez Arcos <[email protected]>
* Print defmt frames output by esp-println * Add changelog entry * Display log level * Bump MSRV to 1.70 * Refactor for opt-in defmt * Make defmt a default feature * Update CHANGELOG.md Co-authored-by: Sergio Gasquez Arcos <[email protected]> * Fix panic on frame containing multiple lines * Fix panic due to newline normalization * Don't enable defmt by default --------- Co-authored-by: Sergio Gasquez Arcos <[email protected]>
Closes #316
Counterpart of esp-rs/esp-println#48 as it relies on the injected framing bytes. Not sure how to best lift this curse yet.
This PR adds a new compile-time
defmt
flag that enables defmt support in the binaries and the library. For this, the PR raises MSRV to 1.70. By default, thedefmt
feature is enabled. defmt detection is done by parsing the elf file.The PR refactors serial port input handling so library users can define their own processing if they so choose. The default processor is able to recognize and parse frames generated by esp-println. Library users can disable this processing by using the
Serial
input processor or by disabling thedefmt
feature (which internally does the same besides disabling the dependencies).