C/MRI infrastructure code needs significant rework

[bobjacobsen]

C/MRI support in current JMRI master has a number of problems.

Formally, that includes these Issues:

• [x] #2827 Cannot create C/MRI connection in 4.6
• [x] #2828 Duplicate C/MRI connection tabs shown in tables

but it also includes a number of JmriUser issues.

• [ ] 2nd C/MRI systems aren't fully functional - some of the 2nd menu doesn't work, and some of the tools are confused about which C/MRI connection to use
• [ ] the Preferences:Defaults pane doesn't seem to properly effect operation
• [x] Reconfiguration, e.g. switching from Sim to Serial and Serial to Sim can sometimes, but not always, lose the node definitions (maybe it should or maybe it shouldn't, but it should be consistent)
• [x] C/MRI is logging errors during normal operation:

   [java] 2017-01-28 14:54:14,983 serialdriver.SerialDriverAdapter      ERROR - getInputStream called before load(), stream not available [main]
   [java] 2017-01-28 14:54:14,983 serialdriver.SerialDriverAdapter      ERROR - getOutputStream called before load(), stream not available [main]
   [java] 2017-01-28 14:54:14,984 jmrix.AbstractMRTrafficController     ERROR - Failed to start up communications. Error was java.lang.NullPointerException [main]

  And there may be problems with the internals that are related to all this:

• [x] A quick check of the initialization sequence shows that the C/MRI code isn't initializing in the same order as other systems. Partly that might be because it has to handle something that other systems don't: Doing node configuration in a Preferences:Connections pane even on initial setup, before there's a working connection. But other parts seem to be just structured differently from other systems.
• [x] We don't have tests for much of this
• [ ] The docs for how this is supposed to work are not in good shape

The problem is that the C/MRI support is important, and it's been getting more broken over the last couple of releases. So we need to show some progress pretty soon. On the other hand, there's enough messed up here that more patches to solve the immediate problem (the two Issues at the top of the list) might make it even harder to get this really fixed.

Having looked at it for a bit, I think we have to invest the time now to fix this. So I've created a bobjacobsen/cmri-init-rework branch to start (re)working on the C/MRI code from the basic level.

So I have some requests to make:

• If you know anything about what's going wrong, e.g. have some notes or a local change, please add a comment here.
• If you do find additional problems in the C/MRI area, please post a comment here.
• Link related PRs and Issues to here (just refer to this Issue's number, that'll do)
• Please discuss before making changes in master to the C/MRI code, defined as anything in jmri.jmrix.cmri or below. This is going to be a big enough change that it'll be hard to eventually merge back if there are multiple independent changes going on, so we have to coordinate that.
• And eventually I'm going to be asking for some help testing this.

Thanks - Bob

[rhwood]

I don't know what's going wrong, but do have a suggestion concerning storing the C/MRI nodes. They can be stored outside the connections element in a cmri-nodes element (possibly handled by a C/MRI-specific PreferencesManager). This would make it possible for a C/MRI connection failure that invalidates the connection to not also blow away the nodes (a problem discussed in #2734) (once the connection is up, it could get the nodes from the PreferencesManager or then load the nodes).

[pabender]

if we're going to refactor the code as @rhwood suggests (and I think that is a good idea), we need to refactor ALL of the node-based systems (except perhaps OpenLCB, and maybe the other CAN based systems).

C/MRI was the first, so most of the rest (powerline,ieee802154,acela,etc) followed the same basic model.

[bobjacobsen]

have a suggestion concerning storing the C/MRI nodes. They can be stored outside the connections element in a cmri-nodes element (possibly handled by a C/MRI-specific PreferencesManager).

Is there a straight-forward way to associate something like that to a single connection?

[rhwood]

On Jan 29, 2017, at 3:34 PM, Bob Jacobsen notifications@github.com wrote:

have a suggestion concerning storing the C/MRI nodes. They can be stored outside the connections element in a cmri-nodes element (possibly handled by a C/MRI-specific PreferencesManager).

Is there a straight-forward way to associate something like that to a single connection?

It would be straight-forward If the XML element structure is roughly (this essentially duplicates the connections XML element structure, but keeps the node handling in a separate element):

cmri-nodes connection systemPrefix=“C1” node name=“N1" … node name=“N2” connection systemPrefix=“C2” node name=“N1" … node name=“N2”

The idea behind using a separate element with a PreferencesManager subclass is that the PM subclass can load the node structures regardless of the connection state and will have an API to get/add/edit/delete node in connection that protects the node data from being wiped out by a failure to initialize the connection (if the connection doesn’t explicitly instruct the PM to delete a node, it won’t).

Randall

[bobjacobsen]

Thanks. That makes sense.

One more question: What controls when that preference data appears in InstanceManager at startup?

The C/MRI code has a bunch of circular references in it that make initialization very problematic. #2828 is due to:

• Setting up the node data with the first part of the connection-load process also needs a C/MRI sensor manager, so the "configure managers" is executed.
• But later on, after the port is up and when the TrafficController is being set up, it's getting new managers as part of another run of "configure managers".

This puts two C/MRI SensorManagers in the InstanceManager, which results in two tabs in the Tables. Untangling it is going to be a mess.

If there was a clean way to say "Nodes are read in, constructed and initialized before the first time they're requested", this whole process could be much cleaner.

[KenC57]

I've long thought that getting the node configuration separated from the connection would help this. Same for when flipping the connection around (sim vs real). While I think things have become worse, over the years I've had to re-enter the node configuration a few times while trying move around CMRI layout information from different computers and like before we had profiles. But it seemed the ease of doing something with profiles were making it even easier to mess up.

-ken c

[KenC57]

Paul,

Haven't looked at that part lately but I didn't think we saved any node information for OpenLCB??

Side issue: shouldn't we consider changing the entry to LCC nowadays??

-ken c

[pabender]

On Mon, Jan 30, 2017 at 12:17 PM, Ken Cameron notifications@github.com wrote:

Haven't looked at that part lately but I didn't think we saved any node information for OpenLCB??

That's why I stated OpenLCB was an exception.

Side issue: shouldn't we consider changing the entry to LCC nowadays??

I'm not in favor of that, at least right now.

Paul

[KenC57]

Bob,

Seems we have the same problem with the Acela CTI. It also tries to keep node information in the connection. -ken

[bobjacobsen]

The basic operation should now be ok for 4.7.3.

There's been a bunch of work on documentation, but that's now only about half done.

When using two (or more) C/MRI system connections, several of the C/MRI menu items on the 2nd system will not have correct labels. There are also problems with starting various C/MRI menu items from the preferences. Those won't be fixed for 4.7.3, but will be after.