epics-base / p4p

Python bindings for the PVAccess network client and server.
BSD 3-Clause "New" or "Revised" License
27 stars 38 forks source link

Use of post and put with handlers #150

Open Monarda opened 3 months ago

Monarda commented 3 months ago

Within a program using p4p's PVAccess Server I believe developers have two options to set a SharedPV's value. The most obvious route is to use a post but the other option is to use a client put. An important difference between these two options is that the post will not trigger the SharedPV's handlers while the put will. This means that, for example, if a handler is setup to evaluate valueAlarm settings then the post will not trigger the associated alarm severity or message while a client put will.

Is there an intended design to emulate a handler for a post? Or some other mechanism to allow post values to be reliably evaluated against alarm limits, control limits, etc.? For example, inheriting from SharedPV to override its post method?

Here's a program to demonstrate the put and post difference by implementing a (bad!) handler with a version of control.minStep:

import random, time

from p4p.nt import NTScalar
from p4p.server import Server
from p4p.server.thread import Handler, SharedPV
from p4p.client.thread import Context, RemoteError

class demo_handler(Handler):
    def put(self, pv, op):
        pv_raw = pv.current().raw
        if abs( pv_raw['value'] - op.value().raw['value'] ) < pv_raw['control.minStep']:
            op.done(error="<minStep")
            return

        pv.post(op.value()) # just store and update subscribers
        op.done()

def create_sharedpv():
    pv = SharedPV(nt=NTScalar('d', control=True),
                  handler = demo_handler(),
                  initial=0.0)
    pv.post({'control.minStep':2})
    return pv

pvs = {
    'demo:pv:post':create_sharedpv(),
    'demo:pv:put' :create_sharedpv(),
}

pvaserver = Server(providers=[pvs])

try:
    while True:
        time.sleep(1)
        newval = random.randint(1,5)

        pvs['demo:pv:post'].post(newval)
        try:
            Context('pva').put('demo:pv:put', newval)
        except RemoteError as err:
            print(err)
except KeyboardInterrupt:
    pass
finally:
    pvaserver.stop()

If monitored although the PVs demo:pv:post and demo:pv:put are set with the same values they will show divergent behaviour as the minStep causes the demo:pv:put to change less frequently.

If a SharedPV is set as read-only through its handler then only the post may be used to change it's value, e.g.

@pv.put
    op.done(error='read-only')

This means write protected PVs can't use the client put route.

There is the obvious mechanism of implementing a check each time, e.g.

def check_minstep(pv_value : Value, newvalue : float):
    pv_raw = pv.current().raw
    if abs( pv_raw['value'] - newvalue ) < pv_raw['control.minStep']:
        return False

    return True

if check_minstep(pv, newval):
    pv.post(newval)

but this seems error prone.

And a simplified version using inheritance to override post and implement the minStep:

class CustomPV(SharedPV):
    def post(self, value, **kws):
        try:
            V = self._wrap(value, **kws)
        except: # py3 will chain automatically, py2 won't
            raise ValueError("Unable to wrap %r with %r and %r"%(value, self._wrap, kws))

        pv_raw = self.current().raw
        if abs( pv_raw['value'] - V['value'] ) < pv_raw['control.minStep']:
            return

        super().post(V)

Still bad since like the earlier handler example it doesn't correctly deal with changes to the minStep. I think it still has potential issues around code duplication with the handler, especially as a developer adds more Normative Type fields or custom logic.

mdavidsaver commented 3 months ago

Is there an intended design to emulate a handler for a post?

No.

If it is helpful, I see .post() as analogous to db_post_event() is epics-base record and device support code, which is "trusted" to provide a valid Value. However, .put() is not a good analogy to a record support process() function.

In the past I have sometimes "cheated" by using a nt= wrapper which maintains a shadow copy of the current Value. However, this is imperfect (a cache of a cache...).

I have so far shied away from designing "database records in python" as a feature of P4P. Pointing rather to pythonSoftIOC or pyDevSup which provide access to the full power (and historical limitations) of the EPICS Process DataBase. Either can be used in combination with QSRV to serve up via PVAccess protocol.

My worry is that my lack of attention/motivation/imagination would only produce a slow, partial, clone of pyDevSup when it seems like there is latitude for more.

Still, maybe the time has come to reconsider?

What could "records in python" look like in 2024? How much, if any, of the PDB infrastructure should transfer over? (scan tasks, I/O Intr, lock sets, ...?)

Am I making too much of what could be no more than adding some notion of validator callback(s) to SharedPV?

Monarda commented 3 months ago

Thanks for sharing your thoughts and those links. I was aware of pythonSoftIOC (the principal developers are just across the campus from us) but hadn't taken a good look at it yet. And I'm embarrassed to say I wasn't aware of pyDevSup at all.

I should say that my experience with both Channel Access and more conventional IOCs is very limited. I'm afraid I don't know how a db_post_event() or process() works in an "ordinary" IOC. To give a little background we're in the process of converting from our existing control system software to EPICS and so far using exclusively PVAccess. To create a bridge to our existing control system and allow parallel operation a colleague developed software using pvapy. Since then we've standardised on p4p for our Python development. Only in the last few months have we deployed our first IOCs and those have been existing community developed software (interfacing with Modbus and FINS protocol) where our concern was with configuration and deployment. Though I have (rusty) C/C++ knowledge I've never done any IOC development.

Getting back to the topic of this issue, I think what we're (currently) looking for is more limited than general support for database records in Python. Currently p4p supplies the structure of PVAccess Normative Types and we'd like to add their internal logic. It would still do things like set alarm.severity based on valueAlarm settings and apply control limits when the value is changed, etc. That means we would end up with something that would be similar to simple database records but without the more complicated functionality such as you listed.

We will probably press on from there but for us next steps are likely to focus on how to implement similar features to CALC records and to the autosave module. We do already have a prototype PyRecCaster we're close to ready to open-source. I anticipate they for convenience we might ask for additional hooks and callbacks. For example, PyRecCaster might benefit from having .on_open() and .on_close() hooks.

Answering only the very final question I think we are looking for something a little more than a validator callbacks. I think the existing .post() could be extended to interact with the handlers fairly easily and provide the necessary extended functionality. I have some suggestions based on our work on creating a NTHandler with the existing p4p interfaces which I'll add in further comments.

Thanks again for your time on this!

Monarda commented 3 months ago

As promised / threatened (!) some ideas based on attempting an implementation of an NTHandler for p4p.

1. Changes to Value

It would be useful for Value to allow something like:

Value.update(other_value: Value, fields: Optional[str, list[str], None] = None) -> None:

The obvious default behaviour here is that I have value1 and value2 with the same structure and if I use value1.update(value2) then any fields marked as changed in value2 overwrite those in value1 in an in-place operation. In this case value1 and value2 would probably be required to have the same Type. Or for value2 to be a strict subset of value1. However, something like value1.update(value2, ["value", "control"]) could be used when only a subset of the structure needs to be updated and in that case only those fields would be required to have the same Types.

But what's needed for handlers is almost the opposite. Consider the .put(pv: SharedPV, op: ServerOperation). Assuming we're applying the logic for the control or valueAlarm fields then we will need to use:

current_value = pv.current().raw
op_value = op.value().raw

to get the information needed. However, neither Value has all the information needed by the handler.

What we want is an in-place update of op_value replacing all its unchanged fields with values from the current_value but continuing to mark those fields as unchanged. Once this is done we have a complete set of information necessary for evaluating the fields control, valueAlarm, etc.

That might look like

op_value.update_unchanged(current_value)

or maybe

op_value.update(current_value, fields=["value", "control", "valueAlarm"], marked=False)
Very long explanation of the problem with full NTScalar structures and a code sample with partial solution Consider an example NTScalar with timeStamp, control, and valueAlarm: ``` struct "epics:nt/NTScalar:1.0" { int32_t value = 3 struct "alarm_t" { int32_t severity = 0 int32_t status = 0 string message = "" } alarm struct "time_t" { int64_t secondsPastEpoch = 123 int32_t nanoseconds = 456 int32_t userTag = 0 } timeStamp struct { int32_t limitLow = 1 int32_t limitHigh = 10 int32_t minStep = 0 } control struct { bool active = true int32_t lowAlarmLimit = -5 int32_t lowWarningLimit = 0 int32_t highWarningLimit = 10 int32_t highAlarmLimit = 15 int32_t lowAlarmSeverity = 2 int32_t lowWarningSeverity = 1 int32_t highWarningSeverity = 1 int32_t highAlarmSeverity = 2 double hysteresis = 0 } valueAlarm string descriptor = "An example int PV with control limits and valueAlarm settings" } ``` and I wish to update it with ``` {"value": 7, "control.limitHigh": 6, "valueAlarm.highWarningLimit": 5, "valueAlarm.highAlarmLimit": 10} ``` when it's received by the handler after a `.put()` or `.post()` it will look like this (where I've used `*` to mark changed fields): ``` struct "epics:nt/NTScalar:1.0" { * int32_t value = 7 struct "alarm_t" { int32_t severity = 0 int32_t status = 0 string message = "" } alarm struct "time_t" { int64_t secondsPastEpoch = 0 int32_t nanoseconds = 0 int32_t userTag = 0 } timeStamp struct { int32_t limitLow = 0 * int32_t limitHigh = 6 int32_t minStep = 0 } control struct { bool active = true int32_t lowAlarmLimit = 0 int32_t lowWarningLimit = 0 * int32_t highWarningLimit = 5 * int32_t highAlarmLimit = 10 int32_t lowAlarmSeverity = 0 int32_t lowWarningSeverity = 0 int32_t highWarningSeverity = 0 int32_t highAlarmSeverity = 0 double hysteresis = 0 } valueAlarm string descriptor = "" } ``` Neither of the above two NTScalars are suitable for a handler evaluating the fields according to the Normative Type specification. What we want is something that combines the information from both as follows: ``` struct "epics:nt/NTScalar:1.0" { * int32_t value = 7 struct "alarm_t" { int32_t severity = 0 int32_t status = 0 string message = "" } alarm struct "time_t" { int64_t secondsPastEpoch = 123 int32_t nanoseconds = 456 int32_t userTag = 0 } timeStamp struct { int32_t limitLow = 1 * int32_t limitHigh = 6 int32_t minStep = 0 } control struct { bool active = true int32_t lowAlarmLimit = -5 int32_t lowWarningLimit = 0 * int32_t highWarningLimit = 5 * int32_t highAlarmLimit = 10 int32_t lowAlarmSeverity = 2 int32_t lowWarningSeverity = 1 int32_t highWarningSeverity = 1 int32_t highAlarmSeverity = 2 double hysteresis = 0 } valueAlarm string descriptor = "An example int PV with control limits and valueAlarm settings" } ``` After evaluation by the handlers the resulting NTScalar should then be (changes highlighted with `-->`): ``` struct "epics:nt/NTScalar:1.0" { --> int32_t value = 6 struct "alarm_t" { --> int32_t severity = 1 int32_t status = 0 --> string message = "highWarning" } alarm struct "time_t" { --> int64_t secondsPastEpoch = 789 --> int32_t nanoseconds = 123 int32_t userTag = 0 } timeStamp struct { int32_t limitLow = 1 --> int32_t limitHigh = 10 int32_t minStep = 0 } control struct { bool active = true int32_t lowAlarmLimit = -5 int32_t lowWarningLimit = 0 --> int32_t highWarningLimit = 5 --> int32_t highAlarmLimit = 10 int32_t lowAlarmSeverity = 2 int32_t lowWarningSeverity = 1 int32_t highWarningSeverity = 1 int32_t highAlarmSeverity = 2 double hysteresis = 0 } valueAlarm string descriptor = "An example int PV with control limits and valueAlarm settings" } ``` This is my own attempt to implement the required in-place merge: ```python """Utilities related to p4p Values and Types""" from typing import Callable, Optional, cast from p4p import Value def recurse_values(value1: Value, value2: Value, func: Callable[[Value, Value, str], None], keys=None) -> bool: """Recurse through two Values with the same structure and apply a supplied to the leaf nodes""" if not keys: keys = cast(list[str], value1.keys()) for key in keys: if isinstance(value1[key], Value) and isinstance(value2[key], Value): if not recurse_values(value1[key], value2[key], func): return False else: func(value1, value2, key) return True def overwrite_unmarked(current: Value, update: Value, fields: Optional[list[str]] = None) -> None: """ Overwrite all of the unmarked fields in one Value with fields from another Value. This makes the changes in place rather than returning a copy. """ def overwrite_unchanged_key(update_leaf: Value, current_leaf: Value, key: str) -> None: """ Given a leaf node in the update Value tree, check whether it is unchanged and, if so, set it equal to the equivalent leaf node in the current Value tree. Then mark the new value for the leaf as unchanged. """ if not update_leaf.changed(key): update_leaf[key] = current_leaf[key] update_leaf.mark(key, val=False) if not fields: fields = cast(list[str], current.keys()) recurse_values(update, current, overwrite_unchanged_key, fields) ```

I believe this not a breaking change as it shouldn't affect existing code (unless someone has derived from Value and implemented it themselves). However, I don't have a good idea about the size of the change. I suspect my own code sample won't deal with more complicated and deeper structures. And I strongly suspect it won't be performant. But that might take implementing it in pvxs which might be a much larger request?

Monarda commented 3 months ago

2. Post in Handler

This is relatively straightforward in the code I've implemented:

class PostHandler(Handler):
    def post(self, pv: SharedPV, new_state: Value) -> None:
        pass

class PostSharedPV(SharedPV):
    """Implementation that applies specified rules to post operations"""

    def post(self, value, **kws) -> None:
        """
        Override parent post method in order to apply post_rules.
        Rules application may be switched off using `rules=False`.
        """

        evaluate_rules = kws.pop("rules", True)

        # Attempt to wrap the user provided value
        try:
            newval = self._wrap(value, **kws)
        except Exception as err:
            raise ValueError(f"Unable to wrap {value} with {self._wrap} and {kws}") from err

        # Apply rules unless they've been switched off
        if evaluate_rules:
            self._handler.post(self, newval)  # type: ignore

        super().post(newval, **kws)

A fuller implementation would need to catch errors from the post handler but it's not complex in principle to allow .post()s to be evaluated like .put()s. In fact my own implementation pretty much passes .put()s straight to the .post()s with only a check to see if the PV is set to be externally read-only.

Monarda commented 3 months ago

Last one! Apologies my answer has been so long-winded 😞

3. List of Handlers

Currently SharedPV only allows a single handler. It would be nice if it were possible to supply a List or OrderDict of handlers so that handlers could be evaluated in a defined sequence.

Only having a single handler causes two issues. First of all it means that handlers have to be monolithic and handle all the cases they might see rather than allowing each handler to carry out a single task. A handler that deals with Normative Types has to be able to handle controls, valueAlarms, timestamps, etc. instead of a simpler handler for each purpose. The second issue is that it makes it hard to provide a library handler. How does an end user conveniently mix my handler with any custom logic of their own? Our solution has been to create a handler which manages an OrderedDict of handlers. But it might make more sense to standardise this in the library?

The issue with a sequence of handlers is that there is a definite order that makes sense. Usually it's authentication/authorisation handlers, handlers that change values, other handlers, timestamp at the end. It's up to the end user to manage that but it's not clear to me what the best interface to make it easy is?