[CPP-154]Fusion Engine Status Bar for Advanced->INS tab. #71

john-michaelburke · 2021-06-10T21:06:05Z

Added Qt object/model for fusion engine status.
Added fusion_engine_status.rs type which is spawned off by the AdvancedIns tab.
Created an IsRunning atomic bool helper for aiding with killing/safely exiting a thread.
A bunch of unittests for the multiple types in fusion_engine_status.rs.

If you want to see a file with a few Ok/Warnings you can pull lfs and then run.

cargo make run -- --tab=ADVANCED_INS file console_backend/tests/data/ins_updates.sbp

console_backend/src/fusion_engine_status.rs

john-michaelburke · 2021-06-10T21:10:49Z

I attempted comparing the output visually with the old console but seem to be unable to get the "realtime playback" feature to work correctly. I have been using this command and it blows through the 5min file in a few seconds:
python -m piksi_tools.console --playback --file -p /path/to/file/ins_updates.sbp

john-michaelburke · 2021-06-10T21:14:02Z

console_backend/src/fusion_engine_status.rs

+const SET_STATUS_THREAD_SLEEP_SEC: f64 = 0.05;
+
+// No updates have been attempted in the past `STATUS_PERIOD`
+const UNKNOWN: &str = "\u{2B1B}"; // Unicode Character “⬛” (U+2B1B)


I know @ notoriaga you probably used these unicode chars because of limitations with the traits api but we could switch to svgs if it is preferred. I would probably rewrite the logic a bit to use a shared enum (maybe save this for a future PR)

Another limitation if not already apparent is the OK circle seems to be hard anchored to the top right corner of the text space so fast switching between status' shows a noticeable shift.

Hmm yea the unicode characters were because it was the only thing we could get working, so if svgs behave better i'd say go for it

We'll need to do something different here, on Windows this currently doesn't render very well:

brutal that may be cause I did not set the font size however that would not explain why the color does not show up 😕

I have switched to using svgs and the color's look to be working on Linux + Windows.

john-michaelburke · 2021-06-10T21:19:41Z

console_backend/src/fusion_engine_status.rs

