Duplicate items (e.g. route control sensors) in panel xml

[SteveRawlinson]

I started getting warnings when loading a panel that I had exceeded the maximum number of sensors per route on every route in the file. Opening the file showed that the single control sensor for each route had been duplicated a further three times.

I have not been able to work out why or where they get duplicated. Instead, I have written a patch which checks for duplicates and does not load them. It logs a warning if it encouters a control sensor in the xml file whose name and mode match an existing sensor. The next save of the panel file drops the duplicates.

PR #677 implements this behaviour.

[bobjacobsen]

I have not been able to work out why or where they get duplicated Instead,

It’s possible that loading a file multiple just appends to the list.

We have a couple of JUnit tests that load a test file, write it out and then check to see if it’s the same. Might be good to have a similar test that does two loads to see what’s duplicated. I’ll see what I can do.

[SteveRawlinson]

It’s possible that loading a file multiple just appends to the list.

I've tested and that is indeed what happens. Which is handy because that's exactly what this PR fixes. I seem to have addressed the root of the problem by accident.

[bobjacobsen]

I've got an update to the load and store tests that loads twice, then does the store check. Eight files fail, six of which are fixed by #677; that's great!

The remaining two are failing with an extra "path" definition in a block, e.g.:

ERROR -    inLine = "    </block>" [TestRunner-Thread] jmri.configurexml.LoadAndStoreTest.checkFile()
ERROR -   outLine = "      <path todir="64" fromdir="128" block="IB2">" 

I'm not real familiar with the block-related parts of the code. Do duplicate paths make sense? Is there an order to paths? If those are both "no", we can just drop duplicate paths on read and fix this.

[SteveRawlinson]

I have not progressed to looking at paths yet. I'd be surprised if duplicate paths make sense, or if path order matters, but I'm not qualified to say so with confidence.

I've found that there is another problem when loading a file twice and then saving the panel: the Layout Panel itself is duplicated and this causes a DuplicatePanel warning on every subsequent load of the file (even if there are no panels loaded). I'm confident I can fix this too.

It might make sense to rename this issue to something more generic, such as "Duplicate objects following multiple panel file loads."

[bobjacobsen]

I've now got code for equals operators for Path, BeanSetting and Block plus a don't-add-sups check that I'll make available shortly.

[SteveRawlinson]

OK, I'll leave this till your code is up.

[bobjacobsen]

