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

Path reservation fixes #115

Draft
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

gfgit
Copy link
Contributor

@gfgit gfgit commented Apr 19, 2024

Some experiments with path reservation system.
Based on #113

Some issues already noted in #114

@gfgit gfgit force-pushed the work/gfgit/path_release branch 2 times, most recently from 1873009 to 5b1a08a Compare April 22, 2024 09:38
@gfgit gfgit mentioned this pull request Apr 22, 2024
@reinder
Copy link
Member

reinder commented Apr 22, 2024

Maybe we can add some tests to check e.g. reservation and changing state of a direction tile while it is reserved.

@reinder
Copy link
Member

reinder commented Apr 23, 2024

I created a small test for the direction tile state change while it is reserverd, can you apply the patch to you branch?
0001-test-added-Direction-path-reservation-using-NX-and-c.zip

I zipped it because GitHub refuses to attach the PATCH file...but the error explains that PATCH is supported 🤷🏼‍♂️

@gfgit
Copy link
Contributor Author

gfgit commented Apr 25, 2024

Hi, I've applied your patch.
Also I've noticed sometimes NX Buttons work even if block rail tile is not adjacent, like NX Button + straight rail + Block tile.
Sometimes it does not work. But when I read the findBlock() code I think it is supposed to never work.

@reinder
Copy link
Member

reinder commented Apr 25, 2024

Awesome.

It is indeed required to be adjacent to a block but, passive tiles are ignored. On the boardModified event a new graph network is built of the board. It only creates nodes for tiles that have a function, passive tiles like straight en curved rail pieces are ignored. So for a NX button there may be straight/curved rail pieces between them, but not something like a signal/turnout/direction control etc.

@gfgit
Copy link
Contributor Author

gfgit commented Apr 25, 2024 via email

@reinder
Copy link
Member

reinder commented Apr 26, 2024

A bridge is a node (due to two seperate reservation paths), so that's why it doesn't work, That is a bug, a bridge is a passive tile so it should be allowed. Going to fix that.

@reinder
Copy link
Member

reinder commented Apr 26, 2024

If you merge/rebase latest master the bridge nx button issue is fixed :)

@gfgit
Copy link
Contributor Author

gfgit commented Jun 11, 2024

Rebased on current master.
Some commits are really trivial fixes which could be merged already, while others contain more complex changes on which we should discuss.
If you want I can split in 2 distinct PRs

@gfgit
Copy link
Contributor Author

gfgit commented Jun 11, 2024

Fixed missing include in blockpath.cpp
Also now trivial changes are moved in first commits while last commits contain complex changes

@reinder
Copy link
Member

reinder commented Jun 11, 2024

Nice improvements, I'm going to do some test with it :)

@gfgit
Copy link
Contributor Author

gfgit commented Jun 12, 2024 via email

- This avoids heap allocation
- Added asserts for timerId to be null before being created again
@reinder
Copy link
Member

reinder commented Jun 12, 2024

Good suggenstion, QTimerEvent :)

Just did some simulations tests, it works as expected :)

I was thinking, should we log a warning if a turnout is modified externally an Traintastic corrects it? Should we also have some kind of max retry to prevent loops? and maybe Estop the train if the max retry count is reached and log an error then too?

For the path release, there must be some kind of delay I think, the train can still be on the turnout but already left the tail block. (Ideally we have occupy sensors on the turnout's too offcourse, but Traintastic should be able to handle both situations.)

@gfgit
Copy link
Contributor Author

gfgit commented Jun 12, 2024

Yes we should log a warning when turnout is externally changed. But I don't know how to add a new log message...
As for the max retry count when it's reached maybe yes we E-stop the train. Although this is a strong action which might not be good for some users. Maybe we add a setting like "E-stop train if on path and turnouts are changed externally".

I think I've added the retry count logic to signals, if it's working fine I'll copy it also to turnouts.

For the path release, I bet very few layouts have dedicated occupy detectors for switches so yes we should workaround it with some kind of delay.
Better yet if we know the real train speed and it's real length we can estimate traveled space and so have a varying delay according to train speed and length plus a security margin.
We don't even have to know path length which would depend on turnout state in case of multiple possible paths for the same pair of NX buttons. We start to count from when exit block gets occupied, until train tail is inside the exit block and entrance block is free.

@reinder
Copy link
Member

reinder commented Jun 12, 2024

Yes we should log a warning when turnout is externally changed. But I don't know how to add a new log message...
I'll explain :)

To add a log message you need to add a enum value in [/shared/src/traintastic/enum/logmessage.hpp](https://github.com/traintastic/traintastic/blob/master/shared/src/traintastic/enum/logmessage.hpp).
In this case it is a train/board related message so it is in the 3xxx range, just use the next free number.
You also need to add at least an English term for the message in, as the server always uses English for the console log.

I've numbered all the log messages so it is easy to look up, in the source and if we receive a screenshot/log text in a language we don't understand :)

As for the max retry count when it's reached maybe yes we E-stop the train. Although this is a strong action which might not be good for some users. Maybe we add a setting like "E-stop train if on path and turnouts are changed externally".

Good suggestion to add a world setting for that, maybe we can add three options:

  • Ignore
  • Estop train (= default?)
  • Stop world

I think I've added the retry count logic to signals, if it's working fine I'll copy it also to turnouts.

I didn't test that yet, but it seems fine, else we can improve it later :)

For the path release, I bet very few layouts have dedicated occupy detectors for switches so yes we should workaround it with some kind of delay. Better yet if we know the real train speed and it's real length we can estimate traveled space and so have a varying delay according to train speed and length plus a security margin. We don't even have to know path length which would depend on turnout state in case of multiple possible paths for the same pair of NX buttons. We start to count from when exit block gets occupied, until train tail is inside the exit block and entrance block is free.

Good suggestions/ideas! I think it is best we move that to a separate PR, then we can finish this one :)

- If position is externally changed while a path is reserve
  Turnout will try to reset it's position to reserved one
  If it fails and reaches maximum retry count it will stop trains in
path.
- When locked and retry count is reached, stop trains in path
- Evaluate only if a path is reserved
@reinder
Copy link
Member

reinder commented Jun 18, 2024

Nice improvements!

Just some thoughts:

  • The log message is Position externally changed while locked in a path shouldn't we add a separate message for the signal? (A signal has an aspect not a position.) Should it also be mentioned that Traintastic is going to correct it?
  • When a Train is E-stopped, I think a error must be logged to explain why the train is E-stopped.

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.

None yet

2 participants