Adding protocol MRP (IEC 62439-2)

[DJtime]

Adding protocol MRP (IEC 62439-2). Media Redundancy Protocol acts similar to RSTP, but provides a upper time limit needed for recovery in case of link failure.

[rhornig]

Thanks a lot for your work. This looks really interesting and it would be a nice addition to INET. We had a bit of time to briefly look into the PR and the code generally looks OK but it would need some more refining before merge. Are you willing to invest some more time? If so, we can continue discussing the details.

[DJtime]

Yes, i can invest some more time. Although my master thesis and study will be finished, i am willing to support this topic until merge is completed.

[avarga]

This is great, thank you! Rudolf and I looked at the model together yesterday, and agreed that the thing we missed most was high-level documentation. Information like: what part of IEC 62439-2 does the model implement, and, which parts of spec are NOT included in the model? How do concepts in the specification represented in the simulation model? E.g. how switch roles like Media Redundancy Master (MRM), Media Redundancy Client (MRC), Media Redundancy Checker (MRCk), Auto Client (AC), Interconnection (IC) manifest themselves in the model? How to set up single-ring and multi-ring networks?

As a side note, can you give us a hint (maybe via email) where to get detailed info about the IEC 62439-2 protocol? Buying the spec isn't exactly an option, and apart from that, so far we've only found a couple of web pages with various bits of overview-level information.

[avarga]

Thanks for the docs, we are currently reading them, and familiarizing ourselves with the model.

You asked: "I can write some more comments regarding roles and set-up. Where would be the appropriate place, header file or ned-file?" It should go into the NED file, right above the modules definitions. Every module should have a few lines of docu that answers questions like:

• What is the module's purpose (e.g. an L2 MAC forwarding table)
• What modules is it used together with, if applicable (e.g. the Ipv4 module mentions Ipv4RoutingTable, and vica versa)
• What protocol or specification it implements (and which version)
• List of notable features it implements, and notable features it does not
• Some hints of how to configure it, maybe with brief examples where it helps.

The NED docu should be useful for a potential user of the model. By reading it, the person should be able to decide whether this is the module s/he is looking for, whether it implements all features s/he needs for the simulation, etc.

In any case, the NED docu should not go into C++ details, because it's irrelevant on that level. If you catch yourself writing down the names of C++ methods, classes, enum values etc, that's the wrong track, please stop :)

You can find examples browsing existing doc: https://doc.omnetpp.org/inet/api-current/neddoc/index.html

[avarga]

Thank you for adding the comments, great work! It is also extremely useful that you added detailed comments for parameters, too. Much appreciated! Documentation was the most important thing we wanted to get right, because writing it without your help would be quite hard for us to do.

Tests

• The documentation done, the next most important item would be automated tests, because writing them also requires detailed knowledge of the protocol, so we would not be able to do it that easily in-house. The simulation you put under tests/networks/mrp is great but it currently does not really function as a test, because it lacks automated checking of outcome (there's no PASS / FAIL output). We suggest placing a copy of it under examples/ as a dedicated example simulation; meanwhile we are pondering how to best turn the copy under tests/ into a real test.

Some smaller observations:

NED Parameters:

• int expectedRoleByNum -- should be type string with @enum; BTW why is it called "expected"?

• Not all parameters are read by the code: processingDelayMean, processingDelayDev, linkDetectionDelayMean, linkDetectionDelayDev, possibly others (to be checked).

• linkDetectionDelayMean, linkDetectionDelayDev are currently of type object in NED which does not seem warranted; we recommend replacing them with a single volatile double parameter: volatile double linkDetectionDelay @unit(s) = default(truncnormal(...)); and their current usage linkDetectionDelay = truncnormal(linkDetectionDelayMean, linkDetectionDelayDev); replaced with linkDetectionDelay = linkDetectionDelayPar->doubleValue()where linkDetectionDelayPar is a cPar* pointer.

Statistics:

• For some signals, the signal value is the current simulation time (in us), e.g.: emit(RingStateChangedSignal, simTime().inUnit(SIMTIME_US));. This is redundant because the signal already carries the timestamp, and is recorded into an output vector together with it. We recommend choosing some other meaningful quantity as signal value, e.g. for RingStateChangedSignal the current ring state (OPEN, CLOSED, etc)

Module name:

• The module types MediaRedundancyNode and InterconnectionNode are a bit confusing now, because "nodes" in INET normally refer to devices like routers or switches, and not to protocol implementations like MRP. Modules that implement protocols are normally simply named after the protocol, e.g. Tcp or Ipv4 or its subcomponent like Ieee80211Mac. Maybe rename the module types to Mrp and MrpInterconnect or something like that?

[DJtime]

Tests:
I have copied the existing test (after reworking) into examples/mrp.
NED Parameters:
expectedRolebyNum: Mrp standard calls the parameter internally expectedRole, but I renamed it for clarity to mrpRole. Switching it to string @enum would (as far as I can tell) require an additional parsing function (similiar to parseFcsMode function in linklayer/common/FcsMode.cc), so i left it as INT for now.
I reworked all delays and ned parameters and finally found the error which prevented MrpInterconnection NED from inheriting properly from Mrp NED in the first place.

Signals:
All signals now carry some other value than SimulationTime

Module Name:
MediaRedundancyNode was renamed to Mrp, InterconnectionNode is now MrpInterconnection

[avarga]

I just discovered that the bool checkMediaRedundancy parameter of Mrp has no implementartion. The code just reads it into the class member of the same name, and doesn't use it. Now the question is whether it should be (A) implemented, or (B) removed. The spec says on p41, "Check Media Redundancy: This attribute specifies whether monitoring of MRM state is enabled (TRUE) or disabled (FALSE) in the redundancy domain.", also pn p44 and p50: "Check Media Redundancy:
This parameter selects whether monitoring of MRM state is enabled or disabled."

Do I understand correctly that this is sort of the main switch to turn on/off MRP as a whole? Then I think the parameter can be removed (option B). If someone wants to turn off a MRP in a switch, they can remove the mrp submodule from the switch via NED or ini configuration.

[avarga]

The Mrp module has a parameter called noTopologyChange. Does this correspond to the NO_TC variable in the spec, described in "Table 40 – MRP Local variables of MRM protocol machine" on on p70 as with the comment "Suppress MRP_TopologyChange while in line topology"?

If so, then it is supposed to be an internal variable of Mrp, not a configurable parameter. A search for NO_TC in the spec reveals that various functions set it to TRUE or FALSE under various conditions, so it cannot be a configurable parameter.

[DJtime]

Sorry for my longer period of silence. But I needed some space from MRP for a while after one year of deep dive.

"I was wondering that maybe instead of the two boolean parameters there should be a single interconnectionMode parameter with the allowed values UNDEFINED, LC_MODE, and RC_MODE, as described in the list on page 42. What do you think?"
--> As it does not very much sense to have both enabled, it could help to prevent accidental misconfiguration

checkMediaRedundancy: I have always interpreted it as a way to enable the active Testing of the MRP ring.... So if disabled, only local link detection is done. But this one of the many points where MRP is not very specific.... I have not found any point in spec where this variable is referenced.

noTopologyChange: Yes it corresponds to the internal variable and therefore the parameter should be removed from NED file

I am currently reviewing the changes you already have done and will check the model further.

[avarga]

No worries, I really appreciate your continued support! My apologies for constantly rebasing/editing the last few commits of the branch, I am doing it in order to create a cleaner change history where my mistakes are easier to spot.

[avarga]

My commits are mostly stylistic changes, they do not change the operation of the code. I took care that the fingerprints are unchanged throughout all my commits (only LargeNet model fingerprints change at one point, because I changed the network.)