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 logic to stop waiting on pin if robot raises pin not found error #629

Merged
merged 3 commits into from
Jun 20, 2024

Conversation

DominicOram
Copy link
Contributor

@DominicOram DominicOram commented Jun 17, 2024

Fixes DiamondLightSource/hyperion#1446 and fixes DiamondLightSource/hyperion#1450

Instructions to reviewer on how to test:

  1. Confirm tests pass

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
  • If changing the API for a pre-existing device, ensure that any beamlines using this device have updated their Bluesky plans accordingly
  • Have the connection tests for the relevant beamline(s) been run via dodal connect ${BEAMLINE}

@DominicOram
Copy link
Contributor Author

@coretl and @callumforrester, I'm particularly interested in your thoughts on pin_mounted_or_no_pin_found. Is this the best way to solve the problem? Are there helper functions we can put into ophyd-async for this? Maybe an observe_multiple_values for getting all the updates from multiple signals?

@coretl
Copy link
Collaborator

coretl commented Jun 18, 2024

I think you've done this in the most concise way at present, but I think you're right about adding a helper to ease this, maybe something like:

    async def pin_mounted_or_no_pin_found(self):
        async for signal, value in observe_signals_value(self.error_code, self.goinio_pin_sensor):
            if signal is self.error_code and value == self.NO_PIN_ERROR_CODE:
                # We didn't get a pin
                raise RobotLoadFailed(self.NO_PIN_ERROR_CODE, "Pin was not detected")
            elif signal is self.goinio_pin_sensor and value == PinMounted.PIN_MOUNTED:
                # We did get a pin
                break

@DominicOram
Copy link
Contributor Author

DominicOram commented Jun 18, 2024

Thanks @coretl, that's exactly the sort of thing I was thinking. I've made bluesky/ophyd-async#402. I will leave this as is to maintain velocity on hyperion but will pull the ophyd-async issue into our next sprint


with pytest.raises(RobotLoadFailed) as e:
await device.set(SampleLocation(0, 0))
assert e.value.error_code == expected_error_code
Copy link
Contributor

Choose a reason for hiding this comment

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

These asserts are never called

await sleep(0.01)
with pytest.raises(RobotLoadFailed):
await status

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand what these sleeps are for - you don't enter the coroutine until await is called on it.

@DominicOram DominicOram requested a review from rtuck99 June 19, 2024 14:38
Copy link
Contributor

@rtuck99 rtuck99 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 now

@DominicOram DominicOram merged commit d9a70cd into main Jun 20, 2024
11 checks passed
@DominicOram DominicOram deleted the hyperion_1446_robot_load_fixes branch June 20, 2024 13:43
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.

Robot load: Check pin not mounted Robot load testing 14/06
3 participants