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

refactor(fws): Port PseudoFWC to non-React framework #7736

Merged
merged 36 commits into from Feb 11, 2023

Conversation

Eearslya
Copy link
Contributor

@Eearslya Eearslya commented Jan 26, 2023

Summary of Changes

This PR continues the already existing work on the pseudo-fwc-rewrite branch to move the PseudoFWC code away from React, due to performance and stuttering issues.

Due to the ongoing work with the Rust FWS, @Benjozork and I briefly discussed and decided to rebase the work onto the experimental branch due to a few FWS features that haven't made it to master yet.

The plan is to fully port the PseudoFWC to experimental, then make the minor modifications needed to get it compatible with master in a separate PR.

After further discussion, we've decided to target master first instead. However, moving to experimental should be as simple as uncommenting the FWC warnings and renaming a couple simvars.

Screenshots (if necessary)

References

Existing PseudoFWC code

Additional context

Discord username (if different from GitHub): Eearslya#7831

Testing instructions

If everything has been done correctly, this PR should have no visible change to the aircraft at all, other than improved performance and reduced stuttering. Tests should be focused on ensuring that all ECAM memos and warnings appear when they're expected to. Testing should also ensure that warnings can be silenced, cleared, and recalled as appropriate.

  1. When loading and starting the aircraft, ensure there are no yellow or red ECAM warnings.
  2. Two minutes after both engines have started, ensure the TAKEOFF checklist memo appears.
  3. Set up a Flaps 1 takeoff in the MCDU PERF page. Leave flaps at 0. Observe that pressing T.O CONFIG generates a warning.
  4. Set flaps 1, ensure correct V-speeds are set in MCDU PERF and press T.O CONFIG again. Observe T.O CONFIG NORMAL memo.
  5. Perform a normal takeoff and observe the following:
    • T.O INHIBIT memo appears after 3 seconds of takeoff thrust being applied.
    • T.O INHIBIT memo disappears after reaching approximately 1,500 feet of altitude.
  6. In flight, test a few warnings and ensure they function as expected. e.g. Deploy flaps while at speed to create an OVERSPEED warning.
  7. Use the Failures menu of the EFB to cause a non-critical failure of a system such as ELAC 1 or SEC 1.
  8. Using the CLR and RCL buttons below the ECAM, ensure the warning message can be dismissed and recalled. Disable the failure on the EFB menu.
  9. When coming in for landing, the purple LDG INHIBIT memo should appear below 800 feet of altitude.

How to download the PR for QA

Every new commit to this PR will cause a new A32NX artifact to be created, built, and uploaded.

  1. Make sure you are signed in to GitHub
  2. Click on the Checks tab on the PR
  3. On the left side, click on the bottom PR tab
  4. Click on the A32NX download link at the bottom of the page

@Eearslya Eearslya marked this pull request as ready for review January 28, 2023 06:17
@github-actions github-actions bot added this to 🟡 Code Review: Ready for Review in Quality Assurance Jan 28, 2023
@Eearslya Eearslya changed the base branch from experimental to master January 29, 2023 01:11
@2hwk 2hwk added this to the v0.10.0 milestone Jan 29, 2023
@2hwk 2hwk added the QA Tier 2 label Feb 1, 2023
@Eearslya Eearslya force-pushed the pseudo-fwc-rewrite branch 4 times, most recently from e3dac3f to 3e5ce7a Compare February 5, 2023 21:29
Copy link
Member

@Saschl Saschl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some initial comments.

I appreciate the effort very much :)

fbw-a32nx/src/systems/instruments/src/EWD/index.tsx Outdated Show resolved Hide resolved
fbw-a32nx/src/systems/instruments/src/EWD/NewPseudoFWC.ts Outdated Show resolved Hide resolved
fbw-a32nx/src/systems/instruments/src/EWD/NewPseudoFWC.ts Outdated Show resolved Hide resolved
fbw-a32nx/src/systems/instruments/src/EWD/NewPseudoFWC.ts Outdated Show resolved Hide resolved
fbw-a32nx/src/systems/instruments/src/EWD/NewPseudoFWC.ts Outdated Show resolved Hide resolved
fbw-a32nx/src/systems/instruments/src/EWD/index.tsx Outdated Show resolved Hide resolved
fbw-a32nx/src/systems/instruments/src/EWD/NewPseudoFWC.ts Outdated Show resolved Hide resolved
Copy link
Member

@Saschl Saschl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't check all the warnings thoroughly, but the as the logic was already there and the conditions make sense to me, I approve

Quality Assurance automation moved this from 🟡 Code Review: Ready for Review to 🟣 QA Team Review: Ready to Test Feb 7, 2023
Copy link
Member

@beheh beheh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason for the file rename to NewPseudoFWC? I think keeping the filename as-is is probably just fine.

@Saschl
Copy link
Member

Saschl commented Feb 9, 2023

Quality Assurance/Dev Tester Report

Discord : saschl
Object of testing: #7736
Tier of Testing : 1
Date : 11/01/2023

Testing Process:
Full fight LOWI-ENGM

Performed all scenarios from the testing instructions and performed general obersvation.

LGTM but one message + one question.

After a message has been recalled once, It cannot be cleared again. Is that intentional?

Negatives:
PRED/WS message appears in wrong conditions. I left a comment, I think there is a small error in the condition.

Testing Results:
Passed

Conclusions:
LGTM

Media:

@Saschl
Copy link
Member

Saschl commented Feb 9, 2023

Quality Assurance/Dev Tester Report

Discord : saschl
Object of testing: #7736
Tier of Testing : 2

Testing Process:
Full fight LOWI-ENGM

Performed all scenarios from the testing instructions and performed general observation.

LGTM but one question.

After a message has been recalled once, It cannot be cleared again. Is that intentional?

Negatives:
None

Testing Results:
Recall logic confirmed.

Conclusions:
LGTM

Media:

@Eearslya
Copy link
Contributor Author

Eearslya commented Feb 9, 2023

Is there a reason for the file rename to NewPseudoFWC? I think keeping the filename as-is is probably just fine.

An artifact from when both FWC files existed in the repo at once. I've renamed it now.

@brewers10
Copy link

Quality Assurance Trainee Report

Discord : brewers_10#5706
Object of testing: #7736
Tier of Testing : 2
Date : 09/02/2023

Testing Process:

Loaded into stand 1A at EHRD for full flight to EGGP. Followed standard start-up procedures checking for the following:

  1. When loading and starting the aircraft, ensure there are no yellow or red ECAM warnings. OK

  2. Two minutes after the first engine has started, ensure the TAKEOFF checklist memo appears. FAIL - Waited 5 mins until i manually had to trigger the memo to appear

  3. Set up a Flaps 1 takeoff in the MCDU PERF page. Leave flaps at 0. Observe that pressing T.O CONFIG generates a warning until the button is released. - FAIL No permanent warning. Just made a singular "beep" sound when T.O. Config was pressed

  4. Set flaps 1, ensure correct V-speeds are set in MCDU PERF and press T.O CONFIG again. Observe T.O CONFIG NORMAL memo. OK

  5. Perform a normal takeoff and observe the following:
    T.O INHIBIT memo appears after 3 seconds of takeoff thrust being applied. OK
    T.O INHIBIT memo disappears after reaching approximately 1,500 feet of altitude. OK

  6. In flight, test a few warnings and ensure they function as expected. e.g. Deploy flaps while at speed to create an OVERSPEED warning. OK

  7. Use the Failures menu of the EFB to cause a non-critical failure of a system such as ELAC 1 or SEC 1. OK Tested overspeed, ELAC 1 + 2 SEC 1 + 2 + 3 and FCDC 1 + 2. All clearable and recallable :)

  8. Using the CLR and RCL buttons below the ECAM, ensure the warning message can be dismissed and recalled. Disable the failure on the EFB menu. OK

  9. When coming in for landing, the purple LDG INHIBIT memo should appear below 800 feet of altitude. OK

Negatives:
Not quite sure what happened on the ground at EHRD. Started engine 2 first as per the majority of SOPs. Waited 2 mins as timed by the chrono and nothing appeared. Waiting until 5 mins before manually turning it on. Also the T.O CONFIG warning was not working as anticipated. Didn't appear when clicked and occasionally briefly flashed on the ECAM with a small "ding" to accompany it.

Testing Results:
Not Passed

Conclusions:
Everything else with the flight went well. Just those two events on the ground need reviewing,

@Eearslya
Copy link
Contributor Author

Eearslya commented Feb 9, 2023

Thanks much for the testing. I think I've found what's causing both issues, and I'm not sure they're in the scope of this PR.

Two minutes after the first engine has started, ensure the TAKEOFF checklist memo appears. FAIL - Waited 5 mins until i manually had to trigger the memo to appear

This appears to be an error in the current TypeScript FWC system, which apparently requires both engines to be started for at least 2 minutes before activating the takeoff memo.

EDIT: Scratch that. This is entirely normal behavior, my instructions were faulty. The FWC enters phase 2 when 1 engine is started, but the 120 second timer for T.O MEMO only starts when both engines are running.

Set up a Flaps 1 takeoff in the MCDU PERF page. Leave flaps at 0. Observe that pressing T.O CONFIG generates a warning until the button is released. - FAIL No permanent warning. Just made a singular "beep" sound when T.O. Config was pressed

