riir 🦀🦀🦀 [CPP-405] #15

notoriaga · 2021-11-23T00:22:30Z

Had a really hard time debugging the various settings issues. After spending so much time looking at the c code, I realized it wasn't actually that much to re-implement everything natively

Pros:

No unsafe/c code
All the methods support timeouts/manual cancellation
Somehow less code than the wrapper

Cons:

reinventing the wheel a bit

Overall seems to behave much better, I was able to bump the workers back up to 10 like the python version and haven't seen any problems yet

Using it here - swift-nav/swift-toolbox#249

src/client.rs

john-michaelburke · 2021-11-24T20:34:13Z

src/client.rs

+                if fields.len() < 2 || fields[0] != group || fields[1] != name {
+                    return;
+                }


Can we move this to a function it appears to be used similarly in write_setting_inner. Is this an anticipated mode? If so what settings is this behavior accounting for?

Yeah it is, unfortunately there is no sequence/session associated with messages like fileio. So if two write requests are made at the same time, we register two MsgSettingsWriteResp callbacks. So you might have callback A triggered when the response intended for B comes in. So the check is to make sure that it's the response for the proper message.

But its not 100% foolproof, like if you made two write requests for the same setting, you'd have no way to figure out which one was intended for which, at least as far as I can tell. This is why all the pub functions take &mut self even though we could get away with &self, so we can make sure only one action is performed at a time

And r.e. the refactor, was able to factor out the check, looks a little cleaner

john-michaelburke

This is great work. It is a shame you did not show the change diff in your hackathon presentation seems worthy of recognition.

I wonder if this would be worth setting up with the headless runner for a day or two.

notoriaga · 2021-11-24T21:07:14Z

I wonder if this would be worth setting up with the headless runner for a day or two.

Yeah I think that's a good idea. One thing is afaik the headless running won't actually call any of this api except for the first read_all() call, and then another one each time the connection drops/reconnects. I wonder if it's a good idea to have the runner randomly write a setting, randomly refresh, etc

john-michaelburke · 2021-11-24T21:31:45Z

I wonder if this would be worth setting up with the headless runner for a day or two.

Yeah I think that's a good idea. One thing is afaik the headless running won't actually call any of this api except for the first read_all() call, and then another one each time the connection drops/reconnects. I wonder if it's a good idea to have the runner randomly write a setting, randomly refresh, etc

Yeah that would make sense to me. I could see an integration test suite of sorts that could run various test portfolios like this. Outside of manually validating I think running the headless runner may be sufficient for now until we do implement something like this.

samvrlewis

This is awesome - I learnt some cool Rust techniques.

I wasn't super familiar with this library before this change, so apologies if some of my comments/questions aren't strictly related to this change.

samvrlewis · 2021-11-24T22:32:49Z

src/client.rs

