-
Notifications
You must be signed in to change notification settings - Fork 24
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
Fastcs eiger support #528
Fastcs eiger support #528
Conversation
self._drv.data_type.set( | ||
"uint16" | ||
), # TODO: Get from eiger https://github.com/bluesky/ophyd-async/issues/529 | ||
# TODO: Where should I actually be setting this |
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'm not sure on the best way to do this to best fit into the existing architecture
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'm not sure what "this" is? All the signal sets you mention seem to be in the right place, although I expected num_to_capture
to be 0
so it would go forever...
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.
Sorry, you're right that this
is the number of frames. Ok, so the idea is that we will never run the file writer knowing how many frames it has up front? I think this will have side-effects that we're not used to for example that we will always have N files, where N is the number of processors, though most of the time some of them will be empty but this is probably ok.
|
||
mkdir /tmp/opi | ||
|
||
podman run --rm -dt --net=host -v /tmp/opi/:/epics/opi ghcr.io/diamondlightsource/eiger-fastcs:0.1.0beta5 ioc $eiger_ioc |
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.
Could: Detect whether podman
or docker
are available and choose the command accordingly.
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.
@gilesknap I wonder if this can be achieved with docker-compose
or similar, so we can run it up either in the CI or locally in exactly the same way?
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 didn't want to hold this up on the CI stuff, which could get fiddly so made #531
self._drv = driver | ||
|
||
def get_deadtime(self, exposure: float) -> float: | ||
return 0.0001 |
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.
Where did you get this number from? If from the manual then please can you provide a reference?
class PhotonEnergy(Device): | ||
"""Changing photon energy takes some time so only do so if the current energy is | ||
outside the tolerance.""" | ||
|
||
def __init__(self, prefix, tolerance=0.1, name: str = "") -> None: | ||
self._photon_energy = epics_signal_rw_rbv(float, f"{prefix}PhotonEnergy") | ||
self.tolerance = tolerance | ||
super().__init__(name) | ||
|
||
@AsyncStatus.wrap | ||
async def set(self, value): | ||
current_energy = await self._photon_energy.get_value() | ||
if abs(current_energy - value) > self.tolerance: | ||
await self._photon_energy.set(value) |
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.
Slightly concerned about putting this logic in the IO class. As written this will break load/save as it's not a Signal.
What's the use case for setting within a tolerance? If it's from a custom plan, could the logic be lifted into a plan stub? If not would a set_energy
method on the detector be acceptable?
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.
The use-case is that we want to avoid setting it as much as possible because it increases the time it takes to arm the detector and it's scientifically fine for it to be just within the tolerance. At the moment we bundle all the logic required to set up and arm the detector into a single bps.set
because we need to do it all in parallel to another plan running. If we want to do this via plan_stubs
then I guess we're blocked on bluesky/bluesky#1652. We could put it in a set_energy
and call it during the arm
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 discussed in person, we make a set_energy
and put this in arm
for now. May change when parallel plans is done
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 have put it in prepare
. I would rather put it in the arm
so that we have a better ordering of when to do it but to do so would need to change the arm
interface quite a bit, probably to take something that inherits from TriggerInfo
. Are you happy for me to do this refactor?
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.
@ZohebShaikh will be doing this as part of #405, so it would be useful if you could review and/or add to his PR to make sure the Eiger usecase is covered
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 soon as PVI datastructures are published over PVA then this file will drastically change to something like:
class EigerDriverIO(Device):
bit_depth: SignalR[int]
stale_parameters: SignalR[bool]
...
It will also only need to include the signals that are needed at static analysis time, at runtime then all the extras (like beam_centre_x
, etc) will be filled in automatically.
This will also mean that the class structure and ophyd attribute names will have to exactly match those in PVI.
For this reason I suggest we do this soon, and @GDYendell is going to start looking at this now
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.
It will also only need to include the signals that are needed at static analysis time, at runtime then all the extras (like beam_centre_x, etc) will be filled in automatically.
Eventually all these signals will be needed at static analysis time. I have added in all the ones that we need to re-implement what we currently have on the ophyd
eiger
.
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.
PVI structures will be added in DiamondLightSource/FastCS#54
self._drv.data_type.set( | ||
"uint16" | ||
), # TODO: Get from eiger https://github.com/bluesky/ophyd-async/issues/529 | ||
# TODO: Where should I actually be setting this |
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'm not sure what "this" is? All the signal sets you mention seem to be in the right place, although I expected num_to_capture
to be 0
so it would go forever...
|
||
@AsyncStatus.wrap | ||
async def prepare(self, value: EigerTriggerInfo) -> None: | ||
await self._controller.set_energy(value.energy_ev) |
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.
After #405 then there will be a prepare
method on the controller that takes a TriggerInfoT
so this prepare method will become unnecessary. For now this is correct though.
See DiamondLightSource/dodal#700. This implements enough of the Eiger/Odin for us to prove that we can actually take some data with it. To completely cover all the capability of the
ophyd
device we need to do a lot, at least:To Test
Notes