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

HWP ACU checks for spin-up #688

Merged
merged 13 commits into from
Jul 31, 2024
Merged

HWP ACU checks for spin-up #688

merged 13 commits into from
Jul 31, 2024

Conversation

jlashner
Copy link
Collaborator

@jlashner jlashner commented Jun 6, 2024

This PR adds a check of the ACU elevation before allowing for spinup of the HWP.

Description

Adds ACU state information to the HWPState class. This state info is used by the control-state update function to check the ACU elevation and last update time before enabling spin-up.

Motivation and Context

Motivation is described in Kyohei's discussion here: https://github.com/simonsobs/daq-discussions/discussions/96.

How Has This Been Tested?

This has not yet been tested! @ykyohei can we test this on one of the SATs? To test at a safe elevation, we can raise the acu_min_el param.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

@jlashner jlashner requested review from ykyohei and BrianJKoopman June 6, 2024 19:24
@jlashner jlashner requested a review from mhasself June 6, 2024 19:24
@ykyohei
Copy link
Contributor

ykyohei commented Jun 6, 2024

Thank you so much for doing this quickly. Yes, we can test this.

I have a question. Do you plan to implement the constraints between gripper and ACU by implementing it directly in the gripper agent? Or do you plan to add gripper control in the supervisor function?

socs/agents/hwp_supervisor/agent.py Outdated Show resolved Hide resolved
socs/agents/hwp_supervisor/agent.py Outdated Show resolved Hide resolved
socs/agents/hwp_supervisor/agent.py Outdated Show resolved Hide resolved
socs/agents/hwp_supervisor/agent.py Show resolved Hide resolved
Copy link
Contributor

@ykyohei ykyohei left a comment

Choose a reason for hiding this comment

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

el_commanded_position is monitored but not used for check_acu_ok_for_spinup.
I think we should check this too.

@jlashner
Copy link
Collaborator Author

jlashner commented Jun 6, 2024

@ykyohei My intention is to implement grip / ungrip procedures (or at least a wrapper for the gripper agent ops) in the supervisor, and to add the acu checks here. That will be the next PR.

Copy link
Member

@mhasself mhasself left a comment

Choose a reason for hiding this comment

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

Just one more inline comment from me ...

And my outline comment is that you should probably add some discussion of this feature to the docs, or else make the docstrings and argument descriptions clearer. I can understand "Minimum elevation allowed before restricting spin-up" because I already know what this code is for... but I think there are clearer ways to say this. ("Elevation below which HWP spin-up is not permitted", for example)

socs/agents/hwp_supervisor/agent.py Outdated Show resolved Hide resolved
@ykyohei
Copy link
Contributor

ykyohei commented Jun 18, 2024

I tested the acu_check in daq-dev and made small debugs. I believe it's working now.

@jlashner jlashner requested a review from mhasself June 20, 2024 06:50
@jlashner
Copy link
Collaborator Author

Great, thanks Kyohei!

I just implemented Matthew's suggestion for acquiring the last_updated timestamp from ACU session data, and also added a brief section to the docs on ACU safety checks that we can expand as we add the gripper operations and checks.

Copy link
Member

@mhasself mhasself left a comment

Choose a reason for hiding this comment

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

Thanks for the additional clarifications.

Copy link
Member

@BrianJKoopman BrianJKoopman left a comment

Choose a reason for hiding this comment

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

This seems ready to merge, but there are some merge conflicts in the agent code. Can you resolve these @jlashner?

@jlashner
Copy link
Collaborator Author

Looks like tests are failing due to this issue: simonsobs/ocs#391

Do we need to pin setup-tools in socs or upgrade the ocs version?

@BrianJKoopman
Copy link
Member

Looks like tests are failing due to this issue: simonsobs/ocs#391

Do we need to pin setup-tools in socs or upgrade the ocs version?

I was thinking the later, but neither option is working...I might end up switching build systems, was trying to make that decision yesterday.

@BrianJKoopman BrianJKoopman merged commit 202413f into main Jul 31, 2024
6 checks passed
@BrianJKoopman BrianJKoopman deleted the hwp-acu-checks branch July 31, 2024 02:10
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