Skip to content
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

Update AdSim devices and configuration to use ophyd-async #405

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

DiamondJoseph
Copy link
Contributor

@DiamondJoseph DiamondJoseph commented Apr 2, 2024

Fixes #404

Instructions to reviewer on how to test:

  1. Follow instructions for starting the AreaDetector simulator included in the beamline module or from the epics-containers docs
  2. Run the system tests

Checks for reviewer

  • Would the PR title make sense to a scientist on a set of release notes
  • If a new device has been added does it follow the standards

@DiamondJoseph DiamondJoseph marked this pull request as draft April 2, 2024 12:33
Copy link

codecov bot commented Apr 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.13%. Comparing base (5e95221) to head (e3bc815).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #405      +/-   ##
==========================================
+ Coverage   96.04%   97.13%   +1.08%     
==========================================
  Files         136      132       -4     
  Lines        5639     5548      -91     
==========================================
- Hits         5416     5389      -27     
+ Misses        223      159      -64     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@stan-dot
Copy link
Contributor

stan-dot commented Jul 9, 2024

@Relm-Arrowny you've done some simulation work for p99 afaik, how useful is this PR?

@Relm-Arrowny
Copy link
Contributor

@Relm-Arrowny you've done some simulation work for p99 afaik, how useful is this PR?

It nice to have, at the moment I either use fake or something from bluesky.sim so being able to just add area detector without thinking about it would save time.

@DiamondJoseph
Copy link
Contributor Author

How far does fake_with_ophyd_sim get you? Do you need something that is capable of writing files?

e.g.

def det(
    wait_for_connection: bool = True, fake_with_ophyd_sim: bool = True
) -> AravisDetector:
    return device_instantiation(
        AravisDetector,
        "det",
        "-EA-MAP-01:",
        wait_for_connection,
        fake_with_ophyd_sim,
        directory_provider=get_directory_provider(),
    )

@Relm-Arrowny
Copy link
Contributor

How far does fake_with_ophyd_sim get you? Do you need something that is capable of writing files?

It got me as far as getting arming/trigger to work so I can use it to test plans and emitted the right docs. This is an example of what I do atm.
As for writing file, I am not sure, atm I am not concern about writing files but I can see I may in the future.

@ZohebShaikh ZohebShaikh force-pushed the adsim_async branch 2 times, most recently from ecab867 to 5e2187e Compare August 12, 2024 11:29
src/dodal/devices/areadetector/adsim.py Outdated Show resolved Hide resolved
src/dodal/devices/adsim.py Outdated Show resolved Hide resolved
tests/common/beamlines/test_device_instantiation.py Outdated Show resolved Hide resolved
src/dodal/beamlines/adsim.py Outdated Show resolved Hide resolved
Copy link
Contributor

@DominicOram DominicOram left a comment

Choose a reason for hiding this comment

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

As someone who has never used the ADSim before I think this needs some more info. Ideally I would like a few system tests that spawn the ADSim and run some stuff tests against it using these files. If this is out of scope then I would at least like some documentation at the top of adsim.py pointing to how I would get started using the ADSim (and an issue to do the proper tests).

Comment on lines 1 to 33
from pathlib import Path

from ophyd_async.epics.adsimdetector import SimDetector

from dodal.common.beamlines.beamline_utils import (
device_instantiation,
get_path_provider,
set_path_provider,
)
from dodal.common.beamlines.beamline_utils import set_beamline as set_utils_beamline
from dodal.common.visit import LocalDirectoryServiceClient, StaticVisitPathProvider
from dodal.devices.adsim import SimStage
from dodal.log import set_beamline as set_log_beamline
from dodal.utils import get_hostname

BL = get_hostname()
set_log_beamline(BL)
set_utils_beamline(BL)

set_path_provider(
StaticVisitPathProvider(
BL,
Path("/scratch/adsim/bluesky"),
client=LocalDirectoryServiceClient(),
)
)

"""
Usage Example
-------------

Copy link
Contributor

Choose a reason for hiding this comment

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

needs explanation what adsim is, link do docs maybe

Comment on lines -13 to -25
class AdAravisDetector(SingleTriggerV33, DetectorBase):
cam = Cpt(SynchronisedAdDriverBase, suffix="DET:")
hdf = Cpt(
Hdf5Writer,
suffix="HDF5:",
root="",
write_path_template="",
)
_priming_settings: Mapping[Signal, Any]

def __init__(self, *args, **kwargs) -> None:
super().__init__(*args, **kwargs)
self.hdf.kind = "normal"
Copy link
Contributor

Choose a reason for hiding this comment

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

why is AdAravis related?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The AdSim was a final remaining "used" ophyd device in this module, so with it being deleted the whole module may as well be removed.

@DiamondJoseph
Copy link
Contributor Author

@ZohebShaikh re: discussion on docs and use: since the AD sim that we use on workstations isn't going to be containerised or available in exactly the same form outside of Diamond anytime soon, CF and I discussed and we can instead back it with the example Simulation Beamline that is spun up by EC:

https://epics-containers.github.io/main/tutorials/launch_example.html

It has fewer axes, but still has a camera and should be possible to run up from anywhere with the tutorial.

Please walk through the tutorial for this and alter the beamline to connect to these devices!

@stan-dot
Copy link
Contributor

stan-dot commented Oct 23, 2024

found a bug, possibly with #841 that when in ipython context a device does not connect while in the dodal connect beamlinename it does

from a debugging session with @DiamondJoseph

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.

Update adsim to ophyd_async
5 participants