How to protect attribute write value with critical section?

# 2 years ago
zreszela	Hi Tango dancers! In Sardana project we just tracked the source of a very annoying bug that moves motors to a wrong positions (usually 0) What we basically do is that in the server we have a single worker thread responsible for pushing all change events. Before pushing an event the worker thread updates the attribute write value using the set_write_value() method. This collides with processing the attribute write request since the write value obtained with get_write_value() returns corrupted data (usually at least one zero in case of the spectrum attribute). I imagine that only the call to push_change_event() and processing the attribute write request are protected by the Serialization Monitor, so the arbitrary calls to set_write_value() are not protected. I tried to use my own lock but without success. Do you have any suggestion on how to deal with that? My colleague Miquel Navarro created this reduced example to reproduce this issue: https://github.com/Guspyro/tango-attributes-random-zero. Many thanks in advance for your help! Zibi

# 2 years ago

Hi Tango dancers!

In Sardana project we just tracked the source of a very annoying bug that moves motors to a wrong positions (usually 0)

What we basically do is that in the server we have a single worker thread responsible for pushing all change events. Before pushing an event the worker thread updates the attribute write value using the set_write_value() method.

This collides with processing the attribute write request since the write value obtained with get_write_value() returns corrupted data (usually at least one zero in case of the spectrum attribute).

I imagine that only the call to push_change_event() and processing the attribute write request are protected by the Serialization Monitor, so the arbitrary calls to set_write_value() are not protected.

I tried to use my own lock but without success. Do you have any suggestion on how to deal with that?

My colleague Miquel Navarro created this reduced example to reproduce this issue: https://github.com/Guspyro/tango-attributes-random-zero.

Many thanks in advance for your help!
Zibi

# 2 years ago
Reynald	Hi Zibi, I think you could use the AutoTangoMonitor class. I didn't find any documentation for this class but it seems to be exposed in pyTango. The following seems to work for me: `def backgroundWrite(self): sleep(1.0) # Let the device server initialize correctly while self.runBackgroundWrite: with AutoTangoMonitor(self): self.get_device_attr().get_attr_by_name('Position').set_write_value([1.2, 3.4]) self.debug_stream("Set_write_value [1.2, 3.4]") sleep(0.001)` Rosenberg's Law: Software is easy to make, except when you want it to do something new. Corollary: The only software that's worth making is software that does something new. Edited 2 years ago

# 2 years ago

Reynald

Hi Zibi,

I think you could use the AutoTangoMonitor class.
I didn't find any documentation for this class but it seems to be exposed in pyTango.

The following seems to work for me:


def backgroundWrite(self):
    sleep(1.0) # Let the device server initialize correctly
    while self.runBackgroundWrite:
        with AutoTangoMonitor(self):
            self.get_device_attr().get_attr_by_name('Position').set_write_value([1.2, 3.4])
        self.debug_stream("Set_write_value [1.2, 3.4]")
        sleep(0.001)

Rosenberg's Law: Software is easy to make, except when you want it to do something new.
Corollary: The only software that's worth making is software that does something new.

Edited 2 years ago

# 2 years ago
Reynald	For convenience, I created the following PR in the GIT repository you mentioned. You can see the proposed patch here: https://github.com/Guspyro/tango-attributes-random-zero/pull/1/files Rosenberg's Law: Software is easy to make, except when you want it to do something new. Corollary: The only software that's worth making is software that does something new.

# 2 years ago
guifre.cuni	Thanks Reynald!

# 2 years ago
zreszela	Thanks Reynald! I tried your suggestion with the reduced example and it avoids the data corruption! We will check it with Sardana soon in the systems where we were suffering this issue. Before knowing that we (device classes developers) can take the Tango Serialization Monitor I was thinking on another possible solution to this problem: adding new variants to push_change_event() methods (in cppTango & PyTango) where one could pass the w_value. Just out of curiosity, what do you think about it?

# 2 years ago

zreszela

Thanks Reynald! I tried your suggestion with the reduced example and it avoids the data corruption! We will check it with Sardana soon in the systems where we were suffering this issue.

Before knowing that we (device classes developers) can take the Tango Serialization Monitor I was thinking on another possible solution to this problem: adding new variants to push_change_event() methods (in cppTango & PyTango) where one could pass the w_value. Just out of curiosity, what do you think about it?

# 2 years ago
Reynald	zreszela Before knowing that we (device classes developers) can take the Tango Serialization Monitor I was thinking on another possible solution to this problem: adding new variants to push_change_event() methods (in cppTango & PyTango) where one could pass the w_value. Just out of curiosity, what do you think about it? I think it's a good idea because it's a common use case. Rosenberg's Law: Software is easy to make, except when you want it to do something new. Corollary: The only software that's worth making is software that does something new.

# 2 years ago

Reynald

zreszela
Before knowing that we (device classes developers) can take the Tango Serialization Monitor I was thinking on another possible solution to this problem: adding new variants to push_change_event() methods (in cppTango & PyTango) where one could pass the w_value. Just out of curiosity, what do you think about it?

I think it's a good idea because it's a common use case.

Rosenberg's Law: Software is easy to make, except when you want it to do something new.
Corollary: The only software that's worth making is software that does something new.

# 2 years ago
zreszela	Hi Reynald, I have a doubt regarding fixing it in Sardana, and more preciselly how broad should be the critical section. I think that protecting only call to the set_write_value() is not enough and the critical section should also include call to the push_change_event(). Otherwise, if the worker thread handling the _set_attribute_push() gets preempted (during the context switching) in favor of another thread handing the same attribute write request, then, when the worker thread would be finally resumed the write value would be already modified and incorrect. Is this reasoning correct? Note that the first, option already avoids the wrong moves (when the array were corrupted with zeros). Cheers!

# 2 years ago

zreszela

Hi Reynald,

I have a doubt regarding fixing it in Sardana, and more preciselly how broad should be the critical section. I think that protecting only call to the set_write_value() is not enough and the critical section should also include call to the push_change_event(). Otherwise, if the worker thread handling the _set_attribute_push() gets preempted (during the context switching) in favor of another thread handing the same attribute write request, then, when the worker thread would be finally resumed the write value would be already modified and incorrect. Is this reasoning correct? Note that the first, option already avoids the wrong moves (when the array were corrupted with zeros).

Cheers!

# 2 years ago
Reynald	zreszela Is this reasoning correct? Note that the first, option already avoids the wrong moves (when the array were corrupted with zeros). Cheers! Your reasoning seems correct to me. Rosenberg's Law: Software is easy to make, except when you want it to do something new. Corollary: The only software that's worth making is software that does something new.

# 2 years ago
zreszela	Hi again, Should I create a feature request to cppTango for the new variants of push_change_event()?

# 2 years ago
Reynald	zreszela Hi again, Should I create a feature request to cppTango for the new variants of push_change_event()? Yes, please! Rosenberg's Law: Software is easy to make, except when you want it to do something new. Corollary: The only software that's worth making is software that does something new.