fileio cli improvments [DEVINFRA-602] [DEVINFRA-601] #395

notoriaga · 2022-01-28T01:41:41Z

Based on the feedback here - https://swift-nav.atlassian.net/wiki/spaces/~460004640/pages/2108063854/FileIO+feedback

"Nested” help messages confusing/Current CLI usage of fileio is confusing
- switched to the "scp style" commands for up/downloading like was recommended
Printing progress doesn’t seem to work on Windows (just suddenly appears as 100% done when finished)
- uses an off the shelf progress bar (that supports windows). Also adds care of showing things like upload speed which is nice. Haven't actually tested it on windows yet
Error messages formatted as Rust errors not nice
- the error shown is a special case for tcp connections where we might need to display multiple errors (one for each address we attempt to connect to). We were printing the vec with the debug representation, which is probably why it looked weird. I made a tweak to make the error display similarly to how anyhow displays error chains
list command seems to hang on Windows
- I think not requiring the reader thread to be joined avoids this issue

➜  swift-toolbox git:(steve/fileio-cli) ✗ cargo fileio ./a.txt 10.1.54.1:/persistent/a.txt   
    Finished dev [unoptimized + debuginfo] target(s) in 0.08s
     Running `target/debug/fileio ./a.txt '10.1.54.1:/persistent/a.txt'`
Error: Could not connect to 10.1.54.1:55555
Caused by:
    connection timed out

^ I changed the styling to be a bit more plain, and also added a spinner for reads (because we don't know the file size before hand). I don't really have an opinion on how either should look. It's pretty easy to mess around with if anyone is interested

notoriaga · 2022-01-30T01:33:49Z

I think reading might have a bug, appears to just hang sometimes

notoriaga · 2022-01-30T02:00:47Z

console_backend/src/bin/fileio.rs

+
+impl Remote {
+    fn connect(&self, conn: ConnectionOpts) -> Result<Fileio> {
+        let (reader, writer) = if self.host.parse::<SocketAddr>().is_ok() {


Not sure how well this will work, if it looks like a tcp addr then we try and connect to tcp, otherwise we assume it's serial.

Doesn't this need to be self.host.parse::<IpAddr>().is_ok()? As the port is specified as a different argument? Not sure how this is working for you otherwise, unless I'm misunderstanding how to enter the CLI args?

Ah yup good catch!

Actually how about

let (reader, writer) = if File::open(&self.host).is_ok() { SerialConnection::new(self.host.clone(), conn.baudrate, conn.flow_control) .try_connect(None)? } else { TcpConnection::new(self.host.clone(), conn.port)?.try_connect(None)? };

I think we should support using a hostname, and looking one up takes a bit of time so I think we should do it second. File::open(&self.host).is_ok() seems to work on windows for checking if something is a serial port, haven't tried linux yet

Sounds reasonable to me, would Path::exists be very slightly better to save an additional open?

Tried that and for whatever reason Path::exists returns false for a com port on windows, but opening the file succeeds? I don't really understand windows that well, but might have something to do with it not having quite the same "everything is a file" approach that unix has

notoriaga · 2022-01-30T02:02:15Z

console_backend/src/bin/fileio.rs

+/// Piksi File IO operations.
+#[derive(Parser)]
+#[clap(
+    name = "fileio",


In the context of the "Swift Toolbox" I think this name makes sense, but it also feels a little generic. Maybe piksi-fileio or something would be better

samvrlewis · 2022-01-31T00:31:56Z

console_backend/src/bin/fileio.rs

+        .into_remote()
+        .context("--delete flag requires <SRC> to be a remote target")?;
+    let fileio = remote.connect(conn)?;
+    fileio.remove(remote.path)?;


Over TCP, this doesn't work for me (the file doesn't get deleted) and, as far as I can tell, it's because the socket gets shut down too quickly. It does work when there's a short 1s sleep added (though I think much less than 1s would suffice as well). Comparing wireshark captures for the Python fileio and this version with a 1s delay, I can see:

To Piksi: PSH ACK From Piksi: ACK To Piksi: FIN ACK From Piksi: FIN ACK To Piksi: ACK

rust_with1sdelay.zip

But without the delay I see:

To Piksi: PSH ACK To Piksi: FIN ACK From Piksi: ACK From Piksi: ACK From Piksi: RST, ACK

rust.zip

I think SO_LINGER would help here but it looks like it's only enabled in nightly at the moment. The easy solution is probably just to add a delay here 🤷‍♂️ . I guess read/write should be fine as they wait for SBP layer acks before closing the connection.

Nice catch, added the sleep. https://doc.rust-lang.org/std/net/struct.TcpStream.html#method.set_nodelay might also work, maybe the message never gets set because its not enough data. Regardless I kind of like the sleep, make it feel like the tool is waiting for a ack even though its not 😆

- Show some example usages in help text Co-authored-by: Steven Meyer <[email protected]> Co-authored-by: notoriaga <[email protected]>

notoriaga · 2022-01-31T23:56:08Z

console_backend/src/fileio.rs

            None => continue,
        };
        if elapsed < FILE_IO_TIMEOUT {
-            std::thread::sleep(FILE_IO_TIMEOUT - elapsed);


There was sometimes a bit of a delay sometimes after finishing a write, which was because we were waiting for the timeout thread to join and it was asleep. This lets up wake up the timeout thread when we finish so we can exit faster

notoriaga · 2022-01-31T23:58:43Z

console_backend/src/fileio.rs

 ) {
    let mut batch = Vec::with_capacity(config.batch_size);
    let send_batch = move |batch: &mut Vec<MsgFileioWriteReq>| -> bool {
+        let mut guard = pending_map.lock();


This fixes a weird race condition (which I only seem to ran into reliably on windows) where we'd check to see if we finished writing by checking if pending_map was empty. The map would be empty because the request making thread didn't get a chance to insert the new requests. Holding the lock here seems to make it work

This bug manifests as a write "hanging", I'm willing to bet this has affected things like updating firmware (again, mostly appears to have been a windows issue for whatever reason)

john-michaelburke · 2022-02-01T02:41:44Z

console_backend/src/fileio.rs

        };
        if elapsed < FILE_IO_TIMEOUT {
-            std::thread::sleep(FILE_IO_TIMEOUT - elapsed);
+            timeout_parker.park_timeout(FILE_IO_TIMEOUT - elapsed);


I'm seeing the upgrade process hang when connected to a device via serial but not using a tcp connection is there something we could do in this PR to address it? I do see this message as well if it is related: timeout thread finished. Somewhat unrelated but also appears to be struggling with the speed of serial, the time to load the settings. See:
https://swift-nav.atlassian.net/browse/CPP-641

Is that with this pr? That may have been related to the other change with the lock

I think this issue is on main as well. I was just wondering if there was a timeout here you are touching that could possibly resolve the issue. I could understand addressing this in a separate PR too if it is more involved.

samvrlewis

LGTM!

notoriaga marked this pull request as draft January 28, 2022 01:41

notoriaga mentioned this pull request Jan 29, 2022

fileio: Fix hang on error [DEVINFRA-601] #396

Closed

notoriaga force-pushed the steve/fileio-cli branch 2 times, most recently from a56f544 to 687bb2c Compare January 30, 2022 01:04

notoriaga marked this pull request as ready for review January 30, 2022 01:31

notoriaga changed the title ~~[wip] fileio cli~~ fileio cli Jan 30, 2022

notoriaga changed the title ~~fileio cli~~ fileio cli improvments [DEVINFRA-602] [DEVINFRA-601] Jan 30, 2022

notoriaga added 7 commits January 29, 2022 17:57

clean up cli

94fe2ab

progress bar

9485b05

tweak tcp conn error message

be73470

lockfile

1c81be1

fix tcp conn

bb7b650

fix fileio cargo command

85045e5

cleanup

6170ccf

notoriaga force-pushed the steve/fileio-cli branch from 7e296c6 to fb39aa7 Compare January 30, 2022 01:58

notoriaga commented Jan 30, 2022

View reviewed changes

notoriaga force-pushed the steve/fileio-cli branch from fb39aa7 to 0696b2f Compare January 30, 2022 04:28

fix fileio ci build

6db4df1

notoriaga force-pushed the steve/fileio-cli branch from 0696b2f to 6db4df1 Compare January 30, 2022 04:29

notoriaga added 6 commits January 30, 2022 15:09

fix tcp conn

5fa3f2b

use ToSocketAddrs

44329d7

check serial first

426c0cc

lint

6283ca5

lint

1f751b1

set RUST_LOG

58e518b

samvrlewis reviewed Jan 31, 2022

View reviewed changes

notoriaga and others added 3 commits January 30, 2022 16:39

show read speed

b3afb66

Small fileio additions (#398)

5aeb806

- Show some example usages in help text Co-authored-by: Steven Meyer <[email protected]> Co-authored-by: notoriaga <[email protected]>

add sleep to delete

05ee626

notoriaga added 3 commits January 31, 2022 14:23

add short flag for list

894840d

don't show progress bar when reading to stdout

434c7cb

fix fileio write race condition

9fedda3

notoriaga force-pushed the steve/fileio-cli branch from dcb00cc to 9fedda3 Compare January 31, 2022 23:38

stop timeout thread early if upload finishes

67a7326

notoriaga commented Jan 31, 2022

View reviewed changes

notoriaga requested review from a team and samvrlewis January 31, 2022 23:59

john-michaelburke reviewed Feb 1, 2022

View reviewed changes

notoriaga added 5 commits January 31, 2022 20:12

propagate permission denied errors when connecting

ef0d881

fix read progress

94e9f1a

clean up sender

b0080e1

reenable steady tick

4130ebf

clean up reader timeout const

e00fc5b

samvrlewis approved these changes Feb 2, 2022

View reviewed changes

silverjam approved these changes Feb 2, 2022

View reviewed changes

notoriaga added 2 commits February 2, 2022 13:02

fix window size

41f7601

switch to hashmap

966cb0b

notoriaga merged commit 719622c into main Feb 3, 2022

notoriaga deleted the steve/fileio-cli branch February 3, 2022 00:27

silverjam mentioned this pull request Mar 22, 2022

fix typo in settings GGL -> GLL [CPP-700] #475

Merged

silverjam mentioned this pull request Apr 20, 2022

ci: bump cargo-make to 0.35.10, pin taplo-cli at 0.6.0 #525

Merged

fileio cli improvments [DEVINFRA-602] [DEVINFRA-601] #395

fileio cli improvments [DEVINFRA-602] [DEVINFRA-601] #395

Uh oh!

Conversation

notoriaga commented Jan 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

notoriaga commented Jan 30, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

notoriaga Jan 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

notoriaga Jan 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

notoriaga Jan 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

john-michaelburke Feb 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samvrlewis left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

notoriaga commented Jan 28, 2022 •

edited

Loading

notoriaga Jan 31, 2022 •

edited

Loading

notoriaga Jan 31, 2022 •

edited

Loading

notoriaga Jan 31, 2022 •

edited

Loading

john-michaelburke Feb 1, 2022 •

edited

Loading