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

Adding Pimte area detector devices #429

Closed
wants to merge 11 commits into from

Conversation

Relm-Arrowny
Copy link
Contributor

Fixes #428

Instructions to reviewer on how to test:

  1. unit test should cover almost every time
  2. If IRL test is needed there are three jupyter notebook script here that should cover most of the functionality needed.

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
Copy link
Contributor

I've enabled the CI for you @Relm-Arrowny - do you need adding to the DLS organisation?

@Relm-Arrowny
Copy link
Contributor Author

Yes adding to DLS would be nice

Comment on lines 9 to 17
CURRENT_DIRECTORY = "." # str(Path(__file__).parent)


async def make_detector(prefix: str = "") -> HDFStatsPimte:
dp = StaticDirectoryProvider(CURRENT_DIRECTORY, f"test-{new_uid()}")

async with DeviceCollector(sim=True):
detector = HDFStatsPimte(prefix, dp, "pimte")
return detector
Copy link
Contributor

Choose a reason for hiding this comment

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

should

Suggested change
CURRENT_DIRECTORY = "." # str(Path(__file__).parent)
async def make_detector(prefix: str = "") -> HDFStatsPimte:
dp = StaticDirectoryProvider(CURRENT_DIRECTORY, f"test-{new_uid()}")
async with DeviceCollector(sim=True):
detector = HDFStatsPimte(prefix, dp, "pimte")
return detector
@pytest.fixture
def tmp_directory_provider(tmp_path: Path) -> StaticDirectoryProvider:
return StaticDirectoryProvider(tmp_path)

in conftest.py allows re-use of this, using a new tmp path for each run (this is how it's being done in ophyd-async, where this device may end up eventually)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved this into conftest.py

tests/devices/unit_tests/test_pimte.py Outdated Show resolved Hide resolved
Comment on lines +20 to +39
def count_sim(det: HDFStatsPimte, times: int = 1):
"""Test plan to do the equivalent of bp.count for a sim detector."""

yield from bps.stage_all(det)
yield from bps.open_run()
yield from bps.declare_stream(det, name="primary", collect=False)
for _ in range(times):
read_value = yield from bps.rd(det._writer.hdf.num_captured)
yield from bps.trigger(det, wait=False, group="wait_for_trigger")

yield from bps.sleep(0.001)
set_sim_value(det._writer.hdf.num_captured, read_value + 1)

yield from bps.wait(group="wait_for_trigger")
yield from bps.create()
yield from bps.read(det)
yield from bps.save()

yield from bps.close_run()
yield from bps.unstage_all(det)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should: be somewhere common, since testing our StandardDetector impls create the right documents is going to be a repeated pattern

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure what to do, I stolen it from #332 may be I should move it to somewhere common on this PR? or wait until the #332 merge and use that instead?

Comment on lines 17 to 28
class SpeedMode(str, Enum):
adc_50Khz = "0: 50 KHz - 20000 ns"
adc_100Khz = "1: 100 kHz - 10000 ns"
adc_200Khz = "2: 200 kHz - 5000 ns"
adc_500Khz = "3: 500 kHz - 2000 ns"
adc_1Mhz = "4: 1 MHz - 1000 ns"
adc_2Mhz = "5: 2 MHz - 500 ns"

class TriggerMode(str, Enum):
internal = "Free Run"
ext_trigger = "Ext Trigger"
bulb_mode = "Bulb Mode"
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit:

From the development of the TetrAMM, Pilatus and Aravis I think we've settled on Pimte1SpeedMode, Pimte1TriggerMode outside of the Driver class.
Also in dodal so far devices have all been defined in .py (see e.g. tetramm.py) with all the components inside. I believe ophyd-async is moving in that direction once we get a chance to refactor.

src/dodal/devices/areadetector/pimteAD.py Outdated Show resolved Hide resolved
Comment on lines 30 to 31
def get_deadtime(self, exposure: float) -> float:
return exposure + 0.1
Copy link
Contributor

Choose a reason for hiding this comment

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

Is deadtime exposure + 0.1s ? or is it a flat 0.1s?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Relm-Arrowny
Copy link
Contributor Author

cleaning up, the detector is current under repair will reopen as needed.

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.

create CCD pimte area detector device
2 participants