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

Add support for a continuous acquisition interface for AD dets #793

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

Conversation

jwlodek
Copy link
Member

@jwlodek jwlodek commented Feb 26, 2025

Uses the Circular Buffer plugin.

I've tested that this works with the ContAcqVimbaDetector. Before I continue it would be good if someone could take this for a spin, or have a look-over.

TODO:

  • New tests for the cont acq interface
  • Modify existing AD factory fixture to toggle between cont acq/not-cont acq
  • Update docs to include information on configuring cont acq detector.

Addresses #596

@jwlodek jwlodek changed the title First pass at adding support for a continuous acquisition interface f… Add support for a continuous acquisition interface for AD dets Feb 26, 2025
@jwlodek jwlodek requested a review from coretl February 27, 2025 14:06
@jwlodek jwlodek marked this pull request as ready for review March 11, 2025 14:12
@jwlodek jwlodek requested a review from coretl March 11, 2025 14:27
@jwlodek
Copy link
Member Author

jwlodek commented Mar 11, 2025

The test failures appear to be all tango related, so nothing to do with the changes in the PR...

@jwlodek jwlodek requested a review from thomashopkins32 March 11, 2025 15:45
@jwlodek
Copy link
Member Author

jwlodek commented Mar 11, 2025

Tested w/ real camera and most recent changes and it all works as expected.

Copy link
Collaborator

@coretl coretl left a comment

Choose a reason for hiding this comment

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

Looks good, a couple of small suggestions

self,
prefix: str,
path_provider: PathProvider,
drv_suffix: str = "cam1:",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we add a drv_cls = ADBaseIO here in case we want to expose the extra PVs the class has? Most of the time I imagine that doesn't matter, but it would probably be useful to have the option...

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it also make sense to do this with cb_suffix -> cb_cls?

Copy link
Contributor

Choose a reason for hiding this comment

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

Alternatively, the user might want to pass in a custom controller instead which would already have the custom driver and circular buffer classes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

at the moment there is only one circular buffer class in areaDetector, so I think we should hardcode that, while there are many detector drivers that we should probably support

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, since the Circular Buffer plugin is a default plugin that will be included with every driver, and there is only one variant, I think we can leave as is. There are multiple different writers and drivers, hence passing in classes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good with me then

Comment on lines 33 to 50
@pytest.fixture
def one_shot_trigger_info_factory():
def generate_one_shot_trigger_info(
trigger_mode=DetectorTrigger.INTERNAL, livetime=0.8
):
if trigger_mode != DetectorTrigger.INTERNAL:
return TriggerInfo(
number_of_triggers=1,
trigger=trigger_mode,
livetime=livetime,
deadtime=0.001,
)
else:
return TriggerInfo(
number_of_triggers=1, trigger=trigger_mode, livetime=livetime
)

return generate_one_shot_trigger_info
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not a fan of wrapping functions as fixtures, not sure it adds anything...

Suggested change
@pytest.fixture
def one_shot_trigger_info_factory():
def generate_one_shot_trigger_info(
trigger_mode=DetectorTrigger.INTERNAL, livetime=0.8
):
if trigger_mode != DetectorTrigger.INTERNAL:
return TriggerInfo(
number_of_triggers=1,
trigger=trigger_mode,
livetime=livetime,
deadtime=0.001,
)
else:
return TriggerInfo(
number_of_triggers=1, trigger=trigger_mode, livetime=livetime
)
return generate_one_shot_trigger_info
def generate_one_shot_trigger_info(
trigger_mode=DetectorTrigger.INTERNAL, livetime=0.8
):
if trigger_mode != DetectorTrigger.INTERNAL:
return TriggerInfo(
number_of_triggers=1,
trigger=trigger_mode,
livetime=livetime,
deadtime=0.001,
)
else:
return TriggerInfo(
number_of_triggers=1, trigger=trigger_mode, livetime=livetime
)

Comment on lines 53 to 58
async def test_cont_acq_controller_invalid_trigger_mode(
cont_acq_controller: adcore.ADBaseContAcqController, one_shot_trigger_info_factory
):
trigger_info = one_shot_trigger_info_factory(
trigger_mode=DetectorTrigger.CONSTANT_GATE
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

...

Suggested change
async def test_cont_acq_controller_invalid_trigger_mode(
cont_acq_controller: adcore.ADBaseContAcqController, one_shot_trigger_info_factory
):
trigger_info = one_shot_trigger_info_factory(
trigger_mode=DetectorTrigger.CONSTANT_GATE
)
async def test_cont_acq_controller_invalid_trigger_mode(
cont_acq_controller: adcore.ADBaseContAcqController,
):
trigger_info = generate_one_shot_trigger_info(
trigger_mode=DetectorTrigger.CONSTANT_GATE
)

self,
prefix: str,
path_provider: PathProvider,
drv_suffix: str = "cam1:",
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it also make sense to do this with cb_suffix -> cb_cls?

self,
prefix: str,
path_provider: PathProvider,
drv_suffix: str = "cam1:",
Copy link
Contributor

Choose a reason for hiding this comment

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

Alternatively, the user might want to pass in a custom controller instead which would already have the custom driver and circular buffer classes.

@jwlodek jwlodek requested a review from coretl March 12, 2025 18:57
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.

3 participants