Monitor mode frame parser does not recognize segmented messages

balazsracz commented 7 years ago

From @KenC57 when a multi-segment message is sent on the bus, all segments are shown in the OpenLCB Monitor Pane as Unknown messages. Only the last segment shows the actual message. (current typical use-case is the SNIP reply to JMRI)

14:22:46.050: [[19a08166] 12 DE 04 52 52 2D 43 69] R: Unknown message [19a08166] 12 DE 04 52 52 2D 43 69 14:22:46.063: [[19a08166] 32 DE 72 4B 69 74 73 00] R: Unknown message [19a08166] 32 DE 72 4B 69 74 73 00 14:22:46.066: [[19a08166] 32 DE 54 6F 77 65 72 2D] R: Unknown message [19a08166] 32 DE 54 6F 77 65 72 2D 14:22:46.074: [[19a08166] 32 DE 4C 43 43 00 52 65] R: Unknown message [19a08166] 32 DE 4C 43 43 00 52 65 14:22:46.077: [[19a08166] 32 DE 76 2D 44 00 42 2D] R: Unknown message [19a08166] 32 DE 76 2D 44 00 42 2D 14:22:46.084: [[19a08166] 32 DE 35 00 ] R: Unknown message [19a08166] 32 DE 35 00
14:22:46.087: [[19a08166] 32 DE 02 4B 65 6E 43 20] R: Unknown message [19a08166] 32 DE 02 4B 65 6E 43 20 14:22:46.094: [[19a08166] 32 DE 54 6F 77 65 72 20] R: Unknown message [19a08166] 32 DE 54 6F 77 65 72 20 14:22:46.096: [[19a08166] 32 DE 4C 43 43 20 32 00] R: Unknown message [19a08166] 32 DE 4C 43 43 20 32 00 14:22:46.106: [[19a08166] 22 DE 00 ] R: 02.01.57.00.00.11 - 02.01.12.7F.46.54 Simple Node Ident Info with content '4,RR-CirKits,Tower-LCC,Rev-D,B-5,2,KenC Tower LCC 2,,'

I'm not sure whether the code responsible is in the JMRI codebase or in the OpenLCB codebase.

KenC57 commented 7 years ago

I would have expected that if it knows that's a mult segment, and not the last (so it could decode it) I'd see a 'multi-segment' instead of unknown. It would be much more comforting that things are ok.

-Ken Cameron, Member JMRI Dev Team

www.jmri.org

www.fingerlakeslivesteamers.org

www.cnymod.com

www.syracusemodelrr.org

KenC57 commented 7 years ago

In jmri.src.jmrix.openlcb.swing.MonitorPane.java:109

It is checking some of the bits in the message before getting to this result. I suspect a few lines to id it as a multi part is needed here.

-Ken Cameron, Member JMRI Dev Team

www.jmri.org

www.fingerlakeslivesteamers.org

www.cnymod.com

www.syracusemodelrr.org

KenC57 commented 7 years ago

I've been playing. I see how to deal with it in the JMRI side of things. Like below, but only thing I'm not sure about is if calling them SNIP reply is right or if I should use something else.

                formatted = prefix + ": (Middle of Datagram)";

            } else if ((header & 0xFFFF0000) == 0x19A00000) {

                // SNIP multi frame reply

                if ((content[0] & 0xF0) == 0x10) {

                    formatted = prefix + ": SNIP Reply 1st frame";

                } else if ((content[0] & 0xF0) == 0x20) {

                    formatted = prefix + ": SNIP Reply last frame";

                } else if ((content[0] & 0xF0) == 0x30) {

                    formatted = prefix + ": SNIP Reply middle frame";

                } else {

                    formatted = prefix + ": SNIP Reply unknown";

                }

            } else {

                formatted = prefix + ": Unknown message " + raw;

            }

Looks like this in the monitor:

So does that seem ok??

-Ken Cameron, Member JMRI Dev Team

www.jmri.org

www.fingerlakeslivesteamers.org

www.cnymod.com

www.syracusemodelrr.org

KenC57 commented 7 years ago

I should have added the JMRI code asks for MessageBuilder() in openlcb to figure out the frame. When it replies with no match, then JMRI makes up things like this. So what's happening is you never see the 'last' frame, that one translates so skips this part.

-ken c

balazsracz commented 7 years ago

Yes, I believe the MessageBuilder has internal state to keep track of these frames.

We should try to have a minimal but generic decoding instead of something specific to SNIP, for example:

check that the top bits of the CAN message say that this is OpenLCB generic message. (as opposed to OpenLCB-CAN control frame, datagrams, stream, etc).
If yes, take source alias from lowest 12 bits.
take MTI from middle 12 bits
check MTI if it is addressed
if addressed, take destination address from first two bytes (if those are there)
if framing nibble is non zero, render "01.02.03.04.05.06 -> 02.01.0D.11.22.33 Partial Message of type %s" with %s being either the MessageTypeIdentifier.get(value).name or if the get() returned null then 0x%03x with the MTI value.
let's not try to parse the payload, because that will be shown in the final frame when it gets parsed anyway. We should avoid showing the same payload twice in the monitor log.

KenC57 commented 7 years ago

Are there calls in the library for getting the mti from the packet?? Right now the JMRI code has stuff like this for the start/middle datagram:

            if ((header & 0x0F000000) == 0x0B000000) {

                formatted = prefix + ": (Start of Datagram)";

So my question is if doing those bit matching is the best way or not. If that's the right way, which doc should I look at to figure the right bits for what you said about the mti etc.

-Ken Cameron, Member JMRI Dev Team

www.jmri.org

www.fingerlakeslivesteamers.org

www.cnymod.com

www.syracusemodelrr.org

balazsracz commented 7 years ago

I haven't seen helper functions. Bit masking is probably the way to go. Ideal would be to introduce such helper libraries but that's difficult to do with the split codebase -- it is not possible to submit code atomically to the two codebases, and if we add the helper functions now you can't touch the JMRI code until we get a new openlcb.jar in there. Not sure how quickly you want to fix this issue.

bit definitions: http://openlcb.org/wp-content/uploads/2016/02/S-9.7.3-MessageNetwork-2016-02-06.pdf section 7.3.1 (in part maybe also 3.1.1 is helpful).

KenC57 commented 7 years ago

I would go with getting the helper functions added to the lib, but go with bit mapping for now in JMRI with comments (TODO) that say replace with lib functions when available.

I likely should setup a workspace for the openlcb stuff so I can build local a jar to test or maybe even link to my JMRI workspace for testing some of this stuff.

-ken c

balazsracz commented 7 years ago

On the long run, I'm thinking of moving the entire CAN frame to text rendering code into the library. Something like

String frameToDebugText(CanFrame frame);

Then the MessageBuilder() can be private to the openlcb package and we also fix the too widely visible AliasMap reference too.

pabender commented 7 years ago

The message object should have a toString method.

In JMRI, we usually have two methods, a toString and toMonitorString. The toMonitorString typically provides translated text, but strictly speaking either could.

Other libraries we use provide just a toString method, which frequently provides translation.

Paul

Sent from my iPhone

On Dec 17, 2016, at 9:10 AM, Balazs Racz notifications@github.com wrote:

On the long run, I'm thinking of moving the entire CAN frame to text rendering code into the library. Something like

String frameToDebugText(CanFrame frame);

Then the MessageBuilder() can be private to the openlcb package and we also fix the too widely visible AliasMap reference too. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

balazsracz commented 7 years ago

Paul: the problem is that with the frames in question there is no Message object at all; a frame that is a single segment of a multi-frame message cannot be translated to a Message object. This is why the MessageBuilder returns back null and then the monitor UI tries to figure out something barefoot today.

pabender commented 7 years ago

Sent from my iPhone

the problem is that with the frames in question there is no Message object at all; a frame that is a single segment of a multi-frame message cannot be translated to a Message object. This is why the MessageBuilder returns back null and then the monitor UI tries to figure out something barefoot today.

That may be true, but there is a frame object, and that can be sent on to the monitor.

Paul

balazsracz commented 7 years ago

1) The frame object is not specific to OpenLCB; JMRI uses a frame object that is generic and shared between all CAN-bus interfaces (e.g. MERG) 2) you cannot toString() a frame object without the state of the system, for example you need to have access to alias maps in order to display what it is.

pabender commented 7 years ago

Sent from my iPhone

On Dec 17, 2016, at 12:05 PM, Balazs Racz notifications@github.com wrote:

1) The frame object is not specific to OpenLCB; JMRI uses a frame object that is generic and shared between all CAN-bus interfaces (e.g. MERG) 2) you cannot toString() a frame object without the state of the system, for example you need to have access to alias maps in order to display what it is.

You can always toString an object in Java, and frames are objects.

It may just be a string of the bytes without translation, but, a frame has addressing information in addition to payload. The address and payload can be separated in a toString method.

Also, frames in OpenLCB are not just CAN frames, We can't think of them that way.

Paul

balazsracz commented 7 years ago

Replacing the monitor log's contents with an untranslated toString() of the frame would be causing a major regression to the current usability. There is no way we can expect the user to know the alias mapping table by heart, especially that it is subject to change every time a node is restarted.

pabender commented 7 years ago

Sent from my iPhone

On Dec 17, 2016, at 12:31 PM, Balazs Racz notifications@github.com wrote:

Replacing the monitor log's contents with an untranslated toString() of the frame would be causing a major regression to the current usability. There is no way we can expect the user to know the alias mapping table by heart, especially that it is subject to change every time a node is restarted.

This depends on what the monitor is for.

The monitor is first and foremost a debugging tool. One of the jobs is to show traffic on the wire, but that does not require all data to be translated.

We don't always translate data of other protocols either.

Think of the tool being like wireshark. It reports all data, translating ( and possibly aggregating ) where possible.

Paul

balazsracz commented 7 years ago

Paul: So you're basically saying that you don't care about this issue, because once the wire bytes are displayed in the monitor log (which they are), it doesn't bother you that "Unknown message" is printed next to those bytes. That's a valid point of view. Ken seems to disagree however and is willing to make the code change. Do you have any reason why not to fix this issue?

KenC57 commented 7 years ago

The only thing I'm not doing right now is telling which nodes are involved. I think I can steal some of that code from other examples.

But long term, I'd see a family of helper classes with functions like getMti() that would be passed the array to pull out the right bits. Whole idea would be that be in the library and know the right ones or when to return null because the current frame didn't have what was asked for. Granted if the frame is what JMRI is handling, then I'd expect those to be accessors of the object for the frame. Net result either way is the 'which bit/shift is which' isn't something JMRI keeps track of.

-Ken Cameron, Member JMRI Dev Team

www.jmri.org

www.fingerlakeslivesteamers.org

www.cnymod.com

www.syracusemodelrr.org

pabender commented 7 years ago

Sent from my iPhone

On Dec 17, 2016, at 1:05 PM, Balazs Racz notifications@github.com wrote:

Paul: So you're basically saying that you don't care about this issue, because once the wire bytes are displayed in the monitor log (which they are), it doesn't bother you that "Unknown message" is printed next to those bytes. That's a valid point of view. Ken seems to disagree however and is willing to make the code change. Do you have any reason why not to fix this issue?

Based on experience with other protocols that fragment data, there is value to that for those of us debugging what is happening on the wire to see all the fragments.

We should be able to provide some information in the monitor that decides a fragment enough that we can identify a) where it came from and b) to what stream that fragment belongs to.

Discarding fragments is a non-starter, because not showing them means we don't see the data so we can decipher how a failure occurred.

Paul

KenC57 commented 7 years ago

I'm putting up a PR that leaves the binary display but gives the text as first or middle frame. I think it is more comfort to the user that they aren't seeing 'unknown' traffic.

-Ken Cameron, Member JMRI Dev Team

www.jmri.org

www.fingerlakeslivesteamers.org

www.cnymod.com

www.syracusemodelrr.org

balazsracz commented 7 years ago

sounds good, thanks! ping this thread please with the PR number at your convenience.

KenC57 commented 7 years ago

Resolve intermediate SNIP frames in OLCB monitor #2713

JMRI PR

-ken c

dpharris commented 2 months ago

Resolved with updates.

openlcb / OpenLCB_Java

Monitor mode frame parser does not recognize segmented messages #66