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

hw_pid: delete initial set_direction #703

Merged
merged 3 commits into from
Aug 12, 2024
Merged

hw_pid: delete initial set_direction #703

merged 3 commits into from
Aug 12, 2024

Conversation

ykyohei
Copy link
Contributor

@ykyohei ykyohei commented Jul 29, 2024

Delete initial hwp rotation direction setting of hwp_pid agent to resolve the hwp rotation direction ambiguities.

Description

The "direction" in the hwp_pid memory register doesn't reflect the actual action of pid agent until hwp_pid.tune_freq is called. The default setting of "direction" on the agent startup lead to the ambiguity of hwp rotation direction. This PR delete the default setting.

As long as we use hwp_supervisor.pid_to_freq, we can avoid this issue.

if isinstance(state, ControlState.PIDToFreq):
self.run_and_validate(clients.pid.set_direction,
kwargs={'direction': state.direction})
self.run_and_validate(clients.pid.declare_freq,
kwargs={'freq': state.target_freq})
self.run_and_validate(clients.pmx.use_ext)
self.run_and_validate(clients.pmx.set_on)
self.run_and_validate(clients.pid.tune_freq, timeout=60)
self.run_and_validate(
clients.pcu.send_command,
kwargs={'command': 'off'}, timeout=None,
)

However, if hwp_supervisor.pid_to_freq fails in the middle of its command sequence, we may still have a potential ambiguity in rotation direction, so maybe we should make it more robust?

Motivation and Context

https://github.com/simonsobs/chwp-discussions/discussions/24

How Has This Been Tested?

This has been tested by daq-dev for satp3. Confirmed that the restart of hwp_pid agent doen't lead to the direction ambiguity in both directions.

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.

@ykyohei ykyohei requested a review from bbixler500 July 29, 2024 14:47
@mhasself
Copy link
Member

Since you've removed the .direction attribute, update the class and function docstrings to not mention it.

@BrianJKoopman
Copy link
Member

The build error is unrelated. Working on a fix upstream.

@BrianJKoopman BrianJKoopman merged commit 7123ec5 into main Aug 12, 2024
5 checks passed
@BrianJKoopman BrianJKoopman deleted the hwp_pid_direction branch August 12, 2024 15:23
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