-
Notifications
You must be signed in to change notification settings - Fork 482
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
base: master
Are you sure you want to change the base?
Conversation
adding mrp
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
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. |
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. |
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. |
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:
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
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
Some smaller observations: NED Parameters:
Statistics:
Module name:
|
signals and ned parameters are worked, MediaRedundancyNode was renamed to Mrp, InterconnectionNode to MrpInterconnection. MrpInterconnection now inherits properly from Mrp ned file
Tests: Signals: Module Name: |
remove redundant entry **.mrp.mrpRole = "MRC"
includes adjusted.
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.
I just discovered that the 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 |
The If so, then it is supposed to be an internal variable of |
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?" 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. |
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. |
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.) |
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.