Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update the FGMSIS class #916

Merged
merged 2 commits into from
Jul 20, 2023
Merged

Update the FGMSIS class #916

merged 2 commits into from
Jul 20, 2023

Conversation

bcoconni
Copy link
Member

This PR is a followup of the PRs #902 and #908.

This patch brings a major update to the FGMSIS class but does not plug it in yet. The ability to switch JSBSim atmosphere model to "MSIS" will be brought by another PR meaning that this PR code is non operational.

The changes are:

  • The NRLMSIS00 code will be pure C code rather than being wrapped in a C++ class as is currently done in FGMSIS.cpp, FGMSIS.h and FGMSISData.cpp. This change is motivated by maintenance considerations: as the C code is kept pristine with respect to Dominik Brodowski original code, any update from Brodowski will only require a simple copy/paste of the files to update JSBSim rather manually updating some C++ code based on a diff.
  • FGMSIS is now inheriting from FGStandardAtmosphere instead of FGAtmosphere. This is needed to compute the pressure and density altitudes.
    • The default value of FGStandardAtmosphere::SaturatedVaporPressure as been modified: it is now equal to StdDaySLpressure rather than being initialized to 0. This change is required to avoid a division by zero during the bind/unbind process (which is already tested by the unit test FGMSISTest).
  • The management of the current day and hour has been moved from FGAuxiliary to FGMSIS. The latter is the only class that uses the current day and hour so this avoids splitting responsibilities between classes. IMHO would the current day and hour be needed by another class, I'd rather move them in FGFDMExec as it already manages the simulation time.
  • A unit test FGMSISTest has been added.
  • The CI workflow now validates the C code of NRLMSIS00 by checking that the output from nrlmsise-00_test.c complies with the reference output documented in the DOCUMENTATION file from D. Brodowski.

@codecov
Copy link

codecov bot commented May 27, 2023

Codecov Report

Merging #916 (36e4af0) into master (21d1026) will increase coverage by 1.76%.
The diff coverage is 94.73%.

@@            Coverage Diff             @@
##           master     #916      +/-   ##
==========================================
+ Coverage   23.13%   24.90%   +1.76%     
==========================================
  Files         167      168       +1     
  Lines       19644    18870     -774     
==========================================
+ Hits         4545     4699     +154     
+ Misses      15099    14171     -928     
Impacted Files Coverage Δ
src/models/FGAtmosphere.h 100.00% <ø> (ø)
src/models/FGAuxiliary.cpp 65.44% <ø> (-0.56%) ⬇️
src/models/FGAuxiliary.h 88.23% <ø> (+3.32%) ⬆️
src/models/atmosphere/FGStandardAtmosphere.h 66.66% <ø> (ø)
src/models/atmosphere/FGMSIS.h 80.00% <80.00%> (ø)
src/models/atmosphere/FGMSIS.cpp 97.87% <97.46%> (+97.87%) ⬆️
src/models/atmosphere/FGStandardAtmosphere.cpp 54.72% <100.00%> (+1.11%) ⬆️

... and 6 files with indirect coverage changes

@bcoconni
Copy link
Member Author

Right. It's been 6 weeks since I submitted this PR and it does not seem to attract much comment/attention/suggestion/criticism. I suppose that submitting an unsolicited feature was not a good idea and I no longer want to force my vision to the JSBSim community so I'm closing this PR.

As for the PRs #902 and #908, they are now irrelevant so you can revert them with the following commands:

> git revert db830c68
> git revert 868aed43
> git push origin master

@bcoconni bcoconni closed this Jul 11, 2023
@agodemar
Copy link
Contributor

@bcoconni sorry for my late message. I think you can merge your PR with the master branch. No problem on my side.

@bcoconni
Copy link
Member Author

Well, thanks @agodemar but what about you @seanmcleod ? I know you had some doubts about the features in the PR #902 and #908 and maybe we merged them too hastily ?

@seanmcleod
Copy link
Member

@bcoconni I suggest you continue/re-open this pull request. I hadn't specifically commented on this pull request since I didn't see any specific issues with it, i.e. "no news is good news" 😉

I did raise some concerns with regards to #902 and #908 mainly in terms of wanting to keep things as simple and clear as possible, both in terms of users wanting to run their aircraft models with different atmospheres, different planet constants etc. and also in terms of trying to keep the required code changes as simple and clear as possible.

In terms of FDM users I suggested rather than adding the atmosphere selection and planet constants into the FDM that they should be specified separately/externally to the FDM via something similar to initial conditions, maybe "environment conditions"?

You had replied with:

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 😄

So all good.

@bcoconni
Copy link
Member Author

Great ! Thanks for your feedback @agodemar and @seanmcleod 😄

@bcoconni bcoconni reopened this Jul 20, 2023
@bcoconni bcoconni merged commit 81e590c into JSBSim-Team:master Jul 20, 2023
55 checks passed
@bcoconni bcoconni deleted the NRLMSIS00 branch July 21, 2023 21:15
demonixis pushed a commit to demonixis/jsbsim that referenced this pull request Aug 17, 2023
bcoconni added a commit to bcoconni/jsbsim that referenced this pull request Aug 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants