bluesky / ophyd

hardware abstraction in Python with an emphasis on EPICS
https://blueskyproject.io/ophyd
BSD 3-Clause "New" or "Revised" License
51 stars 79 forks source link

Add notion of "user limits" #742

Open danielballan opened 5 years ago

danielballan commented 5 years ago

Per Ben Ocho, it might be useful to impose optional "user limits" that can be stricter that the EPICS-imposed soft and hard limits.

prjemian commented 5 years ago

+1

On Mon, Jun 24, 2019, 3:04 PM Dan Allan notifications@github.com wrote:

Per Ben Ocho, it might be useful to impose optional "user limits" that can be stricter that the EPICS-imposed soft and hard limits.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/bluesky/ophyd/issues/742?email_source=notifications&email_token=AARMUMDHBGTLQSCKDEMM3FDP4ESFZA5CNFSM4H3B36DKYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4G3MD7VA, or mute the thread https://github.com/notifications/unsubscribe-auth/AARMUMAUWZUZWM42KPC3SGLP4ESFZANCNFSM4H3B36DA .

teddyrendahl commented 5 years ago

Seems like it could be just as confusing as it is helpful. I'm a little wary. Is the issue that users don't have access to soft limits?

mrakitin commented 5 years ago

Thanks for documenting this, @danielballan! I had a conversation with Ben this morning about that, and then discussed it with @tacaswell, who suggested to create a derived class from EpicsMotor in Ophyd to add the 2 components (.LLM, .HLM).

danielballan commented 5 years ago

Great, Ben mentioned it to me as well. I dashed this off to record it without time to consider it. I share @teddyrendahl’s reservation about ending up with a confusing number of layers, maybe too many places to check.

Are HML and LLM soft limits? Ben is asking about another layer on top of soft limits. Even if we decide not to push this into Positioner generically, we could layer it in top easily enough where requested to do so.

teddyrendahl commented 5 years ago

Yes HLM and LLM are the EPICS soft limits on the standard motor record. They get mirrored to the limit values on the setpoint PV which ophyd already reads in , but they aren't easily adjusted. I like the idea of adding them as Components.

mrakitin commented 5 years ago

@teddyrendahl, it is currently not possible to change the limits via the existing interface, they are read-only attributes. However, it's possible to do via caput <motor PV name>.HLM <value>, so it would not be hard for a user to craft an EpicsSignal which does this trick, so why not to have it as a ready-to-use option.

@danielballan, my impression was that Ben wants to change the soft-limits with the existing PVs based on the mode of operation. @stuwilkins suggested it can be more properly done by a soft IOC, which can expose the "mode" field. This way CSS, ophyd and other clients are all in-sync.

prjemian commented 5 years ago

See apstools.devices.EpicsMotorLimitsMixin() (Nearby that code, there are mixins for other motor record features.)

If additional user-controlled limits are also available, they might initialize to .LLM and .HLM. A move command should (overide the default handling and) check these user limits as well as .LLM and .HLM before being permitted to start. There is no such field in the EPICS motor record now for an additional set of limits. We are best to implement this in ophyd, using either ophyd.Signal or ophyd.EpicsSignal if we have PVs for them.

prjemian commented 5 years ago

Ben Ocko?

prjemian commented 5 years ago

... wants to change the soft-limits with the existing PVs based on the mode of operation ...

Use this mixin class only for those motors which might change modes. I'm not sure this merits an additional software layer (such as new support in an IOC). If there is a mode PV to consider, that can be factored into the EpicsMotorLimitsMixin with a subclass.

mrakitin commented 5 years ago

Ben Ocko?

https://staff.ps.bnl.gov/staff.aspx?id=19240, JPLS beamline scientist.

danielballan commented 5 years ago

Yes, sorry. It's a typo---he's actually a grad school friend of my dad's so I know his name well! :- D

awalter-bnl commented 5 years ago

note their was a discussion surrounding a similar thing with Elliot Gann on Gitter, the same conclusion as above was reached !-).

mrakitin commented 5 years ago

Here is a prototype in the beamline profile: https://github.com/NSLS-II-JPLS/profile_collection/commit/03eb955dbe4be9e09d7c7be152804e5da295ca6e. Is it good enough to be ported upstream?


Update: here is the code with missing Cpt import: https://github.com/NSLS-II-JPLS/profile_collection/blob/8140be732b2476cf71a542546dd4434f21149055/startup/zz-dama.py#L1-L6.

teddyrendahl commented 5 years ago

I don't see a reason that this doesn't just go on EpicsMotor no subclass required. All EpicsMotor record implementations will have those PVs.

https://epics.anl.gov/bcda/synApps/motor/R7-0/motorRecord.html#Fields_limit

prjemian commented 5 years ago

We like that the limits are not so easily changed for most axes. For some beam line hardware, there are real financial costs and serious safety considerations to change the soft limits.

On Tue, Jun 25, 2019, 1:50 PM Teddy Rendahl notifications@github.com wrote:

I don't see a reason that this doesn't just go on EpicsMotor no subclass required. All EpicsMotor record implementations will have those PVs.

https://epics.anl.gov/bcda/synApps/motor/R7-0/motorRecord.html#Fields_limit

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/bluesky/ophyd/issues/742?email_source=notifications&email_token=AARMUMDZHSKUUOTXGFPSODTP4JSHFA5CNFSM4H3B36DKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYRHHTY#issuecomment-505574351, or mute the thread https://github.com/notifications/unsubscribe-auth/AARMUMHMAZPX4KIIOWSTX4DP4JSHFANCNFSM4H3B36DA .

tacaswell commented 5 years ago

What @prjemian said, users tend to be clever enough to suss out how to change these things but not wise enough to know they should not!