added acceleration PV to Motor device #9
Conversation
Codecov ReportBase: 47.10% // Head: 47.32% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #9 +/- ##
==========================================
+ Coverage 47.10% 47.32% +0.21%
==========================================
Files 4 4
Lines 242 243 +1
==========================================
+ Hits 114 115 +1
Misses 128 128
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
| @@ -15,6 +15,7 @@ def __init__(self, prefix: str, name="") -> None: | |||
| self.setpoint = EpicsSignalRW(float, ".VAL") | |||
| self.readback = EpicsSignalR(float, ".RBV") | |||
| self.velocity = EpicsSignalRW(float, ".VELO") | |||
| self.acceleration = EpicsSignalR(float, ".ACCL") | |||
There was a problem hiding this comment.
I suggest acceleration time, so it's clear this is not acceleration rate...
| self.acceleration = EpicsSignalR(float, ".ACCL") | |
| self.acceleration_time = EpicsSignalR(float, ".ACCL") |
There was a problem hiding this comment.
The .ACCL field in EPICS is always acceleration time (the time to change the velocity from before to after). Always a stumbling block for experienced control engineers new to EPICS. I believe a change to acceleration_time here introduces some ambiguity which makes this component seem like a special case, somehow different from .ACCL.
Also, it is not typical for a user to want to change .ACCL field of every motor from ophyd. I suggest that for those motors which need this additional feature (or others), subclass locally and add the additional component:
class MyEpicsMotor(EpicsMotor):
steps_per_revolution = Component(EpicsSignal, ".SREV", kind="omitted")There was a problem hiding this comment.
So, please do not add support for the motor .ACCL field to every motor.
There was a problem hiding this comment.
I agree with the comment on SREV but I would argue that ACCL is quite fundamental, for instance the use case here is to work out how long it will take a motor to ramp up to velocity so we can work out where the "move to start" position should be for a flyer. Not sure if it should be an EpicsSignalR or an EpicsSignalRW though...
Also, it was there in the existing Ophyd EpicsMotor:
https://github.com/bluesky/ophyd/blob/5a0f1f3e8c2f453eb5a3262c610714bee67f84db/ophyd/epics_motor.py#L54
I agree with the comment about naming, acceleration is a better name.
Simple PR to add acceleration to the Motor device, so that it can be accessed and read like the other PVs (.e.g .VELO).