-
Notifications
You must be signed in to change notification settings - Fork 12
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
LATRt Updates #419
base: main
Are you sure you want to change the base?
LATRt Updates #419
Conversation
This reverts commit 8ed55f6.
for more information, see https://pre-commit.ci
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.
Thanks for the PR! I just have two comments -- one on docstring consistency, and one on task structure.
"""Tell the controller to hold stages enabled | ||
""" |
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.
The docstring here needs the method signature override, like the first line of the other docstrings in this agent. It also should identify the method as a Task or Process. For this it would be:
"""set_enabled()
**Task** - Tell the controller to hold stages enabled.
Same comment applies to set_disabled
.
@ocs_agent.param('_') | ||
def set_disabled(self, session, params=None): | ||
"""Tell the controller to hold stages enabled | ||
""" | ||
with self.lock.acquire_timeout(timeout=3, job='set_disabled') as acquired: | ||
if not acquired: | ||
self.log.warn( | ||
f"Could not set position because lock held by {self.lock.job}") | ||
return False, "Could not acquire lock" | ||
|
||
self.xy_stage.disable() | ||
return True, "Disabled" |
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.
Docstring comment aside, while this command structure is similar to structures we have in other agents, it would probably be better to combine this two tasks into one that takes a single boolean argument that enables/disables the hold.
We had some weird merging issues that must have gotten through checks when this agent was last moved around. But those are now fixed.
Also fixing some bugs that showed up as we were restarting testing this year.