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

Modify error handling for timed out signals when connecting. #97

Merged
merged 3 commits into from
Feb 16, 2024

Conversation

rosesyrett
Copy link
Collaborator

This commit puts responsibility on anything with a .connect method to raise its own ConnectionTimeoutError, to be then picked up by the DeviceCollector, or not. This simplifies the complex error handling requiring timeouts to be handled at the asyncio level and passed up the called via (now redefined) NotConnected exceptions.

fixes #48

Copy link
Contributor

@callumforrester callumforrester left a comment

Choose a reason for hiding this comment

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

Definitely the right idea and I'm onboard, see individual comments and also the following about approach:

The approach in most default devices seems to be:

try:
    some_device_specific_connection()
except TimeoutError:  # or CANothing or similar
    raise ConnectionTimeoutError(some_device_specific_source)

Does this lose us information from the original error? Would we not be better off with something like this?

try:
    some_device_specific_connection()
except TimeoutError as ex:  # or CANothing or similar
    raise ConnectionTimeoutError(some_device_specific_source) from ex

src/ophyd_async/core/device.py Outdated Show resolved Hide resolved
src/ophyd_async/core/utils.py Outdated Show resolved Hide resolved
@evalott100
Copy link
Contributor

evalott100 commented Feb 9, 2024

New plan:

image

This commit puts responsibility on anything with a .connect method
to raise its own ConnectionTimeoutError, to be then picked up by
the DeviceCollector, or not. This simplifies the complex error
handling requiring timeouts to be handled at the asyncio level and
passed up the called via (now redefined) NotConnected exceptions.
@evalott100 evalott100 force-pushed the add_connect_timeout branch 8 times, most recently from 938b8a8 to 33427af Compare February 14, 2024 13:54
@evalott100 evalott100 force-pushed the add_connect_timeout branch 4 times, most recently from 81b77bf to 2e337d9 Compare February 14, 2024 15:20
src/ophyd_async/core/utils.py Outdated Show resolved Hide resolved
src/ophyd_async/core/utils.py Outdated Show resolved Hide resolved
tests/core/test_utils.py Outdated Show resolved Hide resolved
src/ophyd_async/core/utils.py Outdated Show resolved Hide resolved
src/ophyd_async/core/device.py Outdated Show resolved Hide resolved
Connection timeouts now result in a logging.debug, all other exceptions result
in a logging.exception. All the errors are propogated to the device collector or top
level device and a formatted NotConnected error is outputted
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.

Let's discuss the string formatting offline

src/ophyd_async/epics/_backend/_p4p.py Outdated Show resolved Hide resolved
tests/core/test_utils.py Outdated Show resolved Hide resolved
tests/core/test_utils.py Outdated Show resolved Hide resolved
src/ophyd_async/core/utils.py Outdated Show resolved Hide resolved
src/ophyd_async/core/utils.py Outdated Show resolved Hide resolved
src/ophyd_async/core/utils.py Outdated Show resolved Hide resolved
tests/panda/test_panda.py Outdated Show resolved Hide resolved
@evalott100 evalott100 force-pushed the add_connect_timeout branch 3 times, most recently from f9e45ab to da391b1 Compare February 16, 2024 13:24
@evalott100 evalott100 removed the request for review from callumforrester February 16, 2024 14:22
@evalott100 evalott100 merged commit dcffb97 into main Feb 16, 2024
14 checks passed
@evalott100 evalott100 deleted the add_connect_timeout branch February 16, 2024 14:23
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.

Improve connect protocol to take timeout
4 participants