ISISComputingGroup / lewis-ess

Let's write intricate simulators!
GNU General Public License v3.0
21 stars 19 forks source link

EPICS meta data can be updated at runtime #169

Closed MichaelWedel closed 7 years ago

MichaelWedel commented 7 years ago

This fixes #158.

With this change it is now possible to supply PV with an additional argument metadata_property that returns a dict which updates PV metadata. Additionally, LimitViolationExceptions are now caught in PropertyExposingDriver so that the check_limits decorator can be used to enforce PV limits.

MichaelWedel commented 7 years ago

Does this need any further changes?

MikeHart85 commented 7 years ago

Having trouble seeing how this fits in with everything else... some questions:

  1. Do we have any real use case for this?
  2. How does/will this tie in with @check_limits, if at all? What about distinguishing HIGH / LOW vs HIHI / LOLO?
  3. Where would metadata in general come from? Since Devices must be protocol-agnostic, they must not know anything about EPICS-specific metadata (or its values) like unit, type, etc. So it must be coded in the Interface, right? What would this look like in practice?
  4. Have you considered direct setting over callback update? E.g.:

    class Foo(EpicsAdapter):
    pvs = {
        'foobar': PV('foobar', unit='mm'),
        'unit': PV('unit', type='string')
    }
    
    @property
    def unit(self):
        return self.pvs['foobar'].getParamInfo('unit')
    
    @property.setter
    def unit(self, value):
        self.pvs['foobar'].setParamInfo('unit', value)

    Does this callback approach have advantages over something like the above?

MichaelWedel commented 7 years ago

The use case for this is a device like this:

class SomeDevice(StateMachineDevice):
    p_hilim = 10.0
    p_lolim = 0.0
    _p = 1.0

    @property
    def p(self):
        return self._p

    @p.setter
    @check_limits('p_lolim', 'p_hilim')
    def p(self, new_p):
        self._p = new_p

In reality someone might go to the device and push the button to change the limit. Or log into the device and change it. In a simulation it would probably happen through the control server.

The corresponding Interface:

class SomeDeviceEpicsInterface(EpicsAdapter):
    pvs = {
        'P' : PV('p', meta_data_property='p_meta', unit='mm')
    }

    @property
    def p_meta(self):
        return {
           'lolim': self._device.p_lolim,
           'hilim': self._device.p_hilim,
        }

These are not separate PVs, this is the information that is contained in the response from for example epics.PV.get_ctrlvars from pyepics. Each PV has them, although they might not always be defined by all IOCs.

I did not see a better way to make these kind of changes at runtime possible and it ties in with check_limits as well, as these are the "control limits". The other limits defined by EPICS are as far as I understand warning levels.

Does this answer your questions?

MikeHart85 commented 7 years ago

I see, that makes much more sense thanks!

One alternative could be to make a MetaPV object (which effectively is just a reference to an attribute by name), which would look like this:

class SomeDeviceEpicsInterface(EpicsAdapter):
    pvs = {
        'P' : PV('p', unit='mm', lolim=MetaPV('p_lolim'), hilim=MetaPV('p_hilim'))
    }

This would remove the need for defining a p_meta that returns a dict in the Interface. The MetaPV instances could also do some smart things like caching the old value so you can update only when it changes.

Not sure if there would be any value to that. But it would likely be much more work in the Adapter to glue all this together. I like how little code your solution needs to implement this, this is just an idea I had while looking over it.

MikeHart85 commented 7 years ago

Looks good to merge to me, let me know if still want to make changes.

MichaelWedel commented 7 years ago

I think for this iteration I don't want to change anything else. You're definitely right about the caching aspect, I totally agree on that. But in principle that argument applies to the PV value as well, so we should probably go back to this method and PV in general to see if we can make it better. It would be something for the next release, this time it was StreamAdapter that got all the attention.