ISISComputingGroup / IBEX

Top level repository for IBEX stories
5 stars 2 forks source link

genie_python/DAE: Fix inconsistent period PV alarm #5075

Open Alexandru-Dascalu opened 4 years ago

Alexandru-Dascalu commented 4 years ago

The bug consists of inconsistent setting of the period number and the max number of periods, where if you set the number of period to a number it appears to succeed but later if you try to set the period to a number larger than the previous number of periods but smaller than the current one it will crash.

Acceptance Criteria

Notes

As a result of https://github.com/ISISComputingGroup/IBEX/issues/4992, genie python now shows an exception to the screen when it can not set the total number of periods using g.change_number_soft_periods () or the period number using g.change_period().

The exception is thrown based whether or not the appropriate DAE PV (DAE:HARDWAREPERIODS:SP or DAE:PERIOD:SP) is in an INVALID alarm after you have set the pv to the new value.

Changing the period number should succeed unless the argument is 0 or less or unless it is larger than the total period count. Therefore, if you set the number of periods to 1000 and then try to set the period to 21, it should work. Instead, genie python throws an exception:

g.change_number_soft_periods(20)
g.change_period(13)
g.change_number_soft_periods(1000)
g.change_period(19)
g.change_period(20)
g.change_period(21)
Traceback (most recent call last):
  File "C:\Instrument\Apps\Python\lib\site-packages\genie_python\genie.py", line 1852, in change_period
    __api.dae.set_period(period)
  File "C:\Instrument\Apps\Python\lib\site-packages\genie_python\genie_dae.py", line 574, in set_period
    raise IOError("You are trying to set an invalid period number! The number must be between 1 and the "
IOError: You are trying to set an invalid period number! The number must be between 1 and the number of periods!
g.change_period(999)
Traceback (most recent call last):
  File "C:\Instrument\Apps\Python\lib\site-packages\genie_python\genie.py", line 1852, in change_period
    __api.dae.set_period(period)
  File "C:\Instrument\Apps\Python\lib\site-packages\genie_python\genie_dae.py", line 574, in set_period
    raise IOError("You are trying to set an invalid period number! The number must be between 1 and the "
IOError: You are trying to set an invalid period number! The number must be between 1 and the number of periods!

As you can see, the DAE seems to remember the old number of periods values and not actually update to 1000, making setting the period later fail.

Interestingly, if you try the above commands in a scripting console in the GUI one by one you will see that although trying to set the period to 21 or 999 throws an exception, the period number will change to the new value in the dashboard. If you open an EPICS terminal you can see that the setpoint PV is in an INVALID alarm, but that the readback was also changed to the new value. However, if you try to set the period to 1001, it will throw an exception and the dashboard period number will stay the same, and the readback PV will not change.

Freddie said that this issue might be because the DAE hardware has one period limit, and the software has a different limit, so in this case the operation fails in one place but succeeds in the other so this leads to an inconsistent error. If you try to change the number of periods to an even larger number like 1000000 then it will throw an error from the very beginning.

Alexandru-Dascalu commented 4 years ago

As of now, https://github.com/ISISComputingGroup/IBEX/issues/4992 has not been completed yet, so in order to see the problem in the GUI as I have described it you need to checkout the https://github.com/ISISComputingGroup/genie_python/tree/Ticket4992_raise_invalid_period_count_exception branch.

Alexandru-Dascalu commented 4 years ago

When fixing this bug, we should also add a system test that checks that for medium total number of periods like 1000, change_number_soft_periods and change_period behave correctly.

John-Holt-Tessella commented 4 years ago

pair with @FreddieAkeroyd