Fileio Support [CPP-183] #72

notoriaga · 2021-06-14T19:13:38Z

No description provided.

notoriaga · 2021-06-14T19:15:12Z

console_backend/src/cli_options.rs


+#[derive(Clap, Debug)]
+#[clap(about = "Cli subcommands.")]
+pub enum CliCommand {


Not sure this is the best way to add utility binaries to the console. It would be nice if we could emulate what the current console has with a bunch of different entry points depending on what you are doing

Based on your concern would it make more sense to have a separate bin/crate for fileio and us import the fileio stuctopt and here?

I think a separate bin makes sense. I guess right now there is no rust binary correct? We could add a console_pp/console_backend/src/bin directory and put utils there (specifically utils that don't need to launch the frontend). It wouldn't have to actually be a separate crate, because you can have multiple binaries per crate

john-michaelburke · 2021-06-15T23:59:40Z

@notoriaga To fix your benchmark issue I think you need to add open subcommand here:
https://github.com/swift-nav/console_pp/blob/234174a861eba53851b2a6b72b7e1fcc1fdf3929/utils/bench_runner.py#L115

notoriaga · 2021-06-16T18:25:27Z

@notoriaga To fix your benchmark issue I think you need to add open subcommand here:

https://github.com/swift-nav/console_pp/blob/234174a861eba53851b2a6b72b7e1fcc1fdf3929/utils/bench_runner.py#L115

Hmm I ended up removing the sub command when I broke the io stuff out into it's own tool. Was able to get the benches running locally - it seems like occasionally the window doesn't close when running cargo make frontend-mem-bench which might be the cause of the timeout

john-michaelburke · 2021-06-16T18:58:24Z

@notoriaga To fix your benchmark issue I think you need to add open subcommand here:
https://github.com/swift-nav/console_pp/blob/234174a861eba53851b2a6b72b7e1fcc1fdf3929/utils/bench_runner.py#L115

Hmm I ended up removing the sub command when I broke the io stuff out into it's own tool. Was able to get the benches running locally - it seems like occasionally the window doesn't close when running cargo make frontend-mem-bench which might be the cause of the timeout

Yes from what I'm seeing the file is not opening properly on mac now from cli. I was also able to validate it works running on my linux VM. I also am able to open the file on mac through the GUI. Something is problematic with the cli.

john-michaelburke · 2021-06-16T19:42:57Z

console_backend/src/types.rs

+        shared_state.update_file_history(filename);
+        self.connect(conn, client_send.clone(), shared_state, RealtimeDelay::On);
+        if close_when_done {
+            close_frontend(&mut client_send);


I believe the self.connect is starting up the thread and immediately returning, then this check to see "close_when_done" executes so you probably need to move this logic into the thread.

Success! thanks @john-michaelburke

john-michaelburke · 2021-06-16T22:32:34Z

console_backend/src/types.rs

+    ) -> Result<()> {
+        let conn = Connection::serial(device, baudrate, flow)?;
+        info!("Connected to serialport!");
+        // serial port history?


I don't think saving the serial device history is a great idea because it depends on availability. I think having history of the port used or hardware/software flow could work but we can leave it for now.

Ahh makes sense, removed the comment

Is there a future task in jira for this?
Local devices should have identifiers that can be used to filter out other types of connected devices, so the user can be shown only a list of connected swift-nav devices.

What I meant by depends on availability is that we refresh the device list for serial devices in order to populate which serial devices can be selected. A device previously connected to could not be available for various reasons, maybe a concession would be to at least remember the most recently connected to device and bump it to the top of the list if available. Also I think filtering out non swift devices is a great idea.

I added a couple jiras to log these.
Serial "swift-nav" device filter: https://swift-nav.atlassian.net/browse/CPP-229
Serial/port/flow history: https://swift-nav.atlassian.net/browse/CPP-230

jayvdb · 2021-06-17T03:50:51Z

console_backend/src/fileio.rs

+    }
+
+    pub fn write(&mut self, filename: String, data: impl Read) -> Result<()> {
+        self.remove(filename.clone())?;


should we try to keep the file, i.e. rename it, and only remove it after the write was successful ?

While I think that's a good idea, not sure the best way to handle it. afaik there is no sbp fileio message for renaming files

Oops yeah, didn't realize this was removing the file remotely

I think this should be the current behavior of the console, in piksi_tools the write method has a trunc param which defaults to true and never seems to be overridden anywhere

afaik there is no sbp fileio message for renaming files

Surely there is some atomic operation available on the remote end to support overwriting files without the files being deleted first. If not, one needs to be added to the API.

Perhaps the overwrite occurs atomically only when all chunks have been received? In which case a .remove/trunc before sending the chunks is redundant.

If there is no way to overwrite without first deleting, IMO that non-atomacity should be exposed to the user. i.e. if the file already exists, confirm that the user wish to "first delete the remote file and then upload the new file". And the functions in this module need to clearly described what they are doing, in which case a new destroy_then_write function is needed to do this .remove() followed by the write, as that is in fact performing two distinct steps and the caller needs to know that this function should never be called on a file which is critical for the operation of the device.

@jayvdb

The SBP FileIO API is certainly a bit janky, and we obviously can't change the behavior of old device firmware so improvements here would need to be a new set of SBP messages, or a new mode for the existing messages. Already there's a MSG_FILEIO_CONFIG_REQ message which enables the new "windowed transfer" approach, and if this message isn't ACK'd we need to fall back to the old sequential send/ACK transfer method (otherwise transfers with old devices and firmware versions would fail).

In general, I think there's a number of factors contributing to the oddness of this remove+write sequence:

the "ACK" (write response} packets have a request ID that only serves to signal that a write was received and presumably completed successfully, there's no requirements on the order or value of request IDs

the SBP write request includes an offset, so the writing daemon re-opens the file each time, seeks to the offset, then writes the bytes in the packet (this is what it used to do, the current daemon caches open file descriptors, but it has facilities for knowing when to flush the cache) for reference see: https://github.com/swift-nav/piksi_apps/blob/master/app/sbp_fileio_daemon/sbp_fileio.c#L1191

because of the above, we don't know if a file already exists at the location provided in the write message, since we don't have a "truncate" message, we just issue a remove to make sure that we don't end up corrupting the file upload (if for example the uploaded file ends up being shorter than the existing file)

So TL;DR... I agree this violates the "principle of least surprise" and we should just call this destroy_then_write or remove_then_write (or perhaps just overwrite?) to make it clear what's happening, and perhaps include some explanation on the doc string of the function. I don't think near term (or even long term) it's going to be feasible to make these write message more sane.

jayvdb · 2021-06-17T04:16:58Z

console_backend/src/fileio.rs

+
+impl WriteState {
+    fn new(filename: String) -> Self {
+        let (chunk_size, filename) = if filename.ends_with('\x00') {


where are filenames with trailing nul coming from?

Or if these filenames are typically containing a trailing nul, why are there also cases of the filename not containing them. My guess is that this is allowing the filename to be provided from both local (no nul) and remote (with nul) sources, and this doesn't feel like the best way to handle two different data types.

Is there a significant penalty for removing the nul earlier and then re-adding the nul when building the SbpString?

As this is all internal to this module, I don't mind this being here, but I worry that the filename isnt being cleaned when it is first encountered, and that this may lead to bugs not being noticed in the future (i.e. by someone elses new code) when one datatype of filename is unintentionally used in the wrong context.

My guess is that this is allowing the filename to be provided from both local (no nul) and remote (with nul) sources, and this doesn't feel like the best way to handle two different data types.

Yeah that's sort of what I was going for but in reality I don't this it will matter because, for example, when we list files with this module the trailing nuls are removed. so all inputs should not have the trailing nul even if they come from the remote device.

I'm wondering if SbpString are always null terminated, or if it's just for this usecase. Because if they are would could probably handle adding the nul terminators in the SbpString constructor - maybe @silverjam knows?

Unfortunately libsbp has quite a lot of string encodings, @woodfell has a good write-up of the various encodings we use here: https://swift-nav.atlassian.net/wiki/spaces/~61688916/pages/1722942301/Design+Doc+-+libsbp#Strings

Once libsbp C API improvements are merged we'll have more metadata to work with, for reference see:

https://github.com/swift-nav/libsbp/pull/1000/files#diff-d6e381d88f98862373ae05df3245a916741f89e4135fedadcfbf8fc6583c867bR54

This adds a new encoding: ... field for string members of messages, and we'll be able to use this to make SbpString behave smarter.

In this case we need the null termination to form the write message appropriately, which looks something like <filename>\x00<file-data> on the wire. For this particular case I don't think we need the first if check since all the filenames should be coming from local sources, but we could add an assert and see if it ever fires.

console_backend/src/fileio.rs

…nto steve/fileio-next

notoriaga added 22 commits June 7, 2021 07:37

clean up connection logic

7370036

fileio

43ae614

Merge branch 'main' into steve/fileio

5bbe897

pull in broadcaster

5a02a35

cleanup

1f83c44

fix tests

a1a7caf

remove io

ec00a5b

fmt

b695f4c

lints

a4a5fc0

remove rand

1d7a66d

rebuild

42e6bda

fix broadcaster

42f83d1

lints

394d57e

lintz

01492f7

return result

ed6249b

Add iter methods to receiver

c57e0fa

have sender accept values or refs

da60bf0

Add rand

4059e4a

use slotmap for broadcast

ebc7298

add connection struct

0d27a91

add fileio

b6e84dc

add jank cli

2e13480

notoriaga commented Jun 14, 2021

View reviewed changes

notoriaga requested review from jayvdb, john-michaelburke and silverjam and removed request for john-michaelburke and silverjam June 14, 2021 19:35

notoriaga force-pushed the steve/fileio-next branch from 263d641 to 6bf7bdd Compare June 15, 2021 01:58

cleanup

7d97f41

notoriaga force-pushed the steve/fileio-next branch from 94344a7 to 7d97f41 Compare June 15, 2021 02:35

notoriaga added 5 commits June 14, 2021 19:42

lint

3239268

more tests

234f456

another test

e3a33c8

comments

8dfa5ec

add sleep

234174a

Merge branch 'main' into steve/fileio-next

3f41d68

john-michaelburke reviewed Jun 16, 2021

View reviewed changes

fix close when done

613e0ea

notoriaga force-pushed the steve/fileio-next branch from b46378e to 613e0ea Compare June 16, 2021 20:14

john-michaelburke reviewed Jun 16, 2021

View reviewed changes

john-michaelburke approved these changes Jun 16, 2021

View reviewed changes

remove comment

d609102

jayvdb suggested changes Jun 17, 2021

View reviewed changes

notoriaga force-pushed the steve/fileio-next branch from 27ee004 to 1d2fa6b Compare June 18, 2021 18:00

Move sender id setting/constant to MsgSender

99ef0b6

notoriaga force-pushed the steve/fileio-next branch from 1d2fa6b to 99ef0b6 Compare June 18, 2021 18:06

notoriaga and others added 5 commits June 18, 2021 11:08

Merge branch 'main' into steve/fileio-next

74feb40

build

d908227

Merge remote-tracking branch 'origin/main' into steve/fileio-next

a562726

s/write/overwrite

656169e

Merge branch 'steve/fileio-next' of github.com:swift-nav/console_pp i…

7d50748

…nto steve/fileio-next

john-michaelburke approved these changes Jun 21, 2021

View reviewed changes

notoriaga merged commit eb1cd18 into main Jun 21, 2021

notoriaga deleted the steve/fileio-next branch June 21, 2021 20:03

Fileio Support [CPP-183] #72

Fileio Support [CPP-183] #72

Uh oh!

Conversation

notoriaga commented Jun 14, 2021

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!

john-michaelburke commented Jun 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

notoriaga commented Jun 16, 2021

Uh oh!

john-michaelburke commented Jun 16, 2021

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silverjam Jun 19, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

john-michaelburke commented Jun 15, 2021 •

edited

Loading

silverjam Jun 19, 2021 •

edited

Loading