i15-1: add goniometer interlock #2019
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2019 +/- ##
=======================================
Coverage 99.11% 99.11%
=======================================
Files 327 328 +1
Lines 12761 12774 +13
=======================================
+ Hits 12648 12661 +13
Misses 113 113 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
EmsArnold
force-pushed
the
ea/add_i15-1_gonio
branch
from
44e81f0 to
7472108
Compare
jacob720
left a comment
jacob720
left a comment
There was a problem hiding this comment.
Looks good, just a small comment and question
| async def shutter_safe_to_operate(self) -> bool: | ||
| """Description.""" | ||
| interlock_state = await self.status.get_value() | ||
| return isclose(float(interlock_state), GONIO_SAFE_FOR_OPERATIONS, abs_tol=5e-2) |
There was a problem hiding this comment.
Out of interest, why do we convert to float and have a tolerance here? The comment about 16 bits makes it sound like the self.status is a 16 bit value
There was a problem hiding this comment.
This is emulating the hutch interlock logic to check if the device is safe to operate. In practice, I think the conversion to float is necessary, as the other day during testing I did get an AssertionError that 65535 was not equal to 65535 before I added this in to my check.
With regards to the 16 bits, 65535 is simply the float equivalent of the binary 1111111111111111 - or all 16 flags of the interlock being healthy. This may be something that we ask controls to look at, to see if this can return an enum like some other interlocks.
| ) | ||
|
|
||
| async def shutter_safe_to_operate(self) -> bool: | ||
| """Description.""" |
There was a problem hiding this comment.
Should: Fill out the docstring or remove
|
I am partway through reviewing this but ended up gettign sidetracked, more comments incoming... |
DominicOram
left a comment
DominicOram
left a comment
There was a problem hiding this comment.
Thanks. As I said in the other PR, I think #2022 might help with this?
| GONIO_SAFE_FOR_OPERATIONS = 65535 # All 16 bits are true | ||
|
|
||
|
|
||
| class GonioInterlock(BaseHutchInterlock): |
There was a problem hiding this comment.
Should: BaseHutchInterlock implies that it's only for interlocks on the hutch. I think this is expanding it so maybe we should rename to BaseInterlock
There was a problem hiding this comment.
I agree, though I think this could also be expanded to the other interlocks as well - for example PLCShutterInterlock would also be useful for fast vacuum valves. I would lean towards re-naming these all at the same time.
There was a problem hiding this comment.
Sure, feel free to spin into a new issue if you'd rather
There was a problem hiding this comment.
Will do - I think it would also warrant moving the interlocks out of hutch_shutter.py with similar reasoning. I wouldn't immediately look in hutch_shutter if I only needed an interlock.
EmsArnold
force-pushed
the
ea/add_i15-1_gonio
branch
from
b366281 to
ba0dd68
Compare
That's all done now - updated to use |
DominicOram
left a comment
DominicOram
left a comment
There was a problem hiding this comment.
Great, thanks. Might make sense to wait until DiamondLightSource/crystallography-bluesky#31 is approved to merge both at the same time.
EmsArnold
force-pushed
the
ea/add_i15-1_gonio
branch
from
ba4b507 to
740a95b
Compare
3 participants
Fixes crystallography_bluesky #26
Instructions to reviewer on how to test:
Checks for reviewer
dodal connect ${BEAMLINE}