As of right now, the T.O CONFIG button is programmed to be momentary, meaning it cannot be held down. Part of the Rust FWC PR changed that so that CLR and T.O CONFIG can be held down. As far as I understand it, the warnings generated by a bad config are only supposed to be displayed while T.O CONFIG is held. Since the button currently cannot be held, it explains the very brief warning you're getting.

This issue is resolved with the upcoming rewrite of the FWC in Rust, so I'm not sure whether trying to fix it here is within the scope of the PR. Any opinions from the dev team?

@Saschl
Copy link
Member

Saschl commented Feb 9, 2023

This issue is resolved with the upcoming rewrite of the FWC in Rust, so I'm not sure whether trying to fix it here is within the scope of the PR. Any opinions from the dev team?

Imo, if it already was like this before the rewrite, it is fine like this. Also did my tests based on the knowledge of the limitations.

@Eearslya
Copy link
Contributor Author

Eearslya commented Feb 9, 2023

After a brief conversation with @Saschl, it was found that because the T.O CONFIG button is currently momentary, the solution was to make the warnings appear on screen permanently until cleared or resolved. I accidentally copied a couple extra lines from the experimental version of PseudoFWC which was meant to be used when the button can be held. Since adding in new XML behaviors would probably need even more testing, I've simply removed the extra lines to restore the same behavior that the current development version has.

@tracernz
Copy link
Member

  • SLATS/FLAPS NOT IN T.O CONFIG will not trigger a second time without going to the desired config first.
  • SLATS/FLAPS NOT IN T.O CONFIG triggers by itself when moving out of the correct config, should need to push the button.
  • Can't get green low pressure or reservoir empty to generate a warning.. didn't try the others
    image

@Saschl
Copy link
Member

Saschl commented Feb 10, 2023

SLATS/FLAPS NOT IN T.O CONFIG will not trigger a second time without going to the desired config first.
SLATS/FLAPS NOT IN T.O CONFIG triggers by itself when moving out of the correct config, should need to push the button.

I could reproduce both on the current dev version as well.

@Eearslya
Copy link
Contributor Author

Eearslya commented Feb 10, 2023

SLATS/FLAPS NOT IN T.O CONFIG will not trigger a second time without going to the desired config first.
SLATS/FLAPS NOT IN T.O CONFIG triggers by itself when moving out of the correct config, should need to push the button.

Like Saschl said, this is a result of the fact that the current system "fakes" being able to hold the T.O CONFIG button and makes the warnings permanent. This is consistent with current behavior on master. It's not correct behavior, but fixing it is outside the scope of this PR, I think.

Can't get green low pressure or reservoir empty to generate a warning.. didn't try the others

Hydraulics warnings seem to be in a very weird place right now, but I did port them over 100% faithfully. I'm also unable to create green or blue warnings, same as you. Mostly because it requires 2 things: The "green reservoir low" var, and the "green pump is commanded off" var. However, commanding the green pump off also inhibits the reservoir low signal. So these warnings cannot be generated at all with the current system...

Both of these are issues that exist within the current system and simply got ported over. We can work on fixing them with the Rust FWC branch.

@frankkopp frankkopp dismissed tracernz’s stale review February 11, 2023 18:41

has been addressed

Quality Assurance automation moved this from 🔴 Code Review: In progress to 🟣 QA Team Review: Ready to Test Feb 11, 2023
@frankkopp frankkopp enabled auto-merge (squash) February 11, 2023 18:41
@frankkopp frankkopp merged commit 591eb17 into flybywiresim:master Feb 11, 2023
Quality Assurance automation moved this from 🟣 QA Team Review: Ready to Test to ✔️ Done Feb 11, 2023
@crocket63
Copy link
Contributor

After double-checking the logic and making a couple fixes, I managed to get the green system to give me a warning.

image

However, the conditions for the blue system are a lot more convoluted, requiring a very specific situation that I'm not sure we even have a failure for yet. It requires that the blue pump is on, is not overheating, is not low on air, is not low on fluid, BUT somehow has low pressure indicated. I've implemented this logic, but I'm not sure how to trigger it with our current list of failures.

It's for a low pressure fault is it?
The conditions you are quoting are indeed hard to get. Will be the case if the pump is losing lot of efficiency (which we can do but only through overheat for now), or if you have a hydraulic leak specifically in the pump section.

igor8518 pushed a commit to igor8518/a32nx that referenced this pull request Feb 12, 2023
Co-authored-by: Benjamin Dupont <benjozorkfr@gmail.com>
Co-authored-by: Saschl <sascharudolf46@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

9 participants