-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
fix(fms): use entered fob rather than fqi before start #8664
Conversation
Special thanks to @BlueberryKing for helping to knock some rust off(I've been on a dev hiatus) and throwing me a bone for showing me where the FOB is initially injected! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional things to do:
- Refactor getGrossWeight a little as it duplicates this FQI logic.
- Check that all the consumers of getFoB can handle it being undefined, as it always returned a number before. Some definitely can not, so some changes will be needed. Perhaps a new getFqiFob function could be created, wrapping the simvar read, and the fuel pred page could call it instead.
- Check that the AOC actually wants this value, or does it always want the FQI FoB? (Note that the AOC shouldn't actually directly access the FMS, as it lives in a different computer in the real aircraft, but fixing that is out of scope for this PR.)
Fix the PR title inline with https://www.conventionalcommits.org/en/v1.0.0/. Suggest fix(fms): use entered fob rather than fqi before start
or something like that. Whatever you put here will become the commit title when this PR is squash-merged later.
In general you always need to be very mindful of type safety when working on the legacy JS code, as unlike typescript there is no automatic type checking.
.../html_ui/Pages/VCockpit/Instruments/Airliners/FlyByWire_A320_Neo/FMC/A32NX_FMCMainDisplay.js
Outdated
Show resolved
Hide resolved
.../html_ui/Pages/VCockpit/Instruments/Airliners/FlyByWire_A320_Neo/FMC/A32NX_FMCMainDisplay.js
Outdated
Show resolved
Hide resolved
.../html_ui/Pages/VCockpit/Instruments/Airliners/FlyByWire_A320_Neo/FMC/A32NX_FMCMainDisplay.js
Outdated
Show resolved
Hide resolved
Co-authored-by: Michael Corcoran <tracer@outlook.co.nz>
Have a few questions here:
Feel free to message on Discord to bounce ideas/thoughts if above does not work. |
Part of the job here is understanding the aircraft systems and how things should actually work before we touch the code, as then we can understand how the code should look. That part is unfortunately much more difficult than the code. You happen to have picked an area where the gross weight calculation code is relatively simple, but the systems changes required are definitely not trivial, and spread around quite a wide area of the FMS. All of the consumers of GW/FOB should handle those values not being undefined because they really are not available prior to engine start with no ZFW/FOB entered. Some of the things getGW does look very dubious, especially setting a local var, so there's opportunity for a good clean up there too. |
@tracernz |
removed comments and extra conversion
@@ -4996,10 +5001,9 @@ class FMCMainDisplay extends BaseAirliners { | |||
//TODO: Can this be util? | |||
getGW() { | |||
let fmGW = 0; | |||
if (this.isAnEngineOn() && isFinite(this.zeroFuelWeight)) { | |||
//Simplified to just checking fuelWeight as GetFOB handles what fuel level to use (block vs tank reading) | |||
if (isFinite(this.zeroFuelWeight)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Careful, what happens if you've not entered a block fuel yet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm Yes Thanks for catching
Can the way the current aircraft release handles this scenario be used as a "control" or do the IRL pilots need to be consulted for what the behavior is in this situation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm Yes Thanks for catching
Can the way the current aircraft release handles this scenario be used as a "control" or do the IRL pilots need to be consulted for what the behavior is in this situation?
I'd have to check the places this function is called from, but replicating the current behavior would be a good starting point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK Sounds good.
Added an additional check to check for block fuel being entered to prevent "A32NX_FM_GROSS_WEIGHT" from being Null
@BlueberryKing to approve as well |
@BlueberryKing |
if (mcdu.isAnEngineOn()) { | ||
const currentFob = formatWeight(NXUnits.kgToUser(mcdu.getFOB())); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest we move this if block to the block further down where currentFob
is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BlueberryKing
Thinking you mean like this? (Removing line 106->108 and adding to Line 130)
currentFob = formatWeight(NXUnits.kgToUser(mcdu.getFOB()));
if (currentFob) {
fob = `{small}${currentFob}{end}[color]green`;
}
}`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can remove these lines (106-108) now, right?
@@ -4996,10 +5001,9 @@ class FMCMainDisplay extends BaseAirliners { | |||
//TODO: Can this be util? | |||
getGW() { | |||
let fmGW = 0; | |||
if (this.isAnEngineOn() && isFinite(this.zeroFuelWeight)) { | |||
//Simplified to just checking fuelWeight as GetFOB handles what fuel level to use (block vs tank reading) | |||
if (isFinite(this.zeroFuelWeight) && this.blockFuel) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think checking for this.blockFuel
works here. With an engine running, the gross weight should be available (since fuel is available from FQI) even if no block fuel is entered.
What I suggest is fetching this.getFOB()
and checking whether that's finite instead.
Also, I prefer Number.isFinite
over isFinite
since isFinite(null)
is true which is rarely desired.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
//Does this work? I removed unnecessary else block for simplicity sake as well since fmGw already is at 0.
getGW() {
let fmGW = 0;
const currFob = this.getFOB();
//Simplified to just checking fuelWeight as GetFOB handles what fuel level to use (block vs tank reading)
if (Number.isFinite(this.zeroFuelWeight) && Number.isFinite(currFob)) {
fmGW = (currFob + this.zeroFuelWeight);
}
SimVar.SetSimVarValue("L:A32NX_FM_GROSS_WEIGHT", "Number", fmGW);
return fmGW;
}
Add .isFinite type safety for and isAnEngineOn() logic instead of currentFoB()
Quality Assurance Tester/Trainee Report Discord Username : PilotEyesA350 Testing Process:
Testing Results: Negatives: Conclusions: Media: |
b63af05
to
7ee80c6
Compare
…8664) * FIx for flybywiresim#8639 * Update .github/CHANGELOG.md Co-authored-by: Michael Corcoran <tracer@outlook.co.nz> * Update A32NX_FMCMainDisplay.js removed comments and extra conversion * adding to make sure an engine is running in AOC * additional gw safety Added an additional check to check for block fuel being entered to prevent "A32NX_FM_GROSS_WEIGHT" from being Null * updates per @BlueberryKing suggestions Add .isFinite type safety for and isAnEngineOn() logic instead of currentFoB() * forgot to remove 106->108..doh! * Fix formatting --------- Co-authored-by: Michael Corcoran <tracer@outlook.co.nz> Co-authored-by: BBK <22713769+BlueberryKing@users.noreply.github.com>
#8639
Fixes #[issue_no]
Summary of Changes
Added 2 lines:
1.A check to see if an engine is running->Pull from sensed fuel tank levels.
2. Engine !Running -> Pull from entered block fuel
3. Also added some class and method comments noting where top level parameters originate from...spent many hours on a wild goose chase on this PR! Recommend inserting into docs somewhere down the line.
Screenshots (if necessary)
References
@BlueberryKing and @BravoMike99.
As previously stated I no longer have access to the pilot Discord channel, unable to use previous verification method.
Additional context
Discord username (if different from GitHub):
Testing instructions
Prior to engine start, the Destination EFOB should be pulling from entered block fuel, after start the figure should switch over to using sensed fuel level in the tanks.
How to download the PR for QA
Every new commit to this PR will cause new A32NX and A380X artifacts to be created, built, and uploaded.