-
Notifications
You must be signed in to change notification settings - Fork 0
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
DM-39207-- Power Off ATCalSys #132
Conversation
ba35369
to
30777b5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks pretty good. I have a few inline comments I hope you can address before approving the PR.
|
||
|
||
class PowerOffATCalSys(salobj.BaseScript): | ||
"""Powers on the ATCalSys dome flat illuminator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Powers on -> Powers off
f"Lamp state: {ATWhiteLight.LampBasicState(lamp_state.basicState)!r}." | ||
f"Need to wait {cool_down_wait_time/60} min" | ||
) | ||
asyncio.sleep(60) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to sleep here.
------ | ||
RunTimeError: | ||
If ATWhiteLight is not ENABLED""" | ||
for comp in [self.white_light_source]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's only one component.. You can probably skip the for loop.
RunTimeError: | ||
If ATWhiteLight is not ENABLED""" | ||
for comp in [self.white_light_source]: | ||
summary_state = await comp.evt_summaryState.aget() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sure you add a timeout to aget
or it can block here for ever if there's missing data.
If ATWhiteLight is not ENABLED""" | ||
for comp in [self.white_light_source]: | ||
summary_state = await comp.evt_summaryState.aget() | ||
if salobj.State(summary_state.summaryState) != salobj.State( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to cast neither values:
if summary_state.summaryState != salobj.State.ENABLED:
...
e28128c
to
75728e9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but please take a look at the comment I left. The flush
method is not a coroutine and cannot be awaited.
|
||
async def switch_lamp_off(self): | ||
"""Switches white light source lamp off""" | ||
await self.white_light_source.evt_lampState.flush() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Flush is not a coroutine, remove await
b01c515
to
8558600
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
No description provided.