Experimental Physics and
Industrial Control System

"J. Lewis Muir" <[email protected]> · Mon, 09 Jan 2012 23:51:23 -0000

> Hi Lewis,
> 
> You can't get your diff to be clean because you've been developing against the
> 3.14 branch of Base, but you're proposing a merge into 3.15 which is currently
> missing 4 revisions from the 3.14 branch (those are the additional changes
> that are appearing in your proposal).  Don't worry about them for now, it's
> not hard to ignore those changes since they have nothing to do with the iocsh
> code.

Hi, Andrew.

OK, so I intended to propose a merge into 3.14 but failed to correctly do that.  Here's what I did:

1. $ bzr checkout lp:epics-base/3.14 mirror-3.14
2. $ bzr branch mirror-3.14 iocsh-if-flow-control-3.14
3. $ cd iocsh-if-flow-control-3.14
4. (Implemented the if-statement feature)
5. $ bzr commit
6. $ bzr launchpad-login jlmuir
7. $ bzr push lp:~jlmuir/epics-base/iocsh-if-flow-control-3.14
8. Click "Propose for merging" link in the view of my pushed branch

So I guess my mistake was in step 7 where lp:~jlmuir/epics-base is actually pointing to the 3.15 branch?  To have done this correctly, I think I would have needed to make step 7 the following:

7. $ bzr push lp:~jlmuir/epics-base/3.14/iocsh-if-flow-control-3.14

Is that correct?

> Unfortunately this proposal bothers me for several reasons:
> 
>  1. The current iocsh.cpp code is pretty clean and simple to understand.  Your
> changes add quite a lot of complication to handle the ability to skip the
> commands in non-active conditional sections.  I'm not saying your code is bad,
> that complexity is inevitable if we want to implement conditional execution to
> iocsh.

There's probably not much I can say in response to this reason.  I'm sure you'd agree with me that, usually, adding a feature means adding more code.  My feeling is that if it's done in a good way and the code is easy to understand (and hence easy to maintain), then it's OK.  So maybe it just comes down to whether the feature is considered useful.  If I could do something to make it more likely to be accepted, I'm open to that.

>  2. It is relatively easy to execute external scripts conditionally with the
> existing code.  A construct like this
>     < pilatus-$(PILATUS_ENABLED).cmd
> requires you to put the conditional code in a script named pilatus-YES.cmd and
> to have an empty file named pilatus-NO.cmd.  It is even possible to include
> the conditional code inline if you can choose the conditional variable values;
> if you make them empty or just contain spaces when true, and have the string
> "#" when they're false, then you just do this:
>     $(PILATUS_ENABLED) dbpf("x7PIL1:det1:TriggerMode","Ext. Trigger")
> There are similar options using the $(VAR=DEFAULT) feature of macros; if the
> variable is undefined when false and can be empty (or just contain spaces)
> when true you can use this:
>     $(PILATUS_ENABLED=#) dbpf("x7PIL1:det1:TriggerMode","Ext. Trigger")

My feeling is that the above is a kludge.  Why try to hack a .cmd file to do what I want when I can add support for an if-statement and have it do what I want cleanly?

The "< pilatus-$(PILATUS_ENABLED).cmd" style is weak in that it can violate the symmetry of my .cmd file.  What I mean by this is that I might have very specific calls in st.cmd, for example.  And then as I'm reading it to see what it does I come across "< pilatus-$(PILATUS_ENABLED).cmd".  I now have to find that file and look at it to see what is happening.  If I had an if-statement, I could simply have the specific commands in the st.cmd file and very easily see exactly what happens when reading through st.cmd without having to start looking at other files.  I'm not saying everything needs to be in one st.cmd file, but hopefully you can see how it is useful to have everything together in some situations rather than split into different files.  Readability is important because it has a direct correlation to maintainability and productivity.

The '$(PILATUS_ENABLED=#) dbpf("x7PIL1:det1:TriggerMode","Ext. Trigger")' style is weak in that it makes lines even longer, is less readable, and can only handle Boolean cases.

Here's an example using the if-statement:

===

if ${PILATUS_ENABLED} = YES
  #
  # Configure asyn port for communicating with PILATUS camserver
  #
  drvAsynIPPortConfigure("camserver","10.211.55.17:41234")
  asynOctetSetInputEos("camserver",0,"\030")
  asynOctetSetOutputEos("camserver",0,"\n")

  #
  # pilatusDetectorConfig(const char *portName, const char *camserverPort,
  #     int maxSizeX, int maxSizeY, int maxBuffers, size_t maxMemory,
  #     int priority, int stackSize)
  #   portName       driver asyn port name
  #   camserverPort  server asyn port name
  #   maxSizeX       size of PILATUS detector in X direction
  #   maxSizeY       size of PILATUS detector in Y direction
  #   maxBuffers     maximum number of buffers for NDArrayPool
  #   maxMemory      maximum amount of memory in bytes for NDArrayPool
  #   priority       thread priority for asyn port driver thread if
  #                  ASYN_CANBLOCK is set in asynFlags
  #   stackSize      stack size for asyn port driver thread if ASYN_CANBLOCK is
  #                  set in asynFlags
  #
  pilatusDetectorConfig("PIL1", "camserver", 2463, 2527, 50, 200000000)
  dbLoadRecords("$(AREA_DETECTOR)/ADApp/Db/ADBase.template", "P=x7PIL1:,R=det1:,PORT=PIL1,ADDR=0,TIMEOUT=1")
  dbLoadRecords("$(AREA_DETECTOR)/ADApp/Db/NDFile.template", "P=x7PIL1:,R=det1:,PORT=PIL1,ADDR=0,TIMEOUT=1")
  dbLoadRecords("$(AREA_DETECTOR)/ADApp/Db/pilatus.template","P=x7PIL1:,R=det1:,PORT=PIL1,ADDR=0,TIMEOUT=1,CAMSERVER_PORT=camserver")
endif

===

And here's the same thing with the "${PILATUS_ENABLED=#}" style:

===

#
# Configure asyn port for communicating with PILATUS camserver
#
${PILATUS_ENABLED=#} drvAsynIPPortConfigure("camserver","10.211.55.17:41234")
${PILATUS_ENABLED=#} asynOctetSetInputEos("camserver",0,"\030")
${PILATUS_ENABLED=#} asynOctetSetOutputEos("camserver",0,"\n")

#
# pilatusDetectorConfig(const char *portName, const char *camserverPort,
#     int maxSizeX, int maxSizeY, int maxBuffers, size_t maxMemory,
#     int priority, int stackSize)
#   portName       driver asyn port name
#   camserverPort  server asyn port name
#   maxSizeX       size of PILATUS detector in X direction
#   maxSizeY       size of PILATUS detector in Y direction
#   maxBuffers     maximum number of buffers for NDArrayPool
#   maxMemory      maximum amount of memory in bytes for NDArrayPool
#   priority       thread priority for asyn port driver thread if
#                  ASYN_CANBLOCK is set in asynFlags
#   stackSize      stack size for asyn port driver thread if ASYN_CANBLOCK is
#                  set in asynFlags
#
${PILATUS_ENABLED=#} pilatusDetectorConfig("PIL1", "camserver", 2463, 2527, 50, 200000000)
${PILATUS_ENABLED=#} dbLoadRecords("$(AREA_DETECTOR)/ADApp/Db/ADBase.template", "P=x7PIL1:,R=det1:,PORT=PIL1,ADDR=0,TIMEOUT=1")
${PILATUS_ENABLED=#} dbLoadRecords("$(AREA_DETECTOR)/ADApp/Db/NDFile.template", "P=x7PIL1:,R=det1:,PORT=PIL1,ADDR=0,TIMEOUT=1")
${PILATUS_ENABLED=#} dbLoadRecords("$(AREA_DETECTOR)/ADApp/Db/pilatus.template","P=x7PIL1:,R=det1:,PORT=PIL1,ADDR=0,TIMEOUT=1,CAMSERVER_PORT=camserver")

===

Obviously the lines are longer, and to me, the if-statement is way easier to read.  (It could be even more easy to read if iocsh supported line continuation!  But that's another feature.)

And as I said, the "${PILATUS_ENABLED=#}" style can only handle Boolean cases.  Here's an example of a non-Boolean environment variable use:

===

if ${DETECTOR} = ADSC
  # Attach imcais to areaDetector driver for simulated ADSC Q210
  dbpf("ioc23:Is:AdDrvBasePvName.VAL","x7ADSC1:det1:")
  # Disable exphi interface
  dbpf("ioc23:Is:UseExphi.VAL","No")
endif

if ${DETECTOR} = PILATUS
  # Attach imcais to areaDetector driver for simulated PILATUS 6M
  dbpf("ioc23:Is:AdDrvBasePvName.VAL","x7PIL1:det1:")
  # Enable exphi interface
  dbpf("ioc23:Is:UseExphi.VAL","Yes")
endif

===

In the above case, I have a DETECTOR environment variable indicating the "selected detector."  The "${PILATUS_ENABLED=#}" style can't handle this well.  I would have to create an _SELECTED environment variable for each detector.  If I have two detectors, that might be ADSC_SELECTED and PILATUS_SELECTED.  Now I can apply the "${PILATUS_ENABLED=#}" style.  But if I have more detectors, this just gets more painful since I have to have a separate _SELECTED environment variable for each detector.  Also, to configure which detector is selected, I can't just set one DETECTOR environment variable indicating which one is selected, I now have to set all the _SELECTED environment variables such that the detector I want selected will be selected and all the others not selected.  That's just a royal pain.

With an if-statement, I can also do consistency checking on my configuration environment variables.  For example:

===

# Selected detector should be ADSC if going to run test cases
if ${TEST_CASES_ENABLED} = YES && ${RUN_TEST_CASES_AFTER_IOC_INIT} = YES
  if ${DETECTOR} = PILATUS
    epicsEnvSet DETECTOR ADSC
  endif
endif

===

Above I'm using a Boolean "and" operator.  You could achieve this with multiple "${PILATUS_ENABLED=#}" on a line; something like "${TEST_CASES_ENABLED}=#} ${RUN_TEST_CASES_AFTER_IOC_INIT=#} ${PILATUS_SELECTED=#}".  And then after that mess, you'd have an epicsEnvSet to set an _SELECTED environment variable.  You'd then need a line like that for each detector _SELECTED environment variable.  That's ridiculous.

Here's another example of configuration consistency checking:

===

# Selected detector should not be disabled
if ${DETECTOR} = ADSC
  epicsEnvSet ADSC_ENABLED YES
elif ${DETECTOR} = PILATUS
  epicsEnvSet PILATUS_ENABLED YES
endif

===

This all just makes it easy to have an st-config.cmd which gets loaded near the top of st.cmd and contains lines setting environment variables indicating the configuration I want for my IOC.

Obviously, the other things supported in the if-statement feature have not been talked about.  This is because I don't actually have a real world need for them right now.  But I'll just note that performing a Boolean "or" is not possible with the "${PILATUS_ENABLED=#}" style.  And the integer comparison operators don't have a "${PILATUS_ENABLED=#}" style solution either.

> I agree that these techniques are not quite as convenient as putting
> conditional expressions in a single script file, but you can use them without
> modifying iocsh and can thus include them in modules that you distribute to
> other people.

If the iocsh if-statement support was in EPICS Base, then I would feel fine using them in anything I distribute to other people.  If someone can't upgrade, then obviously they can't use this new feature.  But that's the case with any new feature introduced into EPICS Base (e.g. record aliases).

>  3. As Eric Norum puts it, this is a Camel's Nose issue.  If we introduce
> conditionals someone else will want to add subroutines, and pretty soon we
> could have a full-blown Turing-complete scripting language to maintain.  It
> would be better to embed a standard scripting language interpreter into the
> IOC and add the ability to execute registered iocsh commands from within that
> language.

This is where I don't know what other people are wanting.  All I know is that, even though iocsh can only do a very limited number of things, it works OK for me, *except* for this conditional evaluation problem (and line continuation, but we're not talking about that here :-)).  It's easy to make the claim that if conditionals are added, then people will want whatever other feature.  That could happen, or it might not happen.  Maybe conditionals will be enough to handle most needs like it would be for me.  And you can obviously say no to future feature requests; you don't have to be "fair."  So if this if-statement feature is reasonable to you, you can accept it but reject other requests.

> My first preference for a suitable language would be Lua since the licensing
> is suitable (MIT) and there is a glue layer available for running it on
> vxWorks which allows Lua scripts to call functions found through the vxWorks
> symbol table.  It should be relatively easy to extend Lua to run registered
> iocsh commands as well.
> 
> If you can persuade enough other EPICS users to ask for conditionals in iocsh
> I will reconsider, but I think you'll get similar objections from several
> other people as well.

How many requesting users would make you consider it?

> I do like your iocsh test program though, I might steal that part.

I don't know that it's worth stealing since it just tests iocshCmd's ability to handle multiple line input (which my branch added) and the if-statement.

Thanks,

Lewis
-- 
https://code.launchpad.net/~jlmuir/epics-base/iocsh-if-flow-control-3.14/+merge/87972
Your team EPICS Core Developers is requested to review the proposed merge of lp:~jlmuir/epics-base/iocsh-if-flow-control-3.14 into lp:epics-base.

Experimental Physics and Industrial Control System

Experimental Physics and
Industrial Control System