Do not store the result of FGFDMExec::GetAtmosphere().

[bcoconni]

This PR is a followup of the PR #902.

As was exposed in #902, we might need to modify the atmosphere model when FGFDMExec::LoadModel() is executed and that would result in Model[eAtmosphere] being modified. As a result if a class stores the pointer returned by FGFDMExec::GetAtmosphere() before the atmosphere model could be modified then this pointer could be invalidated.

This sequence of events exists when the constructor of the class FGInitialCondition is called and stores a copy of FGFDMExec::GetAtmosphere() in its member Atmosphere:

jsbsim/src/initialization/FGInitialCondition.cpp

Lines 62 to 67 in db830c6

	FGInitialCondition::FGInitialCondition(FGFDMExec *FDMExec) : fdmex(FDMExec)
	{
	InitializeIC();
	if(FDMExec) {
	Atmosphere=fdmex->GetAtmosphere();

And the constructor of FGInitialCondition is called from the constructor of FGFDMExec i.e. before FGFDMExec::LoadModel() is executed:

jsbsim/src/FGFDMExec.cpp

Line 267 in db830c6

IC = std::make_shared<FGInitialCondition>(this);

This PR addresses this issue by removing the storage of a copy of the result of FGFDMExec::GetAtmosphere(). After the PR will be merged, FGInitialCondition will call FGFDMExec::GetAtmosphere() each time it needs to get data from the atmosphere model. As FGInitialCondition is only used during initialization, the performance impact (if any) is negligible.

The classes FGOutputType, FGOutputTextFile and FGOutputSocket also store a copy of FGFDMExec::GetAtmosphere() but strictly speaking they are not concerned by this issue as they are created by FGOutput::Load() after the atmosphere model would be modified:

jsbsim/src/models/FGOutput.cpp

Lines 202 to 220 in db830c6

	if (type == "CSV") {
	FGOutputTextFile* OutputTextFile = new FGOutputTextFile(FDMExec);
	OutputTextFile->SetDelimiter(",");
	Output = OutputTextFile;
	} else if (type == "TABULAR") {
	FGOutputTextFile* OutputTextFile = new FGOutputTextFile(FDMExec);
	OutputTextFile->SetDelimiter("\t");
	Output = OutputTextFile;
	} else if (type == "SOCKET") {
	Output = new FGOutputSocket(FDMExec);
	name += ":" + port + "/" + protocol;
	} else if (type == "FLIGHTGEAR") {
	Output = new FGOutputFG(FDMExec);
	name += ":" + port + "/" + protocol;
	} else if (type == "TERMINAL") {
	// Not done yet
	} else if (type != string("NONE")) {
	cerr << "Unknown type of output specified in config file" << endl;
	}

Indeed FGOutput is the last FGModel instance that parses the XML files:

jsbsim/src/FGFDMExec.cpp

Lines 940 to 948 in db830c6

	// Process the output element[s]. This element is OPTIONAL, and there may be
	// more than one.
	element = document->FindElement("output");
	while (element) {
	if (!Output->Load(element))
	return false;
	element = document->FindNextElement("output");
	}

However this PR extends the requirement of not storing a copy of FGFDMExec::GetAtmosphere() to all classes because this fortunate call sequence might be (accidentally ?) broken in the future for the classes FGOutputType, FGOutputTextFile and FGOutputSocket. In addition the performance impact is low as well for output classes as outputs are generally not requested at every time step and furthermore the call to FGFDMExec::GetAtmosphere() will be inlined by compilers.

[codecov]

Codecov Report

Merging #908 (734f62a) into master (db830c6) will increase coverage by 0.02%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master     #908      +/-   ##
==========================================
+ Coverage   23.16%   23.18%   +0.02%     
==========================================
  Files         167      167              
  Lines       19644    19657      +13     
==========================================
+ Hits         4551     4558       +7     
- Misses      15093    15099       +6     

Impacted Files	Coverage Δ
src/initialization/FGInitialCondition.h	74.71% <ø> (ø)
src/input_output/FGOutputSocket.cpp	0.00% <0.00%> (ø)
src/input_output/FGOutputTextFile.cpp	0.00% <0.00%> (ø)
src/input_output/FGOutputType.cpp	0.00% <ø> (ø)
src/input_output/FGOutputType.h	0.00% <ø> (ø)
src/initialization/FGInitialCondition.cpp	45.46% <62.50%> (+0.28%)	⬆️

[seanmcleod]

I planned to mention in the earlier pull request, just very busy with work at the moment, before @agodemar merged it in very quickly, that FGFDMExec also stores a copy of the atmosphere model in it's own Atmosphere member variable, so that needs to be taken into account when switching atmospheric models.

jsbsim/src/FGFDMExec.cpp

Line 247 in db830c6

Atmosphere = static_cast<FGAtmosphere*>(Models[eAtmosphere].get());

[seanmcleod]

The other general comment I was going to make, is that in general the switching of models like this is a bit messy. I had started taking a look at the initialisation order, what other models depend on each other before any XML files are loaded in etc. The one other somewhat similiar case I noticed was in terms of the ability to change some of the planet's constants. There is special code to handle that case, involving resetting and re-initiliasing other models again etc. In that particular case there isn't the added complication of property bindings.

As I mentioned I'm quite busy work-wise, so haven't had enough time to go through it in detail and make useful comments/suggestions.

Just that at first glance as I mentioned above, it seems somewhat messy.

So half-wondering whether there isn't some other mechanism to process these sorts of requirements, i.e. planet constants, athmosphere model selection etc. earlier, i.e. before JSBSim currently starts reading XML files, so that we can initialise the Inertial (planet constants) model, atmosphere model etc. once up-front.

Talking of which, where would the user typically specify the atmosphere model that they want to use for a simulation run?

[agodemar]

Sorry guys @bcoconni @seanmcleod for merging too quickly

[seanmcleod]

No problem, I was a bit slow 😉

[bcoconni]

FGFDMExec also stores a copy of the atmosphere model in it's own Atmosphere member variable, so that needs to be taken into account when switching atmospheric models.

That's a good point and fortunately I did not overlook it: https://github.com/bcoconni/jsbsim/blob/2bd695bdca57a0a5167eaf22c29cfcfa6eeb9fc8/src/FGFDMExec.cpp#L825

The other general comment I was going to make, is that in general the switching of models like this is a bit messy.

I agree that these PR (i.e. #902 and the current one) bring some complexity that we'd like to avoid. However the situation might not be as bad as it sounds. Below is an extract of the code I'm currently working on and that would be integrated in FGFDMExec::LoadModel():

      Element* atm_element = element->FindElement("atmosphere");
      if (atm_element) {
        if (atm_element->HasAttribute("model")) {
          string model = atm_element->GetAttributeValue("model");
          if (model == "MSIS") {

            // Replace the existing atmosphere model
            instance->Unbind(Models[eAtmosphere]);
            Models[eAtmosphere] = std::make_shared<MSIS>(this);
            Atmosphere = static_cast<FGAtmosphere*>(Models[eAtmosphere].get());

            // Model initialization sequence
            LoadInputs(eAtmosphere);
            Atmosphere->InitModel();
            Atmosphere->Load(element);
            InitializeModels();
          }
        }
      }

There is indeed some boilerplate but it's not that bad.

The one other somewhat similar case I noticed was in terms of the ability to change some of the planet's constants. There is special code to handle that case, involving resetting and re-initiliasing other models again etc. In that particular case there isn't the added complication of property bindings.

I understand that you are referring to the following code:

jsbsim/src/FGFDMExec.cpp

Lines 802 to 813 in db830c6

	element = document->FindElement("planet");
	if (element) {
	result = Models[eInertial]->Load(element);
	if (!result) {
	cerr << endl << "Planet element has problems in file " << aircraftCfgFileName << endl;
	return result;
	}
	// Reload the planet constants and re-initialize the models.
	LoadPlanetConstants();
	IC->InitializeIC();
	InitializeModels();
	}

You are making a good point and I think that the bind/unbind problem has been overlooked for this particular case. This needs some investigation.

As I mentioned I'm quite busy work-wise, so haven't had enough time to go through it in detail and make useful comments/suggestions.

No problem. There is no rush.

So half-wondering whether there isn't some other mechanism to process these sorts of requirements, i.e. planet constants, atmosphere model selection etc. earlier, i.e. before JSBSim currently starts reading XML files, so that we can initialise the Inertial (planet constants) model, atmosphere model etc. once up-front.

Suggestions are welcome and I mean it. That is really the point of submitting code via a PR.

Talking of which, where would the user typically specify the atmosphere model that they want to use for a simulation run?

I'm currently considering adding an <atmosphere> element to the existing <planet> element that has been introduced by the commit ecb7952. So using the MSIS atmosphere model would require adding the following XML code:

<planet name="Earth">
  <atmosphere model="MSIS"/>
</planet>

The name attribute is for documentation purpose only, users could name the planet "home" if they like so and the result would be the same.

[bcoconni]

Sorry guys @bcoconni @seanmcleod for merging too quickly

@agodemar no problem on my side either 😉

[bcoconni]

The one other somewhat similar case I noticed was in terms of the ability to change some of the planet's constants. There is special code to handle that case, involving resetting and re-initiliasing other models again etc. In that particular case there isn't the added complication of property bindings.

You are making a good point and I think that the bind/unbind problem has been overlooked for this particular case. This needs some investigation.

After some further thoughts, the issue of binding/unbinding does not apply in this case as the instance of FGInertial is not deleted but merely initialized by calling the method FGInertial::Load()

jsbsim/src/FGFDMExec.cpp

Lines 800 to 812 in 1e19876

	// Process the planet element. This element is OPTIONAL.
	element = document->FindElement("planet");
	if (element) {
	result = Models[eInertial]->Load(element);
	if (!result) {
	cerr << endl << "Planet element has problems in file " << aircraftCfgFileName << endl;
	return result;
	}
	// Reload the planet constants and re-initialize the models.
	LoadPlanetConstants();
	IC->InitializeIC();
	InitializeModels();
	}

So half-wondering whether there isn't some other mechanism to process these sorts of requirements, i.e. planet constants, atmosphere model selection etc. earlier, i.e. before JSBSim currently starts reading XML files, so that we can initialise the Inertial (planet constants) model, atmosphere model etc. once up-front.

To avoid the unbing/binding sequence I can imagine 3 options:

1. XML files are loaded by the FGFDMExec constructor
2. The call to FGModel constructors is deferred to FGFDMExec::Load()
3. The call to the bind() methods is made by FGFDMExec

Option 1 - FGFDMExec constructor reads XML

Option 1 would result in a major API change as you would have to supply the XML model name to the FGFDMExec constructor which we currently do not do. In addition there are currently 2 ways to load XML files: either calling FGFDMExec::LoadModel() or calling FGFDMExec::LoadScript() so the new constructor would need to cover both ways of loading XML files. Most likely this would result in a very ugly signature of the constructor and a backward incompatible API change on one of the major interface to JSBSim.

Option 2 - Calls to FGModel constructor are deferred to FGFDMExec::LoadModel()

Option 2 would mean that the FGFDMExec constructor would no longer call FGFDMExec::Allocate() which would result in FGFDMExec containing null pointers until either FGFDMExec::LoadModel() or FGFDMExec::LoadScript() are called. Not something I would feel comfortable with as SEGFAULTs would occur as soon as any FGFDMExec methods are called except FGFDMExec::LoadModel() or FGFDMExec::LoadScript().

This could be a major issue for people who are using our Python module since the occurrence of a SEGFAULT would kill the Python interpreter which is a pain when using the Python interactive shell or a Jupyter Notebook. This option would also result in removing the method FGFDMExec::Allocate() and calling the FGModel constructors as we go while reading the XML data files. All in all, this would be a major rewrite and I barely see this as a simplification of our code.

Option 3 - Delegate to FGFDMExec the call to FGAtmosphere::bind()

With option 3, FGAtmosphere::bind() could be called by FGFDMExec after it has been determined which atmosphere model is requested by the user and that would save the call to FGPropertyManager::Unbind(). The code change would be minimal as we would only need to remove the calls to FGAtmosphere::bind() from the atmosphere models constructors:

jsbsim/src/models/FGAtmosphere.cpp

Lines 54 to 62 in 1e19876

	FGAtmosphere::FGAtmosphere(FGFDMExec* fdmex)
	: FGModel(fdmex),
	StdDaySLsoundspeed(sqrt(SHRatio*Reng0*StdDaySLtemperature))
	{
	Name = "FGAtmosphere";
	bind();
	Debug(0);
	}

jsbsim/src/models/atmosphere/FGStandardAtmosphere.cpp

Line 139 in 1e19876

bind();

However the gain would be minimal as we would trade one line of code for another one:

      Element* atm_element = element->FindElement("atmosphere");
      if (atm_element) {
        if (atm_element->HasAttribute("model")) {
          string model = atm_element->GetAttributeValue("model");
          if (model == "MSIS") {

            // Replace the existing atmosphere model
-           instance->Unbind(Models[eAtmosphere]);
            Models[eAtmosphere] = std::make_shared<MSIS>(this);
            Atmosphere = static_cast<FGAtmosphere*>(Models[eAtmosphere].get());

            // Model initialization sequence
            LoadInputs(eAtmosphere);
            Atmosphere->InitModel();
            Atmosphere->Load(element);
            InitializeModels();
          }
        }
      }
+     Atmosphere->bind();

In addition, this option would not prevent the need for the current PR as we would still delete the current instance of FGAtmosphere and replace it by an instance of FGMSIS. So we would still need to make sure that nowhere a pointer to the deleted instance has been stored.

This change would also make FGAtmosphere one of a kind as it would be the only FGModel inherited class that would delegate the call to its bind() method to FGFDMExec.

These are my 2 cents and hopefully these suggestions will feed the discussion 😃

[seanmcleod]

After some further thoughts, the issue of binding/unbinding does not apply in this case as the instance of FGInertial is not deleted but merely initialized by calling the method FGInertial::Load()

Correct, I did look at how the update to the Inertial model took place with supplied planet constants and noticed that in that case "...there isn't the added complication of property bindings.". So, it was more a somewhat similar case in terms of a model being initialized, and then later on when the user supplied/requested a modified model (different planet constants in this case) that there was a bit of "messy" code in re-initializing the Inertial model and then updating/re-initializing all the other models that may depend on the Inertial model.

So my general question was whether these sorts of requests by the user, planet constants, atmospheric model to use etc. couldn't be read in earlier via some "system initialization" file (initial conditions 'like') so that the relevant models could be initialized once and before any of the other models that take dependencies on them.

I wasn't that familiar of how the planet constants were specified by the user and how you planned for the user to specify the atmosphere model they wanted to use. So looking at the planet case it looks like it gets specified inside the FDM XML file right?

Just seems a bit weird in the sense of it not really being a property of the aircraft model, rather something external. So if I want to run the 737 model using planet constants X and atmosphere model Y I need to add them to the 737's FDM? And then remove them/edit them for another run with a different set?

Shouldn't these rather be specified external to the FDM, a bit like initial condition files?

[bcoconni]

I wasn't that familiar of how the planet constants were specified by the user and how you planned for the user to specify the atmosphere model they wanted to use. So looking at the planet case it looks like it gets specified inside the FDM XML file right?

Correct.

Just seems a bit weird in the sense of it not really being a property of the aircraft model, rather something external. So if I want to run the 737 model using planet constants X and atmosphere model Y I need to add them to the 737's FDM? And then remove them/edit them for another run with a different set?

Correct.

Shouldn't these rather be specified external to the FDM, a bit like initial condition files?

I hadn't thought about that and your suggestion is making sense. I guess we could add a method FGFDMExec::LoadPlanet() that would load the planet constants and the atmosphere before LoadModel() is called. I like your idea, I'll investigate it further 😄