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

Use Telescope Bus 5 and Bus 6 state to calculate relevant derived thermal parameters #218

Merged
merged 4 commits into from
Mar 15, 2021

Conversation

matthewdahmer
Copy link
Contributor

@matthewdahmer matthewdahmer commented Mar 12, 2021

Description

The heater zones within the Telescope Bus 5 and Bus 6 cannot be commanded if either of these busses are disabled, such as when the Telescope Bus 6 is disabled to turn off the stuck on Zone 50 heater. From what I can tell, the bilevel heater telemetry for these zones does not depend on bus state, but rather commanding from the OBC, so if a temperature were to drop below the heater set point within a zone in Bus 5 or Bus 6, the relevant bilevel MSID would likely falsely indicate the heater is ON.

This update uses the Telescope Bus 5 and Bus 6 state to correct any false Telescope Bus 5 or Bus 6 bilevel telemetry, including within Zone 50.

Testing

  • Passes unit tests on linux (HEAD)
  • Functional testing

Functional testing

Local testing of functions (@matthewdahmer )

I completed some testing by redefining the DerivedParameterThermal and affected classes locally (in a notebook), and was able to confirm that the code functions as expected in this context, resulting in a drop in zone 50 power to zero when the Telescope Bus 6 is disabled.

Testing within update processing code on HEAD (@taldcroft)

After checking out this branch on HEAD:

cd ~/git/eng_archive
git fetch origin
git checkout zone_50_disable
mkdir data
python -m cheta.add_derived --start=2016:100 --stop=2016:105 --data-root=$PWD --content=dp_thermal
python -m cheta.update_archive --date-start=2016:100:00:00:00 --date-now=2016:100:04:00:00 \
    --data-root=$PWD --content=dp_thermal

This ran to completion with no errors, indicating that the derived parameter code updates are running. Functional testing that the values are correct was done with the local testing.

Copy link
Member

@taldcroft taldcroft left a comment

Choose a reason for hiding this comment

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

Thanks, looks good! I just have a suggestion that might simplify the logic if I have gotten it right.

Ska/engarchive/derived/thermal.py Outdated Show resolved Hide resolved
@taldcroft
Copy link
Member

taldcroft commented Mar 13, 2021

This will need at least some testing similar to standalone testing at the function level that you've done before. There should be an obvious change in the P50 (zone 50) value during the test intervals.

matthewdahmer and others added 2 commits March 13, 2021 10:48
Modify the `data[msid].vals` array in place rather than redefining it, to account for bus 5 and bus 6 state.

Co-authored-by: Tom Aldcroft <taldcroft@gmail.com>
@matthewdahmer
Copy link
Contributor Author

I completed some testing by redefining the DerivedParameterThermal and DP_P50 classes locally (in a notebook), and was able to confirm that the code functions as expected in this context, resulting in a drop in zone 50 power to zero when the Telescope Bus 6 is disabled.

@matthewdahmer
Copy link
Contributor Author

There's another update I may tack on to this PR. The DP_HADG calculation in cheta is incorrect does not match what is in G_TREND24 and MAUDE, so I am generating an update to fix this.

@matthewdahmer
Copy link
Contributor Author

@taldcroft The HADG fix has been posted, and passes my local test.

Copy link
Member

@taldcroft taldcroft left a comment

Choose a reason for hiding this comment

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

Looks good now and testing is completed.

@taldcroft taldcroft merged commit 1880698 into master Mar 15, 2021
@taldcroft taldcroft deleted the zone_50_disable branch March 15, 2021 16:09
@matthewdahmer
Copy link
Contributor Author

Thank You!

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.

2 participants