+        is_running.set(true);
+        spawn(move || {
+            let mut expired = false;
+            while is_running.get() {


The main reason I went with implementing my own timer thread was that I couldn't find a decent crate which provided the ability to cancel a timer (preventing any remaining functionality from executing). Not that this approach does that per se but the logic in the "restart_timer" would prevent a thread that continues executing but did not expire to not set the value as it would had it expired assuming "is_running" gets set false.

Explanation gets confusing towards the end, can you rephrase?

Instead of rephrasing I will just ask to re-review the timer code as I have now switched toward using crossbeam's after feature. I think it is definitely less complex but seems slightly more reliable. I couldn't best figure out how to keep track of whether or not the timer was running so I have a manually set atomic bool keep track of this now.

john-michaelburke · 2021-06-11T23:37:43Z

console_backend/src/fusion_engine_status.rs

+};
+use crate::types::{IsRunning, MessageSender, SharedState};
+
+const STATUS_PERIOD: f64 = 1.0;


I wondering if it might be worth increasing this status_period here to say 1.1. I think due to the nature of not using callbacks/async, or simply just that this code is not very efficient, the time it takes to cancel/reset the unknown timer when a new message comes in takes too long and will display an unknown for a split second in the case of a pattern such as:

MsgInsUpdates { sender_id: Some(789), tow: 413750200, gnsspos: 16, gnssvel: 0, wheelticks: 0, speed: 0, nhc: 0, zerovel: 0 } MsgInsUpdates { sender_id: Some(789), tow: 413750300, gnsspos: 0, gnssvel: 0, wheelticks: 0, speed: 0, nhc: 0, zerovel: 0 } MsgInsUpdates { sender_id: Some(789), tow: 413750400, gnsspos: 0, gnssvel: 0, wheelticks: 0, speed: 0, nhc: 16, zerovel: 17 } MsgInsUpdates { sender_id: Some(789), tow: 413750500, gnsspos: 0, gnssvel: 0, wheelticks: 0, speed: 0, nhc: 0, zerovel: 0 } MsgInsUpdates { sender_id: Some(789), tow: 413750600, gnsspos: 0, gnssvel: 16, wheelticks: 0, speed: 0, nhc: 0, zerovel: 0 } MsgInsUpdates { sender_id: Some(789), tow: 413750700, gnsspos: 0, gnssvel: 16, wheelticks: 0, speed: 0, nhc: 0, zerovel: 0 } MsgInsUpdates { sender_id: Some(789), tow: 413750800, gnsspos: 0, gnssvel: 16, wheelticks: 0, speed: 0, nhc: 0, zerovel: 0 } MsgInsUpdates { sender_id: Some(789), tow: 413750900, gnsspos: 0, gnssvel: 16, wheelticks: 0, speed: 0, nhc: 0, zerovel: 0 } MsgInsUpdates { sender_id: Some(789), tow: 413751000, gnsspos: 0, gnssvel: 16, wheelticks: 0, speed: 0, nhc: 0, zerovel: 0 } MsgInsUpdates { sender_id: Some(789), tow: 413751100, gnsspos: 0, gnssvel: 16, wheelticks: 0, speed: 0, nhc: 0, zerovel: 0 } MsgInsUpdates { sender_id: Some(789), tow: 413751200, gnsspos: 16, gnssvel: 17, wheelticks: 0, speed: 0, nhc: 16, zerovel: 0 }

where the gnsspos is displaying Ok every 1 full second. This can be seen playing the example file in the PR description.

This sounds reasonable

I think the rewrite in code has reduced this problem somewhat. We could still bump it up and it could reduce the issue even more but for now I think it looks ok. Still think the issue is just that small amount of compute time spent getting the OK message registered before the Unknown timer procs and sets it to unknown.

silverjam

Some initial comments, will review more this afternoon

silverjam · 2021-06-14T18:39:35Z

console_backend/src/errors.rs

@@ -1,6 +1,14 @@
 pub const HEARTBEAT_LOCK_MUTEX_FAILURE: &str = "unable to lock heartbeat mutex";
 pub const SHARED_STATE_LOCK_MUTEX_FAILURE: &str = "unable to lock shared_state mutex";
+pub const IS_RUNNING_LOCK_MUTEX_FAILURE: &str = "unable to lock is_running mutex";


Not used anymore? All of these constants should probably be pub(crate) since we're not a library-- changing to pub(crate) will help identify unused constants that we can delete.

Done. There is one I had to add an allow dead_code because it is used by the server.rs in one of the "disabled features" functions. This may be resolved in @notoriaga PR in which we could remove this allow then.

silverjam · 2021-06-14T18:51:05Z

console_backend/src/fusion_engine_status.rs

+};
+use crate::types::{IsRunning, MessageSender, SharedState};
+
+const STATUS_PERIOD: f64 = 1.0;


This sounds reasonable

silverjam · 2021-06-14T18:54:02Z

console_backend/src/fusion_engine_status.rs

+    ) -> JoinHandle<()> {
+        let start_time = Instant::now();
+        let mut storage = storage;
+        is_running.set(true);


Is it possible to set this inside the thread itself?

silverjam · 2021-06-14T18:54:49Z

console_backend/src/fusion_engine_status.rs

+        delay: f64,
+    ) -> JoinHandle<()> {
+        let start_time = Instant::now();
+        let mut storage = storage;


Should be able to just mark the storage parameter itself as mut

silverjam · 2021-06-14T18:57:09Z

console_backend/src/fusion_engine_status.rs

+                    is_running.set(false);
+                    break;
+                }
+                sleep(Duration::from_millis(5));


The longer we can sleep here the better-- an alternate might be to wait for an event signal with a timeout, this would allow us to immediately wake up the thread if we wanted to cancel, but also sleep for a longer amount of time to lower the CPU burden. You can also randomize the sleep a bit to try to spread out the processor load.

