ALBA-Synchrotron / sardana-tango

Repository for Sardana Tango plugins (TangoAttributeCtrls, etc)
GNU General Public License v3.0
0 stars 3 forks source link

TangoAttrIORController ReadOne returns `None` on invalid position #15

Closed marceloalcocer closed 9 months ago

marceloalcocer commented 1 year ago

The TangoAttrIORController.ReadOne method returns None on invalid (out-of-bounds) values of the underlying Tango attribute. This return value does not conform to the Sardana API for Readable controllers (see e.g. SEP6).

More specifically, TangoAttrIORController.ReadOne asserts the underlying Tango attribute value is within the calibrated bounds, and raises an exception on assertion failure. This exception is subsequently caught, but no value is returned or exception raised in the handler.

The ultimate result of this is that the pool raises a ValueError (with generic message) when attempting to read the value of an IOR device whose underlying Tango attribute is our of bounds, e.g.;

>>> ior = DeviceProxy("my/tango/ior")
>>> dev = DeviceProxy("my/tango/device")

>>> ior.tangoattribute
'my/tango/device/attribute'
>>> dev.attribute
100.0

>>> ior.calibration
'[[0.0, 1.0, 2.0], [10.0, 20.0, 30.0]]'
>>> ior.value
PyDs_PythonError: ValueError: my_tango_ior_ctrl.ReadOne(my_tango_ior[1]) return error: Expected value(s), got None instead

As the original exception is not re-raised, its relevant explanatory message ("Invalid position.") is suppressed. Although currently suppressed, this message itself could also be improved, e.g.;

ovallcorba commented 10 months ago

Hi @marceloalcocer

Thanks for creating the issue. You are right that the error handling and the output messages can be improved to reflect better why the values are not valid. I created a PR (https://github.com/ALBA-Synchrotron/sardana-tango/pull/17) following your suggestions to improve them.

Best regards.

marceloalcocer commented 10 months ago

Hej @ovallcorba

Thanks for taking the time to look into this and crafting a PR :+1:

17 LGTM and should fix the issue