Reorganize fmt.rs to provide correct logging location when using defmt

[andresovela]

This PR is an attempt to fix the log location shown in probe-rs when using defmt.

Right now, every log gets the location of the fmt wrapper macro where the defmt call is, something like:

embassy/embassy-executor/src/fmt.rs:106

With these changes, the logs should actually print the file and line number where the log is coming from.

I haven't thoroughly tested these changes, but they seem to work.

P.S. While working on this I thought maybe it'd be good to create an embassy-fmt crate or something, instead of having so much repetition.

[Dirbaio]

P.S. While working on this I thought maybe it'd be good to create an embassy-fmt crate or something, instead of having so much repetition.

I've already tried this in the past, I ran into a few issues:

• ideally the embassy-fmt crate should be "self-contained" and reexport either defmt, log or "noop" macros. However, defmt macros break when reexported, because they emit code that does defmt::blah which breaks if the embassy-whatever crate depends on embassy-fmt but not defmt.
• cargo features can get out of sync: user depends on crate embassy-foo with defmt enabled. this enables embassy-fmt/defmt. then user also depends on another crate embassy-bar without enabling defmt. embassy-bar will compile thinking defmt is not enabled, but it is enabled in embassy-fmt which causes things to break.

I personally think the fmt.rs duplication is not such a big of a deal. it's trivial to keep updated, just copy the changes to every file named fmt.rs in the entire repo. Can even be scripted, or checked in CI.

The approach you're trying moves some shared stuff to embassy-fmt, but needs copying some boilerplate in every lib.rs file. IMO that's a step back, because this boilerplate is now mixed with other stuff in lib.rs which makes it hard to check/mass-update.

[andresovela]

Yeah I hear you. Tbh while doing this I run into so many weird issues that I feel the whole thing is super fragile. The name resolution system felt broken as I was doing this. I have no idea why the code in this PR works, but it does haha.

I don't even understand how the current implementation on main works. I even feel like there might be compiler bugs or something, because the behavior I saw didn't make sense at all. For example, I added a #[rustfmt::skip] above the big #[macro_use] attribute and suddenly I got errors about every macro being ambiguous with the ones in core. I don't understand why there isn't any ambiguity without the rustfmt::skip, or why it even plays a role.

ideally the embassy-fmt crate should be "self-contained" and reexport either defmt, log or "noop" macros. However, defmt macros break when reexported, because they emit code that does defmt::blah which breaks if the embassy-whatever crate depends on embassy-fmt but not defmt.

I don't think I saw this issue as you describe it.

cargo features can get out of sync: user depends on crate embassy-foo with defmt enabled. this enables embassy-fmt/defmt. then user also depends on another crate embassy-bar without enabling defmt. embassy-bar will compile thinking defmt is not enabled, but it is enabled in embassy-fmt which causes things to break.

I think this is a problem that exists with or without an embassy-fmt crate.

I'm not very convinced whether this PR should be merged or not, but it does fix the location problem and it does remove a lot of duplicated code.

[Dirbaio]

I don't think I saw this issue as you describe it.

because you haven't made it self-contained. by "self-contained" I mean crates can depend on just embassy-fmt and use it and it Just Works. With your solution crates still have to depend on defmt/log and paste a 30-line snippet on lib.rs.

I think this is a problem that exists with or without an embassy-fmt crate.

no, with the code in git main you can make embassy-foo use defmt, embassy-bar use log and embassy-baz use neither. This doesn't work anymore with this PR.

it does remove a lot of duplicated code.

you're trading "good" duplicated code that can be updated with a script (standalone fmt.rs files) with "bad" duplicated code that is embedded in every lib.rs. I don't think this is worth it, even if it's less code.

I'd prefer to fix just the location problem without introducing embassy-fmt if possible. Have you checked whether #[collapse_debuginfo] fixes it? it's stable in 1.79, out today. rust-lang/rust#100758

[Dirbaio]

collapse_debuginfo doesn't work. It seems it's a rust bug (assuming I've understood correctly what collapse_debuginfo is supposed to do). I've minimized it to rust-lang/rust#126363 .

EDIT: rust-lang/rust#126365 fixes it.

[andresovela]

Have you checked whether #[collapse_debuginfo] fixes it? it's stable in 1.79, out today. rust-lang/rust#100758

Nope, wasn't aware of it but it looks like the better fix to me.

[andresovela]

I'll close this PR since it seems that collapse_debuginfo will take care of this more cleanly.