Closed wischhof closed 4 years ago
I'm seeing several suspicious big files in tests/fingerprint
like: argpars, copy, csv, glob, os, re, time, unittest. Those seem to be photoshop images ??? i.e. they are mistakenly committed.
Are the fingerprint tests supposed to PASS? A lot of them are failing currently.
I reviewed the pull request from the INET API usage point of view and added my comments inline. I think this is a big step in the right direction.
@levy @rhornig I read though your comments - very helpful! - and see the point in most of them. We will be checking them in the next days and update the pull request, I'll drop a note when we have solved all the issues.
We have addressed all of the issues/comments and updated the pull request accordingly.
Most were minor changes. However, in two cases (namely the CbrSender/CbrReceiver apps and the LteIp module) the problems you spotted were within previously untested code. (Seems that these parts are also not used within any of the simulations in the INET3 version of simulte.) Therefore, besides fixing the problems we added two additional scenarios within the Demo simulation and the respective fingerprints. Thanks for your detailed review and dentifiying those problems!
That's great!
@rhornig @levy @giovanninardini @kruviser What is your opinion on how we should proceed? Since there are many significant changes required for INET4 and currently I have no clue, if and how many other people have tested the simulations besides us, one intermediate step could be pull not on the master branch but maybe on a separeate inet4-Branch?
We could mention that branch within the README of the master and/or the simuLTE website so that we have hopefully more people using and testing it.
I have no objection in this respect. We are following the discussion from the background but we haven't tested your work yet.
Il giorno gio 21 nov 2019 alle ore 10:01 wischhof notifications@github.com ha scritto:
@rhornig https://github.com/rhornig @levy https://github.com/levy @giovanninardini https://github.com/giovanninardini @kruviser https://github.com/kruviser What is your opinion on how we should proceed? Since there are many significant changes required for INET4 and currently I have no clue, if and how many other people have tested the simulations besides us, one intermediate step could be pull not on the master branch but maybe on a separeate inet4-Branch?
We could mention that branch within the README of the master and/or the simuLTE website so that we have hopefully more people using and testing it.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/inet-framework/simulte/pull/43?email_source=notifications&email_token=AAMPQJRXANFPLCJETNLBE53QUZE45A5CNFSM4JNCH2K2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEEZO4VQ#issuecomment-556985942, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAMPQJRMTY6STKIA472S7OTQUZE45ANCNFSM4JNCH2KQ .
-- Antonio Virdis
Agreed. I have created an 'inet4-porting' branch. Please send a pull request against that and I will publish it and amend the main README file on the master branch.
Agreed. I have created an 'inet4-porting' branch. Please send a pull request against that and I will publish it and amend the main README file on the master branch.
I have sent a new pull request against that branch. I guess we could now close/cancel this one, right?
In order to be able to use INET 4, significant changes had to be made to the simulte sources, including but not limited to:
In total, almost all source code files had to be touched - therfore, this is a quite large pull request. All simulations within the simulation folder (including the cars scenario which makes use of veins) are runnable. However, an in depth-comparison of the simulation results with those obtained with the INET 3 based version has not yet been done.