Skip to content

Conversation

samvrlewis
Copy link
Contributor

Changes the settings tab to only show the settings that the device has
published. This avoids settings being shown that exist within the
libsettings settings.yaml that are old or are for a different mode of
operation.

Tested by making sure I didn't see any INS entries when connecting to a non INS piksi.

Changes the settings tab to only show the settings that the device has
published. This avoids settings being shown that exist within the
libsettings `settings.yaml` that are old or are for a different mode of
operation.
@samvrlewis samvrlewis requested a review from a team January 19, 2022 03:32
@silverjam
Copy link
Contributor

I think this code was just added by @notoriaga -- so we should make sure we aren't undoing a recent fix

@samvrlewis
Copy link
Contributor Author

samvrlewis commented Jan 19, 2022

I think this code was just added by @notoriaga -- so we should make sure we aren't undoing a recent fix

As far as I can tell, this preloading of every setting has been around since the first version of the settings tab: e7c5d73#diff-3781a73d3071fa7b8243bfc06e2ae8ff889747ecb6438ea0b0f91908abbce6b7R356 (though the data structure was a bit different). But maybe there's a special reason for doing so that I'm missing?

Copy link
Contributor

@notoriaga notoriaga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm I don't recall why I did it that way, I think it's probably safe to remove

@samvrlewis
Copy link
Contributor Author

Added some functionality to preserve ordering of groups and settings within a group as discussed here: https://swift-nav.atlassian.net/browse/CPP-563

@samvrlewis samvrlewis requested a review from a team January 19, 2022 22:53
.collect();

// Sort settings within a group by the order in which they're defined within the libsettings settings.yaml file
group.sort_by(|a, b| {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a little puzzled as to how this is working with SETTING_ORDERING being a HashMap instead of IndexMap but testing it out passing the settings-yaml cli argument maintained correct group order.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, SETTING_ORDERING is only holding the position in the YAML file of each setting, so it doesn't matter how its ordered.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see now that makes sense.

@samvrlewis samvrlewis enabled auto-merge (squash) January 19, 2022 23:28
@notoriaga
Copy link
Contributor

notoriaga commented Jan 19, 2022

Might have a two for one here https://swift-nav.atlassian.net/browse/CPP-607

I'm guessing the empty values are from settings not returned by the device?

@samvrlewis samvrlewis merged commit aa9ec04 into main Jan 20, 2022
@samvrlewis samvrlewis deleted the slewis/dont-show-non-published-settings branch January 20, 2022 01:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants