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

Add OMNeT++ 6 preview support. #25

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

karlhto
Copy link
Contributor

@karlhto karlhto commented Sep 3, 2020

As of OMNeT++ 5.3, a new message compiler was introduced. The legacy message compiler is still used by default in OMNeT++ 5, but for compatibility with future versions I have explicitly set the msg4 option, which enables the legacy message compiler. Also updated to C++17. Literally no reason to use C++11.

Implicit casts are now preferred over the use of .longValue() for cPar. You can see why under features introduced in 5.3:
https://doc.omnetpp.org/omnetpp/api/APIChanges.html
.longValue() is replaced by .intValue() in OMNeT++ 6, so casting is the better option anyway in regards to compatibility.

SubmoduleIterator uses * instead of () as an access operator now.
https://doc.omnetpp.org/omnetpp/api/classomnetpp_1_1cModule_1_1SubmoduleIterator.html

From https://github.com/karlhto/RINA-thesis originally, which was a private bare clone of this repo. Check it for original commit dates if wanted.

@karlhto
Copy link
Contributor Author

karlhto commented Sep 3, 2020

Oh yeah, I also added -Wno-inconsistent-missing-override because files generated by the message compiler will spam this warning. Ths warning is unlikely to be encountered anyway.

Also, I added -Wextra because I think it's a good idea to write good code. :)

https://doc.omnetpp.org/omnetpp/api/APIChanges.html under 5.3, implicit
casting is now preferred over .longValue(). In OMNeT++ 6 .longValue()
will be renamed to .intValue(), but using implicit or static_cast<long>
is sufficient for this.

In SubmoduleIterator, the () operator is deprecated. Use * instead. ()
is removed from the OMNeT++ 6 previews.
Source: https://doc.omnetpp.org/omnetpp/api/classomnetpp_1_1cModule_1_1SubmoduleIterator.html
@kvetak
Copy link
Owner

kvetak commented Oct 6, 2020

Hi there!

So what is the OMNeT++ version that you recommend to test your implementation?

So far 5.2.1 does not work when I clone master of https://github.com/karlhto/RINA-thesis.

@karlhto
Copy link
Contributor Author

karlhto commented Oct 6, 2020

I recommend the latest version @kvetak
Either 5.6.2 or 6 preview 8.
Should work out of the box as long as you have inet4 in the same directory as rina. If you go with OMNeT++ preview 8 this should work regardless, but 5.6.2 only requires you to add the inet project as a project reference in the OMNeT++ IDE if you use that. I used functionality from newer versions (where a lot of stuff has been improved).

