search

knurling-rs / defmt

Efficient, deferred formatting for logging on embedded systems
https://defmt.ferrous-systems.com/
Apache License 2.0
750 stars 69 forks source link

Adds a watch flag to watch the elf file and reloads the file if it changed #807

Open JomerDev opened 5 months ago

JomerDev commented 5 months ago

This PR adds a --watch_elf/-w flag that watches the elf file for changes and reloads defmt-print on file modify or create.

To make this work I had to add tokio as dependency, since stdin.read otherwise blocked until some more serial data is received at which point it is too late to make the reload (if you do you end up with broken frames for the first few messages).

I also had to add alterable_logger to defmt-decoder to allow for the global logger to be overwritten after it was already set.

I'm open to making the flag and/or the logger changes depending on a feature.

This PR would fix issue #794

In verbose mode the logger also catches some logs from the notify crate used for the file watcher. This can probably be worked around if need be

Urhengulas commented 5 months ago

Could this not be achieved with cargo watch?

JomerDev commented 5 months ago

In theory yes, however afaik you'd usually run defmt-print by piping data from a serial port into it and I doubt cargo watch sends any through stdin received data to the inner command

jonathanpallant commented 3 weeks ago

I tested this on macOS, and I the file watching didn't appear to work.

I also wonder if some extra logging is useful, to let people know the ELF has been changed and reloaded. The --verbose flag does that.

The problem here appears to be the notifier gave the full path, but I passed a relative path to defmt-print, and they don't match. So it never reloaded the file.

Urhengulas commented 3 weeks ago

Maybe it actually does make sense to use async. In the end defmt-print does a lot of waiting either on stdin or tcp. But I'd like if we first port the current behaviour of defmt-print to async in a separate PR and see if that makes sense. And then we add the watch flag after.

@JomerDev Would you be willing to do that work?

JomerDev commented 3 weeks ago

Maybe it actually does make sense to use async. In the end defmt-print does a lot of waiting either on stdin or tcp. But I'd like if we first port the current behaviour of defmt-print to async in a separate PR and see if that makes sense. And then we add the watch flag after.

@JomerDev Would you be willing to do that work?

That would pretty much just mean that I'll split this PR apart, keeping the file watch stuff in here and move the async stuff into a separate PR, right? I can do that, sure

JomerDev commented 2 weeks ago

Done, see #855

Urhengulas commented 1 week ago

Please rebase on main. I will review next week.