[CPP-119] Minimal CLI with rough Backend + Frontend coexistence. #53

john-michaelburke · 2021-05-06T01:10:41Z

Random Extra Features

Added a CONNECTED/DISCONNECTED state such that the backend can inform the frontend and correct the connected/disconnected button if a connection failed or if a connection was started via cli.
Moved many of the QObjects out of main.py into their own files. I did leave the navbar as I have another PR which will change this quite a bit. Also thinking maybe navbar/log should remain in the main.py since they are on all tabs (not married to the idea though).
@ silverjam, I have separated the Globals from Constants as you requested but in my other PR which should hopefully be done soon as well.
Changed the Makefile command "run" to point to the python file instead of fbs run so we can pass CLI arguments in. As far as I can tell this is not supported via fbs run.
The CLI parsing in the backend has an odd hack which will attempt to remove "python" from the available args. The CLI parser is expecting only one element to equate to the binary/zeroth argument then everything after are potential flags. It looks to see if the main.py is a command line argument and fails. Probably a better way to do this but it seems to work well and is only really needed for local development.
Added "available_refresh_rates" to the bottomnavbar capnproto message so we can verify the CLI argument.
Added a very small javascript file intended to store util functions called utils.js.

Frontend CLI Options

no-opengl
refresh-rate [1, 5, 10, 25]
tab [TRACKING_SIGNALS, TRACKING_SKYPLOT, SOLUTION_POSITION, SOLUTION_VELOCITY, BASELINE, OBSERVATIONS, SETTINGS , UPDATE, ADVANCED]

Backend CLI Options

exit-after // whether to exit after the connection is dropped or file playback ends.

Subcommands:

tcp <host> --port <port>
file <filepath>
serial <serialport> --baudrate [the available baudrates] --flow-control [the available flow-controls]

The python argparser has had the help disabled. I'm not sure if the usage gets correctly suppressed though so this may need to be reworked. In order for python arguments not to cause issues for the Rust backend CLI args, the python args are also implemented in the Rust CliOptions struct.

Fortunately Qt was able to suggest an approach for setting elements in our Constants/Globals file from PySide2. That is how I am able to set refresh-rate/opengl/initialtab/etc. Probably should try to use this sparingly though.

…aelburke/cli

console_backend/Cargo.toml

console_backend/src/server.rs

pylintrc

src/main/python/solution_position_tab.py

src/main/python/solution_table.py

src/main/python/solution_velocity_tab.py

src/main/python/tracking_signals_tab.py

utils/glob.ds

john-michaelburke · 2021-05-06T02:06:05Z

In order to add command line flags when running cargo make run you must add -- for example: cargo make run -- --no-opengl

jayvdb · 2021-05-07T17:07:08Z

utils/glob.ds

@@ -1,8 +1,10 @@
 fn glob_paths_excluding_target
+    exclude_files = array "src/main/python/console_resources.py"


if you add the generated file to .gitignore, it, and target/*, and everything else here can be replaced by the new .gitignore supported added to cargo-make in sagiegurari/cargo-make#542

Done. Unfortunately this required me to use yet another hack.
The gitignore_path_array is giving me results postfixed with "./" whereas the glob_array does not have this postfix for found entries.

postfixed -> prefixed?

sounds like we might want to create an issue upstream about this after we've go it working nicely, so the maintainer can work out how to make it easier for others to use this new voodoo

console_backend/Cargo.toml

silverjam · 2021-05-07T18:29:09Z

console_backend/src/cli_options.rs

+    #[clap(long = "refresh-rate", validator(is_refresh_rate))]
+    pub refresh_rate: Option<u8>,
+
+    #[clap(long = "tab", validator(is_tab))]


Not sure if it's possible in clap, but in structopt you can directly declare a type on your option struct such as

pub tab: Option<Tabs>

Then implement a FromStr for your type and structopt will automatically pick that up to parse the incoming command line argument appropriately. Perhaps something similar is possible with clap?

This is a great idea. I have hit a pickle though. I hadn't realized FromStr is already impled for Tabs because it is a generated common_constant. The error message for incorrect tab variant does not provide any information about available tabs and the helper does not either. The reason I chose for this be a shared constant is because these Tab variant names are used in python. But this means I can't add my own error message without a validator like this.

[cargo-make] INFO - Running Task: run error: Invalid value for '--tab <tab>': Matching variant not found For more information try --help

What do you think we should do?

It looks like strum has a variant names trait maybe I can throw have py2many impl it's own FromStr instead of using enumstring strum trait then spit out https://docs.rs/strum_macros/0.20.0/strum_macros/derive.EnumVariantNames.html for the exhaustive case?

Maybe we could manually implement FromStr for the enum using the EnumString trait?

Sketch of this idea here: https://github.com/swift-nav/console_pp/pull/54/files

I think at the minimum if we had our py2many fork add the enumvariant trait then we could just manually implement FromStr as you did for CliTabs but throw all the available variants into the error messages.

Submit a PR to https://github.com/jayvdb/py2many/tree/console if you need a custom hack, however that branch is now 100% merged into upstream, so if the changes are generic enough to be useful for others, submit them to https://github.com/adsharma/py2many and they should get reviewed and merged quickly enough.

And/Or fork strum and implement improvements there. Possibly there is a PR or issue already in
https://github.com/Peternator7/strum about what we need.

I created a ticket to track the strum feature looks like it would be more involved:
https://swift-nav.atlassian.net/browse/CPP-148

For the short term I created a py2many fork that adds enumvariant to the derives for enums which allows us to make wrappers with a FromStr error containing variant names.

silverjam · 2021-05-07T18:42:39Z

console_backend/src/cli_options.rs

+
+    // // Frontend Options
+    /// Don't use opengl in plots.
+    #[clap(long = "no-opengl", parse(from_flag = ops::Not::not))]


TL;DR: we can leave this for now, but I think we need to think more about this long term

This is going to look really weird when/if we make a command line version of the app without the GUI, or if we make a version that uses a terminal UI instead. I wonder if there's a way to remove the arguments from the argv we pass to the backend?

If we leave the CLI parsing here, then we'd need features to turn off these CLI options for a "backend only" CLI parser.

Alternately, I think we could invert this problem, it seems a lot less weird for front-end to include back-end options. So the front-end would parse the command line, but also include the back-end options, and then pass those down to the backend. In that case we may not even need CLI parsing on the backend (these options would just be parameters to the server.start method, or an initial "init" message that the front-end would send).

I'm game for either it wouldn't take too long to switch it out. The main reason for this was trying to separate as much CLI from python as possible if we were to eventually switch. As for your first suggestion the problem lies with "unhandled args" if python is only removing the expected args then passes everything else to the backend, then all that will be seen is the backend's helper and no info on potential frontend args. It comes down to who do you want to handle unknown args.

Another option would be to throw python completely out and have Rust handle all potential args, which may be easiest in the end to swap out with a non GUI cli.

I'm not opposed to your second option. Two concerns would be just thinking of the potential number of args (considering the current console) we would have to pass to the backend initially if we were to pass it via server.start. This may not be too bad but worth considering. Second is just what we've harped on before about removing as much as possible from Python for a later switch.

Another suggestion @jayvdb was talking to me about earlier this morning in our Donut was framing the backend to catch connection requests as a list of CLI args. Then when saving history we would just save the full list of arguments to a yaml file. This way we could frame all connection requests to just deconstruct a list of cli args. This would be a pretty thorough redesign but could frame how we think about this problem now.

"..to catch connection requests as a list of CLI args.." : "catch" -> "cache"

silverjam · 2021-05-07T18:48:05Z

console_backend/src/server.rs


    #[text_signature = "($self, /)"]
    pub fn start(&mut self) -> PyResult<ServerEndpoint> {
+        let filtered_args: Vec<String> = std::env::args().filter(|x| x != "python").collect();


Would it make sense for Python to pass down the args we need to parse? Having python as the start of the argv array is far from certain (as far as I know)-- when the app is compiled with Nuitka or something else, this will change.

I think this is related to my other comment we can continue the discussion there. As for this approach it is not looking for python but removing it. The general idea is that all other approaches are assuming there is one binary that is run and it seems the "std::env::args" will throw out the first argv then parse everything else afterward as command line args. I confirmed this works for the binary produced by fbs but not nuitka yet. That being said it should work as long as there is a single binary called to start the app.

silverjam · 2021-05-07T18:49:03Z

console_backend/src/types.rs

            } else {
                println!("Couldn't open file...");
            }
+            set_connected_frontend(


Should this move inside shared_state.set_running?

Done. Good suggestion

silverjam · 2021-05-07T18:50:06Z

console_backend/src/utils.rs

-pub fn from_flowcontrol_str(flow_str: &str) -> FlowControl {
+/// - `Ok`: The associated serialport::FlowControl variant.
+/// - `Err`: Error describing available flow controls available.
+pub fn from_flowcontrol_str(flow_str: &str) -> Result<FlowControl, String> {


I'm still not sure why we don't implement FromStr for these types? There's a standard trait for this type of conversion, why can't we use it?

FlowControl is not something I implemented so I can't impl for it. I can create my own type as a wrapper. I think we discussed this in the original PR this feature was added.

Right, but we can implement a wrapper and then implement Deref and FromStr

Simplified the wrapper a bit here: https://github.com/swift-nav/console_pp/pull/54/files

Done per your PR. Thank you for the assist!

silverjam · 2021-05-07T18:50:50Z

console_backend/src/types.rs

            /*close_when_done = */ true,
        );
