[Serana]

Csantucci, on 16 August 2020 - 06:58 AM, said:

Derek,
waiting for a cleaner solution by Serana, in OR NewYear MG rev. 72 I have introduced a temporary patch that could solve your crash.

I don't have a better idea. I don't know this part of the code really well.
You can send your fix to the openrails repository. I will validate it.

[roeter]

I'm actually very unhappy with this situation.
This particular method has been around, mostly unchanged, since the creation of the signalling back in 2012. It is also not this method which fails, but it is called with invalid data. If a train calls this method, it must have a position, and if it has a position, it must have a valid TCSectionIndex.
It has worked that way for 8 years without a problem, now there are quite a number of reports of this issue. So something must have changed somewhere, and that something is related to the setting of TCSectionIndex.
Just patching it up isn't going to make this problem go away. If a train, any train, has an invalid TCSectionIndex, it is going to create a problem sooner or later. If you want to patch all uses of TCSectionIndex, you have quite some work to do.
The change which is causing this problem is not in this part of the code, it is probably not in Signals.cs at all. Most likely it is in Train.cs, perhaps TTTrain.cs or even Simulation.cs - as that is where the TCSectionIndex is set.
An error has been made somewhere with regards to the setting of TCSectionIndex. That can happen. But that error should now be traced, rather than just patching it up where the problem manifests itself. It has been indicated which version was still correct, so it can be traced who committed changes since then.

Regards,
Rob Roeterdink

[cjakeman]

roeter, on 30 August 2020 - 01:59 PM, said:

An error has been made somewhere with regards to the setting of TCSectionIndex. That can happen. But that error should now be traced, rather than just patching it up where the problem manifests itself. It has been indicated which version was still correct, so it can be traced who committed changes since then.

Thank you, Rob. I am sure you are right, both with your diagnosis and the consequences of a patch which hides the root cause.

[Csantucci]

Waiting to understand where the error comes from, a warning could be logged when OR finds that PresentPosition[i].TCSectionIndex (where i may be 0 or 1) is = -1. That could be added to the temporary patch. Some parameters could be included in the warning line, which could help understanding in which cases the problem arises. Maybe Rob could suggest the code for the warning line to be added.

[roeter]

I looked at the logfile from post 1, and checked it against the original code.
The error has been made in method CheckRealDeadlock_locationBased (part of the deadlock initiation) in Train.cs. Aparantly, part of the calculation in this method has been rewritten, using a new method named get_CheckDistanceM. There is no such method in the original code.
However, this new method apparantly uses Scanroute for some reason or another. Scanroute uses the TCSectionIndex. This fails, because the deadlock processing is performed before the train position details are set.
What I do not understand is why CheckRealDeadlock_locationBased was changed, and even less why Scanroute is called for this process. Deadlock processing has nothing to do with signals (apart from a limited check if there are signals along the common section) and also has no need to know the actual distance.
It looks like some attempt at refactoring has been done but without understanding the functionality of the code and without checking if it works in all circumstances.

I suggest to revert this change and reinstate the original code for CheckRealDeadlock_locationBased.

Regards,
Rob Roeterdink

[Serana]

Here's the commit that I introduced for the check distance to correspond to the SNCA value of the first signal encountered by the train.
https://github.com/o...f82382576b6d7f3


This modification was introduced because, on high speed lines, the signals far away were not opening due to the lack of reservation of the route, provoking a slowdown of the train until 5 km before the signal.
In this commit, I unified all check distances to be the same (minCheckDistanceM replaced by CheckDistanceM).

