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

Move timeout handling to functions wrapped with AsyncStatus #318

Merged
merged 5 commits into from
May 20, 2024
Merged

Conversation

coretl
Copy link
Collaborator

@coretl coretl commented May 20, 2024

I think the best place to apply timeouts for motion is in the *Motor.set function, rather than the caller to this function.

This PR shows what this looks like:

  • Removing timeout handling from *AsyncStatus
  • Calculating the timeout from velocity in *Motor.set and adding DEFAULT_TIMEOUT
  • Making a mechanism to apply a timeout via observe_value for Mover.set

I think this makes things easier to use as you are not forced to bps.mv(motor, value, timeout=<something I have to calculate>), it fills in a sensible value for timeout itself. The only question I have is whether we should allow this timeout to be overridden.

@dperl-dls @callumforrester thoughts?

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.

@coretl This seems very nice for motors but how does it affect your common garden signal?

The only question I have is whether we should allow this timeout to be overridden.

I can see the use case of "I want this motor to reach position a in n seconds otherwise my beamline is broken and I want to know about it" being a thing

tests/epics/motion/test_motor.py Outdated Show resolved Hide resolved
@coretl
Copy link
Collaborator Author

coretl commented May 20, 2024

@coretl This seems very nice for motors but how does it affect your common garden signal?

Not at all, they will still take set(value, timeout) which is handled by the underlying control system. It is up to the caller of Signal.set to supply a reasonable titmout. It is only the case of MyDevice.set which we are talking about here.

The only question I have is whether we should allow this timeout to be overridden.

I can see the use case of "I want this motor to reach position a in n seconds otherwise my beamline is broken and I want to know about it" being a thing

So "I want this to move within 2 seconds no matter where it was to begin with"? That's fairly easy to add as an override Motor.set(value, timeout=2). I'll do that.

Copy link
Contributor

@dperl-dls dperl-dls left a comment

Choose a reason for hiding this comment

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

I think this is an elegant solution. The only real concern I would have had is about being able to override the timeout, that should definitely be possible. Is it still possible to wait forever on this?

@coretl
Copy link
Collaborator Author

coretl commented May 20, 2024

I think this is an elegant solution. The only real concern I would have had is about being able to override the timeout, that should definitely be possible. Is it still possible to wait forever on this?

I've made the handling of Signal.set and Motor.set be the same so they take timeout: None | float | Type[CalculateTimeout] = CalculateTimeout, and this CalculateTimeout should give you a sane default

Copy link
Contributor

@dperl-dls dperl-dls left a comment

Choose a reason for hiding this comment

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

lgtm

@coretl coretl merged commit d2ed67e into main May 20, 2024
18 checks passed
@coretl coretl deleted the timeouts branch May 20, 2024 14:49
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