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

Add altitude Delay #124

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

svalencia014
Copy link
Contributor

This PR addresses issue #53 by adding a randomized delay in a similar fashion as the heading delay system.

@mmp
Copy link
Owner

mmp commented Mar 17, 2024

More places to consider:

  • ExpediteDescent() and ExpediteClimb(): they shouldn't issue that 'unable' message if there is a pending altitude change. (Maybe in that case they should start the altitude change immediately anyway.)
  • CrossFixAt() where it does nav.Altitude = NavAltitude{}
  • Halfway through clearedApproach() in the nav.Approach.InterceptState == HoldingLocalizer test.
  • ClimbViaSID, DescendViaSTAR, and a bunch of places in the procedure turn code.

More generally, the tricky thing about this (as was the case with getting deferred turns to work correctly) is that it's necessary to audit all of the code that looks at nav.Altitude, either setting it or clearing it, and think through the implications of having a DeferredAltitude that's going to clobber it a few seconds in the future. Sometimes the action should be based on the deferred altitude, sometimes it's something that should override/clear out the deferred altitude, etc...

@svalencia014
Copy link
Contributor Author

For the first point, the delay would still make sense based on reaction time of the pilots (A pilot wouldn't instantly start dropping altitude as soon as the instruction is transmitted, they would still need to dial it in).

@svalencia014
Copy link
Contributor Author

Following up on this PR, after re-reading your comment here's what makes the most sense:

  • For the first point, keep it as is since there is a delay from when the instruction is issued to when they dial in the altitude and start the expediated descent.
  • For point two, it wouldn't make too much sense, since I would say it's safe to assume that pilots are using automation to cross a fix at a certain altitude (VNAV)
  • For point three, I would argue the same logic as point two, their automation system would most likely be in use when joining the localizer
  • For point four, I would also argue the same logic, except for GA traffic (but that's another tangent)

@mmp
Copy link
Owner

mmp commented May 25, 2024

(Just to clarify, are you saying that changes to the code make sense in those cases, or are you saying you think they're all ok as currently implemented?)

@svalencia014
Copy link
Contributor Author

I would say that leaving those other implementations as is would be fine. I had a few people play-test a build I made with the altitude delay before opening this PR and they noticed no problems.

@mmp
Copy link
Owner

mmp commented May 26, 2024

With your branch, if I give an aircraft a descent and then right afterward give them ED to expedite the descent, I get:
image
So they read back the descent and then claim they aren't descending. In the real world, even if they're still dialing in the new altitude, they're not going to say they're not descending.

So I think that general issue applies to many places in the code: there is a check "is the aircraft changing altitude; if so do X, else do Y" and the check will think they are not changing altitude even though they have been given an instruction to do so...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants