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

chore: cpp framework improvements and additions #8640

Merged
merged 15 commits into from
Sep 13, 2024

Conversation

frankkopp
Copy link
Member

@frankkopp frankkopp commented Apr 30, 2024

Summary of Changes

General additions and improvements to the cpp framework.

  • added arinc library
  • added Google Test GTest framework
  • added unit tests for cpp libraries
  • refactored InterialDampener to a general DampingController
  • additional clang format to better align code for multiple assignment statements
  • gitignore cleanup

Discord username (if different from GitHub): cdr_maverick

Testing instructions

Potentially Pushback, Lighting Presets, Aircraft Presets might be impacted by these changes.
Mainly Pushback (b/o the DampingController refactor).
Other side effects unlikely.

A32NX: see above - test for regression
A380X: see above - test for regression

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.

  1. Make sure you are signed in to GitHub
  2. Click on the Checks tab on the PR
  3. On the left side, find and click on the PR Build tab
  4. Click on either flybywire-aircraft-a320-neo or flybywire-aircraft-a380-842 download link at the bottom of the page

@frankkopp frankkopp self-assigned this Apr 30, 2024
@frankkopp frankkopp added the Not Ready For Review Still draft but needs a GitHub build label Apr 30, 2024
@frankkopp frankkopp marked this pull request as ready for review April 30, 2024 17:09
@2hwk
Copy link
Member

2hwk commented May 3, 2024

Just some early comments, looking good so far.

@frankkopp frankkopp added Needs Code Review and removed Not Ready For Review Still draft but needs a GitHub build labels May 9, 2024
@Popespice
Copy link

Quality Assurance Tester Report

Discord : popespice
Object of testing: 8640
Tier of Testing : 2
Date : 21/8/2024

Testing Process:

  1. Start sim as usual
  2. Load into A32NX
  3. Power up plane as normal
  4. Individually select different presets (lighting and aircraft)
  5. Taxi aircraft through built in taxi mechanism in EFB
  6. Observe all presets work, aircraft flies as expected.

Negatives:
N/A
Testing Results:
Passed
Conclusions:
Working as expected.

@tylerclt
Copy link

Quality Assurance Trainee Report

Discord: Tylerb780#3594
Object of testing: #8640
Tier of Testing: 2
Date: 22/08/2024)

Testing Process:

  1. Loaded into the Aircraft
  2. Powered-up aircraft as normal
  3. Went through each individual lighting and aircraft preset
  4. Pushed using the built-in pushback system
  5. Made sure aircraft flew with no issues or abnormalities

Negatives:
N/A

Testing Results:
Passed

Conclusions:
Working as intended

