Print esp-println generated defmt messages

bugadani commented 11 months ago

Closes #316

Counterpart of https://github.com/esp-rs/esp-println/pull/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, the defmt 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 the defmt feature (which internally does the same besides disabling the dependencies).

bugadani commented 10 months ago

Current status: waiting for other PRs to land before moving forward, to give priority to fixes.

I'm undecided whether I should add a --defmt-flavour=<raw|esp (default)> switch to support defmt sources other than esp-println. espflash will probably not support anything other than serial, but if someone can manage to remove all other serial output, they can save a few bytes on each message without the framing.

I'm also uncertain if we want to add filtering and detail options. espflash has too much switches as it is, though it would probably be nice if users could set what log levels they want to see/not see and if they care about a message's location info.

bugadani commented 10 months ago

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)

MabezDev commented 10 months ago

Dependency hell is real :D. Are these not breaking changes from the library(s)? Surely bumping rust-version indicates a breaking change? Idk but that's really annoying, not sure what to do here other than bump out MSRV too.

bugadani commented 10 months ago

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.

jessebraham commented 10 months ago

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 minor version:
https://doc.rust-lang.org/cargo/reference/semver.html#possibly-breaking-changing-the-minimum-version-of-rust-required

We would need to do a minor release to include these changes anyway, so no harm done.

I have no issue bumping the MSRV as long as it's not the current stable version. I'd prefer to have at least a few versions back, if possible.

Edit: don't know why I didn't check the CI logs 😂 1.70.0 if fine with me!

MabezDev commented 10 months ago

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.

bugadani commented 10 months ago

There is, however, some documentation in The Cargo Book which states that this is allowed if we bump the minor version:

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 🥺

jessebraham commented 10 months ago

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.

bugadani commented 10 months ago

Okay, well we can't merge this then and can't update a number of dependencies ever

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 0.x major versions as major releases.

bugadani commented 10 months ago

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.

bugadani commented 10 months ago

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. I considered adding a runtime switch but the implementation complexity doesn't seem to be worth it.

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.

SergioGasquez commented 9 months ago

Just tried testing your branch and couldnt monitor a esp-tempalte project (tried c3 and s3)

cargo install --force espflash --git https://github.com/bugadani/espflash --branch defmt
cargo generate esp-rs/esp-template --name test -d mcu=esp32c3 -d advanced=false
cd test
cargo r -r

Also tried updating the esp-println: esp-println = { git = "https://github.com/esp-rs/esp-println", features = ["esp32c3", "defmt"] }

But always getting the following:

Hello world!
Error:   × Main thread panicked.
                                  ├─▶ at /rustc/bf9a1c8a193fc373897196321215794c8bebbeec/library/std/src/io/mod.rs:1630:36
                                                                                                                            ╰─▶ range start index 78 out of range for slice of length 77
                                                                                                                                                                                          help: set the `RUST_BACKTRACE=1` environment variable to display a backtrace.

Not sure if its failing, or I am doing something wrong?

bugadani commented 9 months ago

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.

bugadani commented 9 months ago

Okay so the panic happened like this:

println printed a non-defmt frame
The frame delimiter has correctly handled this frame as Raw
The contents were written correctly to ResolvingPrinter. The frame ended with a \n character.
In ResolvingPrinter::write, the \n was expanded to \r\n using Normalized, increasing the length of the string by one
ResolvingPrinter::write returned the length of the normalized string instead of the input's, causing write_all to incorrectly index during the next iteration, causing the panic.

SergioGasquez commented 9 months ago

I did some further testing and seems to be working fine! Just one question/discussion, shall we have the defmt feature enable by default? @jessebraham @MabezDev @bjoernQ

bjoernQ commented 9 months ago

I did some further testing and seems to be working fine! Just one question/discussion, shall we have the defmt feature enable by default? @jessebraham @MabezDev @bjoernQ

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

MabezDev commented 9 months ago

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.

esp-rs / espflash

Print esp-println generated defmt messages #466