artery icon indicating copy to clipboard operation
artery copied to clipboard

Collective Perception Service - Core

Open sodevel opened this issue 2 years ago • 10 comments

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

sodevel avatar Jun 28 '22 11:06 sodevel

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
  • random number (as done upstream)
  • node index (as found in your branch)
  • OMNeT++ host module id? What do you think?
  1. I don't quite understand the need for the ItsG5BasService multi-stage initialization. Can you provide some additional information?
  2. I am still not a fan of "hot-patching" the source code generated from CPM ASN.1. I would prefer a dedicated "CPM_alt" including "CollectivePerceptionMessage_alt" -> "CpmParameters_alt" -> "PerceivedObejctContainer_alt" -> "PerceivedObject_alt" (replace "_alt" by a reasonable suffix name). For example, we could write a CPM_alt.asn1 defining only those modified CPM types and bundle the generated in Artery. I admit this is a little bit more effort but I expect this process to be less surprising, especially for less experienced users.

riebl avatar Jul 02 '22 20:07 riebl

  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.

sodevel avatar Jul 06 '22 11:07 sodevel

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?

riebl avatar Aug 02 '22 20:08 riebl

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.

sodevel avatar Aug 03 '22 22:08 sodevel

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!

riebl avatar Aug 15 '22 17:08 riebl

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

Format related PR seems to be merged.

Any update on this PR?

mfyuce avatar Oct 15 '22 23:10 mfyuce

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

riebl avatar Oct 18 '22 19:10 riebl

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.

mfyuce avatar Oct 18 '22 20:10 mfyuce

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.

sodevel avatar Oct 18 '22 22:10 sodevel

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.

mfyuce avatar Oct 19 '22 06:10 mfyuce