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

[JomerDev]

trafficstars

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]

Could this not be achieved with cargo watch?

[JomerDev]

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]

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]

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]

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]

Done, see #855

[Urhengulas]

Please rebase on main. I will review next week.

[Urhengulas]

@JomerDev ping

[JomerDev]

Sorry, I didn't have much time last week,. I'll try and get it done tomorrow

[JomerDev]

So, I don't know why the lint is complaining, I tried to apply it's suggestion but the rust-analyzer complains. I also don't get the lint when I run clippy locally

[Urhengulas]

So, I don't know why the lint is complaining, I tried to apply it's suggestion but the rust-analyzer complains. I also don't get the lint when I run clippy locally

I do see the suggestion locally and also my RA is happy with it. I am using clippy 0.1.80 (0514789 2024-07-21).

The diff is:

diff --git a/print/src/main.rs b/print/src/main.rs
index 88b0cfd..0125a27 100644
--- a/print/src/main.rs
+++ b/print/src/main.rs
@@ -134,7 +134,7 @@ async fn has_file_changed(rx: &mut Receiver<Result<Event, notify::Error>>, path:
     true
 }
 
-async fn run_and_watch(opts: Opts, mut source: &mut Source) -> anyhow::Result<()> {
+async fn run_and_watch(opts: Opts, source: &mut Source) -> anyhow::Result<()> {
     let (tx, mut rx) = tokio::sync::mpsc::channel(1);
 
     let mut watcher = RecommendedWatcher::new(
@@ -151,7 +151,7 @@ async fn run_and_watch(opts: Opts, mut source: &mut Source) -> anyhow::Result<()
 
     loop {
         select! {
-            r = run(opts.clone(), &mut source) => r?,
+            r = run(opts.clone(), source) => r?,
             _ = has_file_changed(&mut rx, &path) => ()
         }
     }

[JomerDev]

Hmm. I definitely agree that it has to do with the relative path. A possible fix could be to just remove every instance of ../ or ./ we find in the given path (since we only check that the event paths contains the given path), though that could potentially cause issues. A better solution might be to use std::fs::canonicalize on the given path, I would assume that event.paths only contains canonicalized paths (but I do not actually know if that is true)

[Urhengulas]

@JomerDev I played a bit around with it and for it seems it is enough if we canonicalize the ELF path the user provides. Can you please test if that works for you?

[Urhengulas]

@Urhengulas said:

@JomerDev I played a bit around with it and for it seems it is enough if we canonicalize the ELF path the user provides. Can you please test if that works for you?

@jonathanpallant Can you please test if that also works on MacOS?