JSBSim-Team / jsbsim

An open source flight dynamics & control software library
GNU Lesser General Public License v2.1
1.39k stars 455 forks source link

Can't use -1 as a property index in script events? #105

Open seanmcleod opened 6 years ago

seanmcleod commented 6 years ago

I have a multi-engine aircraft and I was trying to script a throttle increase for all engines in a script. So I tried the following:

    <event name="Increase Throttle">
      <description>Increase throttle to max</description>
      <condition> simulation/sim-time-sec >= 1.0 </condition>
      <set name="fcs/throttle-cmd-norm[-1]" value="1" type="FG_RAMP" tc="2" />
    </event>

However that results in an exception being thrown:

throw string("unterminated index (looking for ']')");

If I refer to all the engines individually then the script works as expected, e.g.

    <event name="Increase Throttle">
      <description>Increase throttle to max</description>
      <condition> simulation/sim-time-sec >= 1.0 </condition>
      <set name="fcs/throttle-cmd-norm[0]" value="1" type="FG_RAMP" tc="2" />
      <set name="fcs/throttle-cmd-norm[1]" value="1" type="FG_RAMP" tc="2" />
    </event>

Now FGFCS::SetThrottleCmd() has explicit code to handle an index of -1.

void FGFCS::SetThrottleCmd(int engineNum, double setting)
{
  if (engineNum < (int)ThrottlePos.size()) {
    if (engineNum < 0) {
      for (unsigned int ctr=0; ctr<ThrottleCmd.size(); ctr++)
        ThrottleCmd[ctr] = setting;
    } else {
      ThrottleCmd[engineNum] = setting;
    }

However it looks as if the parsing code used by the script while processing <set> elements doesn't support negative index values.

/**
 * Parse the optional integer index for a path component.
 *
 * Index: "[" [0-9]+ "]"
 */
static inline int
parse_index (const string &path, int &i)
{
  int index = 0;

  if (path[i] != '[')
    return 0;
  else
    i++;

  for (int max = (int)path.size(); i < max; i++) {
    if (isdigit(path[i])) {
      index = (index * 10) + (path[i] - '0');
    } else if (path[i] == ']') {
      i++;
      return index;
    } else {
      break;
    }
  }

  throw string("unterminated index (looking for ']')");
}

Is this limitation an oversight/bug or is this by design in terms of script events explicitly not supporting negative indices?

seanmcleod commented 6 years ago

What I also noticed is that if I refer to the property without an index then the script appears to take that as an alias for the 0 index, e.g.

    <event name="Increase Throttle">
      <description>Increase throttle to max</description>
      <condition> simulation/sim-time-sec >= 1.0 </condition>
      <set name="fcs/throttle-cmd-norm" value="1" type="FG_RAMP" tc="2" />
    </event>

Based on looking at the logged values for the 2 engines, i.e. I see the thrust increasing for engine 0 but no change in thrust for the other engine.

andgi commented 6 years ago

I think both issues here are by the design of the property tree. That the property fcs/throttle-cmd-norm is the same as fcs/throttle-cmd-norm[0] certainly is - and you wouldn't want to have it differently since it'd be ugly and impractical to end the name of all single properties by "[0]".

seanmcleod commented 6 years ago

@andgi so you're suggesting that you think that some-property[-1] isn't supported by the property tree by design. In which case is the -1 convention as seen and supported by functions like FGFCS::SetThrottleCmd(int index, double setting) only available to C++ code and not via script?

andgi commented 6 years ago

Yes, I think so. The property tree code (while not the script interface) comes from FlightGear/SimGear and I've never ever heard about any "-1" indices. Given that the tree can be serialized into XML (also by design) indices outside 0, 1, ... would be very strange indeed. (As would non-continuous ranges of indices but I think those are supported, though.)

Is it a very big problem to repeat the set-element for each engine?

Of course you could add a special case in the script parser fcs/throttle-cmd-norm[-1] but then someone else wants it for some other engine properties, and for all gear units and....

seanmcleod commented 6 years ago

I had come across the -1 option for FGFCS::SetThrottleCmd() a couple of years ago while developing a simulator for test pilot training where I needed to support single and multi-engine models using a single physical throttle that I was interfacing with and so used the -1 option for convenience.

Then more recently I noticed a bug in the engine initialization code for handling <running> -1 </running> elements, see - https://github.com/JSBSim-Team/jsbsim/issues/101.

So I assumed that there was general support for the -1 index option :wink:

In terms of whether it's a big problem to add repeated <set> elements the main issue is I'm trying to write a script, in this case an acceleration test, that is agnostic to how many engines there are, i.e. ideally I'd like to be able to invoke the script for any aircraft independent of exactly how many engines it has.

IF we decided to add more support for the -1 index I wouldn't special case fcs/throttle-cmd-norm.

My thinking is that the script code would detect the -1 index and in that case internally it would query the property manager to figure out exactly how many indices have been created for that property and it would then generate multiple <set> elements internally to match the number of actual indices.

That way script writers could write scripts that are agnostic to the exact number of indices for properties that they want to set uniformly for a range of indices, e.g. across engines etc.

andgi commented 6 years ago

@seanmcleod Neither of those examples are directly indexing properties in the property tree.

seanmcleod commented 6 years ago

Correct, but they led me to believe as a user that there was a good chance that the some-property[-1] case in a script would also be supported given that the C++ API FGFCS::SetThrottleCmd() supported it and the initialization file also supported the use of a -1 index to allow an operation across all indices.

And I think it would be a useful feature for users writing scripts, particularly as I mentioned for writing scripts that want to work across aircraft with varying numbers of engines as one example, and any other aircraft properties exposed via indices.

bcoconni commented 5 years ago

@seanmcleod What is the way forward about this issue ?

seanmcleod commented 5 years ago

@bcoconni I've assigned it to myself in the meantime. I had started writing some code at the time to test out my proposal of having the script code generate multiple set entries for any that use a -1 index, but then got distracted with other things. I'll have to revisit the code.