+    fn try_from(msg: MsgSettingsReadResp) -> Result<Self, Self::Error> {
+        let fields = split_multipart(&msg.setting);
+        match fields.as_slice() {
+            [group, name] => {


Is this a valid response? The SBP spec says it should always be group/name/value?

As far as I can tell this is the response when the setting isn't found - https://github.com/swift-nav/libsettings/blob/6469986c2b0d1f3081d09c7cd1ad71d438d14eeb/src/setting_sbp_cb.c#L213

samvrlewis · 2021-11-24T22:37:04Z

src/client.rs

+    fn try_from(msg: MsgSettingsReadByIndexResp) -> Result<Self, Self::Error> {
+        let fields = split_multipart(&msg.setting);
+        match fields.as_slice() {
+            [group, name, value, fmt_type] => {


Just checking - according to the spec fmt_type is optional, but with the way this is implemented I guess this means optional in that it won't necessarily be included in the response but the delimiting null characters will still be there?

Yeah as far as I can tell it's still there, we just get an empty string

samvrlewis · 2021-11-24T22:42:46Z

src/client.rs

+        });
+        let (settings, errors) = (Mutex::new(Vec::new()), Mutex::new(Vec::new()));
+        let idx = AtomicU16::new(0);
+        thread::scope(|scope| {


This is nice! 🦀

samvrlewis · 2021-11-24T22:49:02Z

src/client.rs

-                sbp::to_writer(&mut wtr, &msg).map_err(Into::into)
+        })
+        .unwrap();
+        assert!(now.elapsed().as_millis() >= 100);


Should there be a sleep here or something to make sure that 100ms has actually elapsed?

assert!(now.elapsed().as_millis() >= 100); should be checking that. It's not super obvious but scope(...) will block in place until all the threads spawned inside the closure have joined. So, client.read_setting_ctx(group, name, ctx); should block for 100ms and then scope will end, and the assert is checked

Ahh right, sorry, that makes total sense - I totally misunderstood the intention of this test.

samvrlewis · 2021-11-24T22:55:09Z

src/client.rs

+                let ctx = ctx.clone();
+                scope.spawn(move |_| loop {
+                    let idx = idx.fetch_add(1, Ordering::SeqCst);
+                    match this.read_by_index(idx, done_rx, &ctx) {


Am I understanding correctly here that this means that the workers will likely read past the last index? I guess there's not a nice way around it with this approach given that there doesn't appear to be a way to query for the length of settings up front. I'm assuming that all the settings registry implementations should be able to handle this? 🙈

Yeah this is how the python version works as well, a way to get the length upfront would be very nice indeed

samvrlewis · 2021-11-24T22:56:37Z

libsettings_wrapper.h

@@ -1,52 +0,0 @@
-#ifndef LIBSETTINGS_WRAPPER_H


Think you should also be able to delete the submodule now? 🎉

We are using the settings.yaml in the root of the repo unfortunately. We could ditch the idea of a default settings.yaml, and move it into the swift-toolbox repo, could be nice

samvrlewis · 2021-11-24T23:13:17Z

src/client.rs

-            WriteSettingError::Timeout => write!(f, "request wasn't replied in time"),
-            WriteSettingError::Unknown => {
-                write!(f, "unknown settings write response")
+            [group, name, value] | [group, name, value, ..] => {


Why would there be more than group, name, value out of interest?

Good point I don't that should happen, I'll try out removing it

notoriaga mentioned this pull request Nov 23, 2021

process settings tab messages in a separate thread [CPP-405] swift-nav/swift-toolbox#249

Merged

riir

68d7808

notoriaga force-pushed the steve/riir branch from 483918e to 68d7808 Compare November 23, 2021 00:47

notoriaga added 5 commits November 22, 2021 19:16

cleanup

59d8e46

cleanup

cd97ab6

cleanup

a389ee5

mutable

4a31915

cleanup

3bd2ecb

notoriaga requested review from a team and silverjam November 23, 2021 22:25

notoriaga changed the title ~~[WIP] riir~~ riir 🦀 🦀 🦀 Nov 23, 2021

john-michaelburke reviewed Nov 24, 2021

View reviewed changes

src/client.rs Outdated Show resolved Hide resolved

john-michaelburke reviewed Nov 24, 2021

View reviewed changes

john-michaelburke requested changes Nov 24, 2021

View reviewed changes

notoriaga added 2 commits November 24, 2021 13:08

replace unwraps

3f56ec7

cleanup handlers

aef6c2c

john-michaelburke approved these changes Nov 24, 2021

View reviewed changes

notoriaga added 2 commits November 24, 2021 13:43

add some logs

8823670

reorganize

d92dbf8

notoriaga force-pushed the steve/riir branch from 59a4851 to d92dbf8 Compare November 24, 2021 22:08

samvrlewis reviewed Nov 24, 2021

View reviewed changes

notoriaga added 3 commits November 29, 2021 12:08

fix duration addition

55b90ed

fix read resp parsing

6c7ed8c

fix test?

15df718

notoriaga force-pushed the steve/riir branch from 3cefd3d to 15df718 Compare November 30, 2021 19:35

silverjam changed the title ~~riir 🦀 🦀 🦀~~ riir 🦀 🦀 🦀 [CPP-405] Dec 1, 2021

silverjam changed the title ~~riir 🦀 🦀 🦀 [CPP-405]~~ riir 🦀🦀🦀 [CPP-405] Dec 1, 2021

silverjam approved these changes Dec 1, 2021

View reviewed changes

notoriaga merged commit acf5628 into main Dec 1, 2021

notoriaga deleted the steve/riir branch December 1, 2021 17:15

riir 🦀🦀🦀 [CPP-405] #15

riir 🦀🦀🦀 [CPP-405] #15

Uh oh!

Conversation

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

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!

john-michaelburke left a comment

Choose a reason for hiding this comment

Uh oh!

notoriaga commented Nov 24, 2021

Uh oh!

john-michaelburke commented Nov 24, 2021

Uh oh!

samvrlewis 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

notoriaga commented Nov 23, 2021 •

edited

Loading