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 time_constant function #179

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
203 changes: 203 additions & 0 deletions src/sorunlib/wiregrid.py
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,112 @@ def _check_temperature_sensors():
_verify_temp_response(resp, 'AIN2C', 0)


def _check_wiregrid_position():
"""Check the wiregrid position."""
actuator = run.CLIENTS['wiregrid']['actuator']
resp = actuator.acq.status()
ls_status = resp.session['data']['fields']['limitswitch']
if (ls_status['LSR1'] == 1 or ls_status['LSL1'] == 1) and not (ls_status['LSR2'] == 1 and ls_status['LSL2'] == 1):
position = 'outside'
elif (ls_status['LSR2'] == 1 or ls_status['LSL2'] == 1) and not (ls_status['LSR1'] == 1 or ls_status['LSL1'] == 1):
position = 'inside'
else:
raise RuntimeError("The wiregrid position is unknown. The limitswitch response is like this:\n" +
"LSR1: {}\n".format('ON' if ls_status['LSR1'] == 1 else 'OFF')
Comment on lines +211 to +212
Copy link
Member

Choose a reason for hiding this comment

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

If this stays here for the moment...pre-commit is complaining about this particular line. Move the + down from line 211 to the start of line 212, like you have the rest of the lines starting with +.

+ "LSL1: {}\n".format('ON' if ls_status['LSL1'] == 1 else 'OFF')
+ "LSR2: {}\n".format('ON' if ls_status['LSR2'] == 1 else 'OFF')
+ "LSL2: {}\n".format('ON' if ls_status['LSL2'] == 1 else 'OFF')
+ "Aborting...")
Comment on lines +205 to +216
Copy link
Member

Choose a reason for hiding this comment

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

I've wondered if there was a method for determining whether the wiregrid was inserted or not for a while now. It would be better to put this logic into the Agent itself, within the acq process' session.data. Then here you would only need to check resp.session['data']['fields']['position']. This also exposes the wiregrid state for other clients, such as ocs-web.

Copy link
Author

@d-hoshino2626 d-hoshino2626 Nov 8, 2024

Choose a reason for hiding this comment

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

I agree with you. I created the PR to implement the position data in session.data.

Copy link
Member

Choose a reason for hiding this comment

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

Great, I just reviewed that. Once it's merged we should drop the code here and use the session.data.

return position


def _stop_hwp_with_wiregrid():
"""Stop the HWP after comfirming the wiregrid is inserted."""
# Check the wiregrid position
position = _check_wiregrid_position()
if position == 'outside':
raise RuntimeError("The wiregrid is not inserted. Aborting...")
elif position == 'inside':
# Stop the HWP
print("Starting to stop the HWP.")
run.hwp.stop(active=True)
print("The HWP stopped successfully.")
else:
raise RuntimeError("Unknown wiregrid position. Aborting...")
Comment on lines +231 to +232
Copy link
Member

Choose a reason for hiding this comment

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

_check_wiregrid_position() only ever returns 'outside' or 'inside', so this code path will never execute.



def _spin_hwp_with_wiregrid(target_hwp_direction):
"""Spin the HWP to the target direction at 2Hz after comfirming the wiregrid is inserted.

Args:
target_hwp_direction (str): Target HWP direction, 'forward' or 'backward'.
"""
# Check the wiregrid position
position = _check_wiregrid_position()
Comment on lines +241 to +242
Copy link
Member

Choose a reason for hiding this comment

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

The checks in both _stop_hwp_with_wiregrid() and _spin_hwp_with_wiregrid() seem like are duplicating the check, and as a result making the functions more complicated than they need to be. They're only used in _reverse_hwp_with_wiregrid(), which is only called after insert(). If insert() fails to insert the wiregrid it raises and the schedule with exit.

If you still want to check the wiregrid is inserted, I'd say do the check in _reverse_hwp_with_wiregrid() before doing anything else. You can then assume its inserted in these other two functions, and they simplify down to:

def _stop_hwp():
    """Stop the HWP."""
    print("Starting to stop the HWP.")
    run.hwp.stop(active=True)
    print("The HWP stopped successfully.")
def _spin_hwp(target_hwp_direction):
    """Spin the HWP to the target direction at 2Hz.
    Args:
        target_hwp_direction (str): Target HWP direction, 'forward' or 'backward'.
    """
    if target_hwp_direction == 'forward':
        print("Starting to spin up the HWP in forward.")
        run.hwp.set_freq(freq=2.0)
        print("The HWP is spinning at 2Hz in forward successfully.")
    elif target_hwp_direction == 'backward':
        print("Starting to spin up the HWP in backward.")
        run.hwp.set_freq(freq=-2.0)
        print("The HWP is spinning at 2Hz in backward successfully.")

if position == 'outside':
raise RuntimeError("The wiregrid is not inserted. Aborting...")
elif position == 'inside':
if target_hwp_direction == 'forward':
print("Starting to spin up the HWP in forward.")
run.hwp.set_freq(freq=2.0)
print("The HWP is spinning at 2Hz in forward successfully.")
elif target_hwp_direction == 'backward':
print("Starting to spin up the HWP in backward.")
run.hwp.set_freq(freq=-2.0)
print("The HWP is spinning at 2Hz in backward successfully.")
else:
raise RuntimeError("Unknown HWP target direction. Aborting...")
else:
raise RuntimeError("Unknown wiregrid position. Aborting...")


def _reverse_hwp_with_wiregrid(initial_hwp_direction, streaming=False, stepwise_before=False, stepwise_after=False):
"""Change the HWP direction after comfirming the wiregrid is inserted.

Args:
initial_hwp_direction (str): Initial HWP direction, 'forward' or 'backward'.
streaming (bool): Do SMuRF streaming during the HWP direction chaging or not . Default is False.
stepwise_before (bool): Do stepwise rotation before changing HWP direction or not. Default is False.
stepwise_after (bool): Do stepwise rotation after changing HWP direction or not. Default is False.
"""
if initial_hwp_direction not in ['forward', 'backward']:
raise RuntimeError("Initial HWP direction should be either 'forward' or 'backward'. Aborting...")

current_hwp_direction = initial_hwp_direction
if current_hwp_direction == 'forward':
target_hwp_direction = 'backward'
elif current_hwp_direction == 'backward':
target_hwp_direction = 'forward'
Comment on lines +273 to +276
Copy link
Member

Choose a reason for hiding this comment

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

I want to make sure we get this convention correct. The HWP supervisor agent has an argument called --forward-dir that defines 'forward' to be clockwise or counter-clockwise, which seems to imply to me this isn't really a fixed direction. Is there some document about what our convention should be?

