Function cached values for aero functions

[jonsberndt]

I see that FGFunctions have been expanded to include caching of aerodynamic values. I'm not sure what drove this, but I am getting a strange error that causes execution to stop in the first pass. Apparently I am getting an FPE when executing aero function code with a vFNative vector that is somehow set to {-inf, 0.0, 0.0). Various example scripts seem to run just fine so perhaps this is a special case. When looking at the function code I noticed this:

  FGFunction()
    : cached(false), cachedValue(-HUGE_VAL), PropertyManager(nullptr),
      pNode(nullptr), pCopyTo(nullptr) {}

I'm wondering why cachedValue is initialized to -HUGE_VAL.

[jonsberndt]

To further describe the issue that I am seeing, this works:

    <function name="aero/force/CDi">
      <description> Induced_drag </description>
      <quotient>
        <pow>
          <value> 2 </value>
          <value> 3.0 </value>
        </pow>   
        <product>
          <value> 2.0 </value>
          <value> 2.0 </value>
        </product>
      </quotient>   
    </function>

This does not:

    <function name="aero/force/CDi">
      <description> Induced_drag </description>
      <quotient>
        <pow>
          <value> 2 </value>
          <value> 3.0 </value>
        </pow>   
        <product>
          <property> aero/qbar-psf </property>   <!-- property here doesn't work -->
          <value> 2.0 </value>
        </product>
      </quotient>   
    </function>

[bcoconni]

Regarding the first function

<function name="aero/force/CDi">
  <description> Induced_drag </description>
  <quotient>
    <pow>
      <value> 2 </value>
      <value> 3.0 </value>
    </pow>   
    <product>
      <value> 2.0 </value>
      <value> 2.0 </value>
    </product>
  </quotient>   
</function>

It is using constant values only which JSBSim optimizes by pre-computing such functions (see the relevant piece of code below).

jsbsim/src/math/FGFunction.cpp

Lines 853 to 879 in 22d57cd

	// Optimize functions applied on constant parameters by replacing them by
	// their constant result.
	if (!Parameters.empty()){
	FGFunction* p = dynamic_cast<FGFunction*>(Parameters.back().ptr());
	if (p && p->IsConstant()) {
	double constant = p->GetValue();
	FGPropertyNode_ptr node = p->pNode;
	string pName = p->GetName();
	Parameters.pop_back();
	Parameters.push_back(new FGRealValue(constant));
	if (debug_lvl > 0)
	cout << element->ReadFrom() << fggreen << highint
	<< "<" << operation << "> is applied on constant parameters."
	<< endl << "It will be replaced by its result ("
	<< constant << ")";
	if (node) {
	node->setDoubleValue(constant);
	node->setAttribute(SGPropertyNode::WRITE, false);
	if (debug_lvl > 0)
	cout << " and the property " << pName
	<< " will be unbound and made read only.";
	}
	cout << reset << endl << endl;
	}

So if everything worked well, JSBSim has (sort of) replaced the code above by

<property value="2.0"> aero/force/CDi </property>

and forced the property to be read only. So, from that perspective, the 2 functions that you have tested are quite differently handled by JSBSim.

[jonsberndt]

The problem happens as the simulation is being initialized.

[jonsberndt]

I put this same construct in a different, existing, XML file in the aerodynamic portion and it worked just fine.

[seanmcleod]

Can you share the complete file which reproduces the error?

I just tried the snippet by adding it to the DRAG section of JSBSim's 737 model without any issues.

[bcoconni]

I see that FGFunctions have been expanded to include caching of aerodynamic values. I'm not sure what drove this, [...]
I'm wondering why cachedValue is initialized to -HUGE_VAL.

Regarding FGFunction I can trace the existence of cached values and the usage of -HUGE_VAL back to at least Jun 13, 2005 (see below line 36 of commit bd905aa - committed by @jonsberndt 😉). It has been kept like that since then.

jsbsim/src/math/FGFunction.cpp

Lines 28 to 36 in bd905aa

	FGFunction::FGFunction(FGPropertyManager* propMan, Element* el, string prefix)
	: PropertyManager(propMan), Prefix(prefix)
	{
	int i;
	Element* element;
	string operation, property_name;
	int size = el->GetNumElements();
	cached = false;
	cachedValue = -HUGE_VAL;

I can only guess that the cached value is initialized to -HUGE_VAL to raise an FPE if someone tries to use it before it has been initialized.

Regarding the reason why aerodynamic values are cached, this was committed in May 22, 2016 (commit 18c143a by @bcoconni 😄) and the comment in the code explains that if values are not cached then the aerodynamic values exported by FGOutput might differ from the values used to compute the aerodynamic forces.

jsbsim/src/models/FGAerodynamics.cpp

Lines 189 to 196 in 22d57cd

	for (f=array->begin(); f != array->end(); ++f) {
	// Tell the Functions to cache values, so when the function values are
	// being requested for output, the functions do not get calculated again
	// in a context that might have changed, but instead use the values that
	// have already been calculated for this frame.
	(*f)->cacheValue(true);
	vFnative(axis_ctr+1) += (*f)->GetValue();
	}

[jonsberndt]

I vaguely remember writing the caching capability due to something I was seeing in flight controls definitions. Or so I thought. That was a long time ago.

It's a strange issue, and just in this one case. I see the FPE in executables compiled under Cygwin and VC++ both. I will try and make a small example file that illustrates the problem. I'm wondering if there is a very unusual and rare memory leak somewhere. Has a memory test been run on the code lately?

[bcoconni]

I know that JSBSim issues a strange error message when no <aerodynamics> section is specified in the model. May be you misspelled aerodynamics ? Just a wild guess...

I'm wondering if there is a very unusual and rare memory leak somewhere. Has a memory test been run on the code lately?

The first 5 seconds of the script Short_S23_3.xml are automatically tested with valgrind every time a commit is pushed to the master branch.

jsbsim/.github/workflows/cpp-python-build.yml

Lines 58 to 61 in 8819410

	- name: Test with Valgrind
	run: |
	valgrind --tool=memcheck --leak-check=full --leak-resolution=high --track-origins=yes --xml=yes --xml-file=valgrind_Short_S23_3.xml build/src/JSBSim scripts/Short_S23_3.xml --end-time=5.
	valgrind-ci valgrind_Short_S23_3.xml --abort-on-errors

This process has already caught a couple of memory leaks that I inadvertently pushed. Having said that, at the current stage the main virtue of this test is to exist. If we have some new sets of conditions to test then they must certainly be added to our CI workflow.

However, for an FPE, I would rather look for uninitialized variables. Such problems can lead to random crashes and in particular to FPEs. We already had such an issue with FGPiston which has been fixed by commit b39803f. Fortunately, for that particular issue, valgrind caught the problem and this is what lead both to the fix and to systemically running valgrind in the CI workflow. I am suspecting that we still have uninitialized variables somewhere in the code since we continue to experience random crashes (FPEs by the way) in our test suite. I have yet to find a way to determine where they are (valgrind has not found anything to date).

On the other hand, FPEs resulting from uninitialized variables occur seldomly while I understand that your model failed systemically. So we might need to seek something else...

I will try and make a small example file that illustrates the problem.

That would help greatly ! 👍