Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cannot change alarm range for memorized attribute #401

Open
tiagocoutinho opened this issue Oct 2, 2017 · 7 comments · May be fixed by #692
Open

Cannot change alarm range for memorized attribute #401

tiagocoutinho opened this issue Oct 2, 2017 · 7 comments · May be fixed by #692
Assignees
Labels
Milestone

Comments

@tiagocoutinho
Copy link
Collaborator

@tiagocoutinho tiagocoutinho commented Oct 2, 2017

Trying to change the alarm range for a memorized attribute gives the following exception:

alarm_min_max

Note that the current set_value is 1.2 and the user is trying to change the max_alarm to 1.5.

  • so, the feature seems to be buggy since it should be possible to change the alarm in this situation
  • I think it should always be possible to change the alarm/warning/value ranges of an attribute. So, IMHO, the feature should simply be dropped.
@bourtemb bourtemb added the bug label Oct 2, 2017
@bourtemb

This comment has been minimized.

Copy link
Member

@bourtemb bourtemb commented Oct 2, 2017

I agree it should always be possible to change the alarm and warning min/max values.
This is a bug which will be fixed.
For the range min and max, I think we can open a discussion on that. What do you do in the case where the current set value is above the new max value? This should never happen. How would you handle this case?

@andygotz

This comment has been minimized.

Copy link
Collaborator

@andygotz andygotz commented Oct 25, 2017

I think it should be possible to change the range even if the current value is out of the new range. But it would be good to indicate this with an alarm for example.

@Ingvord Ingvord added this to the Q1'2018 milestone Feb 5, 2018
@mliszcz

This comment has been minimized.

Copy link
Collaborator

@mliszcz mliszcz commented Dec 18, 2019

Hi all,

The issue here is that we apply the same check for changing both max_alarm and max_value:

else if (strcmp(prop_name,"max_value") == 0 || strcmp(prop_name,"max_alarm") == 0)
{
if ((w_att->is_memorized() == true) && (w_att->mem_value_below_above(MAX,mem_value) == true))
throw_min_max_value(d_name,mem_value,MAX);
}

And mem_value_below_above is always comparing current value to max_value:

if (double_array_val[i] > max_value.db)
{
ss << double_array_val[i];
ret_mem_value = ss.str();
ret = true;
break;
}

In a scenario where max_value is not specified (0), some value is memorized (1.2 in original post), and one tries to change max_alarm to any value (e.g. 1.5), change is rejected and exception (with misleading error message) is thrown because 1.2 > 0:

if (double_array_val[i] > max_value.db)

I guess that author's intention was to reject max_alarm change when set value (memorized) is larger than the new max_alarm (not max_value), because attribute will immediately go into ALARM state.

We have following possibilities:

  1. Correct the code to check against max_alarm if user is changing max_alarm - this will fix issue described by Tiago
    if (double_array_val[i] > max_value.db)
  2. Drop the check for max_alarm change - as Reynald proposes
    else if (strcmp(prop_name,"max_value") == 0 || strcmp(prop_name,"max_alarm") == 0)
  3. Also drop above check for max_value - this is my understanding of Andy's proposal. But then we may write some out-of-range values to the attribute, possibly damaging the hardware.

Please share your opinions how you'd like this to be addressed.

@mliszcz mliszcz moved this from To Do to In Progress in Tango 9 Long Term Support Dec 18, 2019
@mliszcz mliszcz self-assigned this Dec 18, 2019
@bourtemb

This comment has been minimized.

Copy link
Member

@bourtemb bourtemb commented Dec 18, 2019

The issue here is that we apply the same check for changing both max_alarm and max_value:

else if (strcmp(prop_name,"max_value") == 0 || strcmp(prop_name,"max_alarm") == 0)
{
if ((w_att->is_memorized() == true) && (w_att->mem_value_below_above(MAX,mem_value) == true))
throw_min_max_value(d_name,mem_value,MAX);
}

And mem_value_below_above is always comparing current value to max_value:

if (double_array_val[i] > max_value.db)
{
ss << double_array_val[i];
ret_mem_value = ss.str();
ret = true;
break;
}

In a scenario where max_value is not specified (0), some value is memorized (1.2 in original post), and one tries to change max_alarm to any value (e.g. 1.5), change is rejected and exception (with misleading error message) is thrown because 1.2 > 0:

It is clear that if max_value is not specified, this check should not occur in any case and max_value should not be considered as equal to 0.

if (double_array_val[i] > max_value.db)

I guess that author's intention was to reject max_alarm change when set value (memorized) is larger than the new max_alarm (not max_value), because attribute will immediately go into ALARM state.

I think the attribute immediately going to ALARM state is not an issue (and might be what the user actually wants in some situations) and I'm pretty sure this was not the reason behind this choice.
@taurel , any memories about this part of the code?