See https://doc.rust-lang.org/std/sync/struct.Condvar.html#method.wait_timeout

Switching to channels I think it accomplishes this and now the timeout is around 250ms so not too bad.

silverjam · 2021-06-14T18:58:47Z

console_backend/src/fusion_engine_status.rs

+    }
+
+    fn set_status_thread(&mut self) {
+        self.is_running.set(true);


Would be preferable to set this inside the thread if possible

silverjam · 2021-06-14T18:59:31Z

console_backend/src/fusion_engine_status.rs

+    }
+
+    fn stop(&mut self) {
+        self.is_running.set(false);


Should this wait to join the thread so we can be sure the thread stopped?

So, one obvious problem with waiting for the thread to exit: we'd have to wait for the sleep timer to expire, if we used a condvar as I suggested above we can immediately make the thread exit, then here waiting for the thread to exit shouldn't be an issue. The logic would something like this:

In the thread:

Do stuff

Wait on condvar for current sleep interval

If condvar fires, exit the thread, else loop again

In the stop call:

Signal the condvar

Wait for the thread to exit (up to a limit, say 1 second)

Exit successfully or log and error if the thread doesn't exit (panicking might be appropriate too)

I have set up the main loop to wait for a received Option if it gets None passed in that triggers the exit condition. I believe there is not much wait time now. I added a join to the stop and cancel commands as well.

silverjam · 2021-06-14T19:03:40Z

console_backend/src/fusion_engine_status.rs

+        is_running.set(true);
+        spawn(move || {
+            let mut expired = false;
+            while is_running.get() {


Explanation gets confusing towards the end, can you rephrase?

silverjam · 2021-06-14T21:29:02Z

console_backend/src/fusion_engine_status.rs

+const OK: &str = "\u{26AB}"; // Unicode Character “⚫” (U+26AB)
+
+#[derive(Debug)]
+pub struct UpdateStatus(Arc<Mutex<Option<UpdateStatusInner>>>);


Suggested change

pub struct UpdateStatus(Arc<Mutex<Option<UpdateStatusInner>>>);

pub struct FusionStatus(Arc<Mutex<Option<FusionStatusInner>>>);

silverjam · 2021-06-14T21:32:23Z

console_backend/src/fusion_engine_status.rs

+}
+
+#[derive(Debug)]
+struct FlagStatus {


Suggested change

struct FlagStatus {

struct FusionStatusFlag

silverjam · 2021-06-14T21:37:47Z

console_backend/src/fusion_engine_status.rs

+    }
+
+    fn stop(&mut self) {
+        self.is_running.set(false);


So, one obvious problem with waiting for the thread to exit: we'd have to wait for the sleep timer to expire, if we used a condvar as I suggested above we can immediately make the thread exit, then here waiting for the thread to exit shouldn't be an issue. The logic would something like this:

In the thread:

Do stuff

Wait on condvar for current sleep interval

If condvar fires, exit the thread, else loop again

In the stop call:

Signal the condvar

Wait for the thread to exit (up to a limit, say 1 second)

Exit successfully or log and error if the thread doesn't exit (panicking might be appropriate too)

silverjam · 2021-06-14T21:38:25Z

console_backend/src/fusion_engine_status.rs

+/// - `nhc`: Storage for the non-holonomic constraints model status.
+/// - `zerovel`: Storage for the zero velocity status.
+#[derive(Debug)]
+pub struct FusionEngineStatus<S: MessageSender> {


Suggested change

pub struct FusionEngineStatus<S: MessageSender> {

pub struct FusionStatusFlags<S: MessageSender> {

silverjam · 2021-06-14T21:39:49Z

console_backend/src/fusion_engine_status.rs

+        let mut msg_bytes: Vec<u8> = vec![];
+        serialize::write_message(&mut msg_bytes, &builder)
+            .expect(CAP_N_PROTO_SERIALIZATION_FAILURE);


This really feels like we could have a helper for this:

Allocate a vector

Write/serialize a message (and panic/expect)

(Potentially also) send a message to the supplied client sender

silverjam · 2021-06-14T21:41:11Z

console_backend/src/fusion_engine_status.rs

+
+impl<S: MessageSender> Drop for FusionEngineStatus<S> {
+    fn drop(&mut self) {
+        self.gnsspos.stop();


This should be implemented on each object rather than here

silverjam · 2021-06-14T21:49:55Z

console_backend/src/types.rs

 }

+#[derive(Debug, Default)]
+pub struct IsRunning(Arc<AtomicBool>);


Suggested change

pub struct IsRunning(Arc<AtomicBool>);

pub struct ArcBool(Arc<AtomicBool>);

Or maybe:

Suggested change

pub struct IsRunning(Arc<AtomicBool>);

pub struct SingletonBool(Arc<AtomicBool>);

silverjam

john-michaelburke added 2 commits June 9, 2021 19:15

Fusion Engine Status WIP.

35e0878

Fusion Engine Status.

49d6294

john-michaelburke commented Jun 10, 2021

View reviewed changes

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

john-michaelburke commented Jun 10, 2021

View reviewed changes

john-michaelburke force-pushed the john-michaelburke/fusion_engine_status branch 3 times, most recently from f6dd5a7 to ab1d91b Compare June 11, 2021 23:05

john-michaelburke commented Jun 11, 2021

View reviewed changes

john-michaelburke marked this pull request as ready for review June 11, 2021 23:37

john-michaelburke requested review from jayvdb, notoriaga and silverjam June 11, 2021 23:37

Switching to atomicbool.

a8bb03e

john-michaelburke force-pushed the john-michaelburke/fusion_engine_status branch from ab1d91b to a8bb03e Compare June 12, 2021 00:02

silverjam reviewed Jun 14, 2021

View reviewed changes

john-michaelburke added 2 commits June 14, 2021 19:57

working on it.

5fa49e4

Switch to channels.

440b17d

john-michaelburke force-pushed the john-michaelburke/fusion_engine_status branch from d01c4d3 to 440b17d Compare June 15, 2021 17:41

Switch to svg icons.

be03930

john-michaelburke force-pushed the john-michaelburke/fusion_engine_status branch from cb024c2 to 30c29c2 Compare June 15, 2021 23:46

silverjam approved these changes Jun 15, 2021

View reviewed changes

Respond to review requests.

7152d2a

john-michaelburke force-pushed the john-michaelburke/fusion_engine_status branch from 30c29c2 to 7152d2a Compare June 15, 2021 23:50

john-michaelburke merged commit 7be3881 into main Jun 16, 2021

john-michaelburke deleted the john-michaelburke/fusion_engine_status branch June 16, 2021 01:12

	pub struct UpdateStatus(Arc<Mutex<Option<UpdateStatusInner>>>);
	pub struct FusionStatus(Arc<Mutex<Option<FusionStatusInner>>>);

	pub struct FusionEngineStatus<S: MessageSender> {
	pub struct FusionStatusFlags<S: MessageSender> {

	pub struct IsRunning(Arc<AtomicBool>);
	pub struct ArcBool(Arc<AtomicBool>);

	pub struct IsRunning(Arc<AtomicBool>);
	pub struct SingletonBool(Arc<AtomicBool>);

[CPP-154]Fusion Engine Status Bar for Advanced->INS tab. #71

[CPP-154]Fusion Engine Status Bar for Advanced->INS tab. #71

Uh oh!

Conversation

john-michaelburke commented Jun 10, 2021

Uh oh!

Uh oh!

john-michaelburke commented Jun 10, 2021

Uh oh!

john-michaelburke Jun 10, 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!

john-michaelburke Jun 10, 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!

john-michaelburke Jun 11, 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!

silverjam left a comment

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!

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 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

john-michaelburke Jun 10, 2021 •

edited

Loading

john-michaelburke Jun 10, 2021 •

edited

Loading

john-michaelburke Jun 11, 2021 •

edited

Loading

silverjam Jun 14, 2021 •

edited

Loading