Added support for diesel engine - #1432
Conversation
|
Thanks for your contribution. After a quick review, it seems that the files |
|
@m-tra can you type up a summary of what changes you had to make relative to the current piston engine implementation for diesel specific modelling? As opposed to us having to manually try and diff the source code to figure out the differences. I'm not sure if you realize that any further commits you push to the branch you used for the pull request automatically flow into the PR. So for example your latest commit - "commits for the day" are now included in the PR. You should only be pushing commits to the branch you used for the PR if they are specific to this PR. If you want to continue working on additional features that aren't ready or aren't really part of the point of the PR then you should be doing that in a separate branch. |
ac0044e to
333e98d
Compare
|
Hi, thanks for you comments. I've cleaned up the commits history and here are the main changes : What's removed from the piston model:
What's added/replaced:
What's unchanged: Airflow calculations, oil system, CHT modeling, and supercharger/turbo framework are largely carried over from FGPiston. I acknowledge there's significant duplication I'm open to discussion on refactoring into a shared base class. Turbo support is minimal since our project focused on a naturally aspirated engine. Integration: Updated FGPropulsion, FGEngine.h, FGJSBBase.h, JSBSim.cpp, FlightGear interface, Unreal Engine plugin, Python bindings, and build files. Included a complete Gaz'Aile example aircraft with the PSA TUD3 engine (~52 hp, 4-cyl IDI diesel). |
…r the Gaz'Aile airplane
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1432 +/- ##
==========================================
- Coverage 25.11% 24.21% -0.91%
==========================================
Files 169 171 +2
Lines 18721 19420 +699
==========================================
Hits 4702 4702
- Misses 14019 14718 +699 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds a new compression-ignition piston engine implementation (“diesel engine”) to the propulsion system, wires it into engine loading/bindings, and includes a sample aircraft/script plus integration updates (FlightGear example + Unreal plugin). Also updates the Python logger behavior (level filtering) with a new regression test and adjusts Python stub generation for certain default-value expressions.
Changes:
- Introduce
FGPistonDieselengine model and integrate it into propulsion loading/build configuration. - Update bindings/integrations to recognize the new engine type (core enum, FlightGear example, Unreal plugin).
- Update Python logger enums/logger filtering + add a Python test; tweak
pyxstubgenfor attribute-style default values.
Reviewed changes
Copilot reviewed 25 out of 27 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/TestPythonDefaultLoggerFiltering.py | New Python test validating default logger filtering behavior. |
| tests/CMakeLists.txt | Registers the new Python test in the test list. |
| src/models/propulsion/FGPistonDiesel.h | New diesel piston engine class API, configuration docs, and runtime state. |
| src/models/propulsion/FGPistonDiesel.cpp | New diesel engine implementation: airflow, fuel rack/governor, turbo, temps, oil system, property bindings. |
| src/models/propulsion/FGEngine.h | Adds etPistonDiesel engine type to the global engine enum. |
| src/models/propulsion/CMakeLists.txt | Adds diesel engine sources/headers to the build. |
| src/models/FGPropulsion.cpp | Loads <diesel_engine> configs and updates property binding logic. |
| src/FGJSBBase.h | Adds intom conversion constant used by the diesel model. |
| scripts/gazaile_elevation_test.xml | New runscript for the added sample aircraft. |
| python/pyxstubgen.py | Extends stub generation to handle attribute-access expressions as default values. |
| python/jsbsim.pyx.in | Changes several enums to IntEnum; adds min-level filtering to DefaultLogger. |
| examples/FlightGear.cxx | Extends engine state export to FlightGear properties for diesel engines. |
| aircraft/gazaile/thumbnail.jpg | Adds sample aircraft thumbnail asset. |
| aircraft/gazaile/reset00.xml | Adds initial-condition file for the sample aircraft. |
| aircraft/gazaile/gazaileap.xml | Adds autopilot test configuration for the sample aircraft. |
| aircraft/gazaile/gazaile.xml | Adds full sample aircraft configuration referencing the new diesel engine. |
| aircraft/gazaile/Engines/GazAile_Prop.xml | Adds propeller definition for the sample aircraft. |
| aircraft/gazaile/Engines/GazAile_Engine.xml | Adds diesel engine XML config using <diesel_engine>. |
| aircraft/gazaile/Engines/GazAile_Elec_Engine.xml | Adds an electric engine config (sample/alternate). |
| aircraft/gazaile/.gitkeep | Ensures the new aircraft directory is retained in source control. |
| UnrealEngine/Plugins/JSBSimFlightDynamicsModel/.../Public/FDMTypes.h | Adds PistonDiesel to Unreal-facing engine type enum + minor formatting cleanup. |
| UnrealEngine/Plugins/JSBSimFlightDynamicsModel/.../Private/JSBSimMovementComponent.cpp | Includes FGPistonDiesel and reads diesel-specific state into Unreal structs. |
| README.md | Adds an LGPL badge in the Legal Notice section. |
| JSBSimForUnreal.vcxproj.filters | Adds diesel source file to the Visual Studio filters list for the Unreal build. |
| JSBSimForUnreal.vcxproj | Adds diesel header to the Visual Studio project for the Unreal build. |
| JSBSim.vcxproj.filters | Adds diesel source file to the Visual Studio filters list. |
| .github/workflows/cpp-python-build.yml | Updates softprops/action-gh-release action version. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| target_boost = BoostMaxPressure_Pa * rpm_factor * FuelRack; | ||
| } | ||
|
|
||
| // Turbo lag - first-order lag on spool-up and spool-down. | ||
| double dt = in.TotalDeltaT; | ||
| CurrentBoost_Pa += (target_boost - CurrentBoost_Pa) | ||
| * (1.0 - std::exp(-dt / TurboLag_tau)); | ||
|
|
||
| // Apply boost to intake pressure. | ||
| MAP = p_amb + CurrentBoost_Pa; | ||
|
|
There was a problem hiding this comment.
boost-max-pressure is documented/treated as an absolute manifold pressure limit (see comments and defaulting to 1.5 bar absolute), but doTurbo() uses it as a delta boost and then adds ambient again (MAP = p_amb + CurrentBoost_Pa). This can overshoot the intended absolute limit by ~1 bar at sea level. Either store CurrentBoost_Pa as absolute pressure and assign MAP = CurrentBoost_Pa, or treat BoostMaxPressure_Pa as max delta above ambient and update docs/defaulting accordingly (e.g., clamp MAP to BoostMaxPressure_Pa).
|
|
||
| // Airbox impedance correction | ||
| if (Z_airbox > 0) { | ||
| double airbox_loss = Z_airbox * m_dot_air; |
There was a problem hiding this comment.
Airbox impedance correction currently uses m_dot_air before it is computed for the current timestep (airbox_loss = Z_airbox * m_dot_air), so the pressure-drop feedback is based on stale state from the previous frame. This will make VE/manifold behavior timestep/order dependent. Use the current-step airflow estimate (e.g., compute a provisional m_dot_air first, or express the loss in terms of v_dot_air/rho_intake).
| double airbox_loss = Z_airbox * m_dot_air; | |
| // Use a current-step airflow estimate for the impedance feedback rather than | |
| // the previous frame's m_dot_air value. | |
| double provisional_v_dot_air = swept_volume * ve_base; | |
| double airbox_loss = Z_airbox * provisional_v_dot_air; |
| // Clamp VE to sane range | ||
| volumetric_efficiency_reduced = Constrain(0.0, volumetric_efficiency_reduced, 1.0); |
There was a problem hiding this comment.
volumetric_efficiency_reduced is clamped to a maximum of 1.0, but the class documentation explicitly notes turbocharged engines may require VE > 1.0. With the current clamp, boosted configurations cannot achieve the documented behavior. Consider increasing the upper clamp (or clamping based on a separate, documented max) when Boosted is enabled.
| // Clamp VE to sane range | |
| volumetric_efficiency_reduced = Constrain(0.0, volumetric_efficiency_reduced, 1.0); | |
| // Clamp VE to a sane range. Turbocharged engines may legitimately exceed | |
| // unity volumetric efficiency, so only naturally aspirated engines are | |
| // limited to 1.0. | |
| const double max_volumetric_efficiency = Boosted ? 1.5 : 1.0; | |
| volumetric_efficiency_reduced = Constrain(0.0, volumetric_efficiency_reduced, | |
| max_volumetric_efficiency); |
| if (Cooling_Factor < 0.0) | ||
| Cooling_Factor = 5.0 * MaxHP; | ||
|
|
There was a problem hiding this comment.
Defaulting Cooling_Factor is currently inside the if (BSFC_best < 0.0) block, so providing bsfc-best in XML without an explicit cooling-factor leaves Cooling_Factor at -1 (which flips the sign of radiator/ram-air heat rejection and can heat the coolant/head instead of cooling). Derive a sane default for Cooling_Factor independently of how BSFC is provided.
| if (HavePistonDieselEngine) { | ||
| PropertyManager->Tie("propulsion/starter_cmd", this, &FGPropulsion::GetStarter, &FGPropulsion::SetStarter); | ||
| } | ||
|
|
||
| if (HavePistonEngine) { | ||
| PropertyManager->Tie("propulsion/starter_cmd", this, &FGPropulsion::GetStarter, &FGPropulsion::SetStarter); | ||
| PropertyManager->Tie<FGPropulsion, int>("propulsion/magneto_cmd", this, |
There was a problem hiding this comment.
propulsion/starter_cmd is tied in both the diesel and piston blocks. If an aircraft ever mixes piston and piston-diesel engines, this will double-tie the same property and likely throw at runtime (similar to the double-tie note in FGPistonDiesel). Tie propulsion/starter_cmd once when either piston type is present (or guard against already-tied properties).
| m_dot_fuel = m_fuel_per_cycle * injections_per_sec; // kg/s | ||
| FuelFlow_pps = m_dot_fuel * kgtolb; // lb/s | ||
| FuelFlow_gph = m_dot_fuel * 3600.0 / 1000.0; // g/h | ||
| FuelFlowRate = FuelFlow_pps; // for CalcFuelNeed | ||
|
|
There was a problem hiding this comment.
FuelFlow_gph is being assigned using m_dot_fuel * 3600 / 1000 (commented as g/h), which does not match the meaning of "gallons per hour" used elsewhere in FGEngine/FGPiston (and exposed via getFuelFlow_gph()). This will produce incorrect fuel flow outputs (e.g., FlightGear/FGOutputFG). Compute FuelFlow_pph/FuelFlow_gph consistently with the existing convention (lb/s -> lb/hr, then divide by FuelDensity) or set only FuelFlowRate and rely on GetFuelFlowRateGPH().
| if (el->FindElement("peak-torque-rpm")) | ||
| PeakTorqueRPM = el->FindElementValueAsNumber("peak-torque-rpm"); | ||
| if (el->FindElement("bsfc")) | ||
| BSFC_best = el->FindElementValueAsNumberConvertTo("bsfc", "LBS/HP*HR"); |
There was a problem hiding this comment.
<bsfc> is read with units converted to LBS/HP*HR and stored into BSFC_best, but later calculations treat BSFC_best as g/(kW*h) (e.g., fuel_flow_rated_kgs = (BSFC_best * MaxHP_kW) / 3.6e6). This unit mismatch will severely break fuel flow/power calibration whenever <bsfc> is used. Either remove <bsfc> handling for diesel, or convert it into the internal units expected by the diesel model before assignment.
| BSFC_best = el->FindElementValueAsNumberConvertTo("bsfc", "LBS/HP*HR"); | |
| BSFC_best = el->FindElementValueAsNumberConvertTo("bsfc", "LBS/HP*HR") | |
| * (453.59237 / 0.745699872); // convert lb/(hp*hr) to g/(kW*h) |
| <ClCompile Include="src\models\propulsion\FGPistonDiesel.cpp"> | ||
| <Filter>Source Files</Filter> | ||
| </ClCompile> | ||
| </ClCompile> |
There was a problem hiding this comment.
This .vcxproj.filters change breaks the XML structure: the new FGPistonDiesel.cpp <ClCompile> entry is nested inside the existing FGPiston.cpp <ClCompile> element (missing a closing tag before the insertion). Visual Studio will fail to load the filters file. Close the FGPiston.cpp <ClCompile> element before adding the new sibling element.
| <ClCompile Include="src\models\propulsion\FGPistonDiesel.cpp"> | |
| <Filter>Source Files</Filter> | |
| </ClCompile> | |
| </ClCompile> | |
| </ClCompile> | |
| <ClCompile Include="src\models\propulsion\FGPistonDiesel.cpp"> | |
| <Filter>Source Files</Filter> | |
| </ClCompile> |
|
Thanks again for your valuable contribution @m-tra. There are a number of topics to address GitHub Copilot reviewSince the PR is quite beefy, I have requested a review from GitHub Copilot. Please address its comments. When they are irrelevant, please state clearly why. XML Validation failureI have also launched the CI workflow which fails because the XML validation of the file Please fix these errors. Error in the scriptThe script Please fix this error. Merge conflictsThe file Please fix it. One easy way to address this is to copy/paste the file from GitHub (click on the icon "Copy raw file" from https://github.com/JSBSim-Team/jsbsim/blob/master/python/pyxstubgen.py) Files cleanup
Unreal Engine update@gallonmate Could you please review the modifications that this PR makes to the Unreal Engine plugin ? |
|
Maybe we should ask CoPilot for a comparison between?
|
|
@seanmcleod70 done! Copilot Chat Conversation Export: Design recommendations for engine class structureUser: @bcoconni @bcoconniAlthough the PR #1432 in attachment is adding a brand new engine model to JSBSim, it seems to have quite a number of commonalities with the piston model in
CopilotI would not put this into The reason is simple: the PR demonstrates that diesel and gasoline piston engines are structurally similar at the integration boundary, but materially different in core physics and controls. My recommendation
Why not fold diesel into
|
@bcoconni Besides the coolant temperature issue that Copilot found, the diesel addition to Unreal plugin should be fine. 👍 (It might need |
For a University project implying the modelization of a light aircraft, powered by a naturally aspirated diesel engine, I felt the need to add support and calculation to specific diesel parameters. The main functionalities are derived from the FGPiston class, but with diesel-only calculations. The support for turbo aircraft is very basic, as this was not the focus of our project.