Allow thawing on a timer #609
Conversation
DominicOram
requested review from
rtuck99,
callumforrester,
coretl,
d-perl and
olliesilvester
src/dodal/devices/thawer.py
Outdated
| class ThawerStates(str, Enum): | ||
| OFF = "Off" | ||
| ON = "On" | ||
|
|
||
|
|
||
| class Thawer(StandardReadable): | ||
| class ThawingTimer(SignalRW[float]): |
There was a problem hiding this comment.
I'd make it a Device rather than a SignalRW as we're not actually doing anything with the value here...
There was a problem hiding this comment.
I think a Movable is better because of #609 (comment)?
| ) | ||
|
|
||
|
|
||
| async def test_calling_stop_on_thawer_stops_thawing( |
There was a problem hiding this comment.
Similar to my last comment, this test implies calling stop stops the thawing, but if haven't previously called thaw_for_time_s, then stop does nothing
src/dodal/devices/thawer.py
Outdated
| class Thawer(StandardReadable): | ||
| class ThawingTimer(Device): | ||
| def __init__(self, control_signal: SignalRW[ThawerStates]) -> None: | ||
| super().__init__("thawing_timer") |
There was a problem hiding this comment.
Should: I think super().__init__ should go at the end of the init function, just by convention
There was a problem hiding this comment.
Ok, this is an odd one. When making a Device super must go at the end of the function because it will iterate through and change the names of the child signals to be device-signal. We probably want control to be given it's device name from Thawer rather than ThawingTimer, in which case the answer here is for ThawingTimer to not be a Device at all, which I have done and so now it doesn't need to call super at all
There was a problem hiding this comment.
Deleted my previous comment as I don't think it's right. I would go back to what you had before, with __init__ before the self._control_signal = control_signal. I can't think of any other way which makes sense given that a Msg obj should have a name
There was a problem hiding this comment.
That still doesn't work because ThawingTimer overwrites the name. But I might do that anyway and spin into a new issue to think of a better way of doing it.
There was a problem hiding this comment.
Yeah, I can't think of a good solution here. You could do
def __init__(self, prefix: str, name: str = "") -> None:
self.thaw_for_time_s = ThawingTimer(epics_signal_rw(ThawerStates, prefix + ":CTRL"))
self.control = epics_signal_rw(ThawerStates, prefix + ":CTRL")
super().__init__(name)
So you'd have two separate control signals pointing to the same PV, and the names would reflect where they're from. But I can imagine creating two signals to the same PV might cause issues
There was a problem hiding this comment.
There was a problem hiding this comment.
I've proposed a fix over in that ticket, but I was wondering why ThawingTimer is a separate object here? Could you move the set method into the Thawer and dispense with the ThawingTimer completely? @olliesilvester suggested they both needed to be set in bluesky/ophyd-async#410 (comment) but I don't see the other set method here...
There was a problem hiding this comment.
Sorry, yeah, my previous comment was a bit misleading as there isn't a set on Thawer right now. If we did have a Thawer.set it would just be setting the control signal to on or off, (no timer)
There was a problem hiding this comment.
As @olliesilvester said, the Thawer has two modes of operation:
- The plan will turn it on/off
- The plan will turn it on via the timer then not turn it off itself
I feel like having the set on the Thawer doing number 2 is really not obvious. You would logically expect it to be doing number 1.
|
Failing tests are on main also so unrelated to this I believe |
Fixes DiamondLightSource/hyperion#1418
Instructions to reviewer on how to test:
Checks for reviewer
dodal connect ${BEAMLINE}