We have following possibilities:

1. Correct the code to check against `max_alarm` if user is changing `max_alarm` - this will fix issue described by Tiago
   https://github.com/tango-controls/cppTango/blob/99bbaa4b5ba4740e4666caf0f7ebc6775f12b799/cppapi/server/w_attribute.cpp#L3440
2. Drop the check for max_alarm change - as Reynald proposes
   https://github.com/tango-controls/cppTango/blob/99bbaa4b5ba4740e4666caf0f7ebc6775f12b799/cppapi/server/attrgetsetprop.cpp#L1328

I think we should drop this check for max_alarm change indeed.

3. Also drop above check for max_value - this is my understanding of Andy's proposal. But then we may write some out-of-range values to the attribute, possibly damaging the hardware.

I think we will not write out of range values to the hardware in any case.
Tango will protect us against that.
At the time when a new max_value is set, if the current set point is above the new max_value (and in any case), Tango does not write this attribute again. Tango will try to write it when the device is restarted if the attribute is memorized with write hardware at init. But then Tango will forbid this write_attribute because the memorized set value is above the max_value, so nothing should be sent to the hardware.
This is a bit annoying for the user because the device won't be initialized correctly and will report an error with the memorized attribute.
The user will then have to change manually the __value attribute property and restart the device again. Writing the attribute once with a value in the correct range and restart the device might be ok too, but might not be possible in some situations if the device initialization failed.
I think the check when setting max_value in Tango is there to avoid this burden to the user and to warn him immediately that he is doing something inconsistent with the current situation.

Please share your opinions how you'd like this to be addressed.

I am in favour of dropping the check when setting the max_alarm attribute config param.
For the max_value case, I agree with Andy it could be useful to accept it all the time too but then, the users might encounter some issues at the next device restart.

@taurel, what's your opinion?

@mliszcz mliszcz added the question label Feb 3, 2020
@mliszcz

This comment has been minimized.

Copy link
Collaborator

@mliszcz mliszcz commented Feb 3, 2020

@bourtemb do you think we should ask the users / community? Or shall I proced with implementing as you suggested?:

I am in favour of dropping the check when setting the max_alarm attribute config param.
For the max_value case, I agree with Andy it could be useful to accept it all the time too but then, the users might encounter some issues at the next device restart.

@bourtemb

This comment has been minimized.

Copy link
Member

@bourtemb bourtemb commented Feb 3, 2020

@bourtemb do you think we should ask the users / community? Or shall I proced with implementing as you suggested?:

I am in favour of dropping the check when setting the max_alarm attribute config param.
For the max_value case, I agree with Andy it could be useful to accept it all the time too but then, the users might encounter some issues at the next device restart.

It would be nice to get some input from the community on this topic.

@mliszcz

This comment has been minimized.

Copy link
Collaborator

@mliszcz mliszcz commented Feb 13, 2020

Summary of discussion from today's Tango Kernel teleconf meeting:

  • We will not fix: "memorized value (1.2) is above the new limit (1.5)" (which is a bug due to using wrong variable and max_value not being specified) - see first bullet in #401 (comment)
  • We will allow changing alarm thresholds (min/max_alarm) anytime regardless of current setpoint and memorized value (this will obsolete/fix the point above)
  • We will not change any checks and behavior related to changing ranges (min/max_value)
mliszcz added a commit to mliszcz/cppTango that referenced this issue Mar 13, 2020
Test for changing min and max alarm threshold of a memorized attribute
with neither min_ nor max_value specified. Such scenario is reported to
fail (tango-controls#401), with misleading error message claiming that memorized value
is above new limit.
@mliszcz mliszcz linked a pull request that will close this issue Mar 13, 2020
mliszcz added a commit to mliszcz/cppTango that referenced this issue Mar 16, 2020
Do not check if min or max value threshold is exceeded when changing
alarm threshold. This feature was not working as intended and it was
agreed that it shall be removed. Fixes tango-controls#401.
mliszcz added a commit to mliszcz/cppTango that referenced this issue Mar 18, 2020
Test for changing min and max alarm threshold of a memorized attribute
with neither min_ nor max_value specified. Such scenario used to fail
(tango-controls#401), with misleading error message claiming that memorized value
is above new limit.
mliszcz added a commit to mliszcz/cppTango that referenced this issue Mar 18, 2020
Do not check if min or max value threshold is exceeded when changing
alarm threshold. This feature was not working as intended and it was
agreed that it shall be removed. Fixes tango-controls#401.
@mliszcz mliszcz moved this from In Progress to To Be Reviewed in Tango 9 Long Term Support Apr 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Tango 9 Long Term Support
  
To Be Reviewed
Linked pull requests

Successfully merging a pull request may close this issue.

5 participants
You can’t perform that action at this time.