PR to fix #368

[Ingvord]

• memory leak
• push State/Status behavior
  refactor same as in PR to fix #359 #503

[t-b]

@Ingvord Are we now always using C++11?

[Ingvord]

No, this is just an intermediate Progress commit. This PR will be cleaned up before labeled as Ready For Merge

[Ingvord]

Currently it is required to manually set state and/or status before pushing them, see commented lines. Otherwise these values are only send to client and not set in the device i.e. in a few seconds (depending on server side polling rate) client will get an event with an old value:

I have received an event for attribute tango://172.17.0.3:10000/test/debian8/10/Status
Quality = 0
Wed Dec 12 16:42:25 2018 (1544629345,263640 sec) : Status (dim_x = 1, dim_y = 0, w_dim_x = 0, w_dim_y = 0, Data quality factor = VALID, Data format = SCALAR, Data type = DevString)
Element number [0] = some status

I have received an event for attribute tango://172.17.0.3:10000/test/debian8/10/Status
Quality = 0
Wed Dec 12 16:42:34 2018 (1544629354,286715 sec) : Status (dim_x = 1, dim_y = 0, w_dim_x = 0, w_dim_y = 0, Data quality factor = VALID, Data format = SCALAR, Data type = DevString)
Element number [0] = The device is in ON state.

Is this OK?

From API point of view I would add a couple of methods:

push_state_change_event(DevState,...)
//and
push_status_change_event(DevString,...)

[bourtemb]

Currently it is required to manually set state and/or status before pushing them, see commented lines. Otherwise these values are only send to client and not set in the device i.e. in a few seconds (depending on server side polling rate) client will get an event with an old value:

I have received an event for attribute tango://172.17.0.3:10000/test/debian8/10/Status
Quality = 0
Wed Dec 12 16:42:25 2018 (1544629345,263640 sec) : Status (dim_x = 1, dim_y = 0, w_dim_x = 0, w_dim_y = 0, Data quality factor = VALID, Data format = SCALAR, Data type = DevString)
Element number [0] = some status

I have received an event for attribute tango://172.17.0.3:10000/test/debian8/10/Status
Quality = 0
Wed Dec 12 16:42:34 2018 (1544629354,286715 sec) : Status (dim_x = 1, dim_y = 0, w_dim_x = 0, w_dim_y = 0, Data quality factor = VALID, Data format = SCALAR, Data type = DevString)
Element number [0] = The device is in ON state.

Is this OK?

Since DeviceImpl::push_change_event(String, DevState *,...) methods are invoking attr.set_value() or attr.set_value_date_quality(), it would be interesting to invoke set_state() or set_status() at some point in these methods when attr_name is "state" or "status"... or maybe to do it directly in these set_value() or set_value_date_quality() methods when the attribute name is state or status?

From API point of view I would add a couple of methods:

push_state_change_event(DevState,...)
//and
push_status_change_event(DevString,...)

Fine with me.
I would even add:
push_status_change_event(const std::string &)

[Ingvord]

@bourtemb .

Since DeviceImpl::push_change_event(String, DevState *,...) methods are invoking attr.set_value() or attr.set_value_date_quality(), it would be interesting to invoke set_state() or set_status() at some point in these methods when attr_name is "state" or "status"... or maybe to do it directly in these set_value() or set_value_date_quality() methods when the attribute name is state or status?

Frankly speaking, I would not do that. As this introduces yet another if hence some side effect. I am not sure whether in case of non-"State"/"Status" attributes push_ sets the value. Anyway I would keep push_ for pushing into event system while actual set value must be done independently. Correct me if I am wrong but the main use case for a client is to: calculate new value; store it; push it. We can add a new convenience function like: set_and_push_state_change_event and alike

[Ingvord]

This PR changes behavior of push_change_event("state"):

• Before: an event was send
• After: exception - value was not set

Which is IMHO fine, because method push_change_event("state") is basically push_change_event("state", null) where null is an exception that ment to be send. I would suggest to remove default null from the method signature and force clients to use these methods only to send exceptions. May break compatibility though.

[bourtemb]

This PR changes behavior of push_change_event("state"):

* Before: an event was send

* After: exception - value was not set

Which is IMHO fine, because method push_change_event("state") is basically push_change_event("state", null) where null is an exception that ment to be send. I would suggest to remove default null from the method signature and force clients to use these methods only to send exceptions. May break compatibility though.

This is changing the behaviour of DeviceImpl::push_change_event(string, DevFailed*) method which is documented as followed in doxygen:

Push an attribute change event to the Notification service. Should be used to push change events for the state and status attributes as well as pushing an exception as change event.

So I would avoid to change the behaviour of the method if possible.

Is there a way to keep the behaviour of DeviceImpl::push_change_event(string, DevFailed*) and DeviceImpl::push_archive_event(string, DevFailed*) as it was before and also find a way to use the other versions of push_change_event(DevState *, ...) and push-archive_event(DevState *,...) methods with state and status attribute without memory leak?

[bourtemb]

When using push_change_event(DevState *, ...) and push_archive_event(DevState *,...) methods, I think the value provided by the user, as well as the timestamp provided by the user should be taken into account.

About the quality factor. I am not 100% sure of what we should do...
It seems state and status attributes were always considered as a bit different than the other attributes and it seems there were things in the code to force the quality factor of these attributes to VALID.
So, what do we do if the user provided another quality factor for state and status? Do we ignore it?
If we use the quality factor provided by the user, would this break something in the existing SW or the existing SW would simply ignore it?
It seems there are already things you cannot do with these State and Status attributes, for instance, it seems it is not possible to change their label (I tried only with jive).
I think this was done to enforce a standard behaviour for these standard attributes.
I personally think this is useful but in some cases, it would be nice to treat them as normal attributes.
Some users might find it convenient to change their label to have them in another language for instance.
But this is another discussion anyway....

As it is now, when you read a state or status attribute, you always get a VALID quality factor.
So when you push a State event, to be consistent, I think the quality factor should also be forced to VALID but this needs to be documented at least in the methods descriptions in doxygen because this could be confusing for the users.

[Ingvord]

@bourtemb

So when you push a State event, to be consistent, I think the quality factor should also be forced to VALID but this needs to be documented at least in the methods descriptions in doxygen because this could be confusing for the users.

I would put API clarity on top of consistency in this case. When you invoke a method you usually expect certain things, AFAIK you would never expect that your values are ignored. As most of the time you want a certain method due to certain requirements. So it may be perfectly fine to get VALID via read, but push something else. Finally the documentation should not be an excuse to confusing code. The truth is that usually developers do not read docs, until there is a severe issue they fight with...

[andygotz]

I would tend to agree with @Ingvord. We should fix confusing behaviour. The side effects for old code will need to be handled of course. But that can be less of a surprise than non-logical APIs.

[bourtemb]

I would put API clarity on top of consistency in this case. When you invoke a method you usually expect certain things, AFAIK you would never expect that your values are ignored.

I fully agree with you on this point. I don't like that.

As most of the time you want a certain method due to certain requirements. So it may be perfectly fine to get VALID via read, but push something else.

What I don't like in this topic are the inconsistencies... because this also leads to confusions... As it is now, when you read the state attribute, you will always get a VALID quality factor so it is not logical to be able to push events for State attribute with a different quality factor.
Clients listening to events on the state attribute may get a quality factor different than VALID but clients reading the attribute directly will always get a VALID quality factor. And what about the synchronous call during the event subscription? The client will always get a VALID quality factor in this case.

So the big question is: should we allow quality factors different than VALID for state and status attributes? If yes, then I think we should allow it in every situation, and if we don't, we should force the quality factor to VALID in every situation and clearly document this point, not only in doxygen, but also on readthedocs. This could help at least new users which are reading the doc to learn Tango.

Finally the documentation should not be an excuse to confusing code.

I agree with you! But we are in a confused situation and we need to find ways to reduce this confusion...
The problem is that state and status are special attributes... The code is currently handling them in a different way than the other attributes and it looks like this is not well documented, not well known.
So at first sight, it is very confusing for users because State and Status are looking like normal attributes but they actually behave differently in some specific situations. This is historical of course and comes mainly from the fact that State and Status were only commands when Tango was created.
It is probably also a design choice because State and Status do not need quality factors... everything can be described in the status string itself and the state itself is self describing...
Maybe @taurel could comment on this point and share his point of view on this topic if he thinks this could help us to take a decision?

[Ingvord]

Well, we can preserve consistence - in this case push_change_event(string, DevState /DevString, ...) must throw IllegalArgumentException in case "State"/"Status" saying user that these method ignores input values and for "State"/"Status" he must use short form. Then it is 100% clear, State/Status always VALID etc...

[bourtemb]

Well, we can preserve consistence - in this case push_change_event(string, DevState /DevString, ...) must throw IllegalArgumentException in case "State"/"Status" saying user that these method ignores input values and for "State"/"Status" he must use short form. Then it is 100% clear, State/Status always VALID etc...

I like this idea of throwing an IllegalArgumentException.
I would have liked to give the possibility to set the timestamp because I think there are some cases where it could be useful to be able to know precisely when a change of state/status occurred and to be able to record this in an archive database for instance. There might be some hardware devices able to give precise timestamps of when the change of state/status occurred.
I might be wrong but it looks like it is not yet possible for the device server programmer to associate a specific timestamp to state and status attributes values when a client is reading the state and status using read_attribute() or read_attributes() methods. In this case, it looks like, dev_state() and dev_status() methods are called followed by state2attr() or status2attr() methods, which are calling base_state2attr() or base_status2attr() methods which are forcing the timestamp to "now".
Calling att.set_value_date_quality() methods for state and status attributes in dev_state() or dev_status() has currently no effect on the timestamp returned to the client invoking read_attribute() on state or status.

I personally would be in favour of push_change_event(string, DevState /DevString, ...) throwing an IllegalArgumentException if the provided quality factor is different than VALID and give the possibility to the user to send events with specific timestamps.
About the specific timestamps support when invoking read_attribute() methods, I think this is something which could be discussed during a kernel meeting.

[t-b]

@bourtemb I think the question label can be removed or?

[bourtemb]

@bourtemb I think the question label can be removed or?

Right! I remove it.
Here are the minutes of the kernel teleconf where we discussed this:
https://github.com/tango-controls/tango-kernel-followup/blob/master/2019/2019-11-26/Minutes.md#stalled-pull-requests

It was agreed during the meeting to throw an exception if a user is trying to push an event for State or Status using a push_xxx_event method passing a quality factor as parameter. The quality factor for State and Status attributes should always be set to VALID.

It was also agreed that there should be a way to provide a custom state/status value and timestamp when pushing events for State or Status attributes.