Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1996 +/- ##
==========================================
- Coverage 99.11% 99.11% -0.01%
==========================================
Files 318 323 +5
Lines 12354 12464 +110
==========================================
+ Hits 12245 12354 +109
- Misses 109 110 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
DominicOram
left a comment
There was a problem hiding this comment.
There's a lot of repeated code in all the different TemperatureControllers, we should move more into the base class:
class TemperatureController(StandardReadable, Movable[TemperatureControllerPosition]):
def __init__(self, config_client: ConfigClient, xpdf_parameters_path: str, prefix: str):
self.config_client = config_client
self.xpdf_parameters_path = xpdf_parameters_path
with self.add_children_as_readables():
self.motor = Motor(prefix=prefix)
super().__init__(prefix)
def get_full_config():
return self.config_client.get_file_contents(
self.xpdf_parameters_path,
desired_return_type=TemperatureControllersConfig,
force_parser=TemperatureControllersConfig.from_xpdf_parameters,
)
@abstractmethod
def get_config(): ...
@property
def _safe_position(self) -> float:
return self.get_config().safe_position
@property
def _beam_position(self) -> float:
return self.get_config().beam_position
@AsyncStatus.wrap
async def set(self, value: TemperatureControllerPosition):
if value == TemperatureControllerPosition.SAFE:
await self.motor.set(self._safe_position)
elif value == TemperatureControllerPosition.BEAM:
await self.motor.set(self._beam_position)then:
class Blower(TemperatureController):
def get_config(self) -> TemperatureControllerParams:
return self.get_full_config().blowerIt might then make more sense to just have one set of tests that confirms against the base class that the movement works for different sets of safe/in beam and then just have smaller sets of tests that confirm the child classes get the config from the right place
src/dodal/beamlines/i15_1.py
Outdated
|
|
||
| @devices.factory() | ||
| def blower_y() -> Motor: | ||
| def blower_y(config_client: ConfigClient) -> Blower: |
There was a problem hiding this comment.
Should: It feels like a weird holdover from GDA to have two of these, can we just have the Z?
src/dodal/beamlines/i15_1.py
Outdated
| def cobra(config_client: ConfigClient) -> Cobra: | ||
| return Cobra( | ||
| f"{PREFIX.beamline_prefix}-MO-TABLE-01:ENV:X", | ||
| config_client, | ||
| XPDF_PARAMETERS_FILEPATH, | ||
| ) | ||
|
|
||
|
|
||
| @devices.factory() | ||
| def cryostream(config_client: ConfigClient) -> Cryostream: |
There was a problem hiding this comment.
Should: Did we work out that these are interchangeable devices that go on the same rail? Can we add this into the comments here or in the device classes? If you're not sure can you double check with the scientists?
There was a problem hiding this comment.
Yes they're interchangeable and on the same rail, there's a couple details about safety positions which I'll double check.
| BEAM = "Beam" | ||
|
|
||
|
|
||
| class TemperatureController(StandardReadable, Movable[TemperatureControllerPosition]): |
There was a problem hiding this comment.
Could: TemperatureController isn't a great name here because we're not doing anything with the fact they're temperature controllers. Maybe something more like InBeamOrSafePositioner? That's clunky too though but you get the idea
src/dodal/beamlines/i15_1.py
Outdated
|
|
||
|
|
||
| @devices.factory() | ||
| def cobra(config_client: ConfigClient) -> Cobra: |
There was a problem hiding this comment.
Since moving stuff around later makes merges difficult to follow, and existing devices should all be in alphanumeric order, could we start with these new device in a consistent position?
|
|
||
| @devices.factory() | ||
| def blower_z() -> Motor: | ||
| def blower_z(config_client: ConfigClient) -> Blower: |
There was a problem hiding this comment.
I was hoping we might have some way to tell if this motor was configured in Y mode or Z mode before we added devices for both. I worry that people may rely on the metadata of which motor was moved, and then assume that the motor was moving in Y mode when it was actually configured to move in Z, which is why I made a comment rather than a duplicate device.
Fixes #ISSUE
Required by DiamondLightSource/crystallography-bluesky#24
Adds cobra, blower and cryostream devices
Instructions to reviewer on how to test:
Checks for reviewer
dodal connect ${BEAMLINE}