Add pinhole and collimator stages control for I19-2 #1508
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1508 +/- ##
==========================================
+ Coverage 98.79% 98.80% +0.01%
==========================================
Files 260 262 +2
Lines 9464 9558 +94
==========================================
+ Hits 9350 9444 +94
Misses 114 114 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
||
|
|
||
| @device_factory() | ||
| def pincol() -> PinholeCollimatorControl: |
There was a problem hiding this comment.
Nit: I would call this something more descriptive, in a world of autocomplete being descriptive is more important than being concise. e.g. pinhole_and_collimator
| from ophyd_async.epics.core import epics_signal_r, epics_signal_rw, epics_signal_x | ||
|
|
||
|
|
||
| class MAPTConfigurationTable(StandardReadable): |
There was a problem hiding this comment.
Should: What does MAPT mean? Can you add to the docstring?
| LOGGER.debug(f"Moving motors to {aperture_positions}") | ||
|
|
||
| # First move Pinhole motors, | ||
| LOGGER.debug("Move pinhole stage in") |
There was a problem hiding this comment.
Nit:
| LOGGER.debug("Move pinhole stage in") | |
| LOGGER.debug("Moving pinhole stage in") |
| self.pinhole.y.set(aperture_positions.pinhole_y), | ||
| ) | ||
| # Then move Collimator motors | ||
| LOGGER.debug("Move pinhole stage in") |
There was a problem hiding this comment.
Should:
| LOGGER.debug("Move pinhole stage in") | |
| LOGGER.debug("Moving collimator stage in") |
| _CONFIG = "-OP-PCOL-01:" | ||
|
|
||
|
|
||
| class PinColRequest(StrictEnum): |
There was a problem hiding this comment.
Could: This isn't being used in ophyd-async so can just be an Enum not a StrictEnum
|
|
||
|
|
||
| class PinColRequest(StrictEnum): | ||
| """Aperture request IN positions.""" |
There was a problem hiding this comment.
Should: It's not just in positions
| # NOTE. Using subset enum because from the OUT positions should only be used by | ||
| # the beamline scientists from the synoptic. Another option will be needed in the | ||
| # device for OUT position. | ||
| class PinColPosition(SubsetEnum): |
There was a problem hiding this comment.
Should: I think I would change this to _PinColPosition so a user is not tempted use it in the set
| self.pinhole = XYStage(f"{prefix}{pin_infix}") | ||
| self.collimator = XYStage(f"{prefix}{col_infix}") | ||
| self.mapt = PinColConfiguration( |
There was a problem hiding this comment.
Should: I would make all these private so a user isn't tempted to get round the collision detection bit
| def _get_aperture_size(self, ap_request: str) -> int: | ||
| return int(ap_request.strip("um")) | ||
|
|
||
| async def get_motor_positions_for_requested_aperture( |
There was a problem hiding this comment.
Should: This should be private
|
|
||
|
|
||
| def test_pincol_created_without_errors(): | ||
| pincol = PinholeCollimatorControl("", "test_pincol") |
There was a problem hiding this comment.
Should: I think this might be trying to make a real one? I would just reuse the pincol fixture here
Fixes #1504
Instructions to reviewer on how to test:
dodal connect i19-2Checks for reviewer
dodal connect ${BEAMLINE}