-
Notifications
You must be signed in to change notification settings - Fork 46
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
core: add ETCS SVL logic to ETCS braking simulator #10402
base: dev
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## dev #10402 +/- ##
==========================================
- Coverage 80.66% 80.64% -0.03%
==========================================
Files 1098 1080 -18
Lines 111787 110825 -962
Branches 744 745 +1
==========================================
- Hits 90176 89375 -801
+ Misses 21568 21407 -161
Partials 43 43
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
5bfa910
to
8350dfe
Compare
5fd6dac
to
e93a30e
Compare
29d84bb
to
6212f99
Compare
ec6da6b
to
390b821
Compare
a8a8bb4
to
ce5a895
Compare
ce5a895
to
7af533a
Compare
core/envelope-sim/src/main/kotlin/fr/sncf/osrd/envelope_sim/etcs/ETCSBrakingCurves.kt
Outdated
Show resolved
Hide resolved
core/envelope-sim/src/main/kotlin/fr/sncf/osrd/envelope_sim/etcs/ETCSBrakingCurves.kt
Show resolved
Hide resolved
core/envelope-sim/src/main/kotlin/fr/sncf/osrd/envelope_sim/etcs/Constants.kt
Outdated
Show resolved
Hide resolved
core/envelope-sim/src/main/kotlin/fr/sncf/osrd/envelope_sim/etcs/Constants.kt
Outdated
Show resolved
Hide resolved
core/envelope-sim/src/main/kotlin/fr/sncf/osrd/envelope_sim/etcs/ETCSBrakingCurves.kt
Outdated
Show resolved
Hide resolved
core/envelope-sim/src/main/kotlin/fr/sncf/osrd/envelope_sim/etcs/ETCSBrakingCurves.kt
Show resolved
Hide resolved
core/envelope-sim/src/main/kotlin/fr/sncf/osrd/envelope_sim/etcs/ETCSBrakingCurves.kt
Outdated
Show resolved
Hide resolved
7af533a
to
1e7ce69
Compare
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.
@bougue-pe : fixed in e3dbe47 and 1e7ce69.
cb65815
to
19652df
Compare
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.
👏 Nice, thank you very much for the work !
Only nits and not-mandatory comments (or even just ideas for the future): no problem resolving them by yourself once decided/replied 👍
core/envelope-sim/src/main/kotlin/fr/sncf/osrd/envelope_sim/etcs/ETCSBrakingCurves.kt
Outdated
Show resolved
Hide resolved
core/envelope-sim/src/main/kotlin/fr/sncf/osrd/envelope_sim/etcs/ETCSBrakingCurves.kt
Show resolved
Hide resolved
core/envelope-sim/src/main/kotlin/fr/sncf/osrd/envelope_sim/etcs/ETCSBrakingCurves.kt
Show resolved
Hide resolved
core/envelope-sim/src/main/kotlin/fr/sncf/osrd/envelope_sim/etcs/ETCSBrakingCurves.kt
Outdated
Show resolved
Hide resolved
core/envelope-sim/src/main/kotlin/fr/sncf/osrd/envelope_sim/etcs/ETCSBrakingCurves.kt
Show resolved
Hide resolved
Signed-off-by: Erashin <[email protected]>
19652df
to
fb7fde5
Compare
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.
Good job on the tests
I only skimmed through the core changes, I trust PE's review on this
core/envelope-sim/src/main/java/fr/sncf/osrd/envelope_sim/EnvelopeSimPath.java
Outdated
Show resolved
Hide resolved
The path's context only extends until the end of the path. Hence, for an SvL located further along, the tractive effort and the slopes are missing. In this commit, we hack it to make it work, but we could potentially extend the path's context so it goes until the last SvL, if it breaks nothing. Signed-off-by: Erashin <[email protected]>
fb7fde5
to
574a3e7
Compare
first_stop_offset = SVL_STOP_OFFSET | ||
final_stop_offset = 45_060_000 | ||
stop_offsets = [ | ||
0, | ||
first_stop_offset, |
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.
As this SVL_STOP_OFFSET
is not so "test generic", I would remove it and just explicitly use the named value:
first_stop_offset = SVL_STOP_OFFSET | |
final_stop_offset = 45_060_000 | |
stop_offsets = [ | |
0, | |
first_stop_offset, | |
svl_ph1_offset = 41_138_000 | |
final_stop_offset = 45_060_000 | |
stop_offsets = [ | |
0, | |
svl_ph1_offset, |
# Check that the curves does respect the EoA + SvL (EoA = stops), and that there is an | ||
# acceleration then deceleration in between (maintain speed when reach the MRSP). | ||
# Here, the EoA is at 41038m, and the SvL is at the switch at 41138m. | ||
distance_to_svl = 100_000 | ||
first_stop_offset = SVL_STOP_OFFSET - distance_to_svl |
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.
Following the same idea:
# Check that the curves does respect the EoA + SvL (EoA = stops), and that there is an | |
# acceleration then deceleration in between (maintain speed when reach the MRSP). | |
# Here, the EoA is at 41038m, and the SvL is at the switch at 41138m. | |
distance_to_svl = 100_000 | |
first_stop_offset = SVL_STOP_OFFSET - distance_to_svl | |
# Check that the curves does respect the EoA + SvL (EoA = stops), and that there is an | |
# acceleration then deceleration in between (maintain speed when reach the MRSP). | |
# Here, stop (EoA) is 100m before the PH1 switch (SvL). | |
svl_ph1_offset = 41_138_000 | |
first_stop_offset = svl_ph1_offset - 100_000 |
prev_speed = current_speed | ||
|
||
# Check that the braking curve starts and ends at the expected offsets. | ||
offset_start_first_brake = START_SVL_BRAKE_ON_PH1_SWITCH - distance_to_svl |
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.
As mentioned, it looks to me like we shouldn't mutualize it, as it's only "lucky" that both EoA and SvL upper curve parts are under GUI influence and therefore separated by the same value as the EoA-SvL distance.
"Hard-documentation" though variable names seems too strong (and hides a lot of complexity about how the curves are computed) IMO.
Also, the values are not enough "test-generic" IMO (same as the SVL_STOP_OFFSET
).
I wouldn't detail what's happening here at all (or just a comment, if you will).
offset_start_first_brake = START_SVL_BRAKE_ON_PH1_SWITCH - distance_to_svl | |
offset_start_first_brake = 33_361_530 |
🦜 Ditto for the "same location" test above.
// TODO: have a tractive effort curve map which extends until the last SvL instead | ||
// of the end of the path. | ||
context.tractiveEffortCurveMap.get(min(position, context.path.length)) |
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.
This is also part of the "end-of-the-path" hack (tracked it into #11036).
Yet, after thinking of it, we shouldn't have any of the 2 hacks in a separate commit, as the commit without the hacks will break: only one commit for the whole PR would be my pick on this 1️⃣.
Fixes #10404.
Add SVL logic to ETCS braking simulator.
How: compute svl and eoa indications, compute minimum curve, and compare it against overlay.
Limit: approximate braking curves' calculation when SvL is after the end of the path, due to the missing tractive effort and slopes post path.