I took another look at the log file.
In this case, it seems that the PresentPosition used to the signals to calculate the check distance is unusable during initialization (we are in the InitializeAPTrains function of Simulator class, in the PostInit function of AITrain class)
Here, it seams that, during initialization, PresentPosition[0 or 1].TCSectionindex is still equal to -1 (the train doesn't know where it is) in this phase and the calculation fails.

So I think Carlo's fix is enough to cover this initialization case.

[Klaus]

Hello!

Reading this thread I would ask, if the error I posted - perhaps in the wrong thread - ( http://www.elvastowe...71-and-earlier/ ) is also related to the changes discussed in the upper posts im MG Rel. 71?
Could You clarify this? I could not interpret the errormessages and have no idea to fix the problem.

Thanks Klaus

[Serana]

Hello,

On your side, it seems like it is an error when executing the signal script file (in the route_set).
I don't think it is linked to the topic' subject.

[gpz]

Serana, on 31 August 2020 - 03:59 PM, said:

So I think Carlo's fix is enough to cover this initialization case.

Primarily the code degradation described by Rob should be addressed, replacing that with a proper logic, in my opinion. If that would still not be enough, additionally Carlo's fix could also be applied.

Let us keep in mind, that none of us understand this part of the code satisfyingly, so if Rob says he sees an issue here, then there is an issue.

Rob, maybe you could suggest a proper fix for dissolving the 5 km limit triggered this problem?

[roeter]

The signal limit in Explorer mode is a known issue which has been around since the 'new' signalling code was created.
It was introduced because it was envisaged that explorer mode would only be used to, well, explore the route, hopping about, checking branches and sidings etc., and not to run trains as one would do in 'normal' mode.
Well, apparantly that assumption was wrong - my fault.
I can appreciate the need to change this in explorer mode, but the way this change has been implemented is completely wrong.
I had a look at the new code and I can only assume that it has been set up this way due to a complete misunderstanding of the use of the minCheckDistanceM constant.

So, first I will explain the use of this constant. It's intended use is only for signal initiation. The maximum distance defined in minCheckDistanceM is only intended to avoid a needless search along lengthy paths for signals which probably are not there, or are so far out as not to affect the train when it is started.
Actually, the signalling does not just use this constant, but it uses either this constant or a range depending on max. speed and time required to stop for a signal at danger, whatever distance is further. This choice was introduced to avoid problems with trains entering the sim at speed and not finding the first signal in time.
As soon as the first signal is found, the constant will not be used anymore in any way. From that moment on, the search for further signals in dictated by the signal logic.
A 'side' use for this variable is in the deadlock processing. A check is made for a common section if this section is protected by signals. Again, it's no use checking the full path, so again this variable is used to limit the search. It takes a lot of imagination to assume a signal at certain location is actually protecting a junction some 10 miles away. That's just not how signalling works in real life.

Then to explorer mode. This same constant was used - or abused, if you like - to set up the limits for the checks in explorer mode as explained above. But this certainly is not the main function of this constant.

So, if a change is needed in explorer mode to extend the search, the first thing to do is to drop the use of this constant. This use is not what this constant is intended for, so leave it to its intended function and set up another limit check for explorer.

What this change has introduced is that the search limit for the first signal is based on the location of the first signal. Even writing it down makes no sense.
It makes it all needlessly complicated, and its useless for, as stated, once a signal is found the constant is not used anymore.
So I would strongly suggest to revert this change and restore the constant for use in signal initiation and deadlock processing. It's clear, it's simple, and it has been working without any problems for many years.

That leaves the point of what to do with explorer mode. The method where this is all done is CheckExplorerPath, and that is where the changes should be made, and those changes need to be restricted to this method.
One option is to use the calculation as now introduced, but obviously for explorer mode only. But I would argue against that as it is needlessly complicated.
Another option is to determine the maximum scan range in a similar way as for signal initiation, that is either based on this constant, or on the product of max. speed and required reaction time. The reaction time of the TCS systems in known so this would be a quite simple and clear change.
The best option, however, is simply to remove the scan restriction alltogether. By removing the constant minCheckDistanceM from the call to ScanRoute and replacing it by the value -1, the scan is performed for the full (remaining) route of the train, which is similar as to its use in other modes. The only problem could be the bit of code following the scan, which extands or shrinks the scanned area based on the train's progress along its route. It would need to be tested what should be done with this bit of logic, but it's quite well possible it can be removed completely. Removing the scan limit will sort all problems and will make behaviour of the signalling in explorer mode much more like that in the normal modes.

So this is what I would suggest :

• Revert the changes which introduced the present problems, and restore the constant minCheckDistanceM for signal initiation and deadlock processing.
  
• Remove the scan limit from ScanRoute in CheckExplorerPath.
  
• Test what needs to be done with the logic which expands and shrinks the path in explorer mode, also in CheckExplorerPath.

This will keep the code clean and simple, restores the working of signal initiation and deadlock to what it was, and sorts the problems in explorer mode.
As bonus, behaviour of signalling in explorer mode will be more like that in normal mode.

Regards,
Rob Roeterdink