Send AMD at end of alias sequence #224
Conversation
|
I re-read the standard and I don't see a requirement that there be a delay between the RID and the AMD frame. From what I can tell they could be back to back on the CAN-bus. |
| @@ -71,6 +71,10 @@ public OpenLcbCanFrame nextFrame() { | |||
| f = new OpenLcbCanFrame(nida.getNIDa()); | |||
| f.setRIM(nida.getNIDa()); | |||
| complete = true; | |||
| } else if (index == 5) { | |||
There was a problem hiding this comment.
Did you check that this fifth frame is actually sent out by JMRI? I am not sure.
Check the timerExpired function.
There is also a bit of confusion about what the variable complete means now, as it is set to true both after step 4 and after step 5; the done callback is called after the fourth though. Maybe it should be set only after the fifth.
Another minor issue is that after you fix the above, when a CID frame comes in with our alias, processFrame will now do something strange in that it would respond with a RID and an AMD. Maybe processFrame should be changed to not use the timer task in responding to CID frames at all.
There was a problem hiding this comment.
Thank you for the careful review! I've fixed the complete and callback, and have confirmed that it's now sending the AMD.
I'd like to leave the CID -> RID and AMD for a later PR. There seem to be several different sequences that already behave poorly, in particular if the CID*4 and RID sequence is received (e.g. because the node wasn't really reacting in a timely way, for example a slow JMRI), so I'd like to clean them all up at once.
Thanks, you're right. I was mis-remembering where the delay needed to be. Will edit the comment. |
|
This is in use in JMRI now through a custom .jar file. It would be good to get it merged before the end of May so that we can use a production .jar in the next production JMRI release. |
Fix for #121.
See discussion below for further work.
(Note: This commend originally talked about a missing delay between RID and AMD; that's not required by the Standard, so that part of this commented was removed in edit)