riebl / artery

OMNeT++ V2X simulation framework for ETSI ITS-G5
GNU General Public License v2.0
204 stars 132 forks source link

Collective Perception Service - Core #251

Open sodevel opened 2 years ago

sodevel commented 2 years ago

These are the core Artery changes required for #249.

The way the patching of the CP message format is done is not optimal, it results in a dirty source tree. This wasn't done by me because back in the days i had no idea about that CMake stuff, this is a bit different now. It would be better to copy the source files that require patching into the binary tree and patch them there, but currently this doesn't work because the files use quoted includes.

If i understand the CMake files of Vanetza right, they contain the code to actually compile the C++ files that are included in the repository from the ASN1 files and perform some intense post-processing. This post-processing also changes the angle includes into quoted includes. If the angle includes would be kept, putting the binary tree before the source tree in the include path would be sufficient to pick up the patched header file instead of using the original one (selecting the correct source file is easily done by CMake code).

riebl commented 2 years ago

Thanks @sodevel for providing this branch for discussion. I am fine with most commits.

1) Instead of hard-coding the station id assignment, we may realize a configurable mechanism supporting

sodevel commented 2 years ago
  1. This sounds like a good idea. It's been a while i worked with OMNeT++, how should that be done? In plain C++ i would use a simple Enum together with a switch block or a function object.
  2. I don't recall exactly anymore, but in our code base there was the need to switch one of the service base classes to multi stage initialization which required to change every service to be aware of that. With that change this was not necessary anymore and you could follow the OMNeT++ policy to override only one of the initialization methods and call only their direct parent implementation.
  3. Wouldn't that require to duplicate most / much of the contents in the its folder of Vanetza? Is there a use case that requires two different CPM types to be available at the same time? I don't know if it is confusing to use different names than the ones used by the spec. Sadly, the generated code is plain C so we can't use the same names but placed into different namespaces that e.g. encode the version of the spec and patch-marker or something like that.
riebl commented 2 years ago

Hi @sodevel, please have a look at my cpservice_core branch I have just pushed. What do you think of it?

I have implemented 1) by Identity::deriveStationId which is called by several middleware flavours, and also CarIdentityRegistrant. Please note that the upstream CollectivePerceptionMockService already uses multi-stage initialization. No need for any changes at ItsG5BaseService for this purpose. Hence, I have omitted 2). Modified CPM structures 3) will be part of a separate commit. I plan to merge a solution along with the modified service branch (your other PR).

I have also removed the .clang-format because it did not like Artery's existing code style at all. Maybe I am doing something wrong?

sodevel commented 2 years ago

The branch is looking good to me, thanks for the additions.

About the clang-format specification, i tried to replicate the current code style. However, there is not a unified style used accross the whole code base, i tried to use the style that seemed current to me, i changed some small aspects because of personal preference, some results are because of limitations of clang-format itself. I did not use automatic tooling to apply the formatting, i only used the Format Document feature of VS Code regulary on the files i created. The format is not intended to be used for anything inside extern, i placed the file into the root directory only to easily apply it to scenarios and src.

Can you be more specific about what clang-format changes what it shouldn't? For testing, i formatted a bunch of files and these are my observations.

Intended changes:

  1. Tabs get converted to space. I am a space-guy, i had to do this :smile:
  2. Include groups are separated by newline. This looks more clear to me than one huge include block.
  3. Some headers indent the access modifier by one level and the methods below by another level. This is kind of wasting space, i removed one level of indentation.

More or less unwanted changes:

  1. Single line loop bodies get moved in the loop header line. This one slipped through, there is a setting to prevent this, i can fix that.
  2. Multiline comments get aligned at the star. This is default behavior and is also used by Java, so i kept it.
  3. Inline comments are separated by two spaces. Again, this is default behavior, i didn't change it.
  4. The first line after many (all?) macros gets wrongfully indented by one level. This is a formatting error and the only thing i manually corrected in my files. This seems to get worse if a namespace declaration follows the macro. There are clang-format settings for macros, i could take a look at these and try to fix that.
  5. Method parameter lines and constructor initializer list lines get merged or split. This happens due to the pretty high line length limit and binpacking, there are plenty of penalty settings to tune this behavior, i didn't spend time on this.
  6. Clang-format does a pretty poor job at splitting long lines. The very latest version implements some parts of that at least 3 years old PR lingering on their issue tracker to improve that, but is this done only in certain contexts and isn't uniform. I increased the maximum line length as much as possible to reduce the need to split lines, currently we have to live with what clang-format produces.

If you are concerned about to mess up git blame output, you can add a configuration file to ignore formatting-only commits.

Let me know what i can clean up of my other PR before you merge it.

riebl commented 2 years ago

Can you please move the clang-format stuff to a separate branch where we can further discuss the formatting-specific issues? In general, I am fine with clang-format, and I see the value of having a suitable .clang-format in the repository. However, code formatting is unrelated to Collective Perception. I much appreciate your effort!

mfyuce commented 2 years ago

https://github.com/riebl/artery/pull/260

Format related PR seems to be merged.

Any update on this PR?

riebl commented 2 years ago

I have just merged _cpservicecore into master. Are you referring to any specific further parts of this PR @mfyuce?

mfyuce commented 2 years ago

Thank you so much @riebl, really appreciated and want to say from my heart that really good work. I will check out.

Currently researching ways to integrate CPM messages into the VereMI dynamic dataset and hopefully open source it, if you kindly willing to accept.

Any help is appreciated.

sodevel commented 2 years ago

This PR does not contain anything CPM related except the patching of the CPM message format which was not merged. All CPM related functionality is part of #252, but for that one to work, an alternate solution to use "extended" CPMs must be implemented first.

mfyuce commented 2 years ago

Thank you @sodevel, i will look into it.

Recently read a paper implying they have integrated CPM with artery, but no source code published.

@inproceedings{tsukada2022misbehavior,
  title={Misbehavior Detection Using Collective Perception under Privacy Considerations},
  author={Tsukada, Manabu and Arii, Shimpei and Ochiai, Hideya and Esaki, Hiroshi},
  booktitle={2022 IEEE 19th Annual Consumer Communications \& Networking Conference (CCNC)},
  pages={808--814},
  year={2022},
  organization={IEEE}
}

I have contacted the authors, will inform about their response.