Skip to content

Use PVI information to fill in PVs on FastCS Eiger#806

Closed
shihab-dls wants to merge 37 commits intomainfrom
551_use_pvi_information
Closed

Use PVI information to fill in PVs on FastCS Eiger#806
shihab-dls wants to merge 37 commits intomainfrom
551_use_pvi_information

Conversation

@shihab-dls
Copy link
Copy Markdown
Contributor

@shihab-dls shihab-dls commented Mar 6, 2025

Fixes #551

This PR should create FastCS modules for Eiger and Odin, and write the Eiger in a declarative style with typed annotations. Currently annotations are limited to signals internatlly used by the device, and those accessed in the MX DAQ Eiger arming chain. These signals should now follow the expected format of PVI attributes.

Notes for reviewer:
These changes were made for FastCS Odin as well; capture signal, which reads Writing and writes to ConfigHDFWrite was split into two signals, which seems messy. Moreover, attribute names seem quite ambiguous.

@shihab-dls shihab-dls force-pushed the 551_use_pvi_information branch from f8a65da to 4776910 Compare March 6, 2025 13:39
@shihab-dls shihab-dls requested review from GDYendell and coretl March 6, 2025 16:56
Comment thread src/ophyd_async/fastcs/eiger/_eiger_controller.py Outdated
Comment thread src/ophyd_async/fastcs/eiger/_eiger_io.py
@GDYendell
Copy link
Copy Markdown
Contributor

This API doesn't seem very clear now that I am seeing it on the ophyd side... Maybe we can improve the names in fastcs-odin.

@shihab-dls
Copy link
Copy Markdown
Contributor Author

shihab-dls commented Mar 10, 2025

Current API Rec
capture (read_pv) writing writing
capture (write_pv) config_hdf_write write
num_captured frames_written frames_written
num_to_capture frames frames
image_height data_dims_0 Content Cell
image_width data_dims_1 Content Cell
num_frames_chunks data_chunks_0 Content Cell
num_row_chunks data_chunks_1 Content Cell
num_col_chunks data_chunks_2 Content Cell
data_type data_datatype data_datatype

Above is a table of current Odin signal names (i.e., ADOdin), and API signal names (i.e., Fast-CS Odin) that are not in agreement. Recommended names are provided (yet to be filled). config_hdf_write will need to be changed to write once this is properly introspected.

@DominicOram
Copy link
Copy Markdown
Contributor

Can I suggest spinning the name changes off into a separate issue? Doing just a find and replace later won't be too much effort and it decouples getting the issue complete from fastcs changes

@coretl
Copy link
Copy Markdown
Collaborator

coretl commented Mar 11, 2025

Can I suggest spinning the name changes off into a separate issue? Doing just a find and replace later won't be too much effort and it decouples getting the issue complete from fastcs changes

The name changes in ophyd-async? Unfortunately we can't do this, the name in PVI is the name of the attribute in the ophyd-async Device. This ticket is to use PVI to make the Eiger and Odin Devices, so I think we need to decide on names as part of this ticket...

It's possible to make a different ticket that keeps the old attribute names, but fills in the PVs from the new FastCS Eiger, but I'm not sure it is worth the effort to do that intermediate step unless you need to keep switching between the two IOCs, optimizing both...

Can we discuss in standup today?

Comment thread src/ophyd_async/core/_signal.py
Comment thread src/ophyd_async/epics/core/_p4p.py
Comment thread src/ophyd_async/epics/core/_pvi_connector.py Outdated
@shihab-dls
Copy link
Copy Markdown
Contributor Author

As discussed offline I think we don't need trigger_and_wait_for_other_value

This PR is now waiting on FastCS #141, which should remove the need for us to wait on detector status.

@shihab-dls
Copy link
Copy Markdown
Contributor Author

shihab-dls commented Apr 2, 2025

With the merging of FastCS #141, this PR should be ready for re-review.

Current State of Changes

  • eiger and odin moved to fastcs module from epics module, and written declaratively.
  • Signals explicitly used in ophyd_async and MX are annotated.
  • EigerController.arm() now simply returns the SignalX's trigger() async status, where EigerController.wait_for_idle() awaits the status, instead of returning the status of set_and_wait_for_other_value()

EDIT: will do a final check on Odin signals, then re-request

UPDATE:

  1. Upon further testing, I don't see why we don't use config_hdf_write_rbv to check if we are writing, as config_hdf_write is a SignalRW; is there a subtle difference between Writing and the rbv of config_hdf_write?
  2. Top-level Odin now has FP, FR, and MW 'subsystems. These are not provisioned in this manner I think, instead requiring FP as the top controller to bind annotations. This seems fine as I think we only use FP signals explicitly?

Comment thread src/ophyd_async/fastcs/eiger/_eiger.py Fixed
@shihab-dls shihab-dls requested a review from coretl April 2, 2025 11:22
Comment thread src/ophyd_async/fastcs/odin/_odin_io.py Outdated
):
self.drv = EigerDriverIO(prefix + drv_suffix)
self.odin = Odin(prefix + hdf_suffix + "FP:")
self.odin = OdinHdfIO(prefix + hdf_suffix + "FP:")
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed in person, I wonder if this should be structured to point to the top level controller of the FastCS-odin. The we would have det.odin.fp.hdf.file_path rather than det.odin.file_path, in case there are other things to go in fr or other plugins in fp we need to reference later. OdinWriter can still take an OdinHdfIO in its constructor if all the PVs it needs are in that Device

@DominicOram
Copy link
Copy Markdown
Contributor

Following #862 and #863 the changes in here are effectively to move us from ADOdin to fast-cs odin. I would suggets closing this PR and making a new one for that but leave it up to @shihab-dls

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Eiger FastCS: Use PVI information to fill in PVs

5 participants