-
-
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
feat(a380x/vd): Vertical display basic implementation #9831
base: master
Are you sure you want to change the base?
Conversation
c7f6300
to
c06aac1
Compare
# Conflicts: # fbw-a380x/src/systems/systems-host/index.ts
19a987a
to
05b44d7
Compare
if (mode === VerticalMode.ALT_CST || mode === VerticalMode.ALT_CST_CPT || altCstr) { | ||
return '#ff94ff'; | ||
} else if ( | ||
mode === VerticalMode.FINAL || |
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.
There's no FINAL on the A380 although I guess it could replace APP-DES
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.
Yep, but I didn't want to touch this enum here, should be done as part of a bigger FG PR. How about keeping it here to have a reminder on what to change if someone decides to implement F-GS and F-LOC (we don't have APP-DES in the a380)
fbw-a380x/src/systems/instruments/src/ND/VerticalDisplay/VerticalDisplay.tsx
Outdated
Show resolved
Hide resolved
# Conflicts: # fbw-a380x/src/systems/failures/src/a380.ts # fbw-a380x/src/systems/systems-host/index.ts
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.
The VD terr should go inop in polar regions.
}); | ||
|
||
const data: ElevationSamplePathDto = { | ||
pathWidth: 1, |
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.
Not implemented? Should be 1 NM in takeoff and terminal, 2 NM in enroute, and <= 0.5 NM on approach.
-e- You could add a FIXME
for later ofc.
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.
Yep, listed it in the "out of scope" list as "Different TAWS RNP values, using 1nm for now". I'll add a FIXME, needs more verification in SimBridge.
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.
.. And the logic is a lot more complicated, depends on EPU, FMS RNP, predicted RNP, standard TAWS RNP, depending on what we're sampling
|
||
interface AdiruBusBaseEvents { | ||
/** Raw Arinc 429 word. */ | ||
a32nx_adiru_true_track: number; |
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.
a32nx_adiru_true_track: number; | |
a32nx_ir_true_track: number; |
IR and ADR have separate bus outputs, and further they are actually separate computers that talk to each other over ARINC429. The publishers should be arranged around output busses. Thew JSDoc should also specify what data is contained within.
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, I renamed the file to IrBusPublisher.ts and changed everything accordingly. Didn't all of the IR outputs though, only the one I need for now. The PR is massive as it is right now...
type IndexedTopics = 'a32nx_adiru_true_track'; | ||
|
||
type AdiruBusIndexedEvents = { | ||
[P in keyof Pick<AdiruBusBaseEvents, IndexedTopics> as IndexedEventType<P>]: AdiruBusBaseEvents[P]; |
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.
The ADR and IR bus publishers should use a more restrained type that retricts the indices to 1 | 2 | 3. The ADR bus publisher in https://github.com/flybywiresim/aircraft/pull/9659/files#diff-a52ba40e0f76f9fd77751641600f0b1b599c203bc4c018fa4c2515db0065a3ef might be a good reference.
For the topic definitions I largely copy-pasted the a320-simvars.md content and used multi-cursor mode in VSCode to edit it into the desired format.
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.
added as well
@@ -44,6 +45,13 @@ export interface DebugPointPathVector { | |||
|
|||
export type PathVector = LinePathVector | ArcPathVector | DebugPointPathVector; | |||
|
|||
export interface VerticalPathCheckpoint { |
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.
Should this really be in this file? Is this related to path vectors?
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.
It doesn't belong to the a32nx specific defs (hence I'll remove it here), but I think it should be present in fbw-common's PathVector. It's relevant for the vertical path transmission, IMO PathVector.ts could also cover vertical aspects, not only lateral. What's your proposal?
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.
In my branch, I had made the vertical path sending completely separate. And enables via an FMS config option
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 think there are a lot of nice synergies when combining the lateral and vertical path transmission. Maybe in the future it makes sense to de-couple this, when we want to improve the prediction of vertical paths for selected modes, amongst other necessary improvements.
Ah yeah totally, also made that check in the VD for displaying the flag, but it should be at the place where the failure conditions are checked, which is EfisTawsBridge. Moved it there. |
@alexr4339 About "behind FPLN line straight down": Couldn't reproduce it But I'm very sure that we are going to have problems with matching our position to existing trajctories, it's a very tricky problem doing this right. We might have to keep improvements on that for later. |
3c04129
to
6982c8b
Compare
Test run 2 using 6982c8b: Issues: Grey area incorrectly drawn: Another note: See that simbridge has still drawn TERR on ND, even though I just recently turned TERR off VD didnt match VNAV The VD grey zone should darker the TERR as well, which it currently doesnt What did I do? |
@alexr4339 Why are you saying it's "still" too dark? I thought it was too bright before? In your ref image the exposure was pretty low due to a lot of ambient light. In other refs the area looks way brighter. Let's go with #4e4e61 for now, and re-evaluate after merging or when we have more data points. About the grey area: I might have to debug that further. TERR still on ND even if TERR is off: Most likely also a SimBridge cycle refresh issue, can't/won't fix that in this PR. VD didn't match VNAV: Seems like the T/D pseudo waypoint didn't get considered, I'll see whether I'm missing something. |
@flogross89 Just updated the SimBridge build with the new colors |
@flogross89 apologies, meant too bright for sure. I'll re-evaluate once with the new simbridge build! |
Fixes #8728
Fixes #9236
Summary of Changes
Adds an implementation of the vertical display which can be expanded by other contributors
Scope of this PR:
Out of scope (ideas for future PRs):
Screenshots (if necessary)
References
Additional context
Discord username (if different from GitHub):
Testing instructions
You will need a special SimBridge build to test the terrain functionality of the VD: https://we.tl/t-zb8uCnKKPZ
Further test instructions coming soon.
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.