Update Tab + Update Downloader[CPP-46][CPP-47][CPP-296][CPP-303] #110

john-michaelburke · 2021-09-04T00:34:46Z

Implements

Update Tab (Limited Backend, for the most part 100% frontend logic)
Update Downloader

Update Tab Backend Functionality:

Downloads newest firmware and checks for latest version
Compares version current vs latest to decide whether or not to allow upgrading. Currently hardcode current version until Settings available.
Commits firmware file to flash memory and resets piksi. Update tab maintains state when the piksi comes back online.

utils/bench_runner.py

console_backend/src/update_tab.rs

john-michaelburke · 2021-09-09T03:02:28Z

Alright @silverjam I think this is ready to go now. Seeing as how I'll be adding more to this tab this sprint, I will push off unittests until I have a few more features in the bag. Probably won't be able to test much unless we mock a device or set up one on a self hosted runner.

https://swift-nav.atlassian.net/browse/CPP-312

Cargo.lock

console_backend/src/update_downloader.rs

console_backend/src/update_tab.rs

silverjam · 2021-09-09T04:06:22Z

console_backend/src/update_tab.rs

+                            .collect();
+                        let text = text.split('\n').collect::<Vec<&str>>();
+                        let final_text = if text.len() > 1 {
+                            // upgrade tool deliminates lines in stoud with \r, we want penultimate line that is complete to show


The intent for sending '\r' is for the line being sent to erase the previous line, I believe the old console emulated this terminal like behavior, but not sure. We don't need to implement that behavior now, but might be something we should do later.

EDIT: looks like we might already be emulating that behavior with FirmwareUpgradePaneLogger?

Yeah I think the only spot this '\r' logic is used in the old console is in this exact spot to discern which line is the log we are interested in. The old console also has separate functions for either simply appending to the log pane or replacing the previous log before appending. So aside from the broken log messages from the piksi I believe this is working as intended.

silverjam · 2021-09-09T04:16:28Z

console_backend/src/update_tab.rs

+                                            if update.download_latest_firmware && !update_shared.downloading() {
+                                                if let Some(update_downloader_thread) = update_downloader_thread.take()
+                                                {
+                                                    update_downloader_thread.join().expect(THREAD_JOIN_FAILURE);


What is the intent of this code? If we aren't downloading, then the download thread should be gone, correct? Why do we need to join on the thread before we can replace the thread handle? I would think we don't really need the thread handle at all.

Removed. You are correct. I think this was just out of habit for joining all threads.

silverjam · 2021-09-09T04:18:15Z

console_backend/src/update_tab.rs

+        };
+    update_shared.set_firmware_local_filepath(filepath);
+    update_shared.set_downloading(false);
+    log::logger().flush();


Why are we flushing the log?

Removed. We have had odd behavior with the logger not flushing in separate threads at least in our server.rs file. I thought it was related to separate threads but it may be related to trying to flush in a pyo3 module instead. I tested it and the logs look fine to me without the flush.

silverjam · 2021-09-09T04:22:55Z

console_backend/src/utils.rs

+/// Returns:
+/// - `true` if the later_version is greater than the early version.
+/// - `false` if the later_version is not greater than the early version.
+pub fn compare_semvers(early_version: String, later_version: String) -> anyhow::Result<bool> {


Probably fine to keep this, but FYI there's a semver crate: https://docs.rs/semver/1.0.4/semver/struct.Version.html -- I doubt it'd directly handle our "dev" versions though.

I did swap it out. It does appear to handle dev versions but quite methodically i.e. 1.2.3-dev123 < 1.2.3-dev543. I think this should be reasonable though.

Co-authored-by: Jason Mobarak <[email protected]>

silverjam · 2021-09-09T19:33:01Z

console_backend/src/process_messages.rs

-            if !shared_state.is_running() {
-                if let Err(e) = tabs.main.lock().unwrap().end_csv_logging() {
-                    error!("Issue closing csv file, {}", e);
+        let update_shared = tabs


Can we rename this to update_tab_context (or update_tab_ctx) too?

silverjam · 2021-09-09T19:34:01Z

console_backend/src/process_messages.rs

+            .update
+            .lock()
+            .expect(UNABLE_TO_CLONE_UPDATE_SHARED)
+            .update_tab_context_clone();


I'd like to follow the convention with <verb>_<thing> naming for our methods, so clone_update_tab_context instead of update_tab_context_clone

Done. I will take note and follow this for the future. Can also do a pass through in the future to clean up some of these naming schemes we have at least make things consistent.

Sounds good

console_backend/src/types.rs

Co-authored-by: Jason Mobarak <[email protected]>

silverjam · 2021-09-09T22:13:25Z

console_backend/src/process_messages.rs

                    }
-                    sleep(Duration::from_millis(PAUSE_LOOP_SLEEP_DURATION_MS));
                }
+                log::logger().flush();


Can we remove this flush too?

No it looks like this flush is needed or we only get updates every 5-10seconds

john-michaelburke added 3 commits September 2, 2021 17:12

WIP Update Downloader.

9315c6a

Update Tab Frontend.

40cb35f

Working on backend.

c9818ee

john-michaelburke force-pushed the john-michaelburke/update_tab2 branch 5 times, most recently from a6722f4 to ebd3a89 Compare September 7, 2021 21:45

john-michaelburke commented Sep 7, 2021

View reviewed changes

utils/bench_runner.py Outdated Show resolved Hide resolved

john-michaelburke commented Sep 7, 2021

View reviewed changes

console_backend/src/update_tab.rs Outdated Show resolved Hide resolved

john-michaelburke force-pushed the john-michaelburke/update_tab2 branch from ebd3a89 to bf72ffe Compare September 7, 2021 22:46

Made some progress on fileio but everything is broken.

b704a4b

john-michaelburke force-pushed the john-michaelburke/update_tab2 branch from bf72ffe to b704a4b Compare September 7, 2021 23:05

john-michaelburke requested a review from a team September 8, 2021 00:09

john-michaelburke changed the title ~~Update Tab + Update Downloader[CPP-46][CPP-47][CPP-296]~~ Update Tab + Update Downloader[CPP-46][CPP-47][CPP-296][CPP-303] Sep 8, 2021

john-michaelburke force-pushed the john-michaelburke/update_tab2 branch 3 times, most recently from 78f88a1 to b66ae41 Compare September 9, 2021 02:53

Switch to message passing model for update tab.

f9905bc

john-michaelburke force-pushed the john-michaelburke/update_tab2 branch from b66ae41 to f9905bc Compare September 9, 2021 02:54

john-michaelburke requested a review from silverjam September 9, 2021 03:02

silverjam reviewed Sep 9, 2021

View reviewed changes

john-michaelburke and others added 4 commits September 9, 2021 10:25

Update console_backend/src/update_tab.rs

4a94935

Co-authored-by: Jason Mobarak <[email protected]>

Update console_backend/src/update_tab.rs

3cafee6

Co-authored-by: Jason Mobarak <[email protected]>

Update console_backend/src/update_tab.rs

2d674aa

Co-authored-by: Jason Mobarak <[email protected]>

Update tab fixups, renaming structs, ignoring expensive tests.

0eced0a

john-michaelburke requested a review from silverjam September 9, 2021 18:19

silverjam reviewed Sep 9, 2021

View reviewed changes

Update console_backend/src/types.rs

e6bb883

Co-authored-by: Jason Mobarak <[email protected]>

Update tab renaming some variables.

e0cab09

silverjam reviewed Sep 9, 2021

View reviewed changes

silverjam approved these changes Sep 9, 2021

View reviewed changes

john-michaelburke merged commit e7a9776 into main Sep 9, 2021

john-michaelburke deleted the john-michaelburke/update_tab2 branch September 9, 2021 22:45

Update Tab + Update Downloader[CPP-46][CPP-47][CPP-296][CPP-303] #110

Update Tab + Update Downloader[CPP-46][CPP-47][CPP-296][CPP-303] #110

Uh oh!

Conversation

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

Uh oh!

Implements

Update Tab Backend Functionality:

Uh oh!

Uh oh!

Uh oh!

john-michaelburke commented Sep 9, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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 Sep 9, 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

john-michaelburke commented Sep 4, 2021 •

edited

Loading

silverjam Sep 9, 2021 •

edited

Loading