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

Dodal connect on i22 is not actually connecting #864

Open
DominicOram opened this issue Oct 24, 2024 · 5 comments · May be fixed by #865
Open

Dodal connect on i22 is not actually connecting #864

DominicOram opened this issue Oct 24, 2024 · 5 comments · May be fixed by #865
Assignees
Labels
bug Something isn't working i22 python Pull requests that update Python code

Comments

@DominicOram
Copy link
Contributor

If I run dodal connect i22 I get all of the beamline connected suspiciously quickly. It turns out that if I edit one of the PV prefixes in i22.pyto be incorrect then dodal connect i22 will still pass.

Acceptance Criteria

  • If I deliberately break one of the PV prefixes in i22.py then dodal connect i22 actually fails
@DominicOram
Copy link
Contributor Author

@callumforrester and @DiamondJoseph am I right that this is different to #850?

@DominicOram DominicOram mentioned this issue Oct 24, 2024
4 tasks
@DiamondJoseph
Copy link
Contributor

DiamondJoseph commented Oct 25, 2024

This is different than 850: 850 is that when make_all_devices is called, any exceptions in the device_instantiation way of creating and connected devices are swallowed and do not cause test failures.
This is that make_all_devices doesn't ever call connect on the devices given from a @device_factory after they are made- make_all_devices may catch exceptions in instantiating the device and prevent it being an error, but not in the connect which is what's more useful...

@callumforrester
Copy link
Contributor

Additional acceptance criteria: If we could have a regression unit test that would be great

@DiamondJoseph DiamondJoseph linked a pull request Oct 25, 2024 that will close this issue
4 tasks
@DiamondJoseph DiamondJoseph self-assigned this Oct 25, 2024
@DominicOram
Copy link
Contributor Author

With #869 reverted I don't believe this issue is on main now. However, we will need to address it before reinstating the device factory

@stan-dot stan-dot added bug Something isn't working python Pull requests that update Python code i22 labels Oct 28, 2024
@callumforrester
Copy link
Contributor

The cause of this issue that dodal connect <beamline> does not attempt to connect the devices, but expects device_instantiation to do it, which is does as long as wait_for_connection=True is passed. In practice this is true often enough that no one has really noticed, but it is technically still a bug in main because dodal connect does not work for lazy devices. It is just made worse in the world of DeviceInitializationController because all devices are lazy by default there.

So the fix is to make sure the dodal CLI actually calls ensure_connected on the devices it connects to.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working i22 python Pull requests that update Python code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants