epics-base / p4p

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

wrap method for ntenum does not handle changes to alarm or descriptor #153

Open kurup opened 3 weeks ago

kurup commented 3 weeks ago

I've been looking at implementing NTEnum PVs using p4p and I've run into an issue with setting the alarm and descriptor parts of the struct failing when using a put operation from a client. It looks like this has not been implemented yet in the wrap method and only setting the index or choices is supported. The following change to the wrap method fixes this:

    def wrap(self, value, choices=None, **kws):
        """Pack python value into Value
        """
        V = self.type()
        if choices is not None:
            V['value.choices'] = choices
        if isinstance(value, dict):
            for key in value:
                V[key] = value[key]
        elif isinstance(value, Value):
            for key in value.changedSet():
                V[key] = value[key]
        else:
            # index or string
            self.assign(V, value)

        if V.changed('value.choices'):
            self._choices = V['value.choices']

        return self._annotate(V, **kws)

It'd be good to include this in master if you're happy with this fix.

m2es3h commented 3 weeks ago

I recently had a similar issue when initialising an NTEnum struct (see this PR: https://github.com/epics-base/p4p/pull/154). I think the ideal fix is to make it as similar to the NRScalar wrap() code as possible, something like this:

        if isinstance(value, Value):
            V = value
        elif isinstance(value, ntwrappercommon):
            kws.setdefault('timestamp', value.timestamp)
            V = value.raw
        elif isinstance(value, dict):
            V = self.Value(self.type, value)
        else:
            # index or string (use NTEnum.assign() here, different from NTScalar)
            self.assign(V, value)

        self._annotate(V, **kws)

However, the code above would break the existing functionality of setting index/choices like this initial={'index': N, 'choices': [...]}.