Communication with ECOS was much too slow for Rx. I assume the same g… #422
Conversation
…oes for any other direct use of AbstractMRTrafficController. The fix is not to block the UI thread each time a packet is received from ECOS.
|
This change fundamentally changes the behavior of the AbstractMRTrafficController, and should not be merged. If you need to make this change for ECOS specifically, override handleOneIncommingReply in the ECOS specific TrafficController derived from AbstractMRTrafficController. Paul Sent from my iPad On Dec 28, 2015, at 4:43 AM, kjlisby notifications@github.com wrote:
|
|
Why is this change bad (or how does this "fundamentally alter the AbstractMRTrafficController and what is wrong with doing so)? Alternatively, should any traffic controllers that require the Swing UI be allowed to block them, override handleOneIncomingReply while allowing most traffic controllers to hand off the reply and not wait on it? Randall Wood
|
|
On Dec 28, 2015 10:07 AM, "Randall Wood" notifications@github.com wrote:
Because this is a message-response protocol and by using invoke later we
No, because the contract with the AbstractMRTrafficController has always Paul |
|
I agree this needs some thought. Might be good, might not. It's not about the TrafficCintroller needing a UI. It's about which thread layout events occur on: http://jmri.sourceforge.net/help/en/html/doc/Technical/Threads.shtml InvokeAndWait ensures that all the consequences of the layout event happen (the basic object change, eg Sensor or Turnout, plus the cascade that follows) before next going out to the layout. That might or might not be needed. Would interleaving of those be a problem? Not clear. But since this is a base class for lots of protocols, a bit of care is required. Paul's (@pabender) suggestion to make it ECOS-specific is a good one, I think. Then we can get some experience. |
…stem that might find it usefull.
|
I have now made the change specific to ECOS. |
|
As already mentioned, I have made this ECOS specific with the second commit. But regarding the experience gaining that Bob mentions: As far as I can see, the Loconet implementation already uses invokeLater(). This is actually shown in the thread model document that Bob links to above. So I guess there is plenty of experience with using invokeLater(). I think as many as possible should try it out now that I have prepared it for optional use. |
|
The LocoNet TrafficController is already working in an event-driven environment. That's not so clear in poll-response setups. For example, I'm not sure that the CMRI implementation of debouncing will be 100% reliable if responses can get interleaved. Maybe it already is, maybe it's easy to ensure that it is, but I haven't had time to do that yet. The children of AbstractMRTrafficController are:
and some of those have connection-specific subclasses. Thanks for moving the change into the ECOS-specific code so what people can try it out. With your work, ECOS is likely to get some more usage, which is good. I think it would be good to look through the code in a couple others and see if there's anything that indicates a problem. I'll check C/MRI. Then we can make the change in a couple more systems early in this release cycle, and see what emerges. I've opened issue #434 for further discussion. |
Communication with ECOS was much too slow for Rx. I assume the same g…
|
Sent from my iPad On Dec 28, 2015, at 5:49 PM, Bob Jacobsen notifications@github.com wrote:
The z21 code will suffer similar issues because the Roco systems are derived from systems Lenz created for Roco. Paul |
…oes for any other direct use of AbstractMRTrafficController.
The fix is not to block the UI thread each time a packet is received from ECOS.