bluesky / ophyd

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

Revisit the Control Layer concept. #753

Open danielballan opened 4 years ago

danielballan commented 4 years ago

From another in-person discussion with @klauer and @tacaswell:

The "Control Layer" was conceptualized as a way to switch between EPICS, Tango, etc. In practice it is a switch between pyepics and caproto. There is a simpler way to achieve this.

Add PyepicsSignal and CaprotoEpicsSignal. At import time, do:

if os.environ['OPHYD_CONTROL_LAYER'] == 'caproto':
    EpicsSignal = CaprotoEpicsSignal
elif os.environ['OPHYD_CONTROL_LAYER'] == 'pyepics':
    EpicsSignal = PyepicsEpicsSignal
else:
    EpicsSignal = PyepicsEpicsSignal  # This default may change in the future.

Do the same for all the variants of EpicsSignal (read-only, etc.). Mix-in classes may be useful here.

teddyrendahl commented 4 years ago

I think the concept is useful and should live somewhere still. Any thought about just making an EPICS control layer library with some testing that all the pyepics, caproto, and pyca "epics.PV" interfaces stay the same?

From my perspective, the control layer was needed largely when caproto had a differing API. But now that there is a mock-up of the pyepics interface there most of the code between the two portions of the layer are extremely similar.

klauer commented 4 years ago

@teddyrendahl the CaprotoEpicsSignal and PyepicsEpicsSignal, it's important to note, would have 99.9% of the same contents. There would only be subclasses that set the PV class to use, such as:

class PyepicsEpicsSignal(EpicsSignal):
    _pv_class = _pyepics_shim.PV  # thin wrapper around epics.PV

class CaprotoEpicsSignal(EpicsSignal):
    _pv_class = _caproto_shim.PV  #  thin wrapper around caproto.threading.pyepics_compat.PV

Because of that, I don't think anything is really lost here. We also agreed that the dispatch logic could be shared among all backends such that callbacks would be consistent and not control-layer dependent.

Does that change your opinion at all?

teddyrendahl commented 4 years ago

Does that change your opinion at all?

I see. Certainly not opposed to removing it from Ophyd. Just saying that I think the concept has a general use in the community and doing something similar to qtpy where Python - EPICS applications can use a library agnostic API internally to ensure compatibility between facilities seems like a good idea.

That being said, it is a difficult and thankless job to maintain so I understand if nobody wants to jump on that particular grenade.

danielballan commented 4 years ago

I think we want the concept, but we want it as a switch on Channel Access vs Tango vs ..., not as a switch of which implementation of Channel Access.

The qtpy equivalent for Python--EPICS applications would be, I guess, the PV class and its associated API.

klauer commented 4 years ago

My comment up there was slightly inaccurate. The shim PV classes will remain in ophyd, and PyepicsEpicsSignal._pv_class = ophyd._pyepics_shim.PV - sorry about that.

ZLLentz commented 4 years ago

I'm not sure I understand what this change allows us to do, or where the control layer concept is being removed as per the title, considering the control layer shim classes are remaining and the environment variable we're switching on is still called OPHYD_CONTROL_LAYER. That being said, I'm all for making implementations simpler.

danielballan commented 4 years ago

I changed the title, which I think wasn't quite right. Basically, there are two layers we care about:

These are different things. We want both. In our discussion we proposed to change the implementation of the "implementation layer" to be simpler and reserve the "semantics" layer for future work on Tango.

danielballan commented 4 years ago

Sorry for being unclear, and thanks for the push to clarify, all.

ZLLentz commented 4 years ago

Thanks for the clarity :+1: seems logical