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

C/MRI infrastructure code needs significant rework #2953

Closed
bobjacobsen opened this Issue Jan 29, 2017 · 10 comments

Comments

Projects
None yet
4 participants
@bobjacobsen
Member

bobjacobsen commented Jan 29, 2017

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

Formally, that includes these Issues:

  • #2827 Cannot create C/MRI connection in 4.6
  • #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
  • 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)
  • 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:

  • 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.
  • 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

This comment has been minimized.

Show comment
Hide comment
@rhwood

rhwood Jan 29, 2017

Contributor

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).

Contributor

rhwood commented Jan 29, 2017

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

This comment has been minimized.

Show comment
Hide comment
@pabender

pabender Jan 29, 2017

Member

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.

Member

pabender commented Jan 29, 2017

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

This comment has been minimized.

Show comment
Hide comment
@bobjacobsen

bobjacobsen Jan 29, 2017

Member
Member

bobjacobsen commented Jan 29, 2017

@rhwood

This comment has been minimized.

Show comment
Hide comment
@rhwood

rhwood Jan 30, 2017

Contributor
Contributor

rhwood commented Jan 30, 2017

@bobjacobsen

This comment has been minimized.

Show comment
Hide comment
@bobjacobsen

bobjacobsen Jan 30, 2017

Member

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.

Member

bobjacobsen commented Jan 30, 2017

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

This comment has been minimized.

Show comment
Hide comment
@KenC57

KenC57 Jan 30, 2017

Contributor
Contributor

KenC57 commented Jan 30, 2017

@KenC57

This comment has been minimized.

Show comment
Hide comment
@KenC57

KenC57 Jan 30, 2017

Contributor
Contributor

KenC57 commented Jan 30, 2017

@pabender

This comment has been minimized.

Show comment
Hide comment
@pabender

pabender Jan 30, 2017

Member
Member

pabender commented Jan 30, 2017

@KenC57

This comment has been minimized.

Show comment
Hide comment
@KenC57

KenC57 Feb 21, 2017

Contributor

Bob,

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

Contributor

KenC57 commented Feb 21, 2017

Bob,

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

@bobjacobsen

This comment has been minimized.

Show comment
Hide comment
@bobjacobsen

bobjacobsen Apr 16, 2017

Member

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.

Member

bobjacobsen commented Apr 16, 2017

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment