Topics

Unique behavior of AbstractSensorManager

Bob Jacobsen
 

AbstractSensorManager has a bit of code in it to provide custom handling of the getBySystemName(“..”) method, which I propose to remove.

The getBySystemName(..) method for Sensors _only_ treats a number as a special case:

/** {@inheritDoc}
* Special handling for numeric argument, which is treated as the suffix of a new system name
*/
@Override
public Sensor getBySystemName(@Nonnull String key) {
if (isNumber(key)) {
key = makeSystemName(key);
}
return _tsys.get(key);
}

This treats sensors.getBySystemName(“1”) as a special case that’s not present in e.g. Turnouts or Numbers:

print turnouts.provideTurnout("1")
print turnouts.getBySystemName("1")
print turnouts.getTurnout("1")
print sensors.provideSensor("1")
print sensors.getBySystemName("1")
print sensors.getSensor("1")

On LocoNet, this gets:

LT1
None
None
LS1
LS1
LS1

NCE provides the same behavior; C/MRI also if node 0 is predefined. Some other systems override that behavior:

- MERG CBUS treats sensors like turnouts, overriding the special code:
MT+1
None
None
MS+1
None
None

- OpenLCB logs a "Can't parse OpenLCB Turnout system name: 1” ERROR for the Turnout case and just throws an exception from jmri.jmrix.openlcb.OlcbAddress.toEventID for the sensor case.

Note that it’s OK to use a number for provideTurnout and provideSensor; that’s been a convention for a very long time, and there’s no need to change it.

I propose to remove the irregular behavior from the SensorManager#getBySystemName method because, well, a bare number isn’t a system name.

The question is whether to

1) Remove it now (JMRI 4.19.2) so that it’s like the rest of the Manager forms
2) Remove it now (JMRI 4.19.2) but add a warnOnce that at least there will be a log message saying this changed
3) Add a warnOnce saying it’ll be removed it in future (how long?) and remove it then.

None of our default scripts use it; I don’t find a reference to it in CATS.

I lean toward #2. What do people prefer? Is there another option, and somebody willing to implement it?

Bob

--
Bob Jacobsen
@BobJacobsen

Randall Wood <rhwood@...>
 

Number #1 or #2 seem to be the smartest moves, and since #2 is kinder to a user who reads the logs, I’d go with #2.

Randall

On 03-Jan-2020, at 13:47, Bob Jacobsen <@BobJacobsen> wrote:

AbstractSensorManager has a bit of code in it to provide custom handling of the getBySystemName(“..”) method, which I propose to remove.

The getBySystemName(..) method for Sensors _only_ treats a number as a special case:

/** {@inheritDoc}
* Special handling for numeric argument, which is treated as the suffix of a new system name
*/
@Override
public Sensor getBySystemName(@Nonnull String key) {
if (isNumber(key)) {
key = makeSystemName(key);
}
return _tsys.get(key);
}

This treats sensors.getBySystemName(“1”) as a special case that’s not present in e.g. Turnouts or Numbers:

print turnouts.provideTurnout("1")
print turnouts.getBySystemName("1")
print turnouts.getTurnout("1")
print sensors.provideSensor("1")
print sensors.getBySystemName("1")
print sensors.getSensor("1")

On LocoNet, this gets:

LT1
None
None
LS1
LS1
LS1

NCE provides the same behavior; C/MRI also if node 0 is predefined. Some other systems override that behavior:

- MERG CBUS treats sensors like turnouts, overriding the special code:
MT+1
None
None
MS+1
None
None

- OpenLCB logs a "Can't parse OpenLCB Turnout system name: 1” ERROR for the Turnout case and just throws an exception from jmri.jmrix.openlcb.OlcbAddress.toEventID for the sensor case.

Note that it’s OK to use a number for provideTurnout and provideSensor; that’s been a convention for a very long time, and there’s no need to change it.

I propose to remove the irregular behavior from the SensorManager#getBySystemName method because, well, a bare number isn’t a system name.

The question is whether to

1) Remove it now (JMRI 4.19.2) so that it’s like the rest of the Manager forms
2) Remove it now (JMRI 4.19.2) but add a warnOnce that at least there will be a log message saying this changed
3) Add a warnOnce saying it’ll be removed it in future (how long?) and remove it then.

None of our default scripts use it; I don’t find a reference to it in CATS.

I lean toward #2. What do people prefer? Is there another option, and somebody willing to implement it?

Bob

--
Bob Jacobsen
@BobJacobsen