Skip to content

Conversation

@shihab-dls
Copy link
Contributor

Running await eiger_detector.drv.detector.arm() with:

    async def arm(self):
        await self._drv.detector.arm.trigger(timeout=DEFAULT_TIMEOUT)

should return when the Eiger has completed arming, and is now in a ready status. This seems like more appropriate behaviour as opposed to the current:

    async def arm(self):
        self._arm_status = self._drv.detector.arm.trigger(timeout=DEFAULT_TIMEOUT)

    async def wait_for_idle(self):
        if self._arm_status:
            await self._arm_status

Moreover, current implementation result in the following flaky error in unit tests:

ERROR tests/plans/test_scanspec.py::test_plan_produces_expected_start_document[documents_from_expected_shape2-shape2] - pytest.PytestUnraisableExceptionWarning: Exception ignored in: <coroutine object AsyncMockMixin._execute_mock_call at 0x7f65f87ce240>

This PR makes the suggested change to async def arm(self):

@coretl
Copy link
Collaborator

coretl commented May 8, 2025

Running await eiger_detector.drv.detector.arm() with:

    async def arm(self):
        await self._drv.detector.arm.trigger(timeout=DEFAULT_TIMEOUT)

should return when the Eiger has completed arming, and is now in a ready status. This seems like more appropriate behaviour as opposed to the current:

Is that what FastCS Eiger does? Return from arm as soon as it is armed? In which case that is much neater than what areaDetector does (returns from acquire when it has stopped acquiring...).

If this is the case then your implementation of arm looks right, but what about wait_for_idle, how can you tell that Eiger has finished making frames and is idle again?

@shihab-dls
Copy link
Contributor Author

Is that what FastCS Eiger does? Return from arm as soon as it is armed? In which case that is much neater than what areaDetector does (returns from acquire when it has stopped acquiring...).

If this is the case then your implementation of arm looks right, but what about wait_for_idle, how can you tell that Eiger has finished making frames and is idle again?

FastCS-Eiger, given the FastCS changes here, should return as soon as it is armed, yes. But, until the new release of FastCS, we would need to explicitly wait for self._drv.state to be ready in MX plans, since FastCS 0.8.0 will just return immediately. I think it's good to make this change here first though, since plans using Eiger will need to be changed in the near future anyways.

As for wait_for_idle; the Eiger returns to idle once it's completed it's triggers, so I've push a change to wait for this.

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.

Thanks, I agree with getting this in now but if it's going to change with the FastCS version can we document that. I would normally do so by:

  • Adding an ophyd-async issue on the changes that will need to be made
  • Commenting in the code a link to that issue

@shihab-dls
Copy link
Contributor Author

Thanks, I agree with getting this in now but if it's going to change with the FastCS version can we document that. I would normally do so by:

* Adding an `ophyd-async` issue on the changes that will need to be made

* Commenting in the code a link to that issue

I don't believe any amendments would need to be made once the FastCS release is in; currently, this will return immediately on arm, but once its using the newest release, it will correctly wait for the bound process. I'll add a comment explaining the expected behaviour though!

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.

I see, my bad.

@shihab-dls shihab-dls merged commit b5b0d2a into main May 20, 2025
27 checks passed
@shihab-dls shihab-dls deleted the await_eiger_controller_arm branch May 20, 2025 12:19
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.

4 participants