refactor connection logic [CPP-322][CPP-329] #169

notoriaga · 2021-10-18T17:02:09Z

And maybe CPP-328? Also seems to fix an issue (maybe this is happened when running via cargo run not sure) where closing the window would cause the processes running in the terminal to exit with an error. Now it cleanly shuts down

notoriaga · 2021-10-18T17:03:07Z

Makefile.toml

 command = "${PYTHON}"
 args = ["-m", "swiftnav_console.main", "${@}"]

+[tasks.start-console-gdb]


How I've been debugging. Not sure if anyone has a better incantation but seems useful to have a command in the makefile for debugging

You could get python-pretty print for Rust using "rust-gdb".

notoriaga · 2021-10-18T17:03:25Z

Makefile.toml

 command = "cargo"
 cwd = "console_backend"
-args = ["test", "--features", "tests", "--", "--nocapture"]
+args = ["test", "--features", "tests", "${@}", "--", "--nocapture"]


So you can run cargo make test <testname> to filter what tests to run

notoriaga · 2021-10-18T17:06:21Z

console_backend/src/common_constants.rs

-    CLOSE,
-    #[strum(serialize = "CONNECTED")]
-    CONNECTED,
+pub enum ApplicationState {


Rather than two bools, connected/not connected + paused/not paused, this has just one ApplicationState (maybe ConnectionState is a better name?). So what was previously is_running is now matches!(state, CONNECTED | PAUSED).

CLOSING means the app is closing, either because close_when_done was set or a signal came from the frontend

DISCONNECTING is the time between a disconnect triggering and all the threads finishing joining

One of my todos in the other PR is to completely remove the paused logic hopefully that wont cause too much headache with this PR.

notoriaga · 2021-10-18T17:07:38Z

console_backend/src/process_messages.rs

+        .handle_errors(move |e| {
+            debug!("{}", e);
+            match e {
+                sbp::DeserializeError::IoError(_) => ControlFlow::Break,


Just breaking the loop is enough to disconnect the connection now

notoriaga · 2021-10-18T17:10:30Z

console_backend/src/process_messages.rs

            );
        });
+        if conn.settings_enabled() {
+            let tab = SettingsTab::new(


Moved the settings tab out of tabs. This makes it easier to run on its own thread. With this setup the settings tab is now completely separate from the main process messages loop. Previously the tick method was triggered on every new message, now it runs in response to the SettingsTabState changing. Doing it this way also makes it easier to skip building the settings tab for files, which don't need it. Can also skip on the hacky Mutex<Client> I had previously to avoid building the settings client when connected to a file

notoriaga · 2021-10-18T17:24:22Z

Have another branch to make this work with #167

notoriaga · 2021-10-18T17:26:47Z

console_backend/src/server_recv_thread.rs

        loop {
            log::logger().flush();
-            let buf = server_recv.recv();
-            if let Ok(buf) = buf {


Most of this diff is just changing this if let into an early return to reduce the nesting

notoriaga · 2021-10-18T17:34:09Z

console_backend/src/watch.rs

+
+use parking_lot::{Condvar, Mutex};
+
+pub struct Watched<T> {


Based on - https://docs.rs/tokio/1.12.0/tokio/sync/watch/index.html

Like a spmc channel with capacity one. The difference is the channel only retains the last sent element, so sending never blocks. Receivers can get the latest value, or they can wait for a new value to appear. This makes it useful for broadcasting things where you only care about the latest value, like the app state, or the current connection.

john-michaelburke · 2021-10-18T17:15:01Z

Makefile.toml

+  "${PYTHON}",
+  "-m",
+  "swiftnav_console.main",
+  "--",


Wasn't this embedding of "--" found to be causing issues when using cli arguments?

Ah right forgot to update that when I merged in main

john-michaelburke · 2021-10-18T18:45:55Z

Makefile.toml

 command = "${PYTHON}"
 args = ["-m", "swiftnav_console.main", "${@}"]

+[tasks.start-console-gdb]


You could get python-pretty print for Rust using "rust-gdb".

john-michaelburke · 2021-10-18T18:47:51Z

console_backend/src/common_constants.rs

-    CLOSE,
-    #[strum(serialize = "CONNECTED")]
-    CONNECTED,
+pub enum ApplicationState {


One of my todos in the other PR is to completely remove the paused logic hopefully that wont cause too much headache with this PR.

john-michaelburke · 2021-10-19T02:50:57Z

For some reason whenever I start the app the connected button is already enabled. Are you not seeing this behavior?

notoriaga · 2021-10-19T19:15:14Z

For some reason whenever I start the app the connected button is already enabled. Are you not seeing this behavior?

Oh yup fixed. Also I removed the pausing stuff, not sure if you got around to it in your pr but maybe you can avoid doing that?

notoriaga · 2021-10-19T19:29:50Z

console_backend/src/process_messages.rs

+
+/// Wrapper around the iterator of messages that enables other threads
+/// to stop the iterator.
+pub struct Messages {


previously the only way to stop the main loop was to break inside of it. Now it can be stopped elsewhere which means we don't need to wait for a message to come in to disconnect.

So now if the internet drops and you stop getting messages you should still be able to disconnect

notoriaga · 2021-10-19T19:30:28Z

console_backend/src/process_messages.rs

+    fn next(&mut self) -> Option<Self::Item> {
+        crossbeam::select! {
+            recv(self.canceled) -> _ => None,
+            recv(self.messages) -> msg => msg.ok(),


could add a branch for a timeout here, then we could trigger a automatic reconnect or something

Yeah I checked the auto reconnect does not work but does spit out: Releasing settings framework . Disconnecting manually then reconnecting does work which is great.

Is the current version supposed to reconnect? I didn't see any logic for that

See my message below.

resources/NavBar.qml

john-michaelburke · 2021-10-20T03:21:58Z

console_backend/src/connection.rs

-            while shared_state.clone().is_server_running() {
-                if let Ok(conn_option) = receiver.recv_timeout(Duration::from_secs_f64(
-                    SERVER_STATE_CONNECTION_LOOP_TIMEOUT_SEC,
-                )) {
-                    match conn_option {
-                        Some(conn_) => {
-                            conn = Some(conn_);
-                        }
-                        None => {
-                            conn = None;
-                            info!("Disconnected successfully.");
-                        }
-                    }
-                }
-                if let Some(conn_) = conn.clone() {
-                    if let Err(e) =
-                        process_messages(conn_, shared_state.clone(), client_send.clone())
-                    {
-                        error!("unable to process messages, {}", e);
-                    }
-                    if !shared_state.is_running() {
-                        conn = None;
-                    }
-                    shared_state.set_running(false, client_send.clone());
-                }
-                log::logger().flush();


I think the bug we were having with the settings tab was preventing the reconnect logic from working but on tip of tree if you connect to a piksi (not the relay) and disconnect from the internet or lose connection to the piksi you would see the connect button flash on/off and spit out this "unable to process messages" until the connection successfully reconnected. The logic was a bit hacky but relied on whether or not shared_state.is_running was still set to true while looping here. If it was set to false it would assume the app was closing but if it was true it would keep calling the process_messages loop with the same connection where process_messages would perform "try_clone" on the connection. It seemed to do the trick for simple internet drops / piksi restarts but was likely not foolproof.

Ahhh that makes sense. Do you want me to re-implement the reconnect now or as a follow on? I don't think it'd take too long but merging this sooner rather than later is probably better for your stuff

I think we can push it to a future PR. This looks fine now

console_backend/src/watch.rs

notoriaga added 2 commits October 18, 2021 08:21

add make task to run the console with gdb

cb43116

replace mutex with arcbool

52d1f64

notoriaga force-pushed the steve/connection branch 4 times, most recently from f3abd32 to 5036250 Compare October 18, 2021 17:15

notoriaga commented Oct 18, 2021

View reviewed changes

notoriaga force-pushed the steve/connection branch 2 times, most recently from 7e3e064 to 54941ec Compare October 18, 2021 17:23

refactor connection logic

8afedc7

notoriaga force-pushed the steve/connection branch 2 times, most recently from 2cf082a to 61eb01a Compare October 18, 2021 17:36

lint

e9c1e70

notoriaga force-pushed the steve/connection branch from 61eb01a to e9c1e70 Compare October 18, 2021 17:49

notoriaga commented Oct 18, 2021

View reviewed changes

benches

38629a4

notoriaga force-pushed the steve/connection branch 2 times, most recently from 4c9202c to 1d058c1 Compare October 18, 2021 19:59

cache standalone py

5d42257

notoriaga force-pushed the steve/connection branch from 1d058c1 to 5d42257 Compare October 18, 2021 20:03

notoriaga added 4 commits October 18, 2021 13:08

cleanup

4dd5fce

log

7deba8a

don't cache py build

3e73134

fix pause toggle

60b73d6

john-michaelburke requested changes Oct 18, 2021

View reviewed changes

notoriaga added 2 commits October 18, 2021 14:47

fix run gdb

ea499ef

use rust-gdb

89b9e74

notoriaga force-pushed the steve/connection branch from 83bc5ef to adb041f Compare October 19, 2021 01:55

auth download req

56bf91f

notoriaga added 3 commits October 18, 2021 20:45

cleanup

4fbd7c0

remove pause

ff49d7c

Merge remote-tracking branch 'origin' into steve/connection

9311cf2

notoriaga force-pushed the steve/connection branch 3 times, most recently from 4efe3fb to dbe5484 Compare October 19, 2021 19:27

notoriaga commented Oct 19, 2021

View reviewed changes

notoriaga force-pushed the steve/connection branch from dbe5484 to e73caf8 Compare October 19, 2021 19:35

makefile

d8f7508

notoriaga force-pushed the steve/connection branch from e73caf8 to d8f7508 Compare October 19, 2021 19:45

notoriaga added 3 commits October 19, 2021 13:06

bench

c909a35

bench

c56f7b4

bench

57150a8

john-michaelburke reviewed Oct 19, 2021

View reviewed changes

resources/NavBar.qml Outdated Show resolved Hide resolved

notoriaga added 3 commits October 19, 2021 17:09

remove log

43a8071

rename

c8c5830

rename

3785eea

notoriaga force-pushed the steve/connection branch 2 times, most recently from e52d8eb to 9dd98c0 Compare October 20, 2021 02:07

benches?

00e216a

notoriaga force-pushed the steve/connection branch from 9dd98c0 to 00e216a Compare October 20, 2021 02:37

john-michaelburke reviewed Oct 20, 2021

View reviewed changes

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

john-michaelburke approved these changes Oct 20, 2021

View reviewed changes

cleanup

3c08ef0

notoriaga merged commit d689656 into main Oct 20, 2021

notoriaga deleted the steve/connection branch October 20, 2021 04:15

refactor connection logic [CPP-322][CPP-329] #169

refactor connection logic [CPP-322][CPP-329] #169

Uh oh!

Conversation

notoriaga commented Oct 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

notoriaga Oct 18, 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!

notoriaga commented Oct 18, 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!

john-michaelburke commented Oct 19, 2021

Uh oh!

notoriaga commented Oct 19, 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!

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!

2 participants

notoriaga commented Oct 18, 2021 •

edited

Loading

notoriaga Oct 18, 2021 •

edited

Loading