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.

Sign up for GitHub

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

Jump to bottom

Device connect lazily #294

Merged
merged 33 commits into from
May 17, 2024
Merged

Device connect lazily #294

merged 33 commits into from
May 17, 2024

Conversation

ZohebShaikh
Copy link
Contributor

@ZohebShaikh ZohebShaikh commented May 9, 2024 •
edited
Loading

closes #265

@ZohebShaikh ZohebShaikh self-assigned this May 9, 2024
@ZohebShaikh ZohebShaikh force-pushed the device-connect-lazily branch from 88cd870 to 2257ba0 Compare May 9, 2024 14:53
coretl
coretl reviewed May 9, 2024
View reviewed changes
src/ophyd_async/core/device.py Outdated Show resolved Hide resolved
src/ophyd_async/planstubs/ensure_connected.py Show resolved Hide resolved
tests/core/test_device.py Outdated Show resolved Hide resolved
evalott100
evalott100 requested changes May 9, 2024
View reviewed changes
Copy link
Contributor

@evalott100 evalott100 left a comment

Choose a reason for hiding this comment

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

Nice work, nearly there.

src/ophyd_async/core/device.py Outdated Show resolved Hide resolved
src/ophyd_async/core/device.py Outdated Show resolved Hide resolved
src/ophyd_async/core/device.py Outdated Show resolved Hide resolved
evalott100
evalott100 requested changes May 10, 2024
View reviewed changes
Copy link
Contributor

@evalott100 evalott100 left a comment

Choose a reason for hiding this comment

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

Had the idea that the following is probably nicer 🤔

Since we're awaiting the task in init anyway, we only need to store whether or not it was successful

src/ophyd_async/core/device.py Outdated Show resolved Hide resolved
src/ophyd_async/core/device.py Outdated Show resolved Hide resolved
tests/core/test_device.py Outdated Show resolved Hide resolved
tests/core/test_device.py Outdated Show resolved Hide resolved
@ZohebShaikh ZohebShaikh force-pushed the device-connect-lazily branch from 71b02a6 to 1ee4b02 Compare May 10, 2024 11:29
@ZohebShaikh ZohebShaikh force-pushed the device-connect-lazily branch from 76aaade to f1dfd97 Compare May 10, 2024 13:16
@ZohebShaikh ZohebShaikh requested a review from evalott100 May 10, 2024 13:17
evalott100
evalott100 previously requested changes May 13, 2024
View reviewed changes
tests/epics/motion/test_motor.py Outdated Show resolved Hide resolved
@ZohebShaikh ZohebShaikh force-pushed the device-connect-lazily branch 2 times, most recently from 4786964 to f765121 Compare May 13, 2024 14:19
@ZohebShaikh ZohebShaikh force-pushed the device-connect-lazily branch from f765121 to 8cf8ea7 Compare May 13, 2024 14:25
@ZohebShaikh ZohebShaikh force-pushed the device-connect-lazily branch from 0b341bd to b3fa65e Compare May 16, 2024 14:41
@ZohebShaikh ZohebShaikh requested review from coretl and evalott100 and removed request for abbiemery May 16, 2024 15:07
coretl
coretl requested changes May 17, 2024
View reviewed changes
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.

Please could you also update Signal.connect to take the same arguments as Device.connect?

src/ophyd_async/core/device.py Outdated Show resolved Hide resolved
src/ophyd_async/core/device.py Outdated Show resolved Hide resolved
@ZohebShaikh
Copy link
Contributor Author

Please could you also update Signal.connect to take the same arguments as Device.connect?

I have added it to Signal.connect which has mockbackend and backend but they don't use the param force_reconnect

@ZohebShaikh ZohebShaikh requested a review from coretl May 17, 2024 09:50
@coretl coretl mentioned this pull request May 17, 2024
Closed
coretl
coretl requested changes May 17, 2024
View reviewed changes
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.

Thought of a bug...

src/ophyd_async/core/device.py Show resolved Hide resolved
src/ophyd_async/core/device.py Outdated Show resolved Hide resolved
src/ophyd_async/core/device.py Show resolved Hide resolved
src/ophyd_async/core/device.py Show resolved Hide resolved
@coretl coretl merged commit 5bcac12 into main May 17, 2024
18 checks passed
@coretl coretl deleted the device-connect-lazily branch May 17, 2024 14:11
@coretl coretl mentioned this pull request Jun 17, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@coretl coretl coretl approved these changes

@evalott100 evalott100 Awaiting requested review from evalott100

Assignees

@ZohebShaikh ZohebShaikh

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Features to allow Devices to be connected up front, or lazily connected in plans
3 participants
@ZohebShaikh @evalott100 @coretl