Add settings cli #414

notoriaga · 2022-02-06T23:28:51Z

Differs a bit from the python tool, namely the save command can be combined with commands that write to the device, you can write multiple values at once, and you can read a single setting or a group.

# write multiple values and save to flash
piksi-settings /dev/ttyAMC1 --write imu.acc_range=2g --write imu.imu_rate=100 --save

# read all settings in the imu group
piksi-settings /dev/ttyAMC1 --read imu

notoriaga · 2022-02-07T18:18:57Z

Here is the help for the tool

piksi-settings 0.17.0-8-gbc5ffa5
Piksi settings operations

USAGE:
    piksi-settings [OPTIONS] <DEVICE>

    Examples:
        - Read a setting:
            piksi-settings /dev/ttyUSB0 --read imu.acc_range
        - Read a group of settings:
            piksi-settings /dev/ttyUSB0 --read imu
        - Write a setting value:
            piksi-settings /dev/ttyUSB0 --write imu.acc_range=2g
        - Write multiple settings and save to flash:
            piksi-settings /dev/ttyUSB0 -w imu.acc_range=2g -w imu.imu_rate=100 --save
        - Export a device's settings
            piksi-settings /dev/ttyUSB0 --export ./config.ini
        - Import a device's settings
            piksi-settings /dev/ttyUSB0 --import ./config.ini
    

ARGS:
    <DEVICE>    The serial port or TCP stream

OPTIONS:
    -r, --read <GROUP[.SETTING]>
            Read a setting or a group of settings

    -w, --write <GROUP.SETTING=VALUE>
            Write a setting value

        --export <PATH>
            Export the devices settings

        --import <PATH>
            Import an ini file

        --save
            Save settings to flash. Can be combined with --write or --import to save after writing

        --reset
            Reset settings to factory defaults

        --port <PORT>
            The port to use when connecting via TCP [default: 55555]

        --baudrate <BAUDRATE>
            The baudrate for processing packets when connecting via serial [default: 115200]

        --flow-control <FLOW_CONTROL>
            The flow control spec to use when connecting via serial [default: None]

    -h, --help
            Print help information

    -V, --version
            Print version information

I'm not sure the the read flag is very intuitive, I tried to make it clear via the examples + a custom value name to show that you can read a single setting or a group. I have -r, --read <GROUP[.SETTING]> which is supposed to mean you can supply a group, or a group followed by a period and a setting name. We could split it up into two commands if people think it's not super clear

john-michaelburke · 2022-02-07T18:30:41Z

.github/workflows/main.yml

+        env:
+          LIBCLANG_PATH: ${{ env.LIBCLANG_PATH_WIN }}
+        if: matrix.os.name == 'windows-2019'
+


This is not uploading any artifact just simply building it. So we should add an artifact upload. The fileio also only uploads artifacts to builds. So when we create a release it still only shows the installers. Maybe you could add this and the fileio binaries to the release stage as well?

Once you think it is groovy you can push a test tag to see if it is working

Ahh good catch. I can do that. Is the intent have one big "swift-toolbox" zip that contains everything, or should we have separate downloads for each tool? Or perhaps we can have both?

I think having separate files makes more sense. Not sure if we want to sign these as well though. Was this done for the old app @silverjam ?

Ahh good catch. I can do that. Is the intent have one big "swift-toolbox" zip that contains everything, or should we have separate downloads for each tool? Or perhaps we can have both?

I think having both would be great. Having it in console installer would mean you automatically have them if you have the console and a separate download for just the CLI tools in the release would allow people on bandwidth constrained environments to grab just the command line tools.

I think having separate files makes more sense. Not sure if we want to sign these as well though. Was this done for the old app @silverjam ?

We should probably sign the command line tools for the "CLI only" zip, or sign the zip file itself

john-michaelburke · 2022-02-07T18:55:20Z

console_backend/src/bin/settings.rs

+
+fn connect(opts: &Opts) -> Result<Arc<SettingsTab>> {
+    let (reader, writer) =
+        Connection::discover(opts.device.clone(), opts.conn)?.try_connect(None)?;


Although I think it is cool to reduce the connection cli to a single input I am worried about having differing approaches for the main app and other bins. I think a good example of this, is that we recently added a shortcut with the "host" input that allows the user to input a host-port pair e.g. "192.168.0.200:55555" which is not handled with your current implementation. As well as, if we do intend to add more connection types in the future, like UDP or others, having to maintain two separate CLIs is less than ideal.

Is there any way that we could merge the subcommand structure into this tool? I think one issue I see is the ability to add additional CLI args after a subcommand. Is there a way to have a subcommand ignore unknown arguments such that they are consumed by the parent bin? I think then it would not be too different from what you have here. What are your thoughts?

I am worried about having differing approaches for the main app and other bins. I think a good example of this, is that we recently added a shortcut with the "host" input that allows the user to input a host-port pair e.g. "192.168.0.200:55555" which is not handled with your current implementation.

Ohh I missed this, the fileio tool also follows this approach as a part of trying to make the tool more scp-like.

Is there a way to have a subcommand ignore unknown arguments such that they are consumed by the parent bin?

Not that I'm aware of, but yeah that's the main motivation as to why I tried to avoid subcommands, having to have the flags in a specific order can be a little confusing. Also without subcommands piksi-settings --help gives you all the help, rather than having to run it once for each subcommand.

I agree that the behavior/implementation should be shared across all the tools, both for our sake and for the end user. however I'm kind of in the camp that subcommands are not really the correct choice. To me a subcommand implies a different action. For example if we had one parent binary called swift-toolbox that had commands like swift-toolbox console .../swift-toolbox fileio .../swift-toolbox settings .... Different connection options feels more like a set of mutually exclusive options to me. We could share the behavior with something like

#[derive(Args)] struct ConnOpts { #[clap(long, conflicts_with_all = &["serial", "udp"])] tcp: HostPort, #[clap(long, conflicts_with_all = &["tcp", "serial"])] udp: String, #[clap(long, conflicts_with_all = &["tcp", "udp"])] serial: PathBuf, #[clap(long, conflicts_with_all = &["tcp", "udp"])] baudrate: u32, #[clap(long, conflicts_with_all = &["tcp", "udp"])] flow_control: FlowControl, } struct HostPort { host: String, port: u16, } impl FromStr for HostPort { type Err = ParseIntError; fn from_str(s: &str) -> Result<Self, Self::Err> { if let Some(idx) = s.find(":") { let (host, port) = s.split_at(idx); Ok(HostPort { host: host.to_owned(), port: port[1..].parse()?, }) } else { Ok(HostPort { host: s.to_owned(), port: 55555, }) } } }

Which I think would work well for the console and piksi-settings, but not so much for fileio. I think if we want to have the "scp-like" interface it'll have to remain a special case.

That sounds reasonable to me. Maybe for now we go ahead with what you have then in a future PR we collapse the main console's cli to unify the behavior with this work.

Sounds good!

john-michaelburke · 2022-02-07T20:30:01Z

.github/workflows/main.yml

+          name: ${{ runner.os }}-fileio
+          path: |
+            ./target/release/fileio${{ matrix.os.exe_suffix}}
+        if: matrix.os.name != 'macos-10.15' && github.event_name == 'push' && contains(github.ref, 'refs/tags')


I do like the idea of having these only build when there is a release to keep CI runtime down but this will only upload the artifact to the Actions page, currently. Youll need to pull down the artifacts below in the "Create Release" stage and then add them to the files section for them to show up on a release.

notoriaga · 2022-02-08T00:58:13Z

FYI just canceling the normal CI because I'm tagging a release on each push anyway 🤷

notoriaga · 2022-02-08T19:05:13Z

Finally got one working - https://github.com/swift-nav/swift-toolbox/releases/tag/0.17.0-test3

Some of the other assets aren't there because I removed a bunch of stuff while I was iterating on the workflow

john-michaelburke

LGTM we should do one final test release once you have uncommented everything as a sanity check before merging

notoriaga · 2022-02-08T20:33:05Z

LGTM we should do one final test release once you have uncommented everything as a sanity check before merging

🥳 https://github.com/swift-nav/swift-toolbox/releases/tag/0.17.0-test5

notoriaga · 2022-02-08T20:34:53Z

Hmm not sure why it thinks the CI is still going, looks like both the regular CI and the tag push finished. @john-michaelburke you might need to use your admin privileges to merge without the checks

notoriaga added 3 commits February 6, 2022 15:28

add settings cli

8e7c337

cleanup

40151b8

build settings binary

8167fc6

notoriaga force-pushed the steve/settings-cli2 branch from 8b332c0 to 8167fc6 Compare February 7, 2022 17:43

notoriaga added 2 commits February 7, 2022 09:48

typo

cad0e44

custom usage

5460249

notoriaga requested a review from a team February 7, 2022 18:16

john-michaelburke reviewed Feb 7, 2022

View reviewed changes

add release

1287576

john-michaelburke reviewed Feb 7, 2022

View reviewed changes

add files to release

1571bc0

notoriaga force-pushed the steve/settings-cli2 branch from e753172 to 1571bc0 Compare February 7, 2022 20:50

notoriaga added 2 commits February 7, 2022 12:57

consistent names

8abab16

fix

01ea5ea

notoriaga force-pushed the steve/settings-cli2 branch 2 times, most recently from 33ec2c9 to c18df70 Compare February 8, 2022 00:55

notoriaga force-pushed the steve/settings-cli2 branch from c18df70 to 5011d7b Compare February 8, 2022 02:36

use glob

e0b4262

notoriaga force-pushed the steve/settings-cli2 branch from 5011d7b to e0b4262 Compare February 8, 2022 02:38

fix

b8b0a27

notoriaga force-pushed the steve/settings-cli2 branch 3 times, most recently from 1a4a0f8 to 1f95431 Compare February 8, 2022 07:55

hmm

b9cccd7

notoriaga force-pushed the steve/settings-cli2 branch from 1f95431 to b9cccd7 Compare February 8, 2022 07:56

notoriaga added 2 commits February 8, 2022 09:54

fix

b80da32

fix

45e92c3

add back in full workflow

67e833a

notoriaga force-pushed the steve/settings-cli2 branch from 2219e59 to 67e833a Compare February 8, 2022 18:39

notoriaga requested a review from john-michaelburke February 8, 2022 19:06

john-michaelburke approved these changes Feb 8, 2022

View reviewed changes

notoriaga enabled auto-merge (squash) February 8, 2022 20:35

john-michaelburke disabled auto-merge February 8, 2022 21:28

john-michaelburke merged commit 50032a5 into main Feb 8, 2022

john-michaelburke deleted the steve/settings-cli2 branch February 8, 2022 21:28

notoriaga mentioned this pull request Feb 9, 2022

Rename cli tools/cleanup cli args [CPP-633] [CPP-655] #417

Merged

silverjam mentioned this pull request Mar 22, 2022

fix typo in settings GGL -> GLL [CPP-700] #475

Merged

silverjam mentioned this pull request Apr 20, 2022

ci: bump cargo-make to 0.35.10, pin taplo-cli at 0.6.0 #525

Merged

Add settings cli #414

Add settings cli #414

Uh oh!

Conversation

notoriaga commented Feb 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

notoriaga commented Feb 7, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

notoriaga Feb 7, 2022 • 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!

john-michaelburke Feb 7, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

notoriaga commented Feb 8, 2022

Uh oh!

notoriaga commented Feb 8, 2022

Uh oh!

john-michaelburke left a comment

Choose a reason for hiding this comment

Uh oh!

notoriaga commented Feb 8, 2022

Uh oh!

notoriaga commented Feb 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

notoriaga commented Feb 6, 2022 •

edited

Loading

notoriaga Feb 7, 2022 •

edited

Loading

john-michaelburke Feb 7, 2022 •

edited

Loading

notoriaga commented Feb 8, 2022 •

edited

Loading