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

v2: Refactor and Test DeviceCollector Error Handling #1151

Closed
wants to merge 3 commits into from

Conversation

callumforrester
Copy link
Contributor

@callumforrester callumforrester commented Aug 21, 2023

Changes:

  • Add tests to verify that DeviceCollector handles exceptions generated by connect()
  • Add tests to verify that exceptions from devices failing to connect are logged
  • Change error logging so that timeouts and exceptions from devices are logged in a uniform way
  • Add comments to error handling code
  • Make pre-existing tests that check the logs consistent with new tests

@callumforrester
Copy link
Contributor Author

Some additional issues have come out of this work
#1152
#1153

Copy link
Contributor

@rosesyrett rosesyrett left a comment

Choose a reason for hiding this comment

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

It looks like the main thing you're changing here is how the error gets logged when a device fails to connect, either because of an exception or because of a timeout error.

Seems sensible

@coretl
Copy link
Collaborator

coretl commented Aug 29, 2023

My aim would be the following:

  • If Device.connect() raises, then report its traceback in full
  • If Device.connect() doesn't complete within timeout, cancel it, if it raises NotConnected then report the message text (which says which child signals are not connected) without traceback, otherwise report its traceback in full

This PR seems to report traceback for NotConnected errors too, do we want that?

@callumforrester
Copy link
Contributor Author

@coretl To get to where you want, I think we would first have to fix #1153
This PR doesn't change the current behaviour except to make the log formatting more consistent. The information logged is the same.
I suggest that sorting out exactly what is logged under which circumstances should be a separate PR. Possibly also a separate issue pulling together your comment and #1153

@coretl
Copy link
Collaborator

coretl commented Apr 24, 2024

Superceded in ophyd-async

@coretl coretl closed this Apr 24, 2024
@coretl coretl deleted the improve-v2-errors branch April 24, 2024 15:50
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.

3 participants