Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FileIO usage tweaks [DEVINFRA-640] #429

Merged
merged 14 commits into from
Feb 16, 2022
20 changes: 13 additions & 7 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion console_backend/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ serialport = { git = "https://github.com/swift-nav/serialport-rs.git", default-f
directories = "4"
anyhow = { version = "1", features = ["backtrace"] }
serde_yaml = "0.8.23"
clap = { version = "3.0.14", features = ["derive"] }
clap = { version = "3.1.0", features = ["derive"] }
indexmap = { version = "1.8.0", features = ["serde"] }
serde_json = { version = "1" }
crossbeam = "0.8"
Expand Down
129 changes: 81 additions & 48 deletions console_backend/src/bin/files.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
use std::{
borrow::Cow,
convert::Infallible,
fs::{self, File},
io::{self, Write},
path::PathBuf,
Expand All @@ -9,11 +8,9 @@ use std::{
};

use anyhow::{anyhow, Context};
use clap::{
AppSettings::{ArgRequiredElseHelp, DeriveDisplayOrder},
Args, Parser,
};
use clap::{AppSettings::DeriveDisplayOrder, Args, Parser};
use indicatif::{ProgressBar, ProgressStyle};
use lazy_static::lazy_static;
use sbp::{link::LinkSource, SbpIterExt};

use console_backend::{
Expand All @@ -29,13 +26,13 @@ fn main() -> Result<()> {
}
env_logger::init();
let opts = Opts::parse();
if opts.list {
list(opts.src, opts.conn)
} else if opts.delete {
delete(opts.src, opts.conn)
if let Some(target) = opts.list {
list(target, opts.conn)
} else if let Some(target) = opts.delete {
delete(target, opts.conn)
} else {
if let Some(dest) = opts.dest {
transfer(opts.src, dest, opts.conn)
if let (Some(src), Some(dest)) = (opts.src, opts.dest) {
transfer(src, dest, opts.conn)
} else {
Err(anyhow!(
"file transfers require both <SRC> and <DEST> to be set"
Expand All @@ -44,52 +41,76 @@ fn main() -> Result<()> {
}
}

/// A SwiftNav fileio API client
#[derive(Parser)]
#[clap(
name = "swift-files",
version = include_str!("../version.txt"),
setting = ArgRequiredElseHelp | DeriveDisplayOrder,
override_usage = "\
swift-files <SRC> <DEST>
swift-files --list <SRC>
swift-files --delete <SRC>
#[cfg(target_os = "windows")]
const SERIAL_NAME: &str = "COM1";
#[cfg(target_os = "linux")]
const SERIAL_NAME: &str = "/dev/ttyUSB0";
#[cfg(target_os = "macos")]
const SERIAL_NAME: &str = "/dev/cu.usbserial";

lazy_static! {
static ref FILEIO_USAGE: String = format!(
"\
To copy a local file to a Swift device:
swift-files <FILE_PATH> <HOST>:<FILE_PATH>
To copy a file from a Swift device:
swift-files <HOST>:<FILE_PATH> <FILE_PATH>
To list files in a directory on a Swift device:
swift-files --list <HOST>:<DIRECTORY_PATH>
To delete a file on a Swift device:
swift-files --delete <HOST>:<FILE_PATH>

<HOST> can either be an IP address when the Swift device is connected
via TCP (for eg: 192.168.0.222) or the name of the serial device when
the Swift device is connected via serial (for eg: {serial}).

TCP Examples:
- List files on Piksi:
- List files on Swift device:
swift-files --list 192.168.0.222:/data/
- Read file from Piksi:
- Read file from Swift device:
swift-files 192.168.0.222:/persistent/config.ini ./config.ini
- Write file to Piksi:
- Write file to Swift device:
swift-files ./config.ini 192.168.0.222:/persistent/config.ini
- Delete file from Piksi:
- Delete file from Swift device:
swift-files --delete 192.168.0.222:/persistent/unwanted_file

Serial Examples:
- List files on Piksi:
swift-files --list /dev/ttyUSB0:/data/
- Read file from Piksi:
swift-files /dev/ttyUSB0:/persistent/config.ini ./config.ini
- Write file to Piksi:
swift-files ./config.ini /dev/ttyUSB0:/persistent/config.ini
- Delete file from Piksi:
swift-files --delete /dev/ttyUSB0:/persistent/unwanted_file
"
- List files on Swift device:
swift-files --list {serial}:/data/
- Read file from Swift device:
swift-files {serial}:/persistent/config.ini ./config.ini
- Write file to Swift device:
swift-files ./config.ini {serial}:/persistent/config.ini
- Delete file from Swift device:
swift-files --delete {serial}:/persistent/unwanted_file
",
serial = SERIAL_NAME
);
}

/// A SwiftNav fileio API client
#[derive(Parser)]
#[clap(
name = "swift-files",
version = include_str!("../version.txt"),
arg_required_else_help = true,
setting = DeriveDisplayOrder,
override_usage = &**FILEIO_USAGE
)]
struct Opts {
/// The source target
src: Target,
src: Option<Target>,

/// The destination when transfering files
dest: Option<Target>,

/// List a directory
#[clap(long, short, conflicts_with_all = &["dest", "delete"])]
list: bool,
#[clap(long, short, value_name="TARGET", conflicts_with_all = &["dest", "delete"])]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slight change, this will mean you won't be able to do swift-files 192.168.0.222:/data --list any longer but IMO makes reading the code slightly easier.

list: Option<Target>,

/// Delete a file
#[clap(long, conflicts_with_all = &["dest", "list"])]
delete: bool,
#[clap(long, value_name="TARGET", conflicts_with_all = &["dest", "list"])]
delete: Option<Target>,

#[clap(flatten)]
conn: ConnectionOpts,
Expand All @@ -116,9 +137,9 @@ struct ConnectionOpts {
}

fn list(target: Target, conn: ConnectionOpts) -> Result<()> {
let remote = target
.into_remote()
.context("--list flag requires <SRC> to be a remote target")?;
let remote = target.into_remote().context(
"--list flag requires <TARGET> to be a remote target of the form <HOST>:<DIRECTORY_PATH>",
)?;
let mut fileio = remote.connect(conn)?;
let files = fileio.readdir(remote.path)?;
for file in files {
Expand All @@ -128,9 +149,9 @@ fn list(target: Target, conn: ConnectionOpts) -> Result<()> {
}

fn delete(target: Target, conn: ConnectionOpts) -> Result<()> {
let remote = target
.into_remote()
.context("--delete flag requires <SRC> to be a remote target")?;
let remote = target.into_remote().context(
"--delete flag requires <TARGET> to be a remote target of the form <HOST>:<FILE_PATH>",
)?;
let fileio = remote.connect(conn)?;
fileio.remove(remote.path)?;
// without this sleep the program exits and the connection closes before the delete message
Expand All @@ -157,7 +178,13 @@ fn read(src: Remote, dest: PathBuf, conn: ConnectionOpts) -> Result<()> {
let (dest, pb): (Box<dyn Write + Send>, _) = if dest.to_str() == Some("-") {
(Box::new(io::stdout()), ReadProgress::stdout())
} else {
(Box::new(File::create(dest)?), ReadProgress::file())
(
Box::new(
File::create(&dest)
.with_context(|| format!("Could not open {:?} for writing", &dest))?,
),
ReadProgress::file(),
)
};
let mut fileio = src.connect(conn)?;
pb.set_message("Reading...");
Expand All @@ -170,7 +197,8 @@ fn read(src: Remote, dest: PathBuf, conn: ConnectionOpts) -> Result<()> {

fn write(src: PathBuf, dest: Remote, conn: ConnectionOpts) -> Result<()> {
let mut fileio = dest.connect(conn)?;
let file = fs::File::open(src)?;
let file =
fs::File::open(&src).with_context(|| format!("Could not open {:?} for reading", &src))?;
let size = file.metadata()?.len();
let pb = ProgressBar::new(size);
pb.enable_steady_tick(1000);
Expand Down Expand Up @@ -203,12 +231,17 @@ impl Target {
}

impl FromStr for Target {
type Err = Infallible;
type Err = String;

fn from_str(s: &str) -> std::result::Result<Self, Self::Err> {
match s.find(':') {
Some(idx) => {
let (host, path) = s.split_at(idx);

if path == ":" {
return Err(format!("No remote path given in '{}'", s));
}

Ok(Target::Remote(Remote {
host: host.to_owned(),
path: path[1..].to_owned(),
Expand Down
15 changes: 13 additions & 2 deletions console_backend/src/fileio.rs
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,8 @@ impl Fileio {
return Err(err);
};

let mut total_bytes_read = 0;

loop {
select! {
recv(rx) -> msg => {
Expand All @@ -97,6 +99,7 @@ impl Fileio {
return Err(err.into());
}
let bytes_read = msg.contents.len();
total_bytes_read += bytes_read;
on_progress(bytes_read as u64);
if bytes_read != READ_CHUNK_SIZE {
break;
Expand All @@ -116,13 +119,21 @@ impl Fileio {
},
recv(channel::tick(FILE_IO_TIMEOUT)) -> _ => {
self.link.unregister(key);
bail!("Timed out waiting for file read response. Ensure SBP FILEIO message {} ({}) is enabled.", MsgFileioReadResp::MESSAGE_TYPE, MsgFileioReadResp::MESSAGE_NAME);
bail!("Timed out waiting for file read response. Ensure SBP FILEIO message {} ({}) is enabled.", MsgFileioReadResp::MESSAGE_TYPE, MsgFileioReadResp::MESSAGE_NAME);
}
}
}

self.link.unregister(key);
Ok(())

if total_bytes_read == 0 {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's possible to differentiate between whether the file attempting to be read is actually a 0 byte file or if the file read failed here (because the file didn't exist or because the user isn't allowed to read the file).

Thinking about this some more, maybe the error message should be more descriptive? Something like "File {} read was 0 bytes long. This could indicate that the file doesn't exist or is not accessible."

We should also potentially delete the file locally, as the user will see a 0 bytes file after running this at the moment.

What do others think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the more informative message, IMO leaving the file is fine if we have the warning since the remote file could be a zero byte file.

Err(anyhow!(
"File {} read was 0 bytes long. This could indicate that the file doesn't exist or is not accessible.",
path
))
} else {
Ok(())
}
}

/// Deletes `filename` on the remote device (if it exists) and writes the contents of `data` to the file.
Expand Down