Duplicate items in the Java/test tree load/store tests now seem ok. With PRs to date (up to #682) those files load, load, store without duplicates (some logging needs a bit of cleanup in testing, but that's OK).

There might be things missing from those, though. Sounds like they don't have layout panels, for example.

I'm not sure what the right behavior for layout panels should be. Reading a file (twice) shouldn't duplicate panels that are equal, but what's equal? Can you have more than one layout panel? Or do you add items to an existing one?

[rhwood]

Would it make sense to simply prevent a file from being opened twice?

I know that does not prevent two files from loading overlapping or identical content, but it does prevent an accidental reopening.

[SteveRawlinson]

I think if you were designing this from scratch you would allow multiple layout panels to be open and keep the instances completely separate so that panel1 and panel2 can both have a sensor with the system name LS1 but they are represent by two different objects, one 'attached' to each panel, even if they have identical content. I'm too new to the JMRI internals to know how practical that would be to implement now. Since layout panels rely on objects also used by other parts of the system (eg. panel editor) I'm guessing that would be a lot of work for not much gain.

Another option would be to prevent the user from opening a second file if it contains any object referenced by a file that's already open. That would be a clean solution. I'm not sure if that restriction would affect the way people actually use layouts. It certainly wouldn't affect me at all. Having overlapping content open at the same time sounds like a recipe for all sorts of unintended consequences.

[pabender]

Sent from my iPad

On Jan 21, 2016, at 5:58 AM, Steve Rawlinson notifications@github.com wrote:

I think if you were designing this from scratch you would allow multiple layout panels to be open and keep the instances completely separate so that panel1 and panel2 can both have a sensor with the system name LS1 but they are represent by two different objects, one 'attached' to each panel, even if they have identical content.

You're thinking about this from the wrong perspective.

Each running instance of JMRI on a computer ( and yes, you can have more than one ) describes the state of a single layout from the connection to that layout all the way to modeled objects.

The panel file ( which more appropriately is a layout configuration file ) contains one or more panels and the objects required to build those panels. There are several reasons for allowing multiple panels to be stored in a single file. Initially, with the panel editor, we wanted people to be able to have panels for different sections of the layout. Now, with the layout editor, we can also capture logical connections as well. The information required for these panels can be stored as one or more files.

There is a lot more work to make that separation than you might think. It starts with the need to include connection information in the panel file, but that leads to problems too ( you can't have more than one connection to the same serial port, and even if you can, how do you know that the right hardware is connected to the port ).

Another option would be to prevent the user from opening a second file if it contains any object referenced by a file that's already open. That would be a clean solution. I'm not sure if that restriction would affect the way people actually use layouts. It certainly wouldn't affect me at all. Having overlapping content open at the same time sounds like a recipe for all sorts of unintended consequences.

That approach doesn't work well because there are reasons for loading multiple independent panels on a single computer. As an example, there are some layouts busy enough to justify two dispatchers, but, sometimes there are not enough crew members available to fill both spots. In that case, one dispatcher may cover both jobs for at least part of an operating session. It is highly likely the panels in this case will share sensors, especially at the panel boundaries.

Paul

[SteveRawlinson]

Each running instance of JMRI on a computer ( and yes, you can have more than one ) describes
the state of a single layout from the connection to that layout all the way to modeled objects.

I'm not sure why that means that you couldn't allow (say) two copies of a layout to be open at once with duplicate objects represented by different (thought identical) JMRI objects, each associated with a different instance of the layout. That would mean two or more JMRI objects mapping to a single real word thing, and that might cause issues for the user, but it's not impossible from a software point of view.

However I'm quite prepared to believe that this is not a practical solution because of the amount of work involved.

If we don't want to stop people loading files with overlapping objects because there is a use case for that then we are left with spotting duplicate objects as they are loaded and not creating them twice. Which is what my PR does, but only for control sensors. There are other things that get duplicated. At the very least the Layout Panel object itself it duplicated too.

[pabender]

On Jan 21, 2016, at 9:18 AM, Steve Rawlinson notifications@github.com wrote:

Each running instance of JMRI on a computer ( and yes, you can have more than one ) describes
the state of a single layout from the connection to that layout all the way to modeled objects.

I'm not sure why that means that you couldn't allow (say) two copies of a layout to be open at once with duplicate objects represented by different (thought identical) JMRI objects, each associated with a different instance of the layout. That would mean two or more JMRI objects mapping to a single real word thing, and that might cause issues for the user, but it's not impossible from a software point of view.

It's the connection information that is problematic. For serial devices, you can only have one process connected to each port at any given time.

Paul

[SteveRawlinson]

I wasn't suggesting having more than one process, just more than one layout panel open (which we allow now) but with their own 'scope' for sensors, turnouts, etc rather than all using a sort of global space of those objects like we do now.

[bobjacobsen]

two copies of a layout to be open at once with duplicate objects represented by different (thought identical) JMRI objects

more than one layout panel open (which we allow now) but with their own 'scope' for sensors, turnouts, etc

Could you say a few more words about what you mean by this? I’m pretty sure I don’t understand the idea.

There is (usually) an actual physical turnout on the layout: Just one physical-object that’s being referred to. What’s the gain from having that be represented by multiple software-objects in the program?

[SteveRawlinson]

Users need to be able to open multiple panels and those panels need to be able to display some of the same objects, for example a sensor on the border between the two (per @pabender's message above). This is what we allow right now but it gives rise to this duplication. Possible solutions:

1. don't allow it - not good because people are using it
2. catch duplicates as they appear and don't load them - this is what my PR does (but only with one type of object)
3. allow multiple panels and load each into its own 'namespace' or 'scope' so that if two panels both have a reference to the same sensor JMRI creates two different sensor objects with identical data except that each would only exist in one panel's 'scope'. That would be achieved by (for example) creating an attribute for sensors (and turnouts, etc.) called panelInstanceID which used to determine which panel owns that copy of the sensor. Saving the panel would only write its own objects. No duplication.

It's possible that solution 3 would entail a substantial re-write of large parts if JMRI, I have no idea, so I'm not advocating it. Probably the best solution is an expansion of option 2, or something completely different I haven't thought of.

[pabender]

On Thu, Jan 21, 2016 at 1:30 PM, Steve Rawlinson notifications@github.com
wrote:

Users need to be able to open multiple panels and those panels need to be
able to display some of the same objects, for example a sensor on the
border between the two (per @pabender https://github.com/pabender's
message above). This is what we allow right now but it gives rise to this
duplication. Possible solutions:

1. don't allow it - not good because people are using it
2. catch duplicates as they appear and don't load them - this is what
   my PR does (but only with one type of object)

In memory, there are no duplicates. i.e. there is NEVER more than one
sensor with the system name LS1. (as Bob said, there is a physical sensor
associated with LS1, so there can't be more than one).

Having a sensor loaded from multiple sources can cause the properties of
the sensor to change, but the second (or 3rd or 4th or ... ) loading of a
sensor can't change that physical connection with the layout.

We want this updating of information about the object to occur. As an
example, XPressNet and LocoNet systems (possibly others) can automatically
create some objects based on information received from the layout. If we
automatically create a device and then load a panel file that provides a
user name for the device, we MUST let the information loaded from the file
populate the appropriate fields of the device so that the panel can
function properly.

Paul

[SteveRawlinson]

In memory, there are no duplicates. i.e. there is NEVER more than one
sensor with the system name LS1.

I appreciate that is how JMRI is supposed to work at the moment, but that is a design decision. It doesn't necessarily follow that because there is only one physical sensor that you can't have more than one object in memory which models it. (You might need to serialise communication between them the layout but I doubt that's insurmountable.)

In fact, it looks like JMRI does indeed have more than one sensor with the name LS1 if the same file is loaded more than once. As far as I can tell this behaviour is unintended, and it's why I wrote this patch. I'm just saying that eliminating duplicates is not the only approach. We could allow duplicates if they are marshalled properly.

[pabender]

On Thu, Jan 21, 2016 at 3:25 PM, Steve Rawlinson notifications@github.com
wrote:

In fact, it looks like JMRI does indeed have more than one sensor with the
name LS1 if the same file is loaded more than once. As far as I can tell
this behaviour is unintended, and it's why I wrote this patch.

Prove that to me please. I.e. what makes you believe this is occurring?

Paul

[SteveRawlinson]

Prove that to me please. I.e. what makes you believe this is occurring?

I saw multiple occurrences of the same sensor in a panel file. I wrote a method to check, when loading a control sensor for a route, whether a sensor with the same system name was already in the list of sensors for the route, and log a warning if so. There were lots of warnings. PR #677 implements that method, and removes the duplicates.

@bobjacobsen guessed that loading the same xml file twice might be the cause of the duplication and some testing demonstrated that this was indeed the case.

[pabender]

On Thu, Jan 21, 2016 at 3:51 PM, Steve Rawlinson notifications@github.com
wrote:

Prove that to me please. I.e. what makes you believe this is occurring?

I saw multiple occurrences of the same sensor in a panel file. I wrote a
method to check, when loading a control sensor for a route, whether a
sensor with the same system name was already in the list of sensors for the
route, and log a warning if so. There were lots of warnings. PR #677
#677 implements that method, and
removes the duplicates.

@bobjacobsen https://github.com/bobjacobsen guessed that loading the
same xml file twice might be the cause of the duplication and some testing
demonstrated that this was indeed the case.

I don't see any evidence that the sensor object was created more than
once. I only see that you're preventing the route from creating more than
one reference to a given sensor object, but that's not the same thing.

Paul

[SteveRawlinson]

I see what you mean. You're right, I'm preventing duplicate references to (what may very well be) a single object in memory.

I'm not sure this makes any difference to the options for dealing with this duplication. But I'm going to stop trying to explain an solution which I don't think is a practical approach in any case.

Preventing users from opening files that reference objects already open might be a restriction too far, but is there a good reason not to prevent two panels with the same name being open at once? That might stop duplicate references in panel files when they are saved.

[pabender]

On Jan 21, 2016, at 5:03 PM, Steve Rawlinson notifications@github.com wrote:

Preventing users from opening files that reference objects already open might be a restriction too far, but is there a good reason not to prevent two panels with the same name being open at once?

How do you know that two panels with the same name are the same? Some people are really bad ( or lazy ) about naming things, so they might have two panels with the same name but different contents.

Also, working in the modular world, two modules can certainly have the same name, so panels with the same name are appropriate

Paul

[rhwood]

If two panel files with different layout elements are opened in the same PanelPro session and then saved, do they then have the same set of layout elements, or is there some other discriminator to say that element 1 should only be in panel file 1 while element 2 should only be in panel file 2?

[bobjacobsen]

I saw multiple occurrences of the same sensor in a panel file. I wrote a method to check, when loading a control sensor for a route, whether a sensor with the same system name was already in the list of sensors for the route, and log a warning if so. There were lots of warnings. PR #677 implements that method, and removes the duplicates.

There are two difference to things here.

In a normal JMRI app (not test code) there cannot be two Sensor objects with the same system name. Sensors are NamedBeans, managed by a Manager object that ensures this. There's only one Sensor named "LS1".

But it's certainly true that multiple objects can reference the LS1 Sensor. You can even have multiple references from one single object, eg one Route, to that LS1 Sensor.

[bobjacobsen]

is there a good reason not to prevent two panels with the same name being open at once? That might stop duplicate references in panel files when they are saved.

NamedBeans like Sensors, Turnouts and Routes use an "overwrite when loading" approach. Reading a Sensor with a name that already exists just updates the existing one: if the file has the same setup, no change, but different options/content might change or add to the NamedBean. (Bug in this were duplicating the Route Sensors and Block Paths)

If you read the same file twice, nothing changes. Automatically.

If you read files for various parts, they (mostly) just accrete properly.

1. I'm not sure whether that idea works or not for panels. Could be, could be not, might vary with type.

2. Do people currently have multiple panels by the same name? If so, it might be better to add a form of unique ID to the panel so it could be recognized when read again.

[pabender]

On Jan 21, 2016, at 6:22 PM, Randall Wood notifications@github.com wrote:

If two panel files with different layout elements are opened in the same PanelPro session and then saved, do they then have the same set of layout elements, or is there some other discriminator to say that element 1 should only be in panel file 1 while element 2 should only be in panel file 2?

No. Because you are allowed to have two panels in a single file that describe different parts of a layout, and those panels may have overlapping definitions.

Remember that the idea is each "panel" file actually describes the layout configuration, so that is all state from all panels loaded at the time the file was created.

What I have done in some cases is develop panels for each controlled section ( typically modules ) of a layout, but then combined the panels to build the layout as a whole. While you can load individual panels at startup, it is sometimes more convenient to store all the configuration to one file.

Paul

[bobjacobsen]

I appreciate that is how JMRI is supposed to work at the moment, but that is a design decision. It doesn't necessarily follow that because there is only one physical sensor that you can't have more than one object in memory which models it. (You might need to serialise communication between them the layout but I doubt that's insurmountable.)

You could have two (e.g. two different chunks of memory)

But they’d have to always have identical content. All the content: The transient state that models the layout object’s transient state (e.g. THROW). The static state that records the hardware configuration (e.g. inverting or not). All the content.

What possible value could it add to have more than one if you have to go to the trouble of always and really, really quickly keeping them consistent? If they use the same memory, e.g. are the same object, you get the consistency for free.

What does having more than one buy?

It doesn’t buy you any kind of partitioning between e.g. two separate files: They’re content is the same.

I must be missing something in how you’re thinking about this.

[bobjacobsen]

If two panel files with different layout elements are opened in the same PanelPro session and then saved, do they then have the same set of layout elements, or is there some other discriminator to say that element 1 should only be in panel file 1 while element 2 should only be in panel file 2?

Mu.

There’s no way to do what you’re asking about, so there’s no way to answer the question.

If you load two files into JMRI (or one or none or three), your only options are “store configuration only to file” and “store panels (and configuration) to file”. Everything in JMRI that it’s going to write gets written to one file. There is no way to store separate contents back to the two separate files.

The conceptual problem here, which a lot of people have, is that JMRI isn’t a word processor.

When using JMRI, you don’t “open” a file/document, mess with that file/document and “save” that file/document.

You load information into JMRI’s memory, make other changes to that model, and then store that model. There’s ever only one model.

Should it be possible to have more than one model? If you allow overlaps (which people find very useful, because there’s really a layout out there to control), the separation into “needed for part 1” and “not needed for part 1” is an Extremely Hard CS Problem, formally analogous to the Halting Problem.

[pabender]

On 01/21/2016 11:18 PM, Bob Jacobsen wrote:

Should it be possible to have more than one model? If you allow overlaps
(which people find very useful, because there’s really a layout out
there to control), the separation into “needed for part 1” and “not
needed for part 1” is an Extremely Hard CS Problem, formally analogous
to the Halting Problem.

And there you go, bringing NP-Completeness into this discussion :D.
(and why didn't the Computer Scientist (i.e. me) bring it up... but I am
not a theorist, so I only brush this stuff off when I need it....)

More formally, this is a form of the Set Partition problem, which, as
Bob states, can be reduced to the Halting Problem (see
https://en.wikipedia.org/wiki/Partition_problem ). While there might be
approximate solutions to the problem that get you close to the right
answer, there aren't any polynomial time algorithms known to solve the
problem optimally.

I am always amazed at the number of problems that have been proven to be
NP-Complete, especially some of the ones that on the surface are "easy"
to solve. (see
https://en.wikipedia.org/wiki/List_of_NP-complete_problems ).

Paul

[SteveRawlinson]

What possible value could it add to have more than one if you have to go to the trouble of
always and really, really quickly keeping them consistent? If they use the same
memory, e.g. are the same object, you get the consistency for free.

Well, the value, as I understood it when I wrote that, is that it allows two open panels to use the same object on the layout.

In my world a reference is analagous to a C pointer. It is only ever interpreted as the location of the real data, it has no attributes of its own. Whereas the 'references' to the sensor I was seeing being loaded twice are new instances of class ControlSensor, derived from class OutputSensor, which has its own attributes and methods. It registers its own listener which I was assuming is how it gets its values in sync with the real sensor on the layout.

This is probably my ignorance of how Java works but that doesn't look like a reference at all, it looks like an object in its own right.

[rhwood]

Is there any reason not to add a set of sources to the NamedBeans so that each bean knows which file(s) it has already been defined in (or if it was discovered or automatically generated) so that the question of which panels should be saved with it is somewhat explicit?

[pabender]

On Fri, Jan 22, 2016 at 7:05 AM, Randall Wood notifications@github.com
wrote:

Is there any reason not to add a set of sources to the NamedBeans so that
each bean knows which file(s) it has already been defined in (or if it was
discovered or automatically generated) so that the question of which panels
should be saved with it is somewhat explicit?

Because you can't selectively save the configuration of the layout. As Bob
mentioned before, JMRI IS NOT A WORD PROCESSOR. We have two options: save
the configuration or save the panel(s) along with the configuration. In
either case, you save the complete layout configuration as represented in
memory.

Paul

[pabender]

On Fri, Jan 22, 2016 at 7:04 AM, Steve Rawlinson notifications@github.com
wrote:

In my world a reference is analagous to a C pointer. It is only ever
interpreted as the location of the real data, it has no attributes of its
own. Whereas the 'references' to the sensor I was seeing being loaded twice
are new instances of class ControlSensor, derived from class OutputSensor,
which has its own attributes and methods. It registers its own listener
which I was assuming is how it gets its values in sync with the real sensor
on the layout.

This is probably my ignorance of how Java works but that doesn't look like
a reference at all, it looks like an object in its own right.

I have not looked at this part of the code recently, but there may very
well be a new ControlSensor object created, with that object referencing
the jmri.Sensor object that represents the actual connection to the
layout. There is only one jmri.Sensor object representing the real sensor
on the layout.

Paul

[SteveRawlinson]

OK, well it was those ControlSensor objects that were being duplicated and which caused the problem. If you replace 'sensor' with 'ControlSensor instance' in my suggestion for handling duplication it might make more sense. On the other hand it might not.

Once the code that Bob has merged makes it into a dev release I'll have another look and see if the LayoutEditor object duplication in the xml has been taken care of.

[bobjacobsen]

With #705, JMRI should be able to read the configuration data (Turnouts, Sensors, ..., Routes, Logix) as many times as necessary. There's a JUnit test that does load, load, store and checks that the file is the same to look for duplications. (There are no "panels" in those test files, because as of now that are duplicated on read).