Problems for axis attributes with same name but different properties

This might be the same as reported here: https://github.com/sardana-org/sardana/issues/39

The problem arises when for example two Motor controllers define an axis attribute with the same name, but with different properties. When the second controller is set up during Pool start, this somehow messes up the first one.

These two PseudoMotor controllers can be used to reproduce the problem:

from sardana import pool, DataType
from sardana.pool import PoolUtil
from sardana.pool.controller import PseudoMotorController, Memorized, Description, DefaultValue, Type, Access, DataAccess

class BuggyA(PseudoMotorController):
    """
    Bugtest A
    """
    pseudo_motor_roles = ("bb",)
    motor_roles = ("aa",)

    #same error with the older ctrl_extra_attributes
    axis_attributes = { #order of the arttributes isn't important, but can't cause bug with less than three
                            "OffendingValue": #causes trouble if different types, or different R / RW
                                 {Type: DataType.Double,
                                  Access: DataAccess.ReadWrite,
                                  },
                            "BestValueA":
                                 {Type: DataType.Double,
                                  Access: DataAccess.ReadWrite,
                                  },

                            "WorstValue": #must be read-only, can have different names
                                 {Type: DataType.Double,
                                  Access: DataAccess.ReadOnly,
                                  },
                             }

    def __init__(self, inst, props):
        PseudoMotorController.__init__(self, inst, props)
        self.ccc = 0
        self.ddd = 0
        self.eee = 0

    def GetAxisExtraPar(self, axis, name):
        print("get AAAAAAAAAAAAAAAAAAAAAAAAA")
        if name.lower() == "bestvaluea":
            return self.ccc
        if name.lower() == "offendingvalue":
            return self.ddd
        if name.lower() == "worstvalue":
            return self.eee

    def SetAxisExtraPar(self, axis, name, value):
        print("set AAAAAAAAAAAAAAAAAAAAAAAAA")
        if name.lower() == "bestvaluea":
            self.ccc = value
        if name.lower() == "offendingvalue":
            self.ddd = value
        if name.lower() == "worstvalue":
            self.eee = value

class BuggyB(PseudoMotorController):
    """
    Bugtest B
    """
    pseudo_motor_roles = ("bb",)
    motor_roles = ("aa",)

    axis_attributes = {
                            "OffendingValue":
                                 {Type: DataType.Double,
                                  Access: DataAccess.ReadOnly,
                                  },
                            #"BestValue":
                            #     {Type: DataType.Double,
                            #      Access: DataAccess.ReadWrite,
                            #      },
                         #
                            #"WorstValue2":
                            #     {Type: DataType.Double,
                            #      Access: DataAccess.ReadOnly,
                            #      },
                            }

    def __init__(self, inst, props):
        PseudoMotorController.__init__(self, inst, props)
        self.c = 0
        self.d = 0
        self.e = 0

    def GetAxisExtraPar(self, axis, name):
        print("get BBBBBBBBBBBBBBBBBBBBBBBB")
        if name.lower() == "bestvalue":
            return self.c
        if name.lower() == "offendingvalue":
            return self.d
        if name.lower() == "worstvalue2":
            return self.e

    def SetAxisExtraPar(self, axis, name, value):
        print("set BBBBBBBBBBBBBBBBBBBBBBBB")
        if name.lower() == "bestvalue":
            self.c = value
        if name.lower() == "offendingvalue":
            self.d = value
        if name.lower() == "worstvalue2":
            self.e = value

Start with an empty Pool, with just a dummy motor or two. Then add a BuggyA:

defctrl BuggyA a_ctrl aa=mot01 bb=mota

And a BuggyB:

defctrl BuggyB b_ctrl aa=mot01 bb=motb

Restart the Pool (not sure if this is really required)

Then write to BestValueA:

Door_dummy_1 [15]: mota.BestValueA = 3
PyTango_WriteAttributeMethodNotFound: write_WorstValue method not found for BestValueA
(For more detailed information type: tango_error)

Not that it is looking for the wrong write method (write_WorstValue).

I believe something bad happens when the second BuggyB controller calls remove_unwanted_dynamic_attributes https://github.com/sardana-org/sardana/blob/76a8cf344b7fb5233c35e35a4a8429f721d34655/src/sardana/tango/pool/PoolDevice.py#L216 This seems to somehow affect the previously created BuggyA controller.

This is causing quite serious issues for us when there are several types of Motor controllers at a beamline.

First, many thanks to @HEnquist for this comprehensive bug report. Indeed it looks like the same thing as #39.

As, together with @HEnquist, we have advanced with the investigation using other channels now is the moment to share the results with the rest of the community.

As you know, Sardana heavily uses very dynamic attributes. By very I mean that we even allow attributes with the same name with different characteristics (type, format, writable) for different device instances of the same class. Something what seems to be not possible with Tango when you keep these devices in the same device server. When you move them to a separate device server the limitation does not apply. Currently Sardana does a trick and removes the attribute from an internal list of attributes of a Tango device class before adding a conflicting attribute in order to avoid exceptions comming from the Tango library. This trick works quite well, but is not perfect - usually problems appear when changing the writable charactaristic. @HEnquist opened this issue https://github.com/tango-controls/cppTango/issues/814 to ask if Tango would eventually fully accept device level dynamic attributes as it is done for commands.

Here I list which features of Sardana rely on this:

axis attributes: any controller can define axis attribute with different characteristic with an arbitrary name which eventually could collide with other attribute from other controller
controller attributes: the same as above but for controller attribute
position attribute of moveables: e.g. discreate pseudo motor
value attribute of experimental channels: e.g. pseudo counters with different format

With @HEnquist we have not seen an easy solution in Sardana, without redesigning a lot of concepts, in case Tango does not accept this featuer request. Unless you see an obvious workaround let's wait for an official answer from Tango before starting to brainstorm on what we could do.

Just to keep you all updated, today we had a meeting with the Tango experts and they accepted the idea of dynamic attributes on the device level. Some of the argumentes mentioned were:

be coherent with the current implementation for commands
be coherent with the Tango Java implementation
it makes sense in some occassions to relax the OOP principles e.g. calculation devices or Sardana case

The idea is to work on a PR for cppTango and target the 9.4 release (mid 2021). @HEnquist and myself plan to start work on it in the following weeks. If you want to help, just let us know:)

Thanks to all!

@HEnquist, I wonder what should we do with this issue. The limitation is actually in Tango. I don't have in mind any workaround we could do in Sardana. Maybe just raise an early exception on the controller load/initialization time instead of failing when accessing the attribute. What do you think? Many thanks!

With @cmft we found a related issue when two controller classes define the same attribute but using different letter casing e.g. "FORMULA" and "Formula". Note that both attributes have the same characteristics, both are: Type: str and Access: DataAccess.ReadWrite. The issue appears when we define a new axis of one of these controllers:

SardanaTP.W001 DEBUG    2021-08-05 11:38:44,744 Door/zreszela/1.MacroExecutor: [START] runMacro Macro 'defelem(tangoattr0402, tangoattrctrl04, 2) -> ed3ddbae-f5d0-11eb-bfce-50eb7137bebe'
SardanaTP.W001 DEBUG    2021-08-05 11:38:44,771 expchan/tangoattrctrl04/2: -> init_device
SardanaTP.W001 DEBUG    2021-08-05 11:38:44,774 Controller.tangoattrctrl04: AddDevice 2
SardanaTP.W001 DEBUG    2021-08-05 11:38:44,780 expchan/tangoattrctrl04/2: <- init_device
SardanaTP.W001 WARNING  2021-08-05 11:38:44,786 expchan/tangoattrctrl04/2: Error removing dynamic attribute shape
SardanaTP.W001 DEBUG    2021-08-05 11:38:44,786 expchan/tangoattrctrl04/2: Details:
Traceback (most recent call last):
  File "/homelocal/zreszela/workspace/sardana/src/sardana/tango/pool/PoolDevice.py", line 251, in remove_unwanted_dynamic_attributes
    self.remove_attribute(attr_name)
  File "/homelocal/zreszela/miniconda3/envs/sardana/lib/python3.9/site-packages/tango/device_server.py", line 398, in __DeviceImpl__remove_attribute
    self._remove_attribute(attr_name)
