-
Notifications
You must be signed in to change notification settings - Fork 8
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
Added watsonmarlow323 device for i22 ppump. #575
base: main
Are you sure you want to change the base?
Added watsonmarlow323 device for i22 ppump. #575
Conversation
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.
Looking good, thanks @barnettwilliam
I've made a few comments, mostly minor stuff.
You also need to rebase against main
and fix the linting.
You can run ruff format
and ruff check --fix
to do that.
̶A̶d̶d̶i̶t̶i̶o̶n̶a̶l̶l̶y̶,̶ ̶i̶f̶ ̶y̶o̶u̶ ̶u̶s̶e̶ ̶v̶s̶c̶o̶d̶e̶,̶ ̶I̶ ̶r̶e̶c̶o̶m̶m̶e̶n̶d̶ ̶c̶o̶p̶y̶/̶p̶a̶s̶t̶i̶n̶g̶ ̶i̶n̶ ̶[̶t̶h̶i̶s̶ ̶s̶e̶t̶t̶i̶n̶g̶s̶ ̶d̶i̶r̶e̶c̶t̶o̶r̶y̶]̶(̶h̶t̶t̶p̶s̶:̶/̶/̶g̶i̶t̶h̶u̶b̶.̶c̶o̶m̶/̶D̶i̶a̶m̶o̶n̶d̶L̶i̶g̶h̶t̶S̶o̶u̶r̶c̶e̶/̶p̶y̶t̶h̶o̶n̶-̶c̶o̶p̶i̶e̶r̶-̶t̶e̶m̶p̶l̶a̶t̶e̶/̶t̶r̶e̶e̶/̶m̶a̶i̶n̶/̶.̶v̶s̶c̶o̶d̶e̶)̶ ̶(̶i̶t̶'̶s̶ ̶g̶i̶t̶i̶g̶n̶o̶r̶e̶d̶)̶ ̶t̶o̶ ̶m̶a̶k̶e̶ ̶i̶t̶ ̶a̶u̶t̶o̶ ̶f̶o̶r̶m̶a̶t̶ ̶e̶t̶c̶.̶
Scratch that, wrong repo, sorry! You should already have the settings so just need to make sure your environment is set up correctly. We recommend using the dev container: https://dev-portal.diamond.ac.uk/guide/containers/tutorials/devcontainer/
src/dodal/devices/watsonmarlow323.py
Outdated
STOPPED = "STOP" | ||
STARTED = "START" | ||
|
||
class WatsonMarlow323(StandardReadable): |
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.
Nit: I would prefer a name that gives some suggestion as to what it does, maybe:
class WatsonMarlow323(StandardReadable): | |
class WatsonMarlow323Pump(StandardReadable): |
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.
Renamed as suggested also updated filenames in 417aaa1.
}, | ||
) | ||
|
||
async def assert_reading( |
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.
Should: There is a utility function in ophyd-async
that does this, you can just import that
Example: https://github.com/DiamondLightSource/dodal/blob/main/tests/devices/unit_tests/test_undulator.py
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.
Updated to use the ophyd-async definition as suggested in 417aaa1, assert_reading()
is defined in test_slits.py
the file I initially looked at when creating the test for this device.
src/dodal/devices/watsonmarlow323.py
Outdated
"""Watson Marlow 323 Pump device""" | ||
def __init__(self, prefix: str, name: str= "") -> None: | ||
|
||
self.enabled = epics_signal_r(WatsonMarlow323Enable, prefix + "DISABLE") |
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.
I haven't read the manual, but is this PV really read-only?
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.
Checking this its not read only so I've made it rw in 417aaa1.
src/dodal/devices/watsonmarlow323.py
Outdated
class WatsonMarlow323Direction(str, Enum): | ||
CW = "CW" | ||
CCW = "CCW" |
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.
Nit: More meaningful names would be good here, we need to retain the cryptic acronyms on the epics side, but the ophyd side can have long, descriptive names e.g.
class WatsonMarlow323Direction(str, Enum): | |
CW = "CW" | |
CCW = "CCW" | |
class WatsonMarlow323Direction(str, Enum): | |
CLOCKWISE = "CW" | |
COUNTER_CLOCKWISE = "CCW" |
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.
Updated names in 417aaa1
417aaa1
to
c2790bf
Compare
Thanks @callumforrester I've incorporated your suggested changes, rebased, and linted. Also, made I just need to test this with an actual device to confirm it communicates correctly after which I'll undraft and submit. |
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 sensible
src/dodal/beamlines/i22.py
Outdated
def ppump( | ||
wait_for_connection: bool = True, | ||
fake_with_ophyd_sim: bool = False | ||
) -> WatsonMarlow323: | ||
return device_instantiation( | ||
WatsonMarlow323, | ||
"ppump", | ||
"-EA-PUMP-01:", | ||
wait_for_connection, | ||
fake_with_ophyd_sim | ||
) |
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.
Please also add a fake_with_ophyd_sim: bool = True
equivalent to p38.py, such that it can be kept in a simulated mirror state of i22 (we will make it use the same beamline module file eventually)
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.
Fixed in 6d5ae3f
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.
@barnettwilliam not quite, you've made the i22.py
one fake_with_ophyd_sim = True
, that needs to be False
, and another device_instantiation added to p38.py
with fake_with_ophyd_sim = True
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.
Oops, thanks corrected in d21684d
src/dodal/beamlines/i22.py
Outdated
def ppump( | ||
wait_for_connection: bool = True, fake_with_ophyd_sim: bool = False | ||
) -> WatsonMarlow323Pump: | ||
return device_instantiation( | ||
WatsonMarlow323Pump, | ||
"ppump", | ||
"-EA-PUMP-01:", | ||
wait_for_connection, | ||
fake_with_ophyd_sim, | ||
) |
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.
Please also add to p38.py with fake_with_ophyd_sim: bool = True
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.
As per #575 (comment), fixed in 6d5ae3f.
read_pv=prefix + "DISABLE", | ||
write_pv=prefix + "DISABLE", |
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.
write_pv isn't required if they're both the same.
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.
Fixed in 6d5ae3f
self.limit_high = epics_signal_rw( | ||
float, read_pv=prefix + "SET:SPD.DRVH", write_pv=prefix + "SET:SPD.DRVH" | ||
) | ||
self.limit_low = epics_signal_rw( | ||
float, read_pv=prefix + "SET:SPD.DRVL", write_pv=prefix + "SET:SPD.DRVL" | ||
) |
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.
Limits (and units, precision...) will propagate with the speed signal once this change is merged, these signals shouldn't be required: as is, it's not clear what they are the limits of, but when the change is merged they'll become metadata on the speed signal.
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.
Ok I'll remove these when bluesky/ophyd-async#137 is merged
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.
Rebased and fixed in 84824ca.
|
||
|
||
@skip_device | ||
def ppump( |
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.
Is this deliberately double p? If so what does the first p stand for?
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.
Interjecting here, the double p matches what's on i22's synoptic (it feels like matching the synoptic name <-> bluesky name makes sense?).
The p is for peristaltic pump, as opposed to spump which is a syringe pump.
Might make sense to add a comment somewhere though with the "peristaltic pump" expanded out.
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.
Can we put this in the docstring? Otherwise people will (rightly) think it looks like a typo
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.
Or could call the device peristaltic_pump
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.
I prefer peristaltic_pump
. It's up to the scientist though, do they have strong views?
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.
Added docstring description in 6d5ae3f, I'll find out what the scientists would prefer as the device name.
…nd used ophyd-async assert_reading utility function for tests.
…skip device, enabled fake_with_ophyd_sim, and don't specify read write pv if they are the same.
…ir corresponding signal.
d21684d
to
84824ca
Compare
self.enabled = epics_signal_rw( | ||
WatsonMarlow323PumpEnable, | ||
prefix + "DISABLE", | ||
) | ||
|
||
self.direction = epics_signal_rw( | ||
WatsonMarlow323PumpDirection, | ||
read_pv=prefix + "INFO:DIR", | ||
write_pv=prefix + "SET:DIR", | ||
) | ||
self.state = epics_signal_rw( | ||
WatsonMarlow323PumpState, | ||
read_pv=prefix + "INFO:RUN", | ||
write_pv=prefix + "SET:RUN", | ||
) | ||
self.speed = epics_signal_rw( | ||
float, read_pv=prefix + "INFO:SPD", write_pv=prefix + "SET:SPD" | ||
) | ||
|
||
self.set_readable_signals( | ||
read=[self.state, self.speed, self.direction], | ||
config=[ | ||
self.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.
nit: this could be done with the add_children_as_readables
context, which is now the recommended way to do it in the ophyd-async docs
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.
nit, but otherwise looks good
Added Watson Marlow 323 pump device for use on I22 - draft PR for concept review.
This device depends on an updated support module interface (support/watson-marlow) that was made more consistent while making this change - release TBC.
Instructions to reviewer on how to test:
Checks for reviewer
dodal connect ${BEAMLINE}