(I tried to avoid this question myself when writing hwp.set_freq(), but it seems I can't escape it.)

Copy link
Author

@d-hoshino2626 d-hoshino2626 Nov 8, 2024

Choose a reason for hiding this comment

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

I found the confluence page about the rotation direction definition: CHWP main page -- Rotation direction definition. The cw/ccw seems to correspond to the PID value depending on each satp and the PID corresponds to 'forward' or 'reverse' not depending on the satp. We should pay attention which physical direction the 'forward' and 'reverse' mean - cw or ccw. This document uses 'reverse' instead of 'backward'.
Also, I found the 'forwards' and 'backwards' (not 'reverse') in the observation plans on the confluence. I guess a lot of people may make the 'forwards' correspond to the plus frequency like run.hwp.set_freq(freq=+2) and 'backwards' to the minus.

It may be good time to integrate the convention.


# Stop and spin up reversely the HWP
try:
# Enable SMuRF streams
if streaming:
stream_tag = f'wiregrid, wg_time_constant, hwp_change_to_{target_hwp_direction}'
if stepwise_before or stepwise_after:
stream_tag += ', wg_stepwise'
if _check_zenith():
stream_tag += ', wg_el90'
run.smurf.stream('on', subtype='cal', tag=stream_tag)
# Stepwise rotation before stopping the HWP
if stepwise_before:
rotate(False)
# Stop the HWP
_stop_hwp_with_wiregrid()
Copy link

Choose a reason for hiding this comment

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

I suggest to add following here, to stream separately for forward and backward

run.smurf.stream('off')
run.smurf.stream('on')

# Spin up the HWP in the opposite direction
_spin_hwp_with_wiregrid(target_hwp_direction)
current_hwp_direction = target_hwp_direction
# Stepwise rotation after spinning up the HWP
if stepwise_after:
rotate(False)
finally:
# Stop SMuRF streams
if streaming:
run.smurf.stream('off')

return current_hwp_direction


# Public API
def insert():
"""Insert the wiregrid."""
Expand Down Expand Up @@ -305,3 +411,100 @@ def calibrate(continuous=False, elevation_check=True, boresight_check=True,
finally:
# Stop SMuRF streams
run.smurf.stream('off')


def time_constant(initial_hwp_direction, stepwise_first=True, stepwise_last=True, stepwise_mid=False, repeat=1):
"""
Run a wiregrid time constant measurement.

Args:
initial_hwp_direction (str): Initial HWP direction, 'forward' or 'backward'.
stepwise_first (bool): Do stepwise rotation or not before the first HWP speed change. Default is True.
stepwise_last (bool): Do stepwise rotation or not after the last HWP speed change. Default is True.
stepwise_mid (bool): Do stepwise rotation between each HWP speed change. Default is False.
repeat (int): Number of repeats. Default is 1.
If this is odd, the HWP direction will be changed to the opposite of the initial direction.
If this is even, the HWP direction will be the same as the initial direction.
"""

# Check the initial HWP direction
if initial_hwp_direction not in ['forward', 'backward']:
raise RuntimeError("Initial HWP direction should be either 'forward' or 'backward'. Aborting...")
current_hwp_direction = initial_hwp_direction
Comment on lines +430 to +433
Copy link
Member

Choose a reason for hiding this comment

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

This must be able to be determined automatically, no? I would think from the monitor process' session.data, from 'hwp_freq'.

Copy link
Author

Choose a reason for hiding this comment

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

I asked the HWP team about how to know its current spin direction in a robust and fast way, but I was told there is no way. The scheduler can remember the last command to set the direction (the arugment in run.hwp.set_freq()) and whether it was finished successful, so the scheduler can feed its information into the time_constant() function. I thought this is safer than reading the session.data. Does this seem overly verbose?

Copy link

Choose a reason for hiding this comment

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

'direction' in session.data of pid controller agent might be good for initial direction? This value is not reliable when we spin down hwp, or when hwp is stopped, but it is reliable when hwp is spinning stablly.

Copy link
Author

Choose a reason for hiding this comment

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

For now, I assumed the scheduler knows the HWP spin direction but this is also under the condition which the HWP is already spinning. Using 'direction' would be more simple than now. I will modify. Thanks Kyohei!


# Check the repeat
if repeat < 1 or not isinstance(repeat, int):
raise RuntimeError("The repeat should be int and larger than 0. Aborting...")

_check_agents_online()
_check_motor_on()
_check_telescope_position(elevation_check=True, boresight_check=False)

if _check_zenith:
Copy link
Member

Choose a reason for hiding this comment

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

_check_zenith() is a function call, this is missing the parentheses.

el_tag = ', wg_el90'
else:
el_tag = ''

# Rotate for reference before insertion
rotate(continuous=True, duration=10)

# Bias step (the wire grid is off the window)
bs_tag = f'biasstep, wg_time_constant, wg_ejected, hwp_2hz_{current_hwp_direction}' + el_tag
Copy link
Member

Choose a reason for hiding this comment

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

A few comments on these tags:

  • Do we need a 'biasstep' tag when taking a bias step? I'll admit I don't know how these tags are used downstream, but is the operation not implicit?
  • The ideal state might be wiregrid ejected and HWP rotating at 2Hz, but that isn't actually checked until _reverse_hwp_with_wiregrid() is called later on. I would want to verify those before tagging a bias step with them. My recommendation is to add them to the checks above, at the start of the method.

run.smurf.bias_step(tag=bs_tag, concurrent=True)

# Insert the wiregrid with streaming
time.sleep(5)
try:
# Enable SMuRF streams
stream_tag = f'wg_time_constant, wg_inserting, hwp_2hz_{current_hwp_direction}' + el_tag
run.smurf.stream('on', tag=stream_tag, subtype='cal')
# Insert the wiregrid
insert()
finally:
# Stop SMuRF streams
run.smurf.stream('off')
time.sleep(5)

for i in range(repeat):
# Bias step (the wire grid is on the window)
bs_tag = f'biasstep, wg_time_constant, wg_inserted, hwp_2hz_{current_hwp_direction}' + el_tag
run.smurf.bias_step(tag=bs_tag, concurrent=True)

stepwise_before = False
stepwise_after = False
if stepwise_first and i == 0:
# Stepwise rotation before the first HWP speed change
stepwise_before = True
if stepwise_mid and i != 0:
# Stepwise rotation between changing HWP speed
stepwise_before = True
if stepwise_last and i == repeat - 1:
# Stepwise rotation after the last HWP speed change
stepwise_after = True

# Spin the HWP in the opposite direction of the initial direction with streaming and stepwise rotation
current_hwp_direction = _reverse_hwp_with_wiregrid(current_hwp_direction,
streaming=True,
stepwise_before=stepwise_before,
stepwise_after=stepwise_after)

# Bias step (the wire grid is on the window)
bs_tag = f'biasstep, wg_time_constant, wg_inserted, hwp_2hz_{current_hwp_direction}' + el_tag
run.smurf.bias_step(tag=bs_tag, concurrent=True)

# Eject the wiregrid with streaming
time.sleep(5)
try:
# Enable SMuRF streams
stream_tag = f'wg_time_constant, wg_ejecting, hwp_2hz_{current_hwp_direction}' + el_tag
run.smurf.stream('on', tag=stream_tag, subtype='cal')
# Eject the wiregrid
eject()
finally:
# Stop SMuRF streams
run.smurf.stream('off')
time.sleep(5)

# Bias step (the wire grid is off the window)
bs_tag = f'biasstep, wg_time_constant, wg_ejected, hwp_2hz_{current_hwp_direction}' + el_tag
run.smurf.bias_step(tag=bs_tag, concurrent=True)