PyTango.DevFailed: DevFailed[
DevError[
    desc = Device --> a/b/c
           Property writable_attr_name for attribute FORMULA is set to FORMULA, but these two attributes do not support the same data type
  origin = MultiAttribute::check_associated
  reason = API_AttrOptProp
severity = ERR]

DevError[
    desc = Attribute Shape is not defined as attribute for your device.
           Can't remove it
  origin = DeviceImpl::remove_attribute
  reason = API_AttrNotFound
severity = ERR]
]
SardanaTP.W001 WARNING  2021-08-05 11:38:44,789 expchan/tangoattrctrl04/2: Error removing dynamic attribute formula
SardanaTP.W001 DEBUG    2021-08-05 11:38:44,790 expchan/tangoattrctrl04/2: Details:
Traceback (most recent call last):
  File "/homelocal/zreszela/workspace/sardana/src/sardana/tango/pool/PoolDevice.py", line 251, in remove_unwanted_dynamic_attributes
    self.remove_attribute(attr_name)
  File "/homelocal/zreszela/miniconda3/envs/sardana/lib/python3.9/site-packages/tango/device_server.py", line 398, in __DeviceImpl__remove_attribute
    self._remove_attribute(attr_name)
PyTango.DevFailed: DevFailed[
DevError[
    desc = Device --> a/b/c
           Property writable_attr_name for attribute FORMULA is set to FORMULA, but these two attributes do not support the same data type
  origin = MultiAttribute::check_associated
  reason = API_AttrOptProp
severity = ERR]

DevError[
    desc = Attribute FORMULA is not defined as attribute for your device.
           Can't remove it
  origin = DeviceImpl::remove_attribute
  reason = API_AttrNotFound
severity = ERR]
]
fish: Job 1, 'Sardana zreszela --log-level=de…' terminated by signal SIGSEGV (Address boundary error)

We found this issue between the AlbaEm2CoTiController and TangoAttributeCTController.

@HEnquist, I wonder what should we do with this issue. The limitation is actually in Tango. I don't have in mind any workaround we could do in Sardana. Maybe just raise an early exception on the controller load/initialization time instead of failing when accessing the attribute. What do you think? Many thanks!

Sorry didn't see this! I also don't think there is any reasonable way to work around this in Sardana. I'm working on the needed changes in Tango but it's difficult to get time for it. Once that is done, then pyTango needs a small update. And finally after than, Sardana needs to add one argument to create the tango attributes as device-level.

Since the current workaround of removing and re-adding attributes causes difficult-to-diagnose trouble, it could definitely be a good idea to stop doing it. So then when a new attribute doesn't match the definition already in the class, just don't add the attribute, and give a clear warning. This would be easy to implement, but could cause trouble in cases where the current approach happens to work. I can make a quick MR for that, could make it easier to discuss pros and cons.

Since the current workaround of removing and re-adding attributes causes difficult-to-diagnose trouble, it could definitely be a good idea to stop doing it. So then when a new attribute doesn't match the definition already in the class, just don't add the attribute, and give a clear warning. This would be easy to implement, but could cause trouble in cases where the current approach happens to work. I can make a quick MR for that, could make it easier to discuss pros and cons.

If I remember correctly, there are at least two cases when it does not work:

two attributes of the same name and equal letter casing e.g. Formula and Formula, one with Access: DataAccess.ReadWrite and another one with Access: DataAccess.ReadOnly
two attributes of the same name but different letter casing e.g. Formula and FORMULA, both with the same characteristics.

Maybe we could start with raising an exception for these two cases?

Maybe we could start with raising an exception for these two cases?

That seems like a good idea.

I'm thinking if perhaps we should remove self.remove_unwanted_dynamic_attributes() from here: https://github.com/sardana-org/sardana/blob/develop/src/sardana/tango/pool/PoolDevice.py#L204 and instead put the self.add_dynamic_attribute() call in a try-except, and handle these cases there.

This problem does not happen on old system (sardana 2.8 and Tango8).

sardana-org / sardana

Problems for axis attributes with same name but different properties #1436