@@ -30,23 +30,22 @@ class Table1502_A380X {
* @return A 2D array representing the CN2 - correctedN1 pairs.
*/
static constexpr double table1502[13][4] = {
Copy link
Member

Choose a reason for hiding this comment

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

The values in the comments here dont reflect the table values and seem copied from the a32nx

Copy link
Member Author

Choose a reason for hiding this comment

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

see above - did not touch the values

{15000, 8.000, 24.000, 95.686, 89.907, 58.723}, //
{16000, 5.000, 22.000, 96.160, 89.816, 57.189}, //
{16600, 5.000, 22.000, 96.560, 89.816, 57.189}, //
{-2000, 47.751, 54.681, 84.117, 81.901, 63.498}, // row 20
Copy link
Member

Choose a reason for hiding this comment

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

this line seems to belong to the GA part of the table and not the TO part

Copy link
Member Author

Choose a reason for hiding this comment

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

this line seems to belong to the GA part of the table and not the TO part

I did not touch the values (at least not on purpose) - these should be the same as before (from the FADEC rewrite where I also did not touch values.

The changes to this file are only formatting in this PR.

Copy link
Member

Choose a reason for hiding this comment

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

I know I didnt mean to imply it had, just that this is smth that I noticed recently so wondered if you maybe want to quickly change that

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah ok - no problem. If you want you can also just push to the PR. I'm on vacation with no PC.

@pilotseyea350
Copy link

Quality Assurance Trainee Report

Discord: piloteyesA350
Object of testing: #8640
Tier of Testing: 2
Date: 23/08/2024

Testing Process:

  • Aircraft Loaded at EDDM

  • Power Up using Aircraft States

  • Set different cockpit lights configuration and save them

  • Pushback using the EFB Pushback

  • Fly from EDDM to EDDS

  • Monitor systems

  • Have fun
    Negatives:

  • "Ready to pushback" state completion is reliant on ADIRS alignment time which, if set to Real, does not happen until either ADIRS Alignment time is set to "Instant" or ADIRS is aligned. Known Issue but Fast Loading should override EFB Realism settings

  • As discussed in QA Channel, Transition from "Ready to Pushback" to "Ready To Taxi" triggers Master Warning & Master Caution due to Flaps and Slats not being in TO Position

  • Saving a lightning preset and not renaming it prevents it from being used as an automatic loaded Lightning preset. We could also change the Default names of the presets to "No Name 1" for preset 1, "No Name 2" for preset 2, etc...

  • CKPT, FWD CAB & AFT CAB DUCT OVHT ( Ecam message after switching from an Engines running preset to a engines non running preset)

Testing Results:
Not Passed

Conclusions:
ADIRS Issue was known but Master Caution & Master Warning is not expected.

N. B. :
#8581
https://streamable.com/vkq57o

@frankkopp frankkopp force-pushed the cpp-framework-improvements branch 2 times, most recently from e25e357 to 8791046 Compare August 27, 2024 11:52
@frankkopp
Copy link
Member Author

Negatives:

  • "Ready to pushback" state completion is reliant on ADIRS alignment time which, if set to Real, does not happen until either ADIRS Alignment time is set to "Instant" or ADIRS is aligned. Known Issue but Fast Loading should override EFB Realism settings

This is known and intendent for the current implementation - it is improved in #8581 as it required system internal changes.

  • As discussed in QA Channel, Transition from "Ready to Pushback" to "Ready To Taxi" triggers Master Warning & Master Caution due to Flaps and Slats not being in TO Position

Not an issue caused by this PR (already known issue) - but workaround has been put in place.
Please retest this workaround (it basically waits until the Flaps/Slats are deployed before pressing T.O CONFIG)

This is improved/fixed in #8581 as it required system internal changes.

  • Saving a lightning preset and not renaming it prevents it from being used as an automatic loaded Lightning preset. We could also change the Default names of the presets to "No Name 1" for preset 1, "No Name 2" for preset 2, etc...

How is this prevented? Could not reproduce it.
(Likely also outside this PR)

  • CKPT, FWD CAB & AFT CAB DUCT OVHT ( Ecam message after switching from an Engines running preset to a engines non running preset)

Could not be reproduced - please re-test with latest PR build.

Testing Results: Not Passed

Conclusions: ADIRS Issue was known but Master Caution & Master Warning is not expected.

@pilotseyea350
Copy link

I'll try again this afternoon with the latest build

@pilotseyea350
Copy link

pilotseyea350 commented Sep 10, 2024

Just tested it and it did work with the auto-preset off. Will do a T2 asap to alow you to merge

@pilotseyea350
Copy link

Capture d'écran 2024-09-11 220459
Is the vertical profile green dot below the selected altitude expected ?

@frankkopp
Copy link
Member Author

Capture d'écran 2024-09-11 220459

Is the vertical profile green dot below the selected altitude expected ?

The cpp code of this PR does not touch any of this. Its really only used in the backend (not front end / UI) for Lighting Preset, Aircraft Preset, pushback, FADEC.
Frontend is in Typescript which is why the Automatic Lighting presets is also not touched as this logic is TS only.

@tracernz
Copy link
Member

tracernz commented Sep 12, 2024

And it's also perfectly fine, so no issue 👍 . The vertical profile doesn't take into account the selected altitude.

@pilotseyea350
Copy link

Quality Assurance Trainee Report

Discord Username : PilotEyesA350
Object of testing : #8640
Aircraft : A32NX
Tier of Testing : 2
Date : 11/09/2024

Testing Process:

  • Load aircraft at gate
  • Follow A320 flows
  • Fly from EDDF to EDDF using a Simbrief Flightplan
  • Monitor systems

Testing Results:
Passed

Negatives:
N/A

Conclusions:
After much work it finally works as intended !

Media:
N/A

@frankkopp
Copy link
Member Author

Quality Assurance Trainee Report

Discord Username : PilotEyesA350 Object of testing : #8640 Aircraft : A32NX Tier of Testing : 2 Date : 11/09/2024

Testing Process:

  • Load aircraft at gate
  • Follow A320 flows
  • Fly from EDDF to EDDF using a Simbrief Flightplan
  • Monitor systems

Testing Results: Passed

Negatives: N/A

Conclusions: After much work it finally works as intended !

Media: N/A

Thanks - your effort is very much appreciated!!

@frankkopp frankkopp merged commit c4ab989 into flybywiresim:master Sep 13, 2024
7 checks passed
@frankkopp frankkopp deleted the cpp-framework-improvements branch September 13, 2024 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ✔️ Done
Development

Successfully merging this pull request may close these issues.

8 participants