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

JomerDev · 2024-01-28T00:09:47Z

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 · 2024-01-29T15:37:13Z

Could this not be achieved with cargo watch?

JomerDev · 2024-01-29T16:23:20Z

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

This generally looks OK to me, modulo a few comments.

I am a little bit worried about adding tokio to the defat-print dependency tree, but maybe someone can persuade me that this is better than running the TCP stream read in a thread with a 500ms timeout that checks some shared shutdown flag.

jonathanpallant · 2024-06-12T08:38:53Z

CHANGELOG.md

@@ -11,11 +11,13 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 - [#803]: `CI`: Disable nightly qemu-snapshot tests
 - [#789]: `defmt`: Add support for new time-related display hints
 - [#783]: `defmt-decoder`: Move formatting logic to `Formatter`
+- [#807]: `defmt-print`: Add watch_elf flrag to allow elf reload without restarting `defmt-print`


Suggested change

- [#807]: `defmt-print`: Add watch_elf flrag to allow elf reload without restarting `defmt-print`

- [#807]: `defmt-print`: Add `watch_elf` flag to allow ELF file reload without restarting `defmt-print`

decoder/Cargo.toml

jonathanpallant · 2024-06-12T08:56:51Z

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.

print/Cargo.toml

print/src/main.rs

Urhengulas · 2024-06-12T12:22:38Z

print/src/main.rs

@@ -40,11 +47,14 @@ struct Opts {
    #[arg(short = 'V', long)]
    version: bool,

+    #[arg[short, long]]


Let's be consistent

Suggested change

#[arg[short, long]]

#[arg(short, long)]

Urhengulas · 2024-06-12T15:19:09Z

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 · 2024-06-12T15:41:25Z

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 · 2024-06-15T15:00:07Z

Done, see #855

Urhengulas · 2024-06-26T04:57:16Z

Please rebase on main. I will review next week.

Urhengulas · 2024-07-09T21:38:39Z

@JomerDev ping

JomerDev · 2024-07-09T21:40:29Z

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

Urhengulas

First round of review, still need to actually test it

Urhengulas · 2024-07-16T12:31:44Z

print/src/main.rs

+use notify::{Config, Event, RecommendedWatcher, RecursiveMode, Watcher};

 use tokio::{


Suggested change

use notify::{Config, Event, RecommendedWatcher, RecursiveMode, Watcher};

use tokio::{

use notify::{Config, Event, RecommendedWatcher, RecursiveMode, Watcher};

use tokio::{

Urhengulas · 2024-07-16T12:32:51Z

print/Cargo.toml

 log = "0.4"
 tokio = { version = "1.38", features = ["full"] }
+notify = "6.1.1"
+futures = "0.3"


Please keep alphabetic ordering

Suggested change

log = "0.4"

tokio = { version = "1.38", features = ["full"] }

notify = "6.1.1"

futures = "0.3"

futures = "0.3"

log = "0.4"

notify = "6.1"

tokio = { version = "1.38", features = ["full"] }

Urhengulas · 2024-07-16T12:38:26Z

print/src/main.rs

+        let (tx, mut rx) = tokio::sync::mpsc::channel(1);
+
+        let mut watcher = RecommendedWatcher::new(
+            move |res| {
+                futures::executor::block_on(async {
+                    tx.send(res).await.unwrap();
+                })
+            },
+            Config::default(),
+        )?;
+        let mut directory_path = opts.elf.clone().unwrap();
+        directory_path.pop(); // We want the elf directory instead of the elf, since some editors remove and recreate the file on save which will remove the notifier
+        watcher.watch(directory_path.as_ref(), RecursiveMode::NonRecursive)?;
+
+        let path = opts.elf.clone().unwrap();
+
+        loop {
+            select! {
+                r = run(opts.clone(), &mut source) => r?,
+                _ = has_file_changed(&mut rx, &path) => ()
+            }
+        }


Please move this to a run_and_watch function

Urhengulas · 2024-07-16T12:42:13Z

print/src/main.rs

+        directory_path.pop(); // We want the elf directory instead of the elf, since some editors remove and recreate the file on save which will remove the notifier
+        watcher.watch(directory_path.as_ref(), RecursiveMode::NonRecursive)?;


Could it be that we get notified about some unrelated changes? What if I am running two programs in parallel? Will changing one of the make both reload?

We will, yes. But since there is no way to filter them directly in the watcher, we filter them in has_file_changed instead. There we first check that the path matches the path of the elf file, and then we check the event kind and ignore everything that isn't a create or modify of that file
If you run two programs in parallel, as long as they not use the exact same elf file (with the exact same path) they wont both reload

Urhengulas · 2024-07-16T12:53:15Z

decoder/Cargo.toml

@@ -43,6 +43,7 @@ object = { version = "0.35", default-features = false, features = [
 serde = { version = "1", features = ["derive"] }
 serde_json = { version = "1", features = ["arbitrary_precision"] }
 regex = "1"
+alterable_logger = "1.0.0"


Suggested change

alterable_logger = "1.0.0"

alterable_logger = "1"

print/src/main.rs

Urhengulas · 2024-07-16T14:49:13Z

print/Cargo.toml

@@ -20,3 +20,5 @@ defmt-decoder = { version = "=0.3.11", path = "../decoder", features = [
 ] }
 log = "0.4"
 tokio = { version = "1.38", features = ["full"] }
+notify = "6.1.1"


Should we disable crossbeam? See https://docs.rs/notify/latest/notify/index.html#crossbeam-channel--tokio

Seeing as nobody could reliably confirm the issue and since std has been using the crossbeam implementation since 1.67 anyway I don't see a reason to do so.
See also the last comment on the issue: notify-rs/notify#380 (comment)

Urhengulas · 2024-07-17T11:44:25Z

print/src/main.rs

+                futures::executor::block_on(async {
+                    tx.send(res).await.unwrap();
+                })


Would tokio::runtime::Handle.current() work here? Then we could get rid of the futures dependency.

Suggested change

futures::executor::block_on(async {

tx.send(res).await.unwrap();

})

tokio::runtime::Handle::current().block_on(async {

tx.send(res).await.unwrap();

})

It's probably even easier to use tx.blocking_send instead, that way there is no need for any async context. I'll try that later today

print/src/main.rs

JomerDev · 2024-07-24T19:01:12Z

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

jonathanpallant reviewed Jun 12, 2024

View reviewed changes

Urhengulas requested changes Jun 12, 2024

View reviewed changes

JomerDev mentioned this pull request Jun 15, 2024

Make defmt-print async #855

Merged

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

e658a6c

JomerDev force-pushed the watch-elf-file branch from 5ec4a2b to b6a2429 Compare July 10, 2024 16:27

Add changelog entry, fix rust format

053318d

JomerDev force-pushed the watch-elf-file branch from b6a2429 to 053318d Compare July 10, 2024 16:29

JomerDev requested a review from Urhengulas July 11, 2024 10:19

Address comments

62dddc8

Urhengulas requested changes Jul 16, 2024

View reviewed changes

Urhengulas requested changes Jul 17, 2024

View reviewed changes

Address comments, move code to run_and_watch function

ec2b6ef

JomerDev requested a review from Urhengulas July 23, 2024 10:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

JomerDev commented Jan 28, 2024 •

edited

Loading

Urhengulas commented Jan 29, 2024

JomerDev commented Jan 29, 2024

jonathanpallant left a comment

jonathanpallant Jun 12, 2024

Urhengulas Jul 16, 2024

jonathanpallant commented Jun 12, 2024 •

edited

Loading

Urhengulas Jun 12, 2024

Urhengulas Jul 16, 2024

Urhengulas commented Jun 12, 2024

JomerDev commented Jun 12, 2024

JomerDev commented Jun 15, 2024

Urhengulas commented Jun 26, 2024

Urhengulas commented Jul 9, 2024

JomerDev commented Jul 9, 2024

Urhengulas left a comment

Urhengulas Jul 16, 2024

Urhengulas Jul 16, 2024

Urhengulas Jul 16, 2024

Urhengulas Jul 16, 2024

JomerDev Jul 22, 2024

Urhengulas Jul 16, 2024

Urhengulas Jul 16, 2024

JomerDev Jul 23, 2024

Urhengulas Jul 17, 2024

JomerDev Jul 22, 2024

JomerDev commented Jul 24, 2024

	- [#807]: `defmt-print`: Add watch_elf flrag to allow elf reload without restarting `defmt-print`
	- [#807]: `defmt-print`: Add `watch_elf` flag to allow ELF file reload without restarting `defmt-print`

		use notify::{Config, Event, RecommendedWatcher, RecursiveMode, Watcher};

		use tokio::{

		directory_path.pop(); // We want the elf directory instead of the elf, since some editors remove and recreate the file on save which will remove the notifier
		watcher.watch(directory_path.as_ref(), RecursiveMode::NonRecursive)?;

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

Are you sure you want to change the base?

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

Conversation

JomerDev commented Jan 28, 2024 • edited Loading

Urhengulas commented Jan 29, 2024

JomerDev commented Jan 29, 2024

jonathanpallant left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathanpallant commented Jun 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Urhengulas commented Jun 12, 2024

JomerDev commented Jun 12, 2024

JomerDev commented Jun 15, 2024

Urhengulas commented Jun 26, 2024

Urhengulas commented Jul 9, 2024

JomerDev commented Jul 9, 2024

Urhengulas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JomerDev commented Jul 24, 2024

JomerDev commented Jan 28, 2024 •

edited

Loading

jonathanpallant commented Jun 12, 2024 •

edited

Loading