So, if you use the IDE:

  • Download 5.6.2 or 6 preview 8
  • If 5.6.2:
    • Make sure to check that you want to download inet if not already done and make sure it is version 4.2.0!!!
    • Add inet as a project reference to rina (Right-click rina -> Properties -> Project References, check inet
  • For 6 preview 8 it should work out of the box as long as you choose to include inet4 when prompted by the IDE.

If you do this from outside the IDE, using the Makefile that is supplied:

  • Clone inet4 in the same directory as the rina project is found. This is exactly how it's done with dependent projects in OMNeT++ otherwise (just using ../inet4 as dependency)
  • cd inet4 && git checkout v4.2.0 && make makefiles && make && cd -
  • cd rina && make makefiles && make

Using the simulate.sh script, you need to define nedpath for INET4 NED-files, which can be done like:

./simulate.sh examples/Shim/SwitchShim -G -c PingAll -n ../inet4/src

...where:

  • -G allows you to use Qtenv
  • -c PingAll chooses configuration PingAll
  • -n ../inet4/src supplies ../inet4/src as an additional location for NED-files.

Please notify me if any of this does not work.

@karlhto
Copy link
Contributor Author

karlhto commented Oct 6, 2020

I don't really know why there's a discrepancy between the names of the included INET4 in OMNeT++ 6 and OMNeT++ 5.6.2, but official tools and the docker images use inet4 as the path. Note that OMNeT++ 5.6.2 and OMNeT++ 6 both include version 4.2.0 of INET, but OMNeT++ 5 for some reason uses the wrong name.

IMO they should have kept the name as inet for both releases to not mess up the already fragile setups that OMNeT++ facilitates in regards to dependencies.

@karlhto
Copy link
Contributor Author

karlhto commented Oct 6, 2020

Also note that I have not tested this on Windows, but I don't see any reason why it shouldn't work if you use the IDE. The code should be completely agnostic, so if there's a problem it's probably in the Makefile or something.

@kvetak
Copy link
Owner

kvetak commented Oct 7, 2020

I have spent a decent amount of time trying to run your RINASim version from master of your repo. Unfortunately, I was unable to compile it in neither version following any path (CLI vs GUI) in your guideline out-of-the-box. The problem always lies in cross-reference between inet and rinasim. Currently, I do not have any option how to reproduce your results and check your scenarios.

@karlhto
Copy link
Contributor Author

karlhto commented Oct 7, 2020

Weird. Maybe you need to use a new workspace for OMNeT++?

@kvetak
Copy link
Owner

kvetak commented Oct 7, 2020

Following occurs when trying to compile your master out-of-the-box. inet and rina are in the same directory samples\ and rina references inet project.

OMNeT++ 5.6.2
image

OMNeT++ 6prev8
image

@karlhto
Copy link
Contributor Author

karlhto commented Oct 7, 2020

Interesting! I'll look into it. :)

@kvetak
Copy link
Owner

kvetak commented Oct 7, 2020

@screw was able to compile it, so I will inform at him.

However, let's try to coordinate work on pull-requests aiming to integrate your thesis. Generally, I am okay with your changes in Flow Allocator module as long as they will not break fingerprints on previous non-ShimEthernet scenarios.

@screw
Copy link
Collaborator

screw commented Oct 7, 2020

I had the same error as @kvetak so I manualy added the inet/src as another include location and also comment out one line using some random function from <algorithm.h>.

I wonder if perhaps you have some settings in .cproject, .project, etc. that didn't get pushed to the repo due to gitignore.

@karlhto
Copy link
Contributor Author

karlhto commented Oct 8, 2020

@kvetak Regarding breaking fingerprints:

Since my changes trigger flow allocation of management flows more times, it follows that fingerprints won't match for most examples where that's relevant since it's computed by the set of events that happen. I am open to discussion on it, but we should probably take that in the relevant PR (the two last commits are relevant).
Also, IIRC most of the recorded fingerprints were already incorrect from before I started developing the projects.

@karlhto
Copy link
Contributor Author

karlhto commented Oct 14, 2020

@screw <algorithm.h> is from the C++ standard library (I assume you're pointing to find_if), so I sincerely doubt that it should create an issue, even on Windows. Maybe it was included through nested includes on Linux somehow?

Also this PR in and of itself should compile alright. I think that it's compatible with every version of OMNeT++ after and including 5.3 at least, but maybe before as well. Note that INET 4.2.0 claims that they only support OMNeT++ 5.5.1 or later, so including my other changes it's probably a good idea to support the newest versions regardless.

I tested both with earlier and later versions of OMNeT++, and no examples were broken that weren't already broken from before. I have some additional changes that I haven't put in a PR yet that fixes several examples, like SimpleLS which has random results every time because of uninitialised values.

@screw
Copy link
Collaborator

screw commented Oct 14, 2020

I tested it on macOS and it didn't compile so I was just trying things.
I didn't have a chance to test it on Linux.
The problem is random_shuffle(). Per google search it has been removed from <algorithm.h> in C++17.

Also, I am against -Wno-inconsistent-missing-override.

I agree we should discuss other points in the corresponding PR.

@karlhto
Copy link
Contributor Author

karlhto commented Oct 14, 2020

I tested it on macOS and it didn't compile so I was just trying things.
I didn't have a chance to test it on Linux.
The problem is random_shuffle(). Per google search it has been removed from <algorithm.h> in C++17.

I see, I assumed wrong, sorry. :^)
I'll look into that too.

Also, I am against -Wno-inconsistent-missing-override.

I forgot to specify why I added the change: this warning appears in every single auto-generated _m.h file, so it creates a lot of noise when compiling. I do agree that it's a good idea to include more warnings to prevent bad code, but there are plenty of more serious warnings in the codebase already that should be brought more attention to.

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

Successfully merging this pull request may close these issues.

3 participants