-        sleep(Duration::from_millis(100));
+        sleep(Duration::from_millis(150));


silverjam · 2021-05-07T18:51:22Z

console_backend/src/types.rs

-                        client_send,
-                        RealtimeDelay::Off,
-                    );
+            match from_flowcontrol_str(&flow) {


See comment below about implementing FromStr

Done per your PR.

pylintrc

silverjam · 2021-05-07T18:56:24Z

src/main/python/main.py


-    threading.Thread(target=receive_messages, args=(app, backend_main, messages_main,), daemon=True).start()
+    component = QQmlComponent(engine)
+    component.setData(


If this is a hack/work-around from Qt support, can we explain what it does with a comment?

Done. I added a summary of what Qt conveyed to me in the support request.

resources/BottomNavBar.qml

* Respond to review requests. * simpler FlowControl wrapper, wrap generated Tabs object Co-authored-by: John Michael Burke <[email protected]>

jayvdb · 2021-05-07T23:53:17Z

utils/glob.ds

    handle = glob_array ${1}
    out = array
+    gi_handle = array
+    gi_handle_unfiltered = gitignore_path_array ./


I think should ./ shouldnt be here, as it should be based on a cargo-make variable which defines the root.

I had a bug in my code that was not working for absolute paths. Now I use the pwd variable exposed to duckscript.

silverjam

I'm fine to merge though we may want to capture any ideas we have around strum/py2many improvements for future work.

john-michaelburke · 2021-05-08T03:59:57Z

console_backend/src/common_constants.rs

+
 #![allow(non_snake_case)]
 #![allow(non_upper_case_globals)]
-#![allow(trivial_casts)]


@jayvdb Can you add me as a collaborator to your py2many fork? I think we will need to rely on a custom fork for these imports. I can submit my PR to your fork.

Damn I totally missed this comment. Now that it is merged to main do you want to uprev your fork and I can link back to it?

Keeping it pinned to your fork branch is good. Later, we can switch to v0.2.0, or my JS branch, whichever comes arrives.

But my fork does not have your changes which fix the errors in the current py2many branch?

oh, then use upstream master?

Alright per our conversation on Slack. I created a new branch using the uprevved changes and included the missing clippy lints needed for our work.

I also invited you to collab on my py2many fork @jayvdb

poetry.lock

…aelburke/cli

john-michaelburke added 7 commits April 30, 2021 18:08

[CPP-119]Command Line Interface.

462d0dc

App status.

780ffa6

CLI working for installer+run

ed0b487

Moving some qobjects out of main.py

6c65bc4

Merge branch 'main' of github.com:swift-nav/console_pp into john-mich…

34622cd

…aelburke/cli

asdf

3722d4e

Switchable tabs working.

55cbff5