Signal System Issues - Githubissues

dsand47 commented 1 year ago

There are a number of aspect.xml files in the JMRI distribution that have errors in the content that will affect the aspect mapping process. Aspect mapping uses the aspectMapping in the appearance file to select the appropriate ourAspect based on the destination signal advancedAspect.

Multiple entries.

The schema for aspect.xml allows multiple speed and speed2 elements. When the aspect.xml file is being loaded, the last entry is used. For example, BNSF-1996 has two entries for the Clear aspect.

      <speed>Normal</speed>
      <speed>Slow</speed>

This results in a mast speed of 30 instead of 100.

Invalid speed and speed2 values.

A valid speed value is an integer greater than or equal to zero or a string that has a match in the element in signalSpeeds.xml.

If the element is empty, the first ourAspect will be used. If the element is an invalid value, the mast speed will be set to zero.

The attached file has a list of the invalid speed and speed2 values.

speeds.txt

danielb987 commented 1 year ago

The schema for aspect.xml allows multiple speed and speed2 elements. When the aspect.xml file is being loaded, the last entry is used. For example, BNSF-1996 has two entries for the Clear aspect.

Should the schema file only allow one speedand one speed2?

What's the difference between speed and speed2? When is speed2 used?

bobjacobsen commented 1 year ago

One more thing: There are two schema files for appearance files: appearancetable.xsd and appearancetable-no-check.xsd. The -no-check version omits the checks for

each specificappearances aspect element must refer to an existing appearance aspectname.
each ourAspect element must refer to an existing appearance aspectname

Back in early 2020 (?), a bunch of clean up was done on the appearance files. Most of it was automatic, but fixing those two problems is a manual operation so those problems were just bypassed by using the two-schema hack.

bobjacobsen commented 1 year ago

Should the schema file only allow one speed and one speed2?

Right now, the appearance.xsd schema is explicitly allowing multiple speed and speed2 elements:

                    <xs:element name="speed" type="xs:string" minOccurs="1" maxOccurs="unbounded"></xs:element>
                    <xs:element name="speed2" type="xs:string" minOccurs="1" maxOccurs="unbounded"></xs:element>

When I change that to maxOccurs=1 (thereby requiring exactly one of each element), two files fail:

xml/signals/CCOR-1967/aspects.xml
xml/signals/BNSF-1996/aspects.xml

Do we know that there really should be only one of these elements? If so, I'd be happy to PR this.

devel-bobm commented 1 year ago

Do we know that there really should be only one of these elements?

jmri.implementation.SignalSpeedMap.getAspectSpeed() seems to get the signal system aspect's "speed" property. That method does not provide a mechanism to select "one out of many".

jmri.implementation.SignalSpeedMap.getAspectExitSpeed() seems to get the signal system aspect's "speed2" property. That method does not provide a mechanism to select "one out of many".

To me it implies a maximum of one "speed" element. To me it implies a maximum of one "speed2" element.

It is unclear to me whether it makes sense for a given aspect to have a "speed2" element without a "speed" element. Someone who has a better understanding of the aspect-signaling implementation would need to weigh-in on this question.

bobjacobsen commented 1 year ago

The code for reading the aspect files reads each element, storing the content. I think this means that only the last speed and speed2 elements will be retained.

So that's what those two signal system definitions are doing now.

The question is whether that's the right thing, i.e. what the author intended, for those signal definitions to do.

bobjacobsen commented 1 year ago

It is unclear to me whether it makes sense for a given aspect to have a "speed2" element without a "speed" element. Someone who has a better understanding of the aspect-signaling implementation would need to weigh-in on this question.

The existing schema insists on at least one speed and speed2 elements for every definition. All the existing files pass that check, so they're all there.

dsand47 commented 1 year ago

Here are the direct references to the speed and speed2 properties:

das@dmba jmri % grep -R -E '^.*getProperty\(.*speed.*$' * 
implementation/SignalSpeedMap.java:        String property = (String) system.getProperty(aspect, "speed");
implementation/SignalSpeedMap.java:        String property = (String) system.getProperty(aspect, "speed2");
implementation/DefaultSignalSystem.java:                String speed = (String) getProperty(as, "speed");
implementation/DefaultSignalSystem.java:                retval.append("\n         ").append(getProperty(value, "speed"));
implementation/DefaultSignalMastLogic.java:                            String strSpeed = (String) getSourceMast().getSignalSystem().getProperty(advancedAspect[i], "speed");
implementation/AbstractSignalMast.java:        this.speed = (String) getSignalSystem().getProperty(aspect, "speed");
implementation/DefaultRailCom.java:        Integer t = (Integer)getProperty("actualspeed");
jmrit/dispatcher/AutoActiveTrain.java:            String aspectSpeedStr = (String) _controllingSignalMast.getSignalSystem().getProperty(displayedAspect, "speed");
jmrix/can/cbus/CbusCabSignal.java:            // String speed = (String) mast.getSignalSystem().getProperty(mast.getAspect(), "speed");
jmrix/openlcb/OlcbSignalMast.java:        this.speed = (String) getSignalSystem().getProperty(aspect, "speed");
jmrix/loconet/LNCPSignalMast.java:        this.speed = (String) getSignalSystem().getProperty(aspect, "speed"); // NOI18N
jmrix/loconet/LnCabSignal.java:           String speed = (String) mast.getSignalSystem().getProperty(mast.getAspect(), "speed");

The only indirect references via SignalSpeedMap are from the OBlock/Portal/OPath/Warrants system:

das@dmba jmri % grep -R getAspectExitSpeed *    ( speed2 )
implementation/SignalSpeedMap.java:    public String getAspectExitSpeed(@Nonnull String aspect, @Nonnull jmri.SignalSystem system) {
jmrit/logix/Portal.java:            speed = jmri.InstanceManager.getDefault(SignalSpeedMap.class).getAspectExitSpeed(aspect, signal.getSignalSystem());

das@dmba jmri % grep -R getAspectSpeed *    ( speed )
implementation/SignalSpeedMap.java:    public String getAspectSpeed(@Nonnull String aspect, @Nonnull jmri.SignalSystem system) {
implementation/SignalSpeedMap.java:        log.debug("getAspectSpeed: aspect={}, speed={}", aspect, property);
implementation/SignalSpeedMap.java:        log.debug("getAspectSpeed: aspect={}, speed2={}", aspect, property);
jmrit/logix/Warrant.java:            speedType = jmri.InstanceManager.getDefault(SignalSpeedMap.class).getAspectSpeed(aspect,
jmrit/logix/SCWarrant.java:                speed = _speedMap.getAspectSpeed(aspect, ((SignalMast) _nextSignal).getSignalSystem());
jmrit/logix/Portal.java:            speed = jmri.InstanceManager.getDefault(SignalSpeedMap.class).getAspectSpeed(aspect, signal.getSignalSystem());

dsand47 commented 1 year ago

The question is whether that's the right thing, i.e. what the author intended, for those signal definitions to do.

This entry in BNSF-1996 is clearly an error:

      <name>Clear</name>
      <rule>Rule 9.1.3</rule>
      <indication>Proceed</indication>
      <speed>Normal</speed>
      <speed>Slow</speed>
      <speed2>Normal</speed2>
      <route>Normal</route>
      <dccAspect>22</dccAspect>
    </aspect>

Ideally, the authors of the two affected systems need to identify the correct speed values. Once the aspect.xml files have been updated, there is the potential for unexpected results on layouts that used one of the systems.

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. JMRI is governed by a small group of maintainers which means not all opened issues may receive direct feedback.

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. JMRI is governed by a small group of maintainers which means not all opened issues may receive direct feedback.

bobjacobsen commented 1 year ago

I'd like to make some progress on this, and am happy to edit signal files.

I propose to make the following substitutions for invalid speed and speed2 values:

Clear -> Normal
Diverging -> Medium
- Restricting -> Restricted
40 km/h -> Slow
Reverse -> RestrictedSlow
Push slowly -> RestrictedSlow
Push speedily -> Restricted
Shunting -> Restricted

Some of those are not obvious, but since non-supported strings -> 0 I don't think that'll be a problem in practice.

Once these are done, I can tighten up the schema checks.

Feedback welcome! Will start work on this is a day or two.

danielb987 commented 1 year ago

This is outside of my knowledge, but it seems good to me.

40 km/h -> Slow

Where did you found this? On Swedish railroads, the signals tell Kör 40, (drive 40 km/h), when the speed is restricted.

bobjacobsen commented 1 year ago

CSD-1962-vlozena/aspects.xml has three 40 km/h speeds in it. That's the only ones I've found. Should I change those to Restricted?

danielb987 commented 1 year ago

CSD-1962-vlozena/aspects.xml has three 40 km/h speeds in it. That's the only ones I've found. Should I change those to Restricted?

@sidlo64 Can you check this thread? What's your thoughts about this?

danielb987 commented 1 year ago

CSD-1962-vlozena/aspects.xml has three 40 km/h speeds in it. That's the only ones I've found. Should I change those to Restricted?

Which are the valid options? Are these hardcoded in JMRI or defined in the signal xml files? ČSD is Czechoslovakia railroad which is in Europe, so I would not be surprised if their signas are similar to the Swedish signals. But I'm not sure, which is why it would be good if @sidlo64 answers it since he is the author of this signal system.

But would it be possible and reasonable to add 40 km/h as a valid aspect if desirable?

sidlo64 commented 1 year ago

@danielb987 thank you for the warning. The errors are in the files CSD-1962-vlozena/aspects.xml and CSD-1962-mechaniky/aspects.xml. Valid values are: Normal -- route speed Limited -- 80 km/h Medium -- 60 km/h Slow -- 40 km/h Stop -- stop For warrants to function properly, the Preferences - Warrants - Aspect Speed Name to Value Mapping table must be edited. Should I do the repair or will you do it?

bobjacobsen commented 1 year ago

@sidlo64 Thanks! I'll correct the speeds and make a PR shortly. Then it would be great if you could update the Aspect Speed to Value Mapping table.

sidlo64 commented 1 year ago

@bobjacobsen thank you.

I was wrong about the warrants. JMRI is a system with many options. Some options may not have been intended by the authors.

For example - warrants were designed with Restricted, Slow, Medium, Limited speeds. Later someone added the Fifty, Sixty speeds. I didn't want to expand the number of speeds in the table. Therefore, users of the ČSD signaling system (and its descendants) must edit the mapping table, see https://sites.google.com/site/sidloweb/rozkazy/nastaven%C3%AD-prost%C5%99ed%C3%AD

Or who wants to use the Control Panel Editor in the style of ČSD JOP, should use the catalog according to the pattern https://www.jmri.org/resources/icons/panels/CSD/JOP/template_1/template_1_cs.html

etc

The table mapping is correct

bobjacobsen commented 1 year ago

Closed as completed by #11887

JMRI / JMRI

Signal System Issues #11654

Multiple entries.

Invalid speed and speed2 values.