Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add validator for sun position monitor #292

Merged
merged 1 commit into from
Aug 19, 2023
Merged

Conversation

taldcroft
Copy link
Member

@taldcroft taldcroft commented Jul 31, 2023

Description

This adds a state validator for the sun_pos_mon state.

It also fixes a bug where telemetry values with a trailing space character (e.g. "ACT ") do not match the state codes.

Interface impacts

Adds a new plot in the state validation page.

Testing

Unit tests

  • Mac

Independent check of unit tests by Jean

  • Linux

Functional tests

From within the git repo:

python -m kadi.scripts.validate_states

This generated the following output: https://icxc.cfa.harvard.edu/aspect/test_review_outputs/kadi-pr292/

state_name = "sun_pos_mon"
msids = ["aopssupm"]
plot_attrs = PlotAttrs(title="Sun position monitor", ylabel="Sun position monitor")
min_violation_duration = 300
Copy link
Member Author

Choose a reason for hiding this comment

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

This 300 second value is somewhat empirical from testing. It seems there is variation between the actual eclipse exit (determined by the OBC based on batteries etc) and the predicted which is in commanding. With a 300 second value most of the cases were within the limit.

@jeanconn jeanconn self-requested a review July 31, 2023 19:15
@jeanconn jeanconn requested a review from javierggt July 31, 2023 19:16
@jeanconn
Copy link
Contributor

jeanconn commented Aug 2, 2023

This looks good to me. I note that in review of the report page I see issues with plotting negatives in the Y axis on some of the other plots which I can open as a separate issue.

@taldcroft
Copy link
Member Author

Thanks. If you are good with this then please approve it.

@jeanconn
Copy link
Contributor

jeanconn commented Aug 2, 2023

Still running the unit tests.

@jeanconn
Copy link
Contributor

I think this is ready-to-go.

@taldcroft taldcroft merged commit 761efd0 into master Aug 19, 2023
@taldcroft taldcroft deleted the add-spm-vadliation branch August 19, 2023 10:02
@javierggt javierggt mentioned this pull request Sep 6, 2023
@javierggt javierggt mentioned this pull request Sep 18, 2023
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.

3 participants