Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding protocol MRP (IEC 62439-2) #947

Draft
wants to merge 129 commits into
base: master
Choose a base branch
from
Draft

Conversation

DJtime
Copy link

@DJtime DJtime commented Feb 17, 2024

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.

updated copyright
Added MRP InterfaceData
adding standard compliant code and comment in testMgrNackReq and testPropagateReq
Duplicate packets in RingCheck eliminated
IngressFiltering in Relay introduced
applied OMNeT++ style template and reordered attributes in header files
automatic formatting done by template introduced some errors which were resolved where necessary for readability
@rhornig
Copy link
Member

rhornig commented Feb 20, 2024

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
Copy link
Author

DJtime commented Feb 20, 2024

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
Copy link
Member

avarga commented Feb 21, 2024

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.

@rhornig rhornig marked this pull request as draft February 21, 2024 15:30
@rhornig rhornig added this to the INET 4.6 milestone Feb 21, 2024
@avarga
Copy link
Member

avarga commented Feb 21, 2024

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

NED file comments added, + some smaller errors eliminated,
renamed function initContinuityCheck to startContinuityCheck for clarification
@avarga
Copy link
Member

avarga commented Mar 2, 2024

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?

signals and ned parameters are worked, MediaRedundancyNode was renamed to Mrp,
InterconnectionNode to MrpInterconnection.
MrpInterconnection now inherits properly from Mrp ned file
@DJtime
Copy link
Author

DJtime commented Mar 13, 2024

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 and others added 22 commits April 17, 2024 16:29
also: space changes, some items moved within file
…heckMode

Those parameters only apply to interconnections, so they have to be
prefixed accordingly; "LC Mode" and "RC Mode" are common terms, and
using LC+RC together is questionable, so it makes sense to introduce
a single parameter with values "LC" and "RC".
The only purpose the separate inner and outer rings served was layouting,
and it's just not worth it.

Related configs moved to the bottom of the ini file.
…llNetwork moved to front

Follow section name changes in fingerprints file (fingerprints are
preserved).
This reverts commit 1b3fc9d.
@avarga
Copy link
Member

avarga commented Apr 25, 2024

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
Copy link
Member

avarga commented Apr 25, 2024

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
Copy link
Author

DJtime commented Apr 25, 2024

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
Copy link
Member

avarga commented Apr 25, 2024

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
Copy link
Member

avarga commented Apr 25, 2024

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.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants