genie_python: Simulation mode block values

[mducle]

As a scientist, I would like the IBEX simulation mode to load numerical values from the actual blocks on first starting. This would allow more sophisticated scripts which perform logic based on these values to be simulated.

Acceptance Criteria

• Calling g.cget on a valid block name in simulate mode the first time will yield the actual value / limits of that block rather than a default value (currently 'INITIAL_VALUE').

[mducle]

On MARI there are two instrument-specific functions which read a current block value and computes dependent values (assuming they are numerical) and branches depending on its value. When run in simulate mode, where all blocks are initially set to 'INITIAL_VALUE' (a string) these functions fail with a TypeError. If users include these functions in their script which they test in simulate mode, they might (incorrectly) interpret this error as a problem with their own script.

The two MARI instrument functions are move_changer and set_furnace_temperature which are defined in the files changer.py and mari_routines.py on NDXMARI respectively.

MARI has a paddle-type sample changer with 4 sample positions 90 degrees apart but which should only be rotated in one direction. The input to move_changer is the desired sample index from which it computes a relative rotation from the current position hence it needs to use cget to do this.

In order to protect the heating element in the MARI furnace, two modes of operations should be adhered to. First, when cooling, the power to the heater should not be set immediately to zero (which is what occurs with the PID controller if a simple temperature set point well below the current temperature is set). Second, when heating up, the power to the heater should not be set to the maximum, but rather to an set of intermediate maximum value defined by the current temperature. The set_furnace_temperature function ensures that these restrictions are met, and branches depending on the current furnace temperature, so it needs to cget this.

---

The diff below is a proposed change to the IBEX code to ensure that cget uses the real block value the first time it is run:

--- /babylon/Public/duc/genie_simulate0.py      2020-07-18 13:04:49.733371200 +0100
+++ /babylon/Public/duc/genie_simulate.py       2020-10-05 17:18:31.133035000 +0100
@@ -35,7 +35,7 @@
         from genie_python.genie import _genie_api
         self.genie = genie
         self.previous_api = _genie_api
-        self.new_api = genie_sim.API(None, None, populate_with_current_blocks)
+        self.new_api = genie_sim.API(None, None, populate_with_current_blocks, self.previous_api)
         if populate_with_current_blocks:
             dummy_values = ["INITIAL_VALUE"] * len(self.previous_api.get_block_names())
             self.new_api.set_multiple_blocks(self.previous_api.get_block_names(), dummy_values)
--- /babylon/Public/duc/genie_simulate_impl0.py 2020-10-05 16:49:46.160435300 +0100
+++ /babylon/Public/duc/genie_simulate_impl.py  2020-10-05 17:18:58.758677100 +0100
@@ -920,7 +920,7 @@
     __inst_prefix = None
     pre_post_cmd_manager = PrePostCmdManager()
 
-    def __init__(self, pv_prefix=None, globs=None, strict_block=True):
+    def __init__(self, pv_prefix=None, globs=None, strict_block=True, real_api=None):
         """
         Constructor for the simulated API.
 
@@ -939,6 +939,7 @@
         self.beamline_pars = {}
         self.sample_pars = {}
         self.strict_block = strict_block
+        self.real_api = real_api if not isinstance(real_api, type(self)) else None
 
     def set_instrument(self, pv_prefix, globs, import_instrument_init):
         API.__inst_prefix = pv_prefix
@@ -1009,14 +1010,18 @@
             self.waitfor.start_waiting(block=name, value=value)
 
     def get_block_data(self, block, fail_fast=False):
-        ans = OrderedDict()
-        ans['connected'] = True
-
-        ans['name'] = block
-        ans['value'] = self.block_dict[block].get('value', None)
-        ans['runcontrol'], ans['lowlimit'], ans['highlimit'] = self.get_runcontrol_settings(block)
-
-        ans['alarm'] = self.get_alarm_from_block(block)
+        current_value = self.block_dict[block].get('value', None)
+        if current_value == 'INITIAL_VALUE' and self.real_api is not None:
+            real_data = self.real_api.get_block_data(block)
+            ans = OrderedDict([(parname, parval) for parname, parval in real_data.items()])
+        else:
+            ans = OrderedDict()
+            ans['connected'] = True
+            ans['name'] = block
+            ans['value'] = current_value
+            ans['runcontrol'], ans['lowlimit'], ans['highlimit'] = self.get_runcontrol_settings(block)
+            ans['alarm'] = self.get_alarm_from_block(block)
+            
         return ans
 
     def set_multiple_blocks(self, names, values):

[DominicOram]

Thanks @mducle, we're still having a think how to incorporate patches like this into our workflow and I'll let you know when we do but for the time being I'll review this rather than put it in proposal, just as it will be quicker.

This solution works but I think a better solution here would be to add an additional optional initial_values dictionary argument to the Simulate constructor, which we could then use instead of INITIAL_VALUE, only falling back on this if a value hasn't been supplied. This would require a little more effort on the part of the user but provides greater flexibility in that the user can choose what initial values they would like (and could do the cget themselves if they definitely want the real value for some blocks). It also means that the simulate mode still remains loosely coupled with what is currently going on on the instrument. Does that sound like a good solution to you?

If you'd like to implement that solution then feel free to create a PR on the genie_python repository (you should have the correct permissions to do this) and I'll review it again. If you want us to do it then add the proposal label back on and we'll look at prioritising it with the rest of our work.

[mducle]

@DominicOram I've opened a PR to the genie_python repo to implement your suggestion.

The downside as you say is that users have to know which block values need to be initialised in advance. If it's in their own scripts that's fine; if a block is read in a dependent script (e.g. an inst script) they might not know about it. I'll just change it so that on MARI simulate always initialises the Changer and the set (T_1, T_2, Furnace_heater, Max_power) used in MARI specific instrument scripts which would be imported by users.

[JamesKingWork]

Project board checks is complaining about this being a 0 point ticket, should it be 0 points?

[DominicOram]

Probably not, my